On 02/04/2013 05:26 PM, Dave Chinner wrote: > 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. I hadn't looked at that in as much detail, but now that you point it out I concur. I retract this patch. Thanks. -Alex > > Cheers, > > Dave. > _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
