Re: [PATCH 2/3] xfs: don't hold xattr leaf buffers across transaction rolls

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Jun 27, 2022 at 11:23:55AM +1000, Dave Chinner wrote:
> On Sun, Jun 26, 2022 at 03:03:58PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Now that we've established (again!) that empty xattr leaf buffers are
> > ok, we no longer need to bhold them to transactions when we're creating
> > new leaf blocks.  Get rid of the entire mechanism, which should simplify
> > the xattr code quite a bit.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> 
> Why?

The original justification for using bhold here was to prevent the AIL
from trying to write the empty leaf block into the fs during the brief
time that we release the buffer lock.  The reason for /that/ was to
prevent recovery from tripping over the empty ondisk block.

The bhold didn't totally solve that problem (hence removing the !count
check entirely 3 years later) but it /has/ made things sufficiently more
complicated that the resident expert (allison) and two maintainers (you
and I) tripped over the bheld leaf buffer handling...

> This code isn't there for correctness - it's just a way of
> avoiding needing to look up and lock the buffer immediately after we
> just created it and had a reference to it. This is a valid use of 
> xfs_trans_bhold(), so I'm not convinced that removing it makes the
> code better. Simpler, yes, but not necessarily better.

...so while I agree that this is a valid use of bhold, I also see that
the three people you'd most expect to wield the extra complexity have
not done it well, and decided to simplify the buffer usage back to a
more common use case in xfs.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com