Re: [PATCH 1/2] xfs: aborting inodes on shutdown may need buffer lock

Dave Chinner
2022-03-21T22:48:53Z

On Mon, Mar 21, 2022 at 02:56:20PM -0700, Darrick J. Wong wrote:
> On Mon, Mar 21, 2022 at 12:23:28PM +1100, Dave Chinner wrote:
> > +
> > + /*
> > + * Capture the associated buffer and lock it if the caller didn't
> > + * pass us the locked buffer to begin with.
> > + */
> > + spin_lock(&iip->ili_lock);
> > + bp = iip->ili_item.li_buf;
> > + xfs_iflush_abort_clean(iip);
> > + spin_unlock(&iip->ili_lock);
>
> Is the comment here incorrect? The _shutdown_abort variant will go
> ahead and lock the buffer, but this function does not do that...?

Ah, stale comment from before I refactored it into separate
functions. I'll clean it up....

> > - xfs_iflags_clear(ip, XFS_IFLUSHING);
> > - if (bp)
> > - xfs_buf_rele(bp);
> > +
> > + /*
> > + * Got two references to bp. The first will get droped by
>
> "The first will get dropped by..." (spelling and stgit nagging me about
> trailing whitespace)
>
> > + * xfs_iflush_abort() when the item is removed from the buffer list, but
> > + * we can't drop our reference until _abort() returns because we have to
> > + * unlock the buffer as well. Hence we abort and then unlock and release
> > + * our reference to the buffer.
>
> ...and presumably xfs_iflush_abort will drop the other bp reference at
> some point after where we unlocked the inode item, locked the (held)
> buffer, and relocked the inode item?

Yes, xfs_iflush_abort() will drop the other buffer reference when it
removes the inode from the buffer item list.

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com