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@xxxxxxxxxx>
--D
