Re: [PATCH 5/6] xfs: fix buffer shudown reference count mismatch

[Dave Chinner <david@fromorbit.com>]

On Fri, Nov 02, 2012 at 11:38:43AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> When we shut down the filesystem, we have to unpin and free all the
> buffers currently active in the CIL. To do this we unpin and remove
> them in one operation as a result of a failed iclogbuf write. For
> buffers, we do this removal via a simultated IO completion of after
> marking the buffer stale.
> 
> At the time we do this, we have two references to the buffer - the
> active LRU reference and the buf log item.  The LRU reference is
> removed by marking the buffer stale, and the active CIL reference is
> by the xfs_buf_iodone() callback that is run by
> xfs_buf_do_callbacks() during ioend processing (via the bp->b_iodone
> callback).
> 
> However, ioend processing requires one more reference - that of the
> IO that it is completing. We don't have this reference, so we free
> the buffer prematurely and use it after it is freed. This leads to
> assert failures in xfs_buf_rele() on debug kernels because the
> b_hold count is zero.
> 
> Fix this by making sure we take the necessary IO reference before
> starting IO completion processing on the stale buffer.
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

HOld off on this one for a little while - I think I've found the
source of the subsequent failures that Mark reported.

The reason why this did't fail on every buffer is that not all
buffers drop a reference in xfs_buf_iodone_work(). Only buffers that
are marked with XBF_ASYNC will do this, and they seem to be rare.
Hence the rare failure.

The fix that I've done here means all buffers going through this
path will take an extra reference, but that reference is only
dropped on async buffers. Because all the buffers are markd stale,
they are removed from the LRU, and so xfs_buftarg_wait() during
unmount does not find them and hence the remaining reference is
never removed. Hence the perag reference still remains, and we
assert fail there.

Solution seems simple - set the XBF_ASYNC flag on all buffers so
that the last reference is taken away correctly. Testing that now.

(FWIW, I found this using perag get/put tracing :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs