[Top] [All Lists]

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

To: Alex Elder <elder@xxxxxxxxxxx>
Subject: Re: [PATCH 2/2] xfs: another memory barrier before wake_up_bit()
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 5 Feb 2013 10:26:17 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <510FDE23.9050801@xxxxxxxxxxx>
References: <510FDDE5.4050103@xxxxxxxxxxx> <510FDE23.9050801@xxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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@xxxxxxxxxxx>
> ---
>  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.


Dave Chinner

<Prev in Thread] Current Thread [Next in Thread>