xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: fix buffer shudown reference count mismatch
From: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
Date: Tue, 30 Oct 2012 18:03:33 -0200
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1351556454-29723-1-git-send-email-david@xxxxxxxxxxxxx>
References: <1351556454-29723-1-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Oct 30, 2012 at 11:20:54AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxx>
-- 
--Carlos

<Prev in Thread] Current Thread [Next in Thread>