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

Dave Chinner
2008-09-24T03:10:09Z

On Fri, Sep 19, 2008 at 05:24:44AM -0400, Christoph Hellwig wrote:
> On Sun, Sep 14, 2008 at 12:57:51AM +1000, Dave Chinner wrote:
> > - 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);
>
> ?

Doesn't really matter. I'd prefer to avoid needing another temporary
variable, so it can become:

for (; lip; lip = xfs_trans_next_ail(mp, cur))
ASSERT(lip->li_type != XFS_LI_EFI);

And at that point, I can probably kill the debug wrapper function
altogether. Hmmm - will the compiler optimise out the empty loop?
(i.e. do i need #ifdef DEBUG around it?)

> > +void
> > +xfs_trans_ail_cursor_init(
>
> A little comment describing the function would be nice.

Sure.

> > + 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?

It means that we don't link the embedded push cursor into the list.
That is, initialisation of the push cursor simply clears the "next
item".

> > +/*
> > + * 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?

Doesn't really matter - this saves having to check in every place
we call the function....

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

Ah - should be at least twice - I note that the push code uses
the xfs_mount version rather than the xfs_ail version.

Also, I realised that I forgot to add the traversal restart code
into this function, so it's definitely necessary....

> > +void
> > +xfs_trans_ail_cursor_done(
> > + struct xfs_ail *ailp,
> > + struct xfs_ail_cursor *done)
>
> A little comment describing it, please.

Done.

> > + 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?

Done.

I'll repost the entire patch series with the fixes in a while

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com