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.
|