MFOpenZFS: Fix zpool history unbounded memory usage

[markj]

Summary

In original implementation, zpool history will read the whole history
before printing anything, causing memory usage goes unbounded. We fix
this by breaking it into read-print iterations.

Reviewed-by: Tom Caputi <[e-mail]>
Reviewed-by: Matt Ahrens <[e-mail]>
Reviewed-by: Igor Kozhukhov <[e-mail]>
Reviewed-by: Brian Behlendorf <[e-mail]>
Signed-off-by: Chunwei Chen <[e-mail]>
Closes #9516

PR: 247557
Obtained from: OpenZFS
MFC after: 2 weeks
openzfs/zfs@7125a109dcc55628336ff3f58e59e503f4d7694d

[markj]

Do we need to bump SHLIB_MAJOR for such a change? Should I try to provide a compatibility shim so that the fix can be merged to stable/12?

[freqlabs]

Seems a bit of a waste if you don't MFC, since the plan is for vendored OpenZFS to land in sys/contrib before 13.

Changing this API shouldn't actually affect anyone since libzfs is private and iX are the only ones using it anyway that I'm aware of, but we're already using OpenZFS in 12.
So, I don't feel it should be necessary to provide for compatibility in this case. I don't know whether I would bump the major version or not, but I suppose to shouldn't hurt anyone either.

[markj]

In D25745#570194, @freqlabs wrote:

Seems a bit of a waste if you don't MFC, since the plan is for vendored OpenZFS to land in sys/contrib before 13.

Right, I'm only bothering with this so that 12.2 can get the change.

Changing this API shouldn't actually affect anyone since libzfs is private and iX are the only ones using it anyway that I'm aware of, but we're already using OpenZFS in 12.

Why do you say libzfs is private? There are ports that use it (e.g. py*-libzfs), though I don't see any that reference the zpool_get_history() symbol.

So, I don't feel it should be necessary to provide for compatibility in this case. I don't know whether I would bump the major version or not, but I suppose to shouldn't hurt anyone either.

We shouldn't MFC major version bumps, since that will break libzfs consumers that were linked on 12.0 and 12.1.

I'm fine accepting a claim that this change is very unlikely to break anything, but I wanted to give reviewers a chance to object to committing the diff as-is. Some googling suggests that zpool_get_history() is indeed not really referenced by anything except language bindings.

[freqlabs]

In D25745#570209, @markj wrote:

Why do you say libzfs is private? There are ports that use it (e.g. py*-libzfs), though I don't see any that reference the zpool_get_history() symbol.

The headers for libzfs aren't installed. py-libzfs is rather desperate and requires the src tree to use headers from in order to build. libzfs is a collection of random functions that were duplicated between zpool and zfs. It doesn't have a sensible, stable API for external consumers, that is the goal of libzfs_core. It does still contain a lot of complex functionality that makes some willing to jump through all sorts of hoops to use libzfs anyway, but they should have a pretty good idea what they've signed up for by the time they manage to get anything to even compile.

The binding to zpool_get_history is being removed from py-libzfs, for what it's worth.

[freqlabs]

All this is just to say that even if you don't much care to maintain API compatibility when you MFC, it would be preferable still to not merging this back at all. But if you do maintain compatibility, that's even better.

[markj]

In D25745#570242, @freqlabs wrote:

In D25745#570209, @markj wrote:

Why do you say libzfs is private? There are ports that use it (e.g. py*-libzfs), though I don't see any that reference the zpool_get_history() symbol.

The headers for libzfs aren't installed. py-libzfs is rather desperate and requires the src tree to use headers from in order to build. libzfs is a collection of random functions that were duplicated between zpool and zfs. It doesn't have a sensible, stable API for external consumers, that is the goal of libzfs_core. It does still contain a lot of complex functionality that makes some willing to jump through all sorts of hoops to use libzfs anyway, but they should have a pretty good idea what they've signed up for by the time they manage to get anything to even compile.

The binding to zpool_get_history is being removed from py-libzfs, for what it's worth.

Thanks.

In D25745#570254, @freqlabs wrote:

All this is just to say that even if you don't much care to maintain API compatibility when you MFC, it would be preferable still to not merging this back at all. But if you do maintain compatibility, that's even better.

Ok. I am now inclined to ignore compatibility and commit the change as it is, but I'll wait a couple of days for other folks to chime in if they disagree.