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

[Dave Chinner <david@fromorbit.com>]

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