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.
