On Thu, Jul 07, 2011 at 04:15:15PM -0500, Alex Elder wrote:
> On Mon, 2011-07-04 at 15:27 +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> >
> > The list of active AIL cursors uses a roll-your-own linked list with
> > special casing for the AIL push cursor. Simplify this code by
> > replacing the list with standard struct list_head lists, and use a
> > separate list_head to track the active cursors so that the code can
> > just treat the AIL push cursor (which is still embedded into the
> > struct xfs_ail) as a generic cursor.
> >
> > Further, fix the duplicate push cursor initialisation that the
> > special case handling was hiding, and clean up all the comments
> > around the active cursor list handling.
> >
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
>
> I suggest a few comment changes below. I also have a
> question about why the push cursor isn't treated
> like any other cursors. I added a few bits of
> commentary as well--you addressed a few things
> I had been thinking about earlier.
>
> I guess I'm interested in your response before
> signing off.
>
> -Alex
>
> > ---
> > fs/xfs/xfs_trans_ail.c | 68 +++++++++++++++-------------------------------
> > fs/xfs/xfs_trans_priv.h | 5 ++-
> > 2 files changed, 25 insertions(+), 48 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> > index de7a52a..3b5b5e4 100644
> > --- a/fs/xfs/xfs_trans_ail.c
> > +++ b/fs/xfs/xfs_trans_ail.c
> > @@ -165,15 +165,11 @@ xfs_ail_max_lsn(
> > /*
> > * AIL traversal cursor initialisation.
> > *
> > - * The cursor keeps track of where our current traversal is up
> > - * to by tracking the next ƣtem in the list for us. However, for
> > - * this to be safe, removing an object from the AIL needs to invalidate
> > - * any cursor that points to it. hence the traversal cursor needs to
> > - * be linked to the struct xfs_ail so that deletion can search all the
> > - * active cursors for invalidation.
> > - *
> > - * We don't link the push cursor because it is embedded in the struct
> > - * xfs_ail and hence easily findable.
> > + * The cursor keeps track of where our current traversal is up to by tracking
> > + * the next ƣtem in the list for us. However, for this to be safe, removing an
> ^
> What's up with the weird non-ASCII characters in your code?
>
> > + * object from the AIL needs to invalidate any cursor that points to it. hence
> > + * the traversal cursor needs to be linked to the struct xfs_ail so that
> > + * deletion can search all the active cursors for invalidation.
> > */
> > STATIC void
> > xfs_trans_ail_cursor_init(
> > @@ -181,17 +177,13 @@ xfs_trans_ail_cursor_init(
> > struct xfs_ail_cursor *cur)
> > {
> > cur->item = NULL;
> > - if (cur == &ailp->xa_cursors)
> > - return;
> > -
> > - cur->next = ailp->xa_cursors.next;
> > - ailp->xa_cursors.next = cur;
> > + INIT_LIST_HEAD(&cur->list);
> > + list_add_tail(&cur->list, &ailp->xa_cursors);
>
> This is good. I was thinking as I looked at this earlier
> that it would be nicer if newer cursors were added
> to the end of the list.
>
> > }
> >
> > /*
> > - * Get the next item in the traversal and advance the cursor.
> > - * If the cursor was invalidated (inidicated by a lip of 1),
> > - * restart the traversal.
> > + * Get the next item in the traversal and advance the cursor. If the cursor
> > + * was invalidated (inidicated by a lip of 1), restart the traversal.
> indicated
>
> Actually, it's indicated by a low-order bit of 1.
True.
> Why is it
> that you decided to just set the bit rather than overwrite
> the item pointer? Just for the benefit of debugging? (That
> is a good reason...) If not, I suggest defining an
> XFS_ITEM_INVALID constant pointer rather than just using 1.
Right, it was mainly for debugging. i.e. if we trip over the item
we'll get a panic but we'll also be left with a corpse that has
intact pointers which might aid finding the cause of the problem.
> > - * invalidation and the end of the list when getting the next item
> > + * Invalidate any cursor that is pointing to this item. This is called when an
> > + * item is removed from the AIL. Any cursor pointing to this object is now
> > + * invalid and the traversal needs to be terminated so it doesn't reference a
> > + * freed object. We set the cursor item to a value of 1 so we can distinguish
>
> Fix this comment to reflect the use of the low bit rather than just 1.
Will do.
> > + * between an invalidation and the end of the list when getting the next item
> > * from the cursor.
> > */
> > STATIC void
>
> . . .
>
> > nt_t)cur->item | 1);
> > @@ -416,7 +392,7 @@ xfs_ail_worker(
> > struct xfs_ail *ailp = container_of(to_delayed_work(work),
> > struct xfs_ail, xa_work);
> > xfs_mount_t *mp = ailp->xa_mount;
> > - struct xfs_ail_cursor *cur = &ailp->xa_cursors;
> > + struct xfs_ail_cursor *cur = &ailp->xa_push_cursor;
>
> Is the push cursor defined in the ail structure just
> so it's easier to find?
The reason is that 99.9% of traversals are from the push
code, so 99.9% of cursor lookups would be against that cursor.
Embedding it like this avoided the need to add it to or remove it
from a list, and meant that it would also always be cache hot when
the ail structure was accessed.
Realistically, that was probably a case of over-optimisation.
>
> > xfs_log_item_t *lip;
> > xfs_lsn_t lsn;
> > xfs_lsn_t target;
> > @@ -428,7 +404,6 @@ xfs_ail_worker(
> >
> > spin_lock(&ailp->xa_lock);
> > target = ailp->xa_target;
> > - xfs_trans_ail_cursor_init(ailp, cur);
>
> I don't see why the push cursor should be treated
> any differently from any others. If something gets
> invalidated the push cursor needs to be notified
> also, doesn't it?
>
> I must be missing something here...
No, you are right. Now that the list is generic, there is no reason
to keep the cursor in the AIL structure - each push goes back to the
start of the AIL to begin again, so the only reason for keeping it
in the AIL structure would be if it was cached across multiple push
traversals. I don't think we gain anything by doing that, so we can
just treat it like all other stack based cursors.
I'll update the patch appropriately.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
