Re: [PATCH 1/7] writeback: Move clearing of I_SYNC into inode_sync_complete()

[Jan Kara <jack@suse.cz>]

On Mon 30-04-12 10:36:41, Christoph Hellwig wrote:
> On Tue, Mar 20, 2012 at 11:56:25PM +0100, Jan Kara wrote:
> > Move clearing of I_SYNC into inode_sync_complete().  It is more logical to have
> > clearing of I_SYNC bit and waking of waiters in one place. Also later we will
> > have two places needing to clear I_SYNC and wake up waiters so this allows them
> > to use the common helper. Moving of I_SYNC clearing to a later stage of
> > writeback_single_inode() is safe since we hold i_lock all the time.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> The code changes look good, but should we really remove a comment that
> describes memory barrier?  IMHO any undocumented barrier is a bug
> waiting to happen.
> 
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
  The old comment didn't look too useful to me and the necessity of a
barrier is documented in wake_up_bit(). But yes, some comment should be
there. I added there simple:
        inode->i_state &= ~I_SYNC;
        /* Waiters must see I_SYNC cleared before being woken up */
        smp_mb();
        wake_up_bit(&inode->i_state, __I_SYNC);

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR