I was hoping we might have a chance to discuss the trim work in yesterday's OpenZFS meeting, but we never got to it.
It's coming up on two months since I posted my last update here, but there has been no activity on it. I know everyone is busy, but I wanted to check in to see if there is still interest or if there are any other issues that I can try to address. Thanks, Jerry On Mon, Oct 22, 2018 at 11:30 AM Jerry Jelinek <[email protected]> wrote: > I have merged all of the patches from last years openzfs pull request and > ported that code to the latest version of zfs. There were two significant > bugs I found after I merged in the patches. > > The first bug was an intermittent infinite loop > in metaslab_group_alloc_normal when a txg was being synced out. I tracked > this down to the incorrect sorting in metaslab_exec_trim. The fix was to > ensure that the weight includes the type, as is already done when we > re-sort after a txg sync. > > The second bug was a blown assert in metaslab_group_alloc_normal on a > DEBUG build. I removed the invalid assertion in > metaslab_group_alloc_normal at end of the for loop (line 3350 in the > original review): > ASSERT(!metaslab_should_allocate(msp, asize)); > The assertion is obviously invalid because the check at line 3273 can > return false and we then goto next, which will trip the assertion. It > appears that a manual trim could cause this scenario from time to time. > > I have also made various simple changes based on the code review feedback > I already received on the first round of review I posted. There is an > updated code review at: > > ttps://cr.joyent.us/#/c/4929/ <https://cr.joyent.us/#/c/4929/> > > For people unfamiliar with gerrit, you can see what was changed from the > first round by using the "Diff against:" button to select the latest rev. > > Feel free to post new code review comments there or email them to me > directly. > > Thanks, > Jerry > > > On Wed, Oct 10, 2018 at 11:05 AM, Matthew Ahrens <[email protected]> > wrote: > >> Thanks for pushing this forward, Jerry. I have a few very high level >> questions / points: >> >> It looks like the changes that Saso made in response to the code review >> feedback that I gave in April-May 2017 are not part of your proposed >> changes. I'd like to see those included. >> https://github.com/openzfs/openzfs/pull/172/commits >> >> Do you have any performance results to share, even preliminary? A lot of >> the complexity of this change is due to performance concerns (e.g. >> consolidating trim segments across multiple TXG's). It would be good to >> verify that this complexity is worth it. >> >> --matt >> >> On Wed, Oct 10, 2018 at 9:27 AM Jerry Jelinek <[email protected]> >> wrote: >> >>> I have taken the Nexenta TRIM work that was originally out for code >>> review last year and updated it to work with the latest version of ZFS. The >>> code passes the new trim tests and I have done some additional manual >>> testing with DTrace to verify that the trim requests pass down into the >>> scsi driver and that the proper unmapping is taking place. We have not run >>> this in production yet. I currently have a code review up at: >>> >>> https://cr.joyent.us/#/c/4929/ >>> >>> Anyone with a github account should be able to set themselves up to >>> comment on this CR, or you can just send me feedback in email. I know this >>> is a large review, but it has already gotten a lot of feedback last year >>> when Nexenta put it out, and hopefully people will be able to take another >>> look so we can get this integrated. >>> >>> The following is a summary of the changes I made from the original 2017 >>> Nexenta work to bring it up to date with the latest ZFS code. >>> >>> The names of many of the AVL trees were changed with commit 86714001. >>> Here is the mapping between what was in the Nexenta code and what the new >>> names are: >>> ms_tree -> ms_allocatable >>> ms_freeingtree -> ms_freeing >>> ms_freedtree -> ms_freed >>> ms_defertree -> ms_defer >>> ms_size_tree -> ms_allocatable_by_size >>> >>> usr/src/uts/common/fs/zfs/metaslab.c >>> range_tree_create() calls had the wrong number of args, lock parameter >>> no longer used. >>> >>> metaslab_trim_remove changes; in orig diff it was called in: >>> metaslab_load range_tree_walk defertree name changed >>> to defer >>> metaslab_block_alloc ok >>> metaslab_claim_dva this has heavy modifications. now >>> -> metaslab_claim_impl -> >>> metaslab_claim_concrete >>> I added the metaslab_trim_remove in >>> metaslab_claim_concrete >>> >>> In orig patch, had spa_get_auto_trim() and metaslab_trim_add() calls in >>> metaslab_free_dva() >>> metaslab_free_dva has heavy modifications; now the call path looks >>> like: >>> -> metaslab_free_impl -> metaslab_free_concrete >>> >>> I don't think the spa_get_auto_trim() and metaslab_trim_add() should be >>> in metaslab_free_concrete() though, due to new checkpoint handling. >>> >>> In the Nexenta patch, metaslab_free calls metaslab_free_dva but in >>> metaslab_free for the current code, it calls metaslab_unalloc_dva for the >>> "now" case - this matches the "now" case for the patched >>> metaslab_trim_add() code. metaslab_unalloc_dva doesn't exist in the patch, >>> but this looks like the right place to add the spa_get_auto_trim() and >>> metaslab_trim_add() at end. >>> >>> In metaslab_check_free(), I did not include the debugging code which >>> depended on the debug changes we omitted from the driver changes we've >>> already committed. >>> >>> usr/src/uts/common/fs/zfs/range_tree.c >>> 332 Removed invalid assert. >>> >>> usr/src/uts/common/fs/zfs/spa.c >>> spa_load_impl - changes here added to spa_ld_get_props instead >>> spa_load_impl - added 3rd arg to spa_dir_prop calls >>> Fixed spa_event_notify parameters throughout. >>> >>> usr/src/uts/common/fs/zfs/spa_misc.c >>> Some minor change, but nothing substantial. >>> >>> usr/src/uts/common/fs/zfs/vdev_raidz.c >>> Fixed up vdev_raidz_map_alloc. >>> Added correct ops initialization. >>> In vdev_raidz_map_free(), fixed code to use the new abd_free() behavior >>> which was added in commit 4ee0199ec (due to backout and reimpl). I also >>> removed the size variable which is no longer used, but wasn't cleaned up >>> from this commit. >>> >>> usr/src/uts/common/fs/zfs/vdev_indirect.c >>> Was not included in Nexenta patch. Added correct ops initialization. >>> >>> usr/src/uts/common/fs/zfs/zio.c >>> Removed dfl_ck_func related debug code. >>> >>> usr/src/lib/libzpool >>> Fixed Makefile.com >>> KERNEL_OBJS was including taskq.o which didn't build right in user-land, >>> but was unnecessary so removed it. >>> >>> zpool.1m >>> Fixed all of the new format errors highlighted during the build. >>> >>> 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/Tce91d507e14ab14a-M215d0c884e69739b3b89705d> >> > > ------------------------------------------ openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/Tce91d507e14ab14a-Mc4c1e3dda43c6d8339614758 Delivery options: https://openzfs.topicbox.com/groups/developer/subscription
