Re: [PATCH 4/5] xfs: convert AIL cursors to use struct list_head

[Alex Elder <aelder@sgi.com>]

On Mon, 2011-07-04 at 15:27 +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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@redhat.com>

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

>   */
>  struct xfs_log_item *
>  xfs_trans_ail_cursor_next(
> @@ -208,39 +200,24 @@ xfs_trans_ail_cursor_next(
>  }
>  
>  /*
> - * Now that the traversal is complete, we need to remove the cursor
> - * from the list of traversing cursors. Avoid removing the embedded
> - * push cursor, but use the fact it is always present to make the
> - * list deletion simple.
> + * When the traversal is complete, we need to remove the cursor from the list
> + * of traversing cursors.
>   */
>  void
>  xfs_trans_ail_cursor_done(
>  	struct xfs_ail		*ailp,
> -	struct xfs_ail_cursor	*done)
> +	struct xfs_ail_cursor	*cur)
>  {
> -	struct xfs_ail_cursor	*prev = NULL;
> -	struct xfs_ail_cursor	*cur;
> -
> -	done->item = NULL;

This eliminates the curious situation where the
end of the list was marked by a null *item* pointer
rather than something having to do with the next
pointer.

> -	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;

. . .

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

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

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

>  	lip = xfs_trans_ail_cursor_first(ailp, cur, ailp->xa_last_pushed_lsn);
>  	if (!lip || XFS_FORCED_SHUTDOWN(mp)) {
>  		/*

. . .

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs