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

[Christoph Hellwig <hch@lst.de>]

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 :)


> > +	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.