Re: [REVIEW 1 of 4] Clean up i_flags handling

[David Chinner <dgc@sgi.com>]

On Tue, Oct 24, 2006 at 10:38:22PM +0100, Christoph Hellwig wrote:
> > +static inline int
> > +__xfs_iflags_test(xfs_inode_t *ip, unsigned short flags)
> > +{
> > +	return (ip->i_flags & flags);
> > +}
> > +
> > +static inline int
> > +xfs_iflags_test(xfs_inode_t *ip, unsigned short flags)
> > +{
> > +	int ret;
> > +	spin_lock(&ip->i_flags_lock);
> > +	ret = __xfs_iflags_test(ip, flags);
> > +	spin_unlock(&ip->i_flags_lock);
> > +	return ret;
> 
> This is not actually guaranteed to work on machiens with very weak
> memory ordering.  Please use the *_bit routines from bitops.h instead.

Hmm - don't you have that the wrong way around?

Documentation/memory-barriers.txt:

...
Therefore, from (1), (2) and (4) an UNLOCK followed by an unconditional LOCK is
equivalent to a full barrier, but a LOCK followed by an UNLOCK is not.
....

But the bitops don't guarantee ordering or barriers e.g. from
include/asm-i386/bitops.h:

/**
 * set_bit - Atomically set a bit in memory
 * @nr: the bit to set
 * @addr: the address to start counting from
 *
 * This function is atomic and may not be reordered.  See __set_bit()
 * if you do not require the atomic guarantees.
 *
 * Note: there are no guarantees that this function will not be reordered
 * on non x86 architectures, so if you are writting portable code,
 * make sure not to rely on its reordering guarantees.
 *
 * Note that @nr may be almost arbitrarily large; this function is not
 * restricted to acting on a single-word quantity.
 */

So I think the code is fine as it stands.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group