Re: [PATCH] fs: Make sure data stored into inode is properly seen before unlocking new inode

[Jan Kara <jack@suse.cz>]

On Wed 09-09-09 15:03:34, Andrew Morton wrote:
> On Tue,  8 Sep 2009 13:41:03 +0200
> Jan Kara <jack@suse.cz> wrote:
> 
> > In theory it could happen that on one CPU we initialize a new inode but clearing
> > of I_NEW | I_LOCK gets reordered before some of the initialization. Thus on
> > another CPU we return not fully uptodate inode from iget_locked().
> > 
> > This seems to fix a corruption issue on ext3 mounted over NFS.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/inode.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> >   Since Al doesn't seem to be online, does anybody else have opinion on this
> > patch? I can merge it via my tree but I'd like to get a review from someone
> > else.
> 
> I'll merge it for 2.6.31.
  Thanks!

> Please always remember -stable kernels when preparing bugfixes!  This
> one should have had a Cc:stable in the changelog and in the email
> headers.
  Good point. Thanks for reminding.

> > diff --git a/fs/inode.c b/fs/inode.c
> > index 901bad1..e9a8e77 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -696,6 +696,7 @@ void unlock_new_inode(struct inode *inode)
> >  	 * just created it (so there can be no old holders
> >  	 * that haven't tested I_LOCK).
> >  	 */
> > +	smp_mb();
> >  	WARN_ON((inode->i_state & (I_LOCK|I_NEW)) != (I_LOCK|I_NEW));
> >  	inode->i_state &= ~(I_LOCK|I_NEW);
> >  	wake_up_inode(inode);
> 
> But an uncommented barrier is always a hard thing for a reader to
> understand.  Let's add something to help people.  How's this look?
> 
> --- a/fs/inode.c~fs-make-sure-data-stored-into-inode-is-properly-seen-before-unlocking-new-inode-fix
> +++ a/fs/inode.c
> @@ -697,12 +697,13 @@ void unlock_new_inode(struct inode *inod
>  	}
>  #endif
>  	/*
> -	 * This is special!  We do not need the spinlock
> -	 * when clearing I_LOCK, because we're guaranteed
> -	 * that nobody else tries to do anything about the
> -	 * state of the inode when it is locked, as we
> -	 * just created it (so there can be no old holders
> -	 * that haven't tested I_LOCK).
> +	 * This is special!  We do not need the spinlock when clearing I_LOCK,
> +	 * because we're guaranteed that nobody else tries to do anything about
> +	 * the state of the inode when it is locked, as we just created it (so
> +	 * there can be no old holders that haven't tested I_LOCK).
> +	 * However we must emit the memory barrier so that other CPUs reliably
> +	 * see the clearing of I_LOCK after the other inode initialisation has
> +	 * completed.
>  	 */
>  	smp_mb();
>  	WARN_ON((inode->i_state & (I_LOCK|I_NEW)) != (I_LOCK|I_NEW));
  Looks good.

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