Re: [PATCH 2/2] xfs: remove bli from AIL before release on transaction abort

[Carlos Maiolino <cmaiolino@redhat.com>]

On Tue, Jun 06, 2017 at 08:08:50AM -0400, Brian Foster wrote:
> When a buffer is modified, logged and committed, it ultimately ends
> up sitting on the AIL with a dirty bli waiting for metadata
> writeback. If another transaction locks and invalidates the buffer
> (freeing an inode chunk, for example) in the meantime, the bli is
> flagged as stale, the dirty state is cleared and the bli remains in
> the AIL.
> 

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


> If a shutdown occurs before the transaction that has invalidated the
> buffer is committed, the transaction is ultimately aborted. The log
> items are flagged as such and ->iop_unlock() handles the aborted
> items. Because the bli is clean (due to the invalidation),
> ->iop_unlock() unconditionally releases it. The log item may still
> reside in the AIL, however, which means the I/O completion handler
> may still run and attempt to access it. This results in assert
> failure due to the release of the bli while still present in the AIL
> and a subsequent NULL dereference and panic in the buffer I/O
> completion handling. This can be reproduced by running generic/388
> in repetition.
> 
> To avoid this problem, update xfs_buf_item_unlock() to first check
> whether the bli is aborted and if so, remove it from the AIL before
> it is released. This ensures that the bli is no longer accessed
> during the shutdown sequence after it has been freed.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_buf_item.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 0306168..f6a8422 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -636,20 +636,23 @@ xfs_buf_item_unlock(
>  
>  	/*
>  	 * Clean buffers, by definition, cannot be in the AIL. However, aborted
> -	 * buffers may be dirty and hence in the AIL. Therefore if we are
> -	 * aborting a buffer and we've just taken the last refernce away, we
> -	 * have to check if it is in the AIL before freeing it. We need to free
> -	 * it in this case, because an aborted transaction has already shut the
> -	 * filesystem down and this is the last chance we will have to do so.
> +	 * buffers may be in the AIL regardless of dirty state. An aborted
> +	 * transaction that invalidates a buffer already in the AIL may have
> +	 * marked it stale and cleared the dirty state, for example.
> +	 *
> +	 * Therefore if we are aborting a buffer and we've just taken the last
> +	 * reference away, we have to check if it is in the AIL before freeing
> +	 * it. We need to free it in this case, because an aborted transaction
> +	 * has already shut the filesystem down and this is the last chance we
> +	 * will have to do so.
>  	 */
>  	if (atomic_dec_and_test(&bip->bli_refcount)) {
> -		if (clean)
> -			xfs_buf_item_relse(bp);
> -		else if (aborted) {
> +		if (aborted) {
>  			ASSERT(XFS_FORCED_SHUTDOWN(lip->li_mountp));
>  			xfs_trans_ail_remove(lip, SHUTDOWN_LOG_IO_ERROR);
>  			xfs_buf_item_relse(bp);
> -		}
> +		} else if (clean)
> +			xfs_buf_item_relse(bp);
>  	}
>  
>  	if (!(flags & XFS_BLI_HOLD))
> -- 
> 2.7.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Carlos