xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: Improve scalability of busy extent tracking
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Thu, 22 Apr 2010 07:01:43 -0400
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1271828835-2094-1-git-send-email-david@xxxxxxxxxxxxx>
References: <1271828835-2094-1-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.19 (2009-01-05)
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.

<Prev in Thread] Current Thread [Next in Thread>