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

[Shailendra Tripathi <stripathi@agami.com>]

Christoph Hellwig wrote:
>> +/*
>> + * i_flags helper functions
>> + */
>> +static inline void
>> +__xfs_iflags_set(xfs_inode_t *ip, unsigned short flags)
>> +{
>> +	ip->i_flags |= flags;
>> +}
>> +
>> +static inline void
>> +xfs_iflags_set(xfs_inode_t *ip, unsigned short flags)
>> +{
>> +	spin_lock(&ip->i_flags_lock);
>> +	__xfs_iflags_set(ip, flags);
>> +	spin_unlock(&ip->i_flags_lock);
>> +}
>>     
>
> This is not actually 
>
>   
>> +
>> +static inline void
>> +xfs_iflags_clear(xfs_inode_t *ip, unsigned short flags)
>> +{
>> +	spin_lock(&ip->i_flags_lock);
>> +	ip->i_flags &= ~flags;
>> +	spin_unlock(&ip->i_flags_lock);
>> +}
>> +
>> +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.
>
>   
Isn't true that UNLOCK and LOCK in the given order imply full barrier 
Chris ?
As the flag is modified only within the lock/unlock pair, if one tries 
to access the field (test it), it should be like
LOCK IP
modify ...
UNLOCK IP  -----|
                           |  --->  This pair should act as a full 
barrier. 
LOCK IP       -----|
read ...           
UNLOCK IP

-shailendra