Re: [PATCH 4/6] xfs: allow reusing busy extents where safe

[Alex Elder <aelder@sgi.com>]

On Tue, 2011-03-22 at 15:55 -0400, Christoph Hellwig wrote:
> Allow reusing any busy extent for metadata allocations, and reusing busy
> userdata extents for userdata allocations.  Most of the complexity is
> propagating the userdata information from the XFS_BMAPI_METADATA flag
> to xfs_bunmapi into the low-level extent freeing routines.  After that
> we can just track what type of busy extent we have and treat it accordingly.

Why is it OK to reuse user data extents for user data allocations?
I accept it is, I just haven't worked through in my mind why.

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/xfs_alloc.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_alloc.c	2011-03-21 14:49:14.000000000 +0100
> +++ xfs/fs/xfs/xfs_alloc.c	2011-03-21 14:51:31.746155282 +0100
> @@ -1396,7 +1396,8 @@ xfs_alloc_ag_vextent_small(
>  		if (error)
>  			goto error0;
>  		if (fbno != NULLAGBLOCK) {
> -			xfs_alloc_busy_reuse(args->tp, args->agno, fbno, 1);
> +			xfs_alloc_busy_reuse(args->tp, args->agno, fbno, 1,
> +					     args->userdata);
>  
>  			if (args->userdata) {
>  				xfs_buf_t	*bp;
> @@ -2431,7 +2432,8 @@ int				/* error */
>  xfs_free_extent(
>  	xfs_trans_t	*tp,	/* transaction pointer */
>  	xfs_fsblock_t	bno,	/* starting block number of extent */
> -	xfs_extlen_t	len)	/* length of extent */
> +	xfs_extlen_t	len,
> +	bool		userdata)/* length of extent */

	xfs_extlen_t	len,	/* length of extent */
	bool		userdata)

>  {
>  	xfs_alloc_arg_t	args;
>  	int		error;

. . .


> @@ -2717,7 +2723,7 @@ restart:		(in xfs_alloc_busy_reuse())
>  
>  		overlap = xfs_alloc_busy_try_reuse(pag, busyp,
>  						   fbno, fbno + flen);
> -		if (overlap) {
> +		if (overlap == -1 || (overlap && userdata)) {

xfs_alloc_busy_try_reuse() (still) never returns non-zero,
so this could just be:
    if (overlap == -1 || userdata) {

I understand why we can skip forcing the log if
we're not doing a userdata allocation.  But why
don't you also check busyp->flags here when it's
a userdata allocation, to see if it represents a
busy userdata section and therefore would allow
avoiding the log force (like is done below in
xfs_alloc_busy_trim())?  You would have to grab
the flag value in busyp before the call.

>  			spin_unlock(&pag->pagb_lock);
>  			xfs_log_force(tp->t_mountp, XFS_LOG_SYNC);
>  			goto restart;
> @@ -2754,6 +2760,7 @@ xfs_alloc_busy_trim(
>  
>  	ASSERT(flen > 0);
>  
> +restart:
>  	spin_lock(&args->pag->pagb_lock);
>  	rbp = args->pag->pagb_tree.rb_node;
>  	while (rbp && flen >= args->minlen) {
> @@ -2771,6 +2778,31 @@ xfs_alloc_busy_trim(
>  			continue;
>  		}
>  
> +		if (!args->userdata ||
> +		    (busyp->flags & XFS_ALLOC_BUSY_USERDATA)) {
> +			int overlap;
> +
> +			overlap = xfs_alloc_busy_try_reuse(args->pag, busyp,
> +							   fbno, fbno + flen);
> +			if (unlikely(overlap == -1)) {
> +				spin_unlock(&args->pag->pagb_lock);
> +				xfs_log_force(args->mp, XFS_LOG_SYNC);
> +				goto restart;
> +			}
> +
> +			/*
> +			 * No more busy extents to search.
> +			 */
> +			if (bbno <= fbno && bend >= fend)
> +				goto out;
> +
> +			if (fbno < bbno)
> +				rbp = rbp->rb_left;
> +			else
> +				rbp = rbp->rb_right;
> +			continue;
> +		}
> +
>  		if (bbno <= fbno) {
>  			/* start overlap */
>  

. . .

> Index: xfs/fs/xfs/xfs_ag.h
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_ag.h	2011-03-21 14:48:04.000000000 +0100
> +++ xfs/fs/xfs/xfs_ag.h	2011-03-21 14:49:21.941981228 +0100

. . .

> @@ -3750,6 +3744,7 @@ xfs_bmap_add_free(
>  	new = kmem_zone_alloc(xfs_bmap_free_item_zone, KM_SLEEP);
>  	new->xbfi_startblock = bno;
>  	new->xbfi_blockcount = (xfs_extlen_t)len;
> +	new->xbfi_flags = XFS_BFI_USERDATA;

Couldn't you arrange for the the xfs_bmbt_free_block() path
to *not* set this?  (As it stands, it will always be set.)

>  	for (prev = NULL, cur = flist->xbf_first;
>  	     cur != NULL;
>  	     prev = cur, cur = cur->xbfi_next) {

. . .

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