xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/8] XFS: Use a cursor for AIL traversal.
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 24 Sep 2008 13:11:38 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20080919092444.GB11443@xxxxxxxxxxxxx>
Mail-followup-to: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
References: <1221317877-8333-1-git-send-email-david@xxxxxxxxxxxxx> <1221317877-8333-3-git-send-email-david@xxxxxxxxxxxxx> <20080919092444.GB11443@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
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@xxxxxxxxxxxxx

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