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

[Dave Chinner <david@fromorbit.com>]

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