Re: [PATCH 14/15] xfs: move b_li_list based retry handling to common code

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

On Tue, Jan 07, 2025 at 08:03:22AM +0100, Christoph Hellwig wrote:
> On Mon, Jan 06, 2025 at 10:55:47PM -0800, Darrick J. Wong wrote:
> > On Mon, Jan 06, 2025 at 10:54:51AM +0100, Christoph Hellwig wrote:
> > > The dquot and inode version are very similar, which is expected given the
> > > overall b_li_list logic.  The differences are that the inode version also
> > > clears the XFS_LI_FLUSHING which is defined in common but only ever set
> > > by the inode item, and that the dquot version takes the ail_lock over
> > > the list iteration.  While this seems sensible given that additions and
> > > removals from b_li_list are protected by the ail_lock, log items are
> > > only added before buffer submission, and are only removed when completing
> > > the buffer, so nothing can change the list when retrying a buffer.
> > 
> > Heh, I think that's not quite true -- I think xfs_dquot_detach_buf
> > actually has a bug where it needs to take the buffer lock before
> > detaching the dquot from the b_li_list.  And I think kfence just whacked
> > me for that on tonight's fstests run.
> 
> Ooops :)

...and I think this is now in -rc7 so no worries here.

> > > +	list_for_each_entry(lip, &bp->b_li_list, li_bio_list) {
> > > +		set_bit(XFS_LI_FAILED, &lip->li_flags);
> > > +		clear_bit(XFS_LI_FLUSHING, &lip->li_flags);
> > 
> > Should dquot log items be setting XFS_LI_FLUSHING?
> 
> That would help to avoid roundtrips into ->iop_push and thus a
> dqlock (try)lock roundtrip for them.  So it would be nice to have,
> but it's not functionally needed.

<nod> Sounds like a reasonable cleanup for someone.

For this change,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D