Re: [PATCH 2/2] xfs: shutdown after buf release in iflush cluster abort path

[Allison Henderson <allison.henderson@oracle.com>]

Ok, thanks again for the explanation!  You can add my review:

Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

On 3/29/19 6:40 AM, Brian Foster wrote:
> If xfs_iflush_cluster() fails due to corruption, the error path
> issues a shutdown and simulates an I/O completion to release the
> buffer. This code has a couple small problems. First, the shutdown
> sequence can issue a synchronous log force, which is unsafe to do
> with buffer locks held. Second, the simulated I/O completion does not
> guarantee the buffer is async and thus is unlocked and released.
> 
> For example, if the last operation on the buffer was a read off disk
> prior to the corruption event, XBF_ASYNC is not set and the buffer
> is left locked and held upon return. This results in a memory leak
> as shown by the following message on module unload:
> 
>   BUG xfs_buf (...): Objects remaining in xfs_buf on __kmem_cache_shutdown()
> 
> Fix both of these problems by setting XBF_ASYNC on the buffer prior
> to the simulated I/O error and performing the shutdown immediately
> after ioend processing when the buffer has been released.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>   fs/xfs/xfs_inode.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index f643a9295179..4591598ca04d 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3614,7 +3614,6 @@ xfs_iflush_cluster(
>   	 * inode buffer and shut down the filesystem.
>   	 */
>   	rcu_read_unlock();
> -	xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
>   
>   	/*
>   	 * We'll always have an inode attached to the buffer for completion
> @@ -3624,11 +3623,14 @@ xfs_iflush_cluster(
>   	 * xfs_buf_submit().
>   	 */
>   	ASSERT(bp->b_iodone);
> +	bp->b_flags |= XBF_ASYNC;
>   	bp->b_flags &= ~XBF_DONE;
>   	xfs_buf_stale(bp);
>   	xfs_buf_ioerror(bp, -EIO);
>   	xfs_buf_ioend(bp);
>   
> +	xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> +
>   	/* abort the corrupt inode, as it was not attached to the buffer */
>   	xfs_iflush_abort(cip, false);
>   	kmem_free(cilist);
>