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-M62644b8a2f16e28ce72e020a
Delivery options: https://openzfs.topicbox.com/groups/developer/subscription
