Re: [PATCH 5/6] xfs: add online discard support

[Dave Chinner <david@fromorbit.com>]

On Tue, Mar 22, 2011 at 03:55:55PM -0400, Christoph Hellwig wrote:
> Now that we have reliably tracking of deleted extents in a transaction
> we can easily implement "online" discard support which calls
> blkdev_issue_discard once a transaction commits.
> 
> The actual discard is a two stage operation as we first have to mark
> the busy extent as not available for reuse before we can start the
> actual discard.  Note that we don't bother supporting discard for
> the non-delaylog mode.  While that would be possible with this patch
> performance is awfull, and the optimization in the next patch won't
> work as easily.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
.....
> @@ -361,13 +362,17 @@ xlog_cil_committed(
>  	int	abort)
>  {
>  	struct xfs_cil_ctx	*ctx = args;
> +	struct xfs_mount	*mp = ctx->cil->xc_log->l_mp;
>  	struct xfs_busy_extent	*busyp, *n;
>  
>  	xfs_trans_committed_bulk(ctx->cil->xc_log->l_ailp, ctx->lv_chain,
>  					ctx->start_lsn, abort);
>  
> -	list_for_each_entry_safe(busyp, n, &ctx->busy_extents, list)
> -		xfs_alloc_busy_clear(ctx->cil->xc_log->l_mp, busyp);
> +	list_for_each_entry_safe(busyp, n, &ctx->busy_extents, list) {
> +		if (!abort)
> +			xfs_discard_extent(mp, busyp);
> +		xfs_alloc_busy_clear(mp, busyp);
> +	}
>  
>  	spin_lock(&ctx->cil->xc_cil_lock);
>  	list_del(&ctx->committing);
> Index: xfs/fs/xfs/linux-2.6/xfs_discard.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_discard.c	2011-03-21 14:47:03.614474345 +0100
> +++ xfs/fs/xfs/linux-2.6/xfs_discard.c	2011-03-21 14:51:41.449976366 +0100
> @@ -191,3 +191,38 @@ xfs_ioc_trim(
>  		return -XFS_ERROR(EFAULT);
>  	return 0;
>  }
> +
> +int
> +xfs_discard_extent(
> +	struct xfs_mount	*mp,
> +	struct xfs_busy_extent	*busyp)
> +{
> +	struct xfs_perag	*pag;
> +	int			error = 0;
> +	xfs_daddr_t		bno;
> +	int64_t			len;
> +	bool			done  = false;
> +
> +	if ((mp->m_flags & XFS_MOUNT_DISCARD) == 0)
> +		return 0;

I'd move this check to the callers, otherwise we are going to be
doing lots of function calls in a relatively performance sensitive
loop just to run a single check when discard is not enabled...

> +
> +	bno = XFS_AGB_TO_DADDR(mp, busyp->agno, busyp->bno);
> +	len = XFS_FSB_TO_BB(mp, busyp->length);
> +
> +	pag = xfs_perag_get(mp, busyp->agno);
> +	spin_lock(&pag->pagb_lock);
> +	if (!busyp->length)
> +		done = true;
> +	busyp->flags = XFS_ALLOC_BUSY_DISCARDED;
> +	spin_unlock(&pag->pagb_lock);
> +	xfs_perag_put(pag);
> +
> +	if (done)
> +		return 0;
> +
> +	error = -blkdev_issue_discard(mp->m_ddev_targp->bt_bdev, bno, len,
> +				      GFP_NOFS, 0);
> +	if (error && error != EOPNOTSUPP)
> +		xfs_info(mp, "discard failed, error %d", error);

This would be more informative if it also printed the bno and len of
the discard that failed. A couple of tracepoints here (e.g.
discard_extent_issued, discard_extent_failed) could also be useful
for tracking discard operations.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs