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

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

[Jan Kara]

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.

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);
-- 
1.6.0.2

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

[Christoph Hellwig]

On Tue, Sep 08, 2009 at 01:41:03PM +0200, Jan Kara 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>

Looks good to me.  Impressive that this causes real life issues.

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

[Andrew Morton]

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.

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

> 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));
_

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

[Jan Kara]

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

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

[Al Viro]

On Tue, Sep 08, 2009 at 01:41:03PM +0200, Jan Kara 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.

Nice catch.  ACK.

>   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'm back, actually, and finally had almost crawled from under the pile
of mail in mbox.  Will apply.