Re: [PATCH] xfs: Improve scalability of busy extent tracking

[Christoph Hellwig <hch@infradead.org>]

Having the patches merged into one certainly helps with readability.
This version also passes xfsqa fine while I had some problems with that
with the delayed logging tree.

> o busy extent transaction owner tracked by transaction id, not by
>   address of transaction structure. Tracking by address of the
>   transaction was triggering false ASSERT failures when searching for
>   busy extents with matching start block numbers.

I'm a bit confused by that.  How can we ever find a xfs_busy_extent
structure for a transaction that's been freed?  We always call
xfs_trans_free_busy before freeing the transaction, so this should
never happen.  And if it can happen we can also get a reuse of the tid,
even if it is much more likely.  So if there's a corner case I've
missed we really need to keep a pointer to the xlog_ticket and keep
a reference on it as long as we have busy extents belonging to it in
the rbtree.

Besides that here's a few cosmetic comments:

> -STATIC void
> -xfs_alloc_search_busy(xfs_trans_t *tp,
> -		    xfs_agnumber_t agno,
> -		    xfs_agblock_t bno,
> -		    xfs_extlen_t len);
> +static int
> +xfs_alloc_busy_search(struct xfs_mount *mp, xfs_agnumber_t agno,
> +		    xfs_agblock_t bno, xfs_extlen_t len);

The switch in naming convention looks good to me, but it should
also be applied to xfs_alloc_clear_busy (-> xfs_alloc_busy_clear?)
and xfs_alloc_mark_busy (-> xfs_alloc_busy_add/insert?)

> +xfs_alloc_mark_busy(
> +	struct xfs_trans	*tp,
> +	xfs_agnumber_t		agno,
> +	xfs_agblock_t		bno,
> +	xfs_extlen_t		len)
>  {
> +	struct xfs_busy_extent	*busyp;
>  
> +	busyp = kmem_zalloc(sizeof(struct xfs_busy_extent), KM_MAYFAIL);
> +	if (!busyp) {
> +		/*
> +		 * No Memory!  Since it is now not possible to track the free
> +		 * block, make this a synchronous transaction to insure that
> +		 * the block is not reused before this transaction commits.
> +		 */
> +		trace_xfs_alloc_busy(tp, agno, bno, len, 1);
> +		xfs_trans_set_sync(tp);
> +		return;
>  	}
>  
> +	busyp->agno = agno;
> +	busyp->bno = bno;
> +	busyp->length = len;
> +	busyp->tid = xfs_trans_get_tid(tp);
> +	INIT_LIST_HEAD(&busyp->list);
>  
> +	/* trace before insert to be able to see failed inserts */
> +	trace_xfs_alloc_busy(tp, agno, bno, len, 0);
> +	xfs_alloc_busy_insert(tp, busyp);
> +}

I'd rather inline xfs_alloc_busy_insert into this function, there's no
good reason to keep it separate.

> -	xfs_trans_clear_busy_extents(tp);
> +	xfs_trans_free_busy(tp->t_mountp, &tp->t_busy);
>  	xfs_trans_free(tp);
>  }
>  
> @@ -1013,7 +1016,7 @@ xfs_trans_uncommit(
>  	xfs_trans_unreserve_and_mod_dquots(tp);
>  
>  	xfs_trans_free_items(tp, flags);
> -	xfs_trans_free_busy(tp);
> +	xfs_trans_free_busy(tp->t_mountp, &tp->t_busy);

There's no real need for the prototype change at this point.  I know
you'll need it for the delayed logging, but let's keep the prototype
change in that patchset.

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