diff mbox series

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

Message ID 20210517171722.1266878-3-bfoster@redhat.com (mailing list archive)
State New
Headers show
Series iomap: avoid soft lockup warnings on large ioends | expand

Commit Message

Brian Foster May 17, 2021, 5:17 p.m. UTC
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>
---
 fs/xfs/xfs_aops.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Comments

Darrick J. Wong May 26, 2021, 1:20 a.m. UTC | #1
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
>
diff mbox series

Patch

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;
 }