do a cyclic scan for orphan objects in zstd memory pool

[BrainSlayer]

in non regular usecases allocated memory might stay persistent
in memory pool. this small patch checks every minutes if there are
old objects which can be released from memory pool.

right now with regular use, the pool is checked for old objects on each allocation attempt from this pool. so basicly polling by its use. now consider what happens if someone writes alot of files and stops use of the volume or even unmounts it. so the code will no longer check if objects can be released from the pool. so what has been allocated still stays in pool cache. this is no big issue for common use. but someone discovered this issue while doing tests. personally i know this behaviour and i'm aware of it. its no big issue. just a enhancement. see also #10938

covers #10938

Signed-off-by: Sebastian Gottschall [email protected]

Motivation and Context

Description

How Has This Been Tested?

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[mschilli87]

@BrainSlayer:

in non regular usecases allocated memory might stay persistent in memory pool.

Why is that? Do we know? Don't we care?

this small patch checks every minutes if there are old objects which can be released from memory pool.

This seems a bit hacky IMHO. Wouldn't the proper fix be to investigate why the memory ends up orphaned in the first place and ensure it doesn't?
If you want this as a fallback safeguard for practical reasons, I think it should at least issue a warning when it has to release memory 'forgotten' previously. Otherwise this could end up masking more severe future issues with 'proper' memory release.

[BrainSlayer]

@mschilli87 let me explain. right now with regular use, the pool is checked for old objects on each allocation attempt from this pool. so basicly polling by its use. now consider what happens if someone writes alot of files and stops use of the volume or even unmounts it. so the code will no longer check if objects can be released from the pool. so what has been allocated still stays in pool cache. this is no big issue for common use. but someone discovered this issue while doing tests. personally i know this behaviour and i'm aware of it. its no big issue. just a enhancement. see also #10938

[mschilli87]

@BrainSlayer: Thank you for the explanation. My two cents:

1. This explanation should be in the commit message to document the need for that change.
2. I'd still prefer a fix more closer to the root cause. Why not trigger this check when umounting a volume instead of polling regularly in case it got unmounted? Do we keep pooling unused volumes even after freeing the leftover memory? Or could we stop considering this volume after dealing with it once (within a minute after stopping to use it) and add it back to our list once it get's used again (e.g. in the polling by usage code you mentioned)?

[BrainSlayer]

copied the my explaination to the title. will add it later to the commit message after testrun is done

regarding the check procedure. zstd is just a algorithm. it doesnt know of any higher level operation and does not know if a volume is in use, mounted or unmounted. the pool is also not used per volume, but used for the algorithm which can be used by multiple volumes of course. and yes the polling remains also if all is freed. but it only happens once in a minute and does not cause any measureable performance impact since the code which is executed is very small for checking. forget about thinking in volumes. just think about a compression algorithm which is available globally for everything you do. the case i mentioned was just a example. if a any volume is in use and you just dont write or isnt in use, the preallocated ram stays allocated without that patch. so its basicly just a optimization to reduce memory usage if everything related to zstd is in idle state. of course i can stop the thread after nothing is left and restart it on first new allocation. but this is a more complicated handling and may cause a latency spike on first allocation attempt. keep it simple as possible is the best way from my oppinion. stopping and restarting threads needs also special handling to avoid race conditions in this threaded context. the current simple approach is thread safe

[mschilli87]

That's a good enough explanation for me, as long as it makes it into the commit message and/or a comment in the source code itself. Thanks for taking the time to walk me through.

[PrivatePuffin]

Looks good with a slight nitpick on the note not refering to the 60 seconds.

Ping for @c0d3z3r0 and @allanjude to check this out if they want.

[richardelling]

ISTM that this is the wrong approach. We know when an objectset is removed from the system (umount, destroy, close) and we already have async clean to clear its data from ARC. Why not just extend it to also clear out the zstd memory pool?

[allanjude]

ISTM that this is the wrong approach. We know when an objectset is removed from the system (umount, destroy, close) and we already have async clean to clear its data from ARC. Why not just extend it to also clear out the zstd memory pool?

This is a pool of memory allocations used for compression. It is not tied to any specific objset. It is just that after a period of high activity, the amount of memory used by this pool may be high, and it doesn't shrink when there is no activity.

[mschilli87]

@allanjude: That is what confused me at well until @BrainSlayer explained it.
@richardelling: Good to know I am not alone. 😉

I think this whole discussion just stresses that the change suggested here should come with good documentation.

[BrainSlayer]

ISTM that this is the wrong approach. We know when an objectset is removed from the system (umount, destroy, close) and we already have async clean to clear its data from ARC. Why not just extend it to also clear out the zstd memory pool?

circular dependency. no call to zstd code can be made from functions within zfs. zzstd must be loaded before zfs is loaded. such solutions are only possible with installable hooks

[BrainSlayer]

@allanjude: That is what confused me at well until @BrainSlayer explained it.
@richardelling: Good to know I am not alone. 😉

I think this whole discussion just stresses that the change suggested here should come with good documentation.

according to what he wrote, he did understand very well how it works.

[BrainSlayer]

@behlendorf please check the new solution for handling the memory cleanup.

[BrainSlayer]

@behlendorf regarding the way when to reclaim space and when not. the pool allocator is self managing. waiting until memory is low, if avoidable is not a good way from my point of view. in our testcase we had about 1.5 gb unused memory left. thats no small piece. releasing it if not in use for a while avoids such situations which will slow down zfs. so if avoidable without performance impact is a good approach