Re: [PATCH 2/8] XFS: Use a cursor for AIL traversal.

Christoph Hellwig
2008-09-19T09:23:14Z

On Sun, Sep 14, 2008 at 12:57:51AM +1000, Dave Chinner wrote:
> To replace the current generation number ensuring sanity of the AIL
> traversal, replace it with an external cursor that is linked to the
> AIL.
>
> Basically, we store the next item in the cursor whenever we want to
> drop the AIL lock to do something to the current item. When we
> regain the lock. the current item may already be free, so we can't
> reference it, but the next item in the traversal is already held in
> the cursor.
>
> When we move or delete an object, we search all the active cursors
> and if there is an item match we clear the cursor(s) that point to
> the object. This forces the traversal to restart transparently.
>
> We don't invalidate the cursor on insert because the cursor still
> points to a valid item. If the intem is inserted between the current
> item and the cursor it does not matter; the traversal is considered
> to be past the insertion point so it will be picked up in the next
> traversal.
>
> Hence traversal restarts pretty much disappear altogether with this
> method of traversal, which should substantially reduce the overhead
> of pushing on a busy AIL.

> {
> - int orig_gen = gen;
> -
> do {
> ASSERT(lip->li_type != XFS_LI_EFI);
> - lip = xfs_trans_next_ail(mp, lip, &gen, NULL);
> - /*
> - * The check will be bogus if we restart from the
> - * beginning of the AIL, so ASSERT that we don't.
> - * We never should since we're holding the AIL lock
> - * the entire time.
> - */
> - ASSERT(gen == orig_gen);
> + lip = xfs_trans_next_ail(mp, cur);
> } while (lip != NULL);

for (tmp = lip ; tmp = xfs_rans_next_ail(mp, tmp, &gen, NULL); tmp)
ASSERT(tmp->li_type != XFS_LI_EFI);

?

> +void
> +xfs_trans_ail_cursor_init(

A little comment describing the function would be nice.

> + struct xfs_ail *ailp,
> + struct xfs_ail_cursor *cur)
> +{
> + cur->item = NULL;
> + if (cur == &ailp->xa_cursors)
> + return;

What is this check for? It mans we'll do nothing if the cursor is the
one embedded into the ail. But we should never do this anyway, should
we?

> + cur->next = ailp->xa_cursors.next;
> + ailp->xa_cursors.next = cur;

> +/*
> + * Set the cursor to the next item, because when we look
> + * up the cursor the current item may have been freed.
> + */
> +STATIC void
> +xfs_trans_ail_cursor_set(
> + struct xfs_ail *ailp,
> + struct xfs_ail_cursor *cur,
> + struct xfs_log_item *lip)
> +{
> + if (lip)
> + cur->item = xfs_ail_next(ailp, lip);
> +}

Does it make sense to have the NULL check here and not in the caller?

> +STATIC struct xfs_log_item *
> +xfs_trans_ail_cursor_next(
> + struct xfs_ail *ailp,
> + struct xfs_ail_cursor *cur)
> +{
> + struct xfs_log_item *lip = cur->item;
> +
> + xfs_trans_ail_cursor_set(ailp, cur, lip);
> + return lip;
> +}

I'd say kill this wrapper, it's only used once anyway.

> +void
> +xfs_trans_ail_cursor_done(
> + struct xfs_ail *ailp,
> + struct xfs_ail_cursor *done)

A little comment describing it, please.

> + if (done == &ailp->xa_cursors)
> + return;
> + prev = &ailp->xa_cursors;
> + for (cur = prev->next; cur; prev = cur, cur = prev->next) {
> + if (cur == done) {
> + prev->next = cur->next;
> + break;
> + }
> + }
> +}

Add an assert somewhere that the cursor actually is on the list?