Conversation
e3a03b9 to
4e2ec24
Compare
|
I measured this against its parent on a VM to see where Setup: 31 GiB guest, one virtio disk as a single vdev, At the two small sizes the indirect churn stops outright: 4 to 54 evictions in 45 The target column is a mean of one-second samples, and at the two small sizes the target What bounds the target is the cap in On throughput the ratios of the medians came out 1.107, 1.112, 1.086 and 1.061 across the |
|
while I agree there is a problem to address, this seems a bit hacky. Wouldn't it make more sense to modify the evict() logic so that it doesn't toss a dbuf unless actually stale, or is explicitly associated with a cached data block that the LRU has marked? I don't know if dbufs have a TTL (time to live) or if that would be useful. I think the fundamental problem with the current logic is that it's based on a contrived size ratio to target_ARC, and doesn't treat L1 dbufs etc. as higher priority to stick around over other things like cached data blocks. Along similar lines, target_ARC should always grow based on metadata and dbuf_cache allocations and only tangentially in support of data blocks being read from disk. I realize having to go back to disk to re-read data is extremely expensive, but honestly, the incidence of re-reading the same data blocks over and over is wildly overblown. And if you really do have a workload that accesses cached data blocks that unfortunately get reaped too soon by ARC pressure management, that's what L2ARC (or more RAM) is for. Or cache it yourself in the app or OS virtual memory/page_cache layer. A penchant for Over-caching is just as bad or worse than under-caching. IMO the better solution is to let "naked" dbufs (entries without a corresponding cached data block) fill the entire ARC (or a large proportion) which will force ARC to grow to ARC_MAX if the workload is sustained. If you absolutely must insist on pinning some ratio of CURRENT_ARC_BYTES for purposes of caching data blocks (eg. 25%), then one could write the eviction logic such that these "naked" dbufs are preferentially reaped once that threshold has been met. |
4e2ec24 to
39dbf34
Compare
|
@mkhllr thanks for your great verification, the bound "arc_c_max >> dbuf_cache_shift" is not right, I added a new module parameter (again), default as 2, that mean 1/4 of arc_c_max could be dbuf "extra" cache. Hope you could comment this change, and re-test it. Refer to commit message and code for details. |
As I learned from the code, the commits, the eviction logic, the overall design is quite sound. The only thing is that direct IO, nvme disks are not there before 2018. This PR inserts a logic into an existing thread which will add "extra" room for naked dbuf (I like this term), while keep other things well. |
|
I guess what I didn't articulate well was that your patch should:
Fundamentally the problem with dbuf_cache and the ensuing thrashing/unhelpful eviction under sustained direct I/O, is that it was vastly too small for starters, but artificially constrained as a percentage of target_ARC and target_ARC only grows in response to adding data blocks from "cached reads", or much more slowly from collecting "metadata". Since dbuf_cache has its own simplistic evictor triggered by size+ Since we (should) know that "naked" dbufs are being created from O_DIRECT activity we should be able to count these instances. And when the cache size breaches the hiwater_pct, instead compute and try to set the new arc_target that would allow eviction to be forestalled. ie if lowater_pct = 10 then new target_ARC = current target_ARC / 0.9 and zero the O_DIRECT dbuf counter. There may also be some merit to checking the ratio of "naked dbufs" as a percentage of the total pool since the last counter reset,and if it's below X we just evict. If there is no directIO going on then eviction happens as usual. If setting new target_ARC is rejected, then eviction happens as well. I'm not sure what to do about A patch that eliminated the size constraints altogether (IMO a case of false optimization in the first place) and instead waiting to clean up via the evictor that fires when ARC = ARC_MAX or arc_target can't grow anymore due to OS memory pressure is the best way to address the problem. ZFS seems entirely too wed to the notion that we need to cache user data we just read because it'll be read again in short order. I doubt that was a valid assumption but unfortunately it underpins a variety of data-structures. |
Hi @tb3088 , there are two rules I got in this PR: 1) we should not set module parameters for direct I/O only, the existing link between dbuf cache target and arc target, and there defaults has there logic, and should retain; 2) we should not tune module parameter "internally", e.g. via detecting direct I/O high IOPS. With two rules in mind, I guess we have to go to this PR, by a up/down "internal" variable which is dbuf_cache_extra based on if direct I/O coming/going, and a bounding module parameter which is dbuf_cache_extra_max_shift. And I do think there are too many module parameters now, especially the dbuf_cache_max_bytes which is set to UINT64_MAX. Also 1 second tick can be changed if necessary, e.g. 5 or so? |
|
I don't like "extra" for naming because it doesn't mean anything. But leaving that aside, the 'tick' thing IS the very definition of trying to tune behavior internally in a hidden fashion based on a very coarse 'clock' interval. Strictly speaking if the sysadmin is not paying attention to his workload, and directIO is churning the (way too small) dbuf_cache that really should be on him. He should be changing dbuf_cache_shift and hiwater_pct. However, given that ZFS currently has a naive+incomplete means of influencing target_ARC growth, I think THAT should be addressed. If we count dbufs being created in response to DirectIO, at some point the standard logic will trigger eviction once dbuf_cache_memory_footprint >= (100+hiwater)pct * targetArc * dbuf_cache_shift. and THAT is the point that we intervene by checking how much directIO was going on, and if it was a lot, then we upsize target_ARC and return instead of continuing to eviction. When the dbuf_cache grows to its upper limit again, we apply the same check. If there was little/no DirectIO, then we step out of the way and let the LRU trim the cache. In effect the PR should only have enough code to count dbufs being allocated in response to DirectIO and a conditional right after the hiwater condition is satisfied. |
amotin
left a comment
amotin
left a comment
There was a problem hiding this comment.
This is an interesting topic. I am thinking about it on and off for quite a while, but instead from a perspective of system cold boot, when it simply haven't seen enough data yet to grow the ARC, but it would be good to make it run without constant ARC decompression. The reason why the dbuf cache limit is set so low is to not permanently duplicate data already stored in ARC. ARC is allowed to grow big because it is unmapped and compressed. By duplicating it you risk possibility to KVA exhaustion and waste its compression respectively.
I couple places in ZFS I've already used arc_warm ? arc_c : arc_c_max construct to counteract cold boot issues. Recently was introduced one more arc_warm ? arc_c : (arc_c + arc_c_max) / 2, AKA arc_boot_target_bytes(), which I think is better. I was actually thinking to extend those to other places using arc_c, just haven't got to it yet. Though it is indeed a bit simplistic.
I think it would be cool to make dbuf cache size to dynamically adjust to the active dataset size. You've tried to implement it in this patch, but I don't think good enough.
If your workload is such that your active dataset size is equal to the total dataset size, then it would be cool to make debuf cache not duplicate the ARC, but share buffers with it, giving up on compression and KVA. Though giving up on KVA would limit the maximum cache size to avoid KVA fragmentation, and open a question of what to do with blocks in ARC when their dbuf counterpart is evicted? Evict too or re-compress/re-scatter? May be you would generally like to tune ARC to not compress (some kinds of) metadata blocks to allow the buffer sharing for free, if that is your real scenario, not some absolute synthetic?
amotin
added
the
Status: Revision Needed
|
I re-tested at At the default shift of 2 the indirect churn is gone at all four sizes: 3 to 10 evictions
At 100G the two arms came out closer together this time, and one claim from my previous At shift=4 the ceiling is 1942 MiB and the knee should fall at 60.7 GiB: 50G fits, with |
|
@mkhllr if you run this bench but ignoring the PR entirely, and tune dbuf_cache_shift to 3, 4, 5, set arc_min = 16gb and/or 24gb or 30gb to see how the eviction rate is influenced? @amotin maybe dbuf_cache could use something akin to zfs_arc_meta_balance does on metadata eviction? ie. a ghost hit would initiate a target_ARC increase? I think such a feedback mechanism would gradually taper the prematurely evicted dbuf entries till it arrived at a steady state? The early misses would be painful at first but would self-correct. |
ac661fb to
de66c83
Compare
github-actions
Bot
removed
the
Status: Revision Needed
|
@amotin , I added one internal variable and one module parameter. I will be very happy to see a cleaner solution in another PR, or just make this PR correct. The whole point is, I do not break how dbuf cache target is managed, and how dbuf and arc are evicted, just "find" a signal to add/remove an extra, and must be out of the I/O path. The signal may be not quite "accurate" by dbuf_arc_underutilized and trimming, and the one second tick may be too often, plus the steps (up/down) may be too sharp. The "too often" can be addressed by run the adjust 2 or 4 seconds in arc_reap_cb_check by a random picking. if (random() > 0.5)
dbuf_cache_adjust_tick(arc_no_grow, arc_c_max);For the arc_warm and arc_boot_target_bytes, the problem is still, we need a signal. "trimming", I mean the code: "uint64_t trimmed = evicts - dbuf_cache_prev_evicts;", how much evicts happens in passed 1 second. |
Thanks @mkhllr , I just do a rebase again to catch up master, especially, this PR touches arc/dbuf, core code of zfs, and may not be good for testing :-) The test case added, and your test, confirmed this PR worked. Do you know a better way to simulate a normal I/O, like mixed ARC and direct I/O and different datasets, file sizes. And also more numbers collected, cache_extra_bytes etc. I think we already test and verified at #19076 , that this PR could improve direct IO IOPS. But would this PR hurt the system in other ways. |
Blowing dbuf cache up to 1/4 of RAM, duplicating ARC, is not the answer. It is not a question of tunables present. Lets not go insane about zillions of synthetic Direct I/O IOPS tomorrow, but think strategically. The thinking towards the mentioned ghost states similar to ARC sounds interesting, but I wonder how complicated would we like it to be? Could we differentiate different kinds of blocks? Could we integrate with ARC states? Could we improve dbuf sharing? |
Thanks @amotin . If I understand your comments correctly, it looks I need a different PR or just replace all code here with new ones. Let me try. |
|
First I'd say a little less rush. ;) If the code is significantly different, clean PR may have sense to compact the comments history. |
amotin
added
the
Status: Revision Needed
|
maybe the easiest 'fix' is documentation? sysadmins are largely ignorant of ZFS internals. sysadmins aren't paying attention to arcstat nor their workloads and its knock-on effects. sysadmins also generally don't know the ramifications of ZFS defaults nor the effect of tunables like arc_min, hiwater_pct and dbuf_cache_shift. I could easily take a hard-line stance that the solution here is use those knobs to mitigate your problem. Personally I would run something like arc_min=16gb, hiwater_pct=400 and dbuf_cache_shift=3. (I haven't yet checked if hiwater_pct has been sabotaged to only allow values of 0-100) This workload or say a VM boot-storm is like a pig in a python. Except that ARC doesn't stretch like a snake digestive tract since it has no signal to act upon. My trio of settings pre-stretches the digestive tract and once the pig is in and there is a huge bulge (in dbuf usage) it'll waste space for a while but once it trips hiwater_pct=400 the pig will disappear in a flurry of evict() actions. Till the next pig comes along. A paragraph on the ZFS tuning wiki could go a long way I think? That said adding some smarts to dbuf_cache management gets an upvote. |
This PR is just doing this. And for your suggestion about "documentation fix", the problem is what I said, the system admin could not adjust module parameters time by time, e.g. before the pig is in, make hiwater_pct up, and then set it back. And if the hiwater_pct is not set back to reasonable one, or it is set to 400% without changing, the dbuf evict thread will even do not start or stop quickly, then 400% of dbuf target contains "never accessed again" data, e.g. L1 indirect block, plus compressed ones in arc meta area. |
agreed to a point. I would look at why L1 indirects are overlapping meta instead of there being one source of truth. Perhaps it was just expedient to duplicate. Which then begs the question, if the dbuf_cache had an entry evicted, does it go back to meta and re-insert it into the cache chain? or maybe just read it and return? Temporarily wasting memory in dbuf_cache because of an outsized workload that came thru is not a problem. Memory is cheap. disk access is expensive. A few extra minutes spent letting the pig stick around causes no great harm. The trigger will fire soon enough and whack it back to size. shift=3 and hiwater=400 gives me the benefit of shift=2 without the "waste" being long term. You could choose different values like shift=5 hiwater=3 and temporarily get the benefit of shift=3 |
|
@amotin While I am still on the way for new solution, let me re-state my thinking to your comments. The thinking towards the mentioned ghost states similar to ARC sounds interesting, but I wonder how complicated would we like it to be? Could we differentiate different kinds of blocks? Could we integrate with ARC states? Could we improve dbuf sharing? In a word, as we want to handle the "ghost states" in dbuf cache, we need a signal, a mechanism. And I do think this PR make this works, also has some "defects" I already said before. It has several good points. And the "Blowing dbuf cache up to 1/4 of RAM", if it happens, it means it has to be, and it will drop once it can. |
|
@tb3088, the sweep you asked for, and
At and below a 1024 MiB target the four cells share a common interval, 1.97M to 2.25M. The zero-eviction cell ran 201,531 to 204,579 IOPS, above every repeat of every other That arithmetic holds while The pin cost no memory in this run: Then It accepts 400 and 1000: The eviction medians stayed in a narrow band, 1.24 to 1.56M per 45 s. The 400 cell did IOPS did move. The 100 and 400 cells ran above every repeat of the 10 cell, by 5 to 7% on That is the part worth knowing about this knob. |
I guess I should have read the source code. :) Yeesh, so much for my desired behavior. I think Alternatively, In any case, a bunch of ghost hits would be a nifty way to signal the evict() to back off and just keep the overrun contained to hiwater. |
|
@tb3088 on the assumption you flagged: The eviction loop in while (dbuf_cache_above_lowater() && !dbuf_evict_thread_exit) {
dbuf_evict_one();
}so once it wakes it keeps calling that until the cache is back at the low-water In the 4k Direct I/O workload from the hiwater sweep above, the cache was above the On a bigger bite at a hiwater breach: in the cells where I raised What one-second sampling cannot show is the rate within a second, and the counts say Your ghost-hit idea is separate from this, and I have nothing measured that bears |
IIRC dbuf cache was tied to ARC before ARC got scatter ABDs. In wast majority of cases dbufs shared uncompressed buffers with ARC. If compression was disabled or not very successful, dbuf cache cost very little, and was just differently indexed (logically instead of physically) interpretation of the same blocks. The major divergence happened with ABD introduction, when it was decided that since compressed blocks any way can't be used directly, why do we need to map them into KVA. This one change dramatically broke the buffer sharing even for uncompressed data. While FreeBSD shares buffers at least up to 4KB, for Linux that is even lower. But introduction of ABDs was a required response on a problem of KVA fragmentation -- no matter how big KVA is pre-allocated, at certain workload and time it will be exhausted due to fragmentation. I haven't dug how Linux deals with it, but for Illumos and FreeBSD it ended up in kernel panics. dbuf cache appeared as a response to the need of repeatable decompression and copying data from ARC. So for me this is not a question of dbuf cache size. Sure, we can tune it, we can and probably should make it adaptive to optimize RAM usage, but we can not afford it to consume 1/4 of RAM. It just won't fly. And least I think not too far above that threshold. We have to look for some other ways.
I was thinking if we could use buffer state in ARC to affect its life cycle in dbuf cache too.
No. For me it is the topic, that should be answered first. Otherwise choice whether we allow 1/32 or ARC or 1/16 of ARC is not very productive. I won't object if under proper workload we allow dbuf cache to take 1/16, or may be 1/8, for a very good reason, but against each following step I'll argue more. This is a dead end.
This PR, as I read it, effectively gradually give dbuf cache all that it wants, until we hit a memory pressure from the kernel, when it try to shrink, doing enormous amount of work at once, creating a huge load spike and stalling new I/Os. We need a way to find a balance to cache only what we really need. So that accessed once buffers would be evicted as soon as possible, not wasting expensive RAM, but buffers accessed often could live longer. All the motivation is very similar to ARC MRU/MFU, etc, except that for ARC we do want to use all memory we have, since it is our most efficient way to store data, but for dbuf cache we need it to stay small, since it is uncompressed, plus consumes KVA.
Nope. 1/4 we just can't afford. See above. |
|
@amotin Thanks. Always learn a lot from your comments. Now I guess I have a "much clearer" full picture of your concern. I still have following points: 1) dbuf cache for direct I/O, is same thing like ARC for buffers accessed. While ARC could have the "we do want to use all memory we have", why not dbuf cache, and plus the real data is still in ARC; Look at how @mkhllr test, so much evicts if bounded dbuf cache to arc target. 2) "a huge load spike and stalling new I/Os", is the load spike really bad? And from memory view, in this PR, even very high IOPS, the evicts keep in dozens, compare to millions. Once the direct I/O flood is over, e.g. VMs start, the memory is reclaimed very quickly as It should. The main problem now is about how dbuf cache is evicted. It just constantly evict from high water to low water. In mkhllr's test, while "extra" goes up, the low water goes up too, thus not much evicts. Your suggestion about to align with ARC MRU/MFU is very interest. In a word, let me think about more, see if I could make this PR in a good shape, fix both direct I/O evicts and the eviction policy. |
dbuf_cache_shift is added and updated since 2016 by commit d3c2ae1, e71cade and de4f8d5. It is well designed, however, in direct I/O or cache-off I/O, arc may not grow, arc target size is not growing too. Then, in high IOPS, dbuf cache will in a busy evicting cycle. To ensure, dbuf not starving, an extra is added while arc current size is small or dbuf cache dominates arc size. With this extra increased in arc recap thread, and downgrade too based "trimming" signal of dbuf stats, high IOPS read should be good. Signed-off-by: tiehexue <tiehexue@hotmail.com>
This new module parameter is quite different than dbuf_cache_max_bytes which looks not that in use now, because it has a default value at UINT64_MAX, and historically, the dbuf target size is bound by dbuf_cache_shift related to arc target size. This commit and the previous one added an "extra" bytes to dbuf cache to support direct I/O or cache-off I/O. Now, the extra is bound by dbuf_cache_extra_max_shift related to arc max size. Default is 2, that means, a quarter of arc max can be used by dbuf cache. Signed-off-by: tiehexue <tiehexue@hotmail.com>
This extra value will make the dbuf cache evit thread remove all items until low water meets. If extra is already "big", that will cause a load spike. Signed-off-by: tiehexue <tiehexue@hotmail.com>
And also a ceiling "min" to ensure no above max_extra in any way. Signed-off-by: tiehexue <tiehexue@hotmail.com>
51f6164 to
05c5fdc
Compare
github-actions
Bot
removed
the
Status: Revision Needed
As I have told, if nothing else, ARC is scattered and not mapped into KVA. If you try to use 95% of RAM, as ARC can do, with dbuf cache of buffers of different sizes, you'll end up in KVA fragmentation and exhaustion pretty soon. And you still have ARC there with a duplicate copy of the data.
I had no time to look what that test does. Sure I can write dozen of different tests, defeating one or another aspect of ZFS. The question is how relevant is that to real life.
I'll repeat once more, this patch gives dbuf cache all memory that it asks. So obviously when its size reaches test's active dataset size everything stabilizes. But this is one specific tests. The world is much more complicated. |
|
@amotin thanks, I guess I missed many times as you said about KVA and its fragmentation. So memory are not created equal, dbuf backed by KVA, while ARC has linear or scatter ABD, though are from Linux kernel. And KVA allocate and deallocate not quite same, deallocation may not be easy once some of its pages are referenced. I would go through all background more until I could catch up. |
|
I'm going to take a crack at "fixing" dbuf_cache along a couple points.
|
#19118 is a try. |
4 participants
Motivation and Context
While working on #18902 and #19076 , how to manage dbuf cache size is discussed, and this PR does the work.
As @mkhllr found that in direct I/O or cache-off I/O, arc target size may not increase, and dbuf_cache_shift even set to 0, the max dbuf cache is align to arc target size. However, we should argue, to maximize IOPS in direct I/O, especially read, the dbuf that contains L1 indirect blocks should dominate arc sizing, and chase to arc_c_max if necessary. This PR introduce a new dbuf_cache_extra variable to make the dbuf target increasing as needed.
The whole change is "off" I/O path, so, there should not be performance regression.
Description
dbuf_cache_shift is added and updated since 2016 by commit d3c2ae1, e71cade and de4f8d5. It is well designed, however, in direct I/O or cache-off I/O, arc may not grow, arc target size is not growing too. Then, in high IOPS, dbuf cache will be in a busy evicting cycle. To ensure dbuf not starving, an extra is added while arc current size is small or dbuf cache dominates arc size. With this extra increased in arc recap thread, and downgrade too based "trimming" signal of dbuf stats.
I tried a lot of ways, include breaking the link between dbuf and arc, however this would introduce a lot more code change for accounting etc. Also, for normal arc I/O path, the binding of dbuf cache and arc target is still a very sound design. The new dbuf_cache_extra just make things work and would decay after a direct I/O peek streaming.
In a word, this PR is very similar like de4f8d5, but accounting for direct I/O.
I copied related commit message below for reviewers:
How Has This Been Tested?
A new test is added, and CI forks.
Types of Changes
Checklist
Signed-off-by.