Re: [PATCH 5/9] xfs: convert the xfsaild threads to a workqueue

[Christoph Hellwig <hch@infradead.org>]

On Wed, Apr 06, 2011 at 04:19:14PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Similar to the xfssyncd, the per-filesystem xfsaild threads can be
> converted to a global workqueue and run periodically by delayed
> works. This makes sense for the AIL pushing because it uses
> variable timeouts depending on the work that needs to be done.
> 
> By removing the xfsaild, we simplify the AIL pushing code and
> remove the need to spread the code to implement the threading
> and pushing across multiple files.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/linux-2.6/xfs_super.c |  115 ++++++++++++------------------------
>  fs/xfs/xfs_trans_ail.c       |  133 ++++++++++++++++++++++++------------------
>  fs/xfs/xfs_trans_priv.h      |   15 +++--
>  3 files changed, 121 insertions(+), 142 deletions(-)




> +	tout = 0;
>  	if (!count) {
>  		/* We're past our target or empty, so idle */
> -		last_pushed_lsn = 0;
> +		ailp->xa_last_pushed_lsn = 0;
> +
> +		/*
> +		 * Check for an updated push target before clearing the
> +		 * XFS_AIL_PUSHING_BIT. If the target changed, we've got more
> +		 * work to do.
> +		 */
> +		smp_rmb();
> +		if (ailp->xa_target == target)
> +			clear_bit(XFS_AIL_PUSHING_BIT, &ailp->xa_flags);
> +		else
> +			tout = 50;

I thjink this could be simplified a bit by just returning here if we
have matched the target, and then making the queueing up of work later
on unconditional.

> +/*
> + * xfs_trans_ail_push
> + *

Wha'ts the point of mentioning the function name like this in comments?

> +	/*
> +	 * En??ure that the new target is noticed in push code before it clears

There's some weird character here which my mailer can't display.


Except for that looks fine to me,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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