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 > > *illumos <https://illumos.topicbox.com/latest>* / illumos-developer / see > discussions <https://illumos.topicbox.com/groups/developer> + participants > <https://illumos.topicbox.com/groups/developer/members> + delivery options > <https://illumos.topicbox.com/groups/developer/subscription> Permalink > <https://illumos.topicbox.com/groups/developer/T6b2060bf39b5f383-M8297cc833b4712fc16a3a50c> > ------------------------------------------ openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/Tce91d507e14ab14a-M215d0c884e69739b3b89705d Delivery options: https://openzfs.topicbox.com/groups/developer/subscription
