Review - 16203 zfs: switch refcount tracking from lists to AVL-trees

Review - 16203 zfs: switch refcount tracking from lists to AVL-trees

[Andy Fiddaman]

Following on from 16201 which fixed a bug in ZFS reference tracking, this is
a followup from OpenZFS which makes that reference tracking more usable by
significantly reducing the overhead when it's enabled. I've also found and
fixed another reference tracking panic and am now able to run the entire
ZFS testsuite on a system with reference tracking enabled without a panic
and with only around a 5% increase in total run time.

Please can you review the following change? I have put references to the
corresponding OpenZFS commits in 16203. I have not been able to take these
verbatim as they remove some things required for mdb's ::zfs_refcount
which I thought was important to preserve.

    16203 zfs: switch refcount tracking from lists to AVL-trees
    16424 libavl should expose avl_update avl_update_lt avl_update_gt
    16436 zfs reference tracking panic during zfs_receive_raw_incremental test
    https://www.illumos.org/issues/16203
    https://www.illumos.org/issues/16424
    https://www.illumos.org/issues/16436
    https://code.illumos.org/c/illumos-gate/+/3396

Thanks,

Andy

Re: [developer] Review - 16203 zfs: switch refcount tracking from lists to AVL-trees

[Toomas Soome]

> On 2. Apr 2024, at 14:38, Andy Fiddaman <illumos@fiddaman.net> wrote:
> 
> Following on from 16201 which fixed a bug in ZFS reference tracking, this is
> a followup from OpenZFS which makes that reference tracking more usable by
> significantly reducing the overhead when it's enabled. I've also found and
> fixed another reference tracking panic and am now able to run the entire
> ZFS testsuite on a system with reference tracking enabled without a panic
> and with only around a 5% increase in total run time.
> 
> Please can you review the following change? I have put references to the
> corresponding OpenZFS commits in 16203. I have not been able to take these
> verbatim as they remove some things required for mdb's ::zfs_refcount
> which I thought was important to preserve.
> 
>    16203 zfs: switch refcount tracking from lists to AVL-trees
>    16424 libavl should expose avl_update avl_update_lt avl_update_gt
>    16436 zfs reference tracking panic during zfs_receive_raw_incremental test
>    https://www.illumos.org/issues/16203
>    https://www.illumos.org/issues/16424
>    https://www.illumos.org/issues/16436
>    https://code.illumos.org/c/illumos-gate/+/3396
> 
> Thanks,
> 
> Andy


Um. I’m now a bit grumpy. I really do dislike the approach to squash changes into one - it is very annoying to track those changes later and, more importantly, it makes reviewers work harder too.

IMO we should follow the approach which is commonly used - one change, one commit. And it is not like we are producing hundreds of commits per day and core team has problem with processing the number of commits….

rgds,
toomas

Re: [developer] Review - 16203 zfs: switch refcount tracking from lists to AVL-trees

[Andy Fiddaman]

On Tue, 2 Apr 2024, Toomas Soome via illumos-developer wrote:

>
>
> > On 2. Apr 2024, at 14:38, Andy Fiddaman <illumos@fiddaman.net> wrote:
> >
> > Following on from 16201 which fixed a bug in ZFS reference tracking, this is
> > a followup from OpenZFS which makes that reference tracking more usable by
> > significantly reducing the overhead when it's enabled. I've also found and
> > fixed another reference tracking panic and am now able to run the entire
> > ZFS testsuite on a system with reference tracking enabled without a panic
> > and with only around a 5% increase in total run time.
> >
> > Please can you review the following change? I have put references to the
> > corresponding OpenZFS commits in 16203. I have not been able to take these
> > verbatim as they remove some things required for mdb's ::zfs_refcount
> > which I thought was important to preserve.
> >
> >    16203 zfs: switch refcount tracking from lists to AVL-trees
> >    16424 libavl should expose avl_update avl_update_lt avl_update_gt
> >    16436 zfs reference tracking panic during zfs_receive_raw_incremental test
> >    https://www.illumos.org/issues/16203
> >    https://www.illumos.org/issues/16424
> >    https://www.illumos.org/issues/16436
> >    https://code.illumos.org/c/illumos-gate/+/3396
> >
> > Thanks,
> >
> > Andy
>
>
> Um. I?m now a bit grumpy. I really do dislike the approach to squash changes into one - it is very annoying to track those changes later and, more importantly, it makes reviewers work harder too.

They were submitted together because they've been developed and tested
together, and as per the developers' guide, the unit of commit should be the
unit of testing. The new AVL-tree code is the consumer that requires the
userland exposure of the symbols from libavl, and the reference tracking panic
is a bug encountered during testing that prevented me from running the
testsuite for the AVL-tree change.

This is how I got here, but I do also prefer things to be split into separate
commits where possible and, now that it is complete, I can see that this is a
change that I can go back and break up fairly easily. It will mean I have to
spend more time testing but I will go ahead and do that.

> IMO we should follow the approach which is commonly used - one change, one commit.

That's not the definition that the project uses though. In particular, there
are guidelines around the unit of testing and around introducing changes which
have no consumers that have to be taken into account.

> And it is not like we are producing hundreds of commits per day and core team has problem with processing the number of commits?.

>
> rgds,
> toomas
> ------------------------------------------
> illumos: illumos-developer
> Permalink: https://illumos.topicbox.com/groups/developer/Taae98ed43abedaf6-Mf32e69fb0396cf095f61c00b
> Delivery options: https://illumos.topicbox.com/groups/developer/subscription
>

Re: [developer] Review - 16203 zfs: switch refcount tracking from lists to AVL-trees

[Joshua M. Clulow]

On Tue, 2 Apr 2024 at 07:11, Andy Fiddaman <illumos@fiddaman.net> wrote:
> On Tue, 2 Apr 2024, Toomas Soome via illumos-developer wrote:
> > Um. I?m now a bit grumpy. I really do dislike the approach to squash changes into one - it is very annoying to track those changes later and, more importantly, it makes reviewers work harder too.
> They were submitted together because they've been developed and tested
> together, and as per the developers' guide, the unit of commit should be the
> unit of testing. The new AVL-tree code is the consumer that requires the
> userland exposure of the symbols from libavl, and the reference tracking panic
> is a bug encountered during testing that prevented me from running the
> testsuite for the AVL-tree change.

Yes, that's how we do things here.  Small changes are a fine thing to
aim for, but they are not the most important thing!  With each commit
we're moving the software from one known good, tested state to
another.  Commits are also the unit over which we back things out when
they induce problems, and the unit over which people generally bisect
when they're trying to locate a regression.

Review under Gerrit is pretty good even for larger commits: you can
comment well outside the patch hunk, unlike with some other systems;
there is first class tracking for which files you've already reviewed;
the system makes it pretty clear what's changed between versions of a
patch you're looking at.  People seeking review, especially on a
larger change, are always encouraged to leave their own initial
comments on their review in order to guide reviewers on a particular
journey through the change.

I will note also in general that slicing changes up into microscopic
commits can sometimes actually _hide_ overall issues from reviewers
who don't end up seeing the whole picture.  It's alright for larger
reviews to take longer than smaller reviews: they have more lines,
after all!  As always, it's a set of trade-offs and there are not any
total hard rules.

> > IMO we should follow the approach which is commonly used - one change, one commit.
> That's not the definition that the project uses though. In particular, there
> are guidelines around the unit of testing and around introducing changes which
> have no consumers that have to be taken into account.

That is correct.


Cheers.

-- 
Joshua M. Clulow
http://blog.sysmgr.org