All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-xfs@vger.kernel.org, michaelcallahan@fb.com
Subject: Re: [PATCH 2/3] xfs: don't block the log commit handler for discards
Date: Tue, 18 Oct 2016 10:29:08 +1100	[thread overview]
Message-ID: <20161017232908.GY23194@dastard> (raw)
In-Reply-To: <1476735753-5861-3-git-send-email-hch@lst.de>

On Mon, Oct 17, 2016 at 10:22:32PM +0200, Christoph Hellwig wrote:
> Instead we submit the discard requests and use another workqueue to
> release the extents from the extent busy list.

I lik ethe idea, and have toyed with it in the past. A couple of
questions about the implementation, though....

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 11 ++++++-
>  fs/xfs/xfs_discard.c     | 29 -----------------
>  fs/xfs/xfs_discard.h     |  1 -
>  fs/xfs/xfs_log_cil.c     | 84 +++++++++++++++++++++++++++++++++++++++++++-----
>  fs/xfs/xfs_log_priv.h    |  1 +
>  fs/xfs/xfs_mount.c       |  1 +
>  fs/xfs/xfs_super.c       |  8 +++++
>  fs/xfs/xfs_super.h       |  2 ++
>  8 files changed, 98 insertions(+), 39 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index c27344c..cbea3b2 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3684,10 +3684,12 @@ xfs_bmap_btalloc(
>  	int		tryagain;
>  	int		error;
>  	int		stripe_align;
> +	bool		trydiscard;
>  
>  	ASSERT(ap->length);
>  
>  	mp = ap->ip->i_mount;
> +	trydiscard = (mp->m_flags & XFS_MOUNT_DISCARD);
>  
>  	/* stripe alignment for allocation is determined by mount parameters */
>  	stripe_align = 0;
> @@ -3708,7 +3710,7 @@ xfs_bmap_btalloc(
>  		ASSERT(ap->length);
>  	}
>  
> -
> +retry:
>  	nullfb = *ap->firstblock == NULLFSBLOCK;
>  	fb_agno = nullfb ? NULLAGNUMBER : XFS_FSB_TO_AGNO(mp, *ap->firstblock);
>  	if (nullfb) {
> @@ -3888,6 +3890,13 @@ xfs_bmap_btalloc(
>  			return error;
>  		ap->dfops->dop_low = true;
>  	}
> +
> +	if (args.fsbno == NULLFSBLOCK && trydiscard) {
> +		trydiscard = false;
> +		flush_workqueue(xfs_discard_wq);
> +		goto retry;
> +	}

So this is the new behaviour that triggers flushing of the discard
list rather than having it occur from a log force inside
xfs_extent_busy_update_extent().

However, xfs_extent_busy_update_extent() also has backoff when it
finds an extent on the busy list being discarded, which means it
could spin waiting for the discard work to complete.

Wouldn't it be better to trigger this workqueue flush in
xfs_extent_busy_update_extent() in both these cases so that the
behaviour remains the same for userdata allocations hitting
uncommitted busy extents, but also allow us to remove the spinning
for allocations where the busy extent is currently being discarded?

> +xlog_discard_endio_work(
> +	struct work_struct	*work)
> +{
> +	struct xfs_cil_ctx	*ctx =
> +		container_of(work, struct xfs_cil_ctx, discard_endio_work);
> +	struct xfs_mount	*mp = ctx->cil->xc_log->l_mp;
> +
> +	xfs_extent_busy_clear(mp, &ctx->busy_extents, false);
> +	kmem_free(ctx);
> +}
> +
> +/*
> + * Queue up the actual completion to a thread to avoid IRQ-safe locking for
> + * pagb_lock.  Note that we need a unbounded workqueue, otherwise we might
> + * get the execution delayed up to 30 seconds for weird reasons.
> + */
> +static void
> +xlog_discard_endio(
> +	struct bio		*bio)
> +{
> +	struct xfs_cil_ctx	*ctx = bio->bi_private;
> +
> +	INIT_WORK(&ctx->discard_endio_work, xlog_discard_endio_work);
> +	queue_work(xfs_discard_wq, &ctx->discard_endio_work);
> +}
> +
> +static void
> +xlog_discard_busy_extents(
> +	struct xfs_mount	*mp,
> +	struct xfs_cil_ctx	*ctx)
> +{
> +	struct list_head	*list = &ctx->busy_extents;
> +	struct xfs_extent_busy	*busyp;
> +	struct bio		*bio = NULL;
> +	struct blk_plug		plug;
> +	int			error = 0;
> +
> +	ASSERT(mp->m_flags & XFS_MOUNT_DISCARD);
> +
> +	blk_start_plug(&plug);
> +	list_for_each_entry(busyp, list, list) {
> +		trace_xfs_discard_extent(mp, busyp->agno, busyp->bno,
> +					 busyp->length);
> +
> +		error = __blkdev_issue_discard(mp->m_ddev_targp->bt_bdev,
> +				XFS_AGB_TO_DADDR(mp, busyp->agno, busyp->bno),
> +				XFS_FSB_TO_BB(mp, busyp->length),
> +				GFP_NOFS, 0, &bio);
> +		if (error && error != -EOPNOTSUPP) {
> +			xfs_info(mp,
> +	 "discard failed for extent [0x%llx,%u], error %d",
> +				 (unsigned long long)busyp->bno,
> +				 busyp->length,
> +				 error);
> +			break;
> +		}
> +	}
> +
> +	if (bio) {
> +		bio->bi_private = ctx;
> +		bio->bi_end_io = xlog_discard_endio;
> +		submit_bio(bio);
> +	} else {
> +		xlog_discard_endio_work(&ctx->discard_endio_work);
> +	}

This creates one long bio chain with all the regions to discard on
it, and then when all it completes we call xlog_discard_endio() to
release all the busy extents.

Why not pull the busy extent from the list and attach it to each
bio returned and submit them individually and run per-busy extent
completions? That will substantially reduce the latency of discard
completions when there are long lists of extents to discard....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

  reply	other threads:[~2016-10-17 23:29 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-10-17 20:22 discard improvements Christoph Hellwig
2016-10-17 20:22 ` [PATCH 1/3] xfs: fix the AG loop in xfs_alloc_vextent for busy extents Christoph Hellwig
2016-10-19 13:48   ` Brian Foster
2016-10-21 12:48     ` Christoph Hellwig
2016-10-21 14:41       ` Brian Foster
2016-11-08  6:15   ` Dave Chinner
2016-11-10 19:17     ` Christoph Hellwig
2016-10-17 20:22 ` [PATCH 2/3] xfs: don't block the log commit handler for discards Christoph Hellwig
2016-10-17 23:29   ` Dave Chinner [this message]
2016-10-18  5:05     ` Christoph Hellwig
2016-10-19 10:58     ` Christoph Hellwig
2016-10-28 16:16       ` Michael Callahan
2016-10-31 15:16         ` Christoph Hellwig
2016-10-17 20:22 ` [PATCH 3/3] xfs: merge discard requests Christoph Hellwig
2016-10-28 10:11 ` discard improvements Avi Kivity
2016-10-31 15:14   ` Christoph Hellwig
2016-11-05 18:18     ` Avi Kivity
2016-11-06 16:36       ` Christoph Hellwig
2016-11-06 23:21         ` Dave Chinner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161017232908.GY23194@dastard \
    --to=david@fromorbit.com \
    --cc=hch@lst.de \
    --cc=linux-xfs@vger.kernel.org \
    --cc=michaelcallahan@fb.com \
    --subject='Re: [PATCH 2/3] xfs: don'\''t block the log commit handler for discards' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.