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@xxxxxxxxxxxxx
