Re: [PATCH v2] xfs: re-enable xfsaild idle mode and fix associated races

[Dave Chinner <david@fromorbit.com>]

On Wed, Jun 20, 2012 at 11:59:58AM -0400, Brian Foster wrote:
> On 06/20/2012 04:05 AM, Christoph Hellwig wrote:
> > On Thu, Jun 07, 2012 at 12:49:53PM -0400, Brian Foster wrote:
> [snip]
> >>  	spin_lock(&ailp->xa_lock);
> >> +
> >> +	/* barrier matches the xa_target update in xfs_ail_push() */
> >> +	smp_rmb();
> >> +	target = ailp->xa_target;
> >> +	ailp->xa_target_prev = target;
> >> +
> >>  	lip = xfs_trans_ail_cursor_first(ailp, &cur, ailp->xa_last_pushed_lsn);
> >>  	if (!lip) {
> >>  		/*
> >> +
> >> +		spin_lock(&ailp->xa_lock);
> >> +
> >> +		/*
> >> +		 * Idle if the AIL is empty and we are not racing with a target
> >> +		 * update. The barrier matches the xa_target update in
> >> +		 * xfs_ail_push().
> >> +		 */
> >> +		smp_rmb();
> > 
> > Given that both sides are under xa_lock I can't see any need for
> > barriers here.
> > 
> 
> Actually now that I look at it, it appears xfs_trans_ail_copy_lsn() does
> not necessarily acquire the xa_lock...

Right, it does not acquire the lock on 64 bit systems as reads and
writes are single instructions on 64 bit systems. The lock is
required for 32 bit systems because the write requires separate 32
bit writes to the LSN which can result in unlocked accesses seeing
partially updated (and hence incorrect) LSN values.

So the memory barriers are definitely needed for 64 bit machines
because there is no locking on the update and spinlocks only provide
memory barriers via unlock->lock transitions, not via a single
spin_lock() call.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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