Thanks Jerry, great stuff! I'll jump on it in the coming days. Cheers, -- Saso
On 10/22/18 7:30 PM, Jerry Jelinek 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] > <mailto:[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 > <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] <mailto:[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-M62644b8a2f16e28ce72e020a> > ------------------------------------------ openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/Tce91d507e14ab14a-Meee31df140c26615471658b5 Delivery options: https://openzfs.topicbox.com/groups/developer/subscription
