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

[Shailendra Tripathi <stripathi@xxxxxxxxx>]

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