[PATCH 2/2] xfs: another memory barrier before wake_up_bit()

[Dave Chinner]

On Mon, Feb 04, 2013 at 10:13:23AM -0600, Alex Elder wrote:
> In xfs_inode_item_unpin() there is a call to wake_up_bit() following
> an independent test for whether waiters should be awakened.  This
> requires a memory barrier in order to guarantee correct operation
> (see the comment above wake_up_bit()).
> 
> Signed-off-by: Alex Elder <elder at inktank.com>
> ---
>  fs/xfs/xfs_inode_item.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index d041d47..a7cacf7 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -474,8 +474,10 @@ xfs_inode_item_unpin(
> 
>  	trace_xfs_inode_unpin(ip, _RET_IP_);
>  	ASSERT(atomic_read(&ip->i_pincount) > 0);
> -	if (atomic_dec_and_test(&ip->i_pincount))
> -		wake_up_bit(&ip->i_flags, __XFS_IPINNED_BIT);
> +	if (!atomic_dec_and_test(&ip->i_pincount))
> +		return;
> +	smp_mb();
> +	wake_up_bit(&ip->i_flags, __XFS_IPINNED_BIT);

I'm not sure this a barrier is actually needed here.  The "wake up"
bit is never stored or cleared anywhere in this case, it is used
only to define a wait channel and directed wake up. Hence the "need
a barrier so all CPUs see the cleared bit" case doesn't arise here.
We use an atomic variable instead, and that makes it safe.

If you read Documentation/atomic_ops.txt, you'll find that atomic
modification operations are required to have explicit barrier
semantics. i.e. that atomic_dec_and_test() must behave like it has
both a smp_mb() before and after the atomic operation. i.e:

	Unlike the above routines, it is required that explicit memory
	barriers are performed before and after the operation.  It must be
	done such that all memory operations before and after the atomic
	operation calls are strongly ordered with respect to the atomic
	operation itself.

So, the smp_mb() that is added here is redundant - the
atomic_dec_and_test() call already has the necesary memory barriers
that wake_up_bit() requires.

Cheers,

Dave.
-- 
Dave Chinner
david at fromorbit.com