Date: Mon, 4 Jul 2011 07:16:05 -0400
From: Christoph Hellwig <hch@infradead.org>
To: Dave Chinner <david@fromorbit.com>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 2/5] xfs: use a cursor for bulk AIL insertion
Message-ID: <20110704111605.GA5513@infradead.org>
References: <1309757260-5484-1-git-send-email-david@fromorbit.com>
 <1309757260-5484-3-git-send-email-david@fromorbit.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1309757260-5484-3-git-send-email-david@fromorbit.com>
User-Agent: Mutt/1.5.21 (2010-09-15)

> +	do {
> +		/* no placeholder, so get our insert location */
> +		if (!lip)
> +			lip = __xfs_trans_ail_cursor_last(ailp, cur,
> +								lsn, false);
> +
> +		if (!lip) {
> +			/*
> +			 * The list is empty, so just splice and return.  Our
> +			 * cursor is already guaranteed to be up to date, so we
> +			 * don't need to touch it here.
> +			 */
> +			list_splice(list, &ailp->xa_ail);
> +			return;
> +		}
> +
> +		/* The placeholder was invalidated, need to get a new cursor */
> +		if ((__psint_t)lip & 1)
> +			lip = NULL;
> +
> +	} while (lip == NULL);

Why do we even need a loop here?  Given that we're under xa_lock
no new cursor will get invalidated.  Isn't the simple code below
equivalent?

	/* no valid placeholder, get us a useful one */
	if (!lip || (__psint_t)lip & 1))
		lip = __xfs_trans_ail_cursor_last(ailp, cur, lsn, false);

	if (!lip) {
		/*
		 * The list is empty, so just splice and return.  Our
		 * cursor is already guaranteed to be up to date, so we
		 * don't need to touch it here.
		 */
		list_splice(list, &ailp->xa_ail);
		return;
	}