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

[Andrew Morton]

On Tue,  8 Sep 2009 13:41:03 +0200
Jan Kara <jack@xxxxxxx> 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@xxxxxxx>
> ---
>  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));
_

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/