Re: [PATCH 1/4] xfs: buffer pins need to hold a buffer reference

[Dave Chinner <david@fromorbit.com>]

On Wed, May 17, 2023 at 05:58:55AM -0700, Christoph Hellwig wrote:
> On Wed, May 17, 2023 at 10:04:46AM +1000, Dave Chinner wrote:
> > To fix this, we need to ensure that buffer existence extends beyond
> > the BLI reference count checks and until the unpin processing is
> > complete. This implies that a buffer pin operation must also take a
> > buffer reference to ensure that the buffer cannot be freed until the
> > buffer unpin processing is complete.
> 
> Yeah.  I wonder why we haven't done this from the very beginning..

Likely because the whole BLI lifecycle is completely screwed up and
nobody has had the time to understand it fully and fix it properly.

> > +	 /*
> > +	  * Nothing to do but drop the buffer pin reference if the BLI is
> > +	  * still active
> > +	  */
> 
> Nit: this block comment is indentented by an extra space.
> 
> > +	if (!freed) {
> > +		xfs_buf_rele(bp);
> >  		return;
> > +	}
> >  
> >  	if (stale) {
> 
> Nit: this is the only use of the stale variable now, so we might
> as well just drop it.

Actually, after we've dropped the bli reference, it isn't safe to
reference the bli unless we know the buffer is stale. In this case,
we know the bli still exists because the buffer has been locked
since it was marked stale. However, for the other cases the BLI
could be freed from under us as it's reference count is zero and so
the next call to xfs_buf_item_relse() will free it no matter where
it comes from.

The reference counting around BLIs a total mess - this patch just
gets rid of one landmine but there's still plenty more in this code
that need to be untangled.

> >  		ASSERT(bip->bli_flags & XFS_BLI_STALE);
> 
> .. which then also clearly shows this ASSERT is pointless now.

*nod*

> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks.

-Dave.
-- 
Dave Chinner
david@fromorbit.com