Re: [PATCH 3/4] xfs: set the buffer type after holding the AG[IF] across trans_roll

[Dave Chinner <david@fromorbit.com>]

On Sun, Oct 02, 2022 at 11:19:48AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Currently, the only way to lock an allocation group is to hold the AGI
> and AGF buffers.  If repair needs to roll the transaction while
> repairing some AG metadata, it maintains that lock by holding the two
> buffers across the transaction roll and joins them afterwards.
> 
> However, repair is not the same as the other parts of XFS that employ
> this bhold/bjoin sequence, because it's possible that the AGI or AGF
> buffers are not actually dirty before the roll.  In this case, the
> buffer log item can detach from the buffer, which means that we have to

Doesn't this imply we have a reference counting problem with
XFS_BLI_HOLD buffers? i.e. the bli can only get detached when the
reference count on it goes to zero. If the buffer is clean and
joined to a transaction, then that means the only reference to the
BLI is the current transaction. Hence the only way it can get
detached is for the transaction commit to release the current
transaction's reference to the BLI.

Ah, XFS_BLI_HOLD does not take a reference to the BLI - it just
prevents ->iop_release from releasing the -buffer- after it drops
the transaction reference to the BLI. That's the problem right there
- xfs_buf_item_release() drops the current trans ref to the clean
item via xfs_buf_item_release() regardless of whether BLI_HOLD is
set or not, hence freeing the BLI on clean buffers.

IOWs, it looks to me like XFS_BLI_HOLD should actually hold a
reference to the BLI as well as the buffer so that we don't free the
BLI for a held clean buffer in xfs_buf_item_release(). The reference
we leave behind will then be picked up by the subsequent call to
xfs_trans_bjoin() which finds the clean BLI already attached to the
buffer...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com