Re: [PATCH v3 2/3] xfs: kick large ioends to completion workqueue

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, May 17, 2021 at 01:17:21PM -0400, Brian Foster wrote:
> We've had reports of soft lockup warnings in the iomap ioend
> completion path due to very large bios and/or bio chains. This
> occurs because ioend completion touches every page associated with
> the ioend. It generally requires exceedingly large (i.e. multi-GB)
> bios or bio chains to reproduce a soft lockup warning, but even with
> smaller ioends there's really no good reason to incur the cost of
> potential cacheline misses in bio completion context. Divert ioends
> larger than 1MB to the workqueue so completion occurs in non-atomic
> context and can reschedule to avoid soft lockup warnings.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Will give this a spin on the test farm overnight but at least in
principle this seems fine to me:

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_aops.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 84cd6cf46b12..05b1bb146f17 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -30,6 +30,13 @@ XFS_WPC(struct iomap_writepage_ctx *ctx)
>  	return container_of(ctx, struct xfs_writepage_ctx, ctx);
>  }
>  
> +/*
> + * Completion touches every page associated with the ioend. Send anything
> + * larger than 1MB (based on 4k pages) or so to the completion workqueue to
> + * avoid this work in bio completion context.
> + */
> +#define XFS_LARGE_IOEND	(256ULL << PAGE_SHIFT)
> +
>  /*
>   * Fast and loose check if this write could update the on-disk inode size.
>   */
> @@ -409,9 +416,14 @@ xfs_prepare_ioend(
>  
>  	memalloc_nofs_restore(nofs_flag);
>  
> -	/* send ioends that might require a transaction to the completion wq */
> +	/*
> +	 * Send ioends that might require a transaction or are large enough that
> +	 * we don't want to do page processing in bio completion context to the
> +	 * wq.
> +	 */
>  	if (xfs_ioend_is_append(ioend) || ioend->io_type == IOMAP_UNWRITTEN ||
> -	    (ioend->io_flags & IOMAP_F_SHARED))
> +	    (ioend->io_flags & IOMAP_F_SHARED) ||
> +	    ioend->io_size >= XFS_LARGE_IOEND)
>  		ioend->io_bio->bi_end_io = xfs_end_bio;
>  	return status;
>  }
> -- 
> 2.26.3
>