Matt, Thanks for posting your fix on PR 732. I'll read through it today. I think the only way for us to know for sure if it will actually fix our problem is for us to test the fix on one of our problematic production systems. Getting the logistics of that worked out would take a while.
The change I posted here is to simply make metaslab_unload_delay be self-tuning. If you think that is still worthwhile in spite of the changes for PR 732, I can work on the changes you suggested. I've put my responses to your two questions in-line below. Thanks, Jerry On Sat, Jan 12, 2019 at 11:33 PM Matthew Ahrens <[email protected]> wrote: > From skimming your bug report, the issue you're encountering is consistent > with one that we also hit and have a fix for. The performance impact you > described is very similar to the issue we encountered. However, our > analysis of the code is slightly different from yours. I've just posted > our code here <https://github.com/openzfs/openzfs/pull/732>, but the > quick fix is that in metaslab_group_alloc_normal() we need to set > ms_selected_txg before calling metaslab_activate(). Here is our analysis: > > > Summary: concurrent small allocation defeats large allocation > > If metaslab_group_alloc_normal()'s call to metaslab_activate() fails, it > will skip this metaslab, not trying to allocate from it, and also not > setting ms_selected_txg. The overall impact is a write performance impact, > which is most noticeable on fragmented pools with many concurrent > synchronous (ZIL) writes, or otherwise has some large and some small > allocations. > > metaslab_activate will fail if, while we are in metaslab_load() with the > ms_lock dropped (which can be around 1 second on fragmented metaslabs, even > if the spacemap is cached in the ARC), another zio (with the same > allocator) succeeds in allocating from this metaslab and thus setting > mg_primaries[]. Then when this allocation gets to > metaslab_activate_allocator(), it will see that another metaslab has > already been activated (i.e. there is already a primary metaslab selected > for this vdev and allocator), and fail. > > This can typically happen if the first (victim) allocation is large (e.g. > for a ZIL block) and the concurrent allocation is small. The smaller > allocation may be able to find enough contiguous space in a better (higher > weight) metaslab, thus activating it. > > The impact of not setting ms_selected_txg is that this metaslab may be > unloaded at the end of the txg. So we may enter a cycle where every txg, > we load (via metaslab_activate()) and then unload (via > metaslab_sync_done()) some metaslabs, wasting a lot of time. Loading can > take ~1 second to construct the range_tree, even when the spacemap is > cached in the ARC and no I/O is required. > > > The impact of skipping this metaslab is that we will go on to a worse > metaslab (goodness measured by the metaslab weight). We may hit this race > again and skip the next metaslab, ending up loading (~1 second each) and > then skipping many metaslabs before we are able to complete the > allocation. This can cause us to load (costing time) and keep loaded > (costing memory) many more metaslabs than are actually required to satisfy > the allocations. (Or, instead of keeping them loaded, we may not keep them > loaded if no other zio's set ms_selected_txg, in which case we may reload > them the next txt, costing even more time.) > > This problem was introduced by 9112 Improve allocation performance on > high-end systems. > > > The fix is that if metaslab_activate() fails, > metaslab_group_alloc_normal() needs to still try to allocate from this > metaslab, and set ms_selected_txg. > > > --------------- > > > I'm hoping that our fix makes sense to you and addresses your issue as > well, but here are a few minor suggestions on your code as well: > > > line 2750: shouldn't ms_loaded_txg always be set, since ms_loaded is set? > I think there is a code path which can load a metaslab without having an associated txg. This is in vdev_initialize_ms_load which can be called by the vdev_initialize_thread. I think it's more reliable to always check that the ms_loaded_txg is set here. > Can we set ms_loaded_txg from within metaslab_load() rather than having > the callers do it? > Originally I was thinking that we would only set this when we knew the txg, which is related to your previous question. However, now I think it is cleaner to set it in metaslab_load and I could simply pass 0 for the call path which doesn't have an associated txg. > > On Fri, Jan 11, 2019 at 9:05 AM Jerry Jelinek <[email protected]> > wrote: > >> This is a fix for a performance problem we've hit at Joyent. >> >> https://us-east.manta.joyent.com/jjelinek/public/webrevs/10221/index.html >> >> Thanks, >> Jerry >> >> *openzfs <https://openzfs.topicbox.com/latest>* / openzfs-developer / > see discussions <https://openzfs.topicbox.com/groups/developer> + > participants <https://openzfs.topicbox.com/groups/developer/members> + > delivery > options <https://openzfs.topicbox.com/groups/developer/subscription> > Permalink > <https://openzfs.topicbox.com/groups/developer/Te98a45cd38e36a58-M8af1d887034082ec793107b3> > ------------------------------------------ openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/Te98a45cd38e36a58-M66e916706b9f9c6aa61baa14 Delivery options: https://openzfs.topicbox.com/groups/developer/subscription
