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

[Carlos Maiolino <cmaiolino@redhat.com>]

On Tue, Oct 30, 2012 at 11:20:54AM +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>
> ---
>  fs/xfs/xfs_buf_item.c |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index a8d0ed9..b72fe88 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -526,7 +526,23 @@ xfs_buf_item_unpin(
>  		}
>  		xfs_buf_relse(bp);
>  	} else if (freed && remove) {
> +		/*
> +		 * There are currently two references to the buffer - the active
> +		 * LRU reference and the buf log item. What we are about to do
> +		 * here - simulate a failed IO completion - requires 3
> +		 * references.
> +		 *
> +		 * The LRU reference is removed by the xfs_buf_stale() call. The
> +		 * buf item reference is removed by the xfs_buf_iodone()
> +		 * callback that is run by xfs_buf_do_callbacks() during ioend
> +		 * processing (via the bp->b_iodone callback), and then finally
> +		 * the ioend processing drops the IO reference.
> +		 *
> +		 * Hence we need to take an additional reference here so that IO
> +		 * completion processing doesn't free the buffer prematurely.
> +		 */
>  		xfs_buf_lock(bp);
> +		xfs_buf_hold(bp);
>  		xfs_buf_ioerror(bp, EIO);
>  		XFS_BUF_UNDONE(bp);
>  		xfs_buf_stale(bp);
> -- 
> 1.7.10
> 
Looks good

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
-- 
--Carlos

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