[EXT4 set 4][PATCH 4/5] i_version:ext4 inode version update

[EXT4 set 4][PATCH 4/5] i_version:ext4 inode version update

[Mingming Cao]

This patch is on top of i_version_update_vfs.
The i_version field of the inode is set on inode creation and incremented when
the inode is being modified.

Signed-off-by: Jean Noel Cordenner <jean-noel.cordenner@bull.net>
Signed-off-by: Mingming Cao <cmm@us.ibm.com>

Index: linux-2.6.22-rc4/fs/ext4/ialloc.c
===================================================================
--- linux-2.6.22-rc4.orig/fs/ext4/ialloc.c	2007-06-13 17:16:28.000000000 -0700
+++ linux-2.6.22-rc4/fs/ext4/ialloc.c	2007-06-13 17:24:45.000000000 -0700
@@ -565,6 +565,7 @@ got:
 	inode->i_blocks = 0;
 	inode->i_mtime = inode->i_atime = inode->i_ctime = ei->i_crtime =
 						       ext4_current_time(inode);
+	inode->i_version = 1;
 
 	memset(ei->i_data, 0, sizeof(ei->i_data));
 	ei->i_dir_start_lookup = 0;
Index: linux-2.6.22-rc4/fs/ext4/inode.c
===================================================================
--- linux-2.6.22-rc4.orig/fs/ext4/inode.c	2007-06-13 17:21:29.000000000 -0700
+++ linux-2.6.22-rc4/fs/ext4/inode.c	2007-06-13 17:24:45.000000000 -0700
@@ -3082,6 +3082,7 @@ int ext4_mark_iloc_dirty(handle_t *handl
 {
 	int err = 0;
 
+	inode->i_version++;
 	/* the do_update_inode consumes one bh->b_count */
 	get_bh(iloc->bh);
 
Index: linux-2.6.22-rc4/fs/ext4/super.c
===================================================================
--- linux-2.6.22-rc4.orig/fs/ext4/super.c	2007-06-13 17:19:11.000000000 -0700
+++ linux-2.6.22-rc4/fs/ext4/super.c	2007-06-13 17:24:45.000000000 -0700
@@ -2846,8 +2846,8 @@ out:
 		i_size_write(inode, off+len-towrite);
 		EXT4_I(inode)->i_disksize = inode->i_size;
 	}
-	inode->i_version++;
 	inode->i_mtime = inode->i_ctime = CURRENT_TIME;
+	inode->i_version = 1;
 	ext4_mark_inode_dirty(handle, inode);
 	mutex_unlock(&inode->i_mutex);
 	return len - towrite;

Re: [EXT4 set 4][PATCH 4/5] i_version:ext4 inode version update

[Aneesh Kumar K.V]

Mingming Cao wrote:
> 
> Index: linux-2.6.22-rc4/fs/ext4/super.c
> ===================================================================
> --- linux-2.6.22-rc4.orig/fs/ext4/super.c	2007-06-13 17:19:11.000000000 -0700
> +++ linux-2.6.22-rc4/fs/ext4/super.c	2007-06-13 17:24:45.000000000 -0700
> @@ -2846,8 +2846,8 @@ out:
>  		i_size_write(inode, off+len-towrite);
>  		EXT4_I(inode)->i_disksize = inode->i_size;
>  	}
> -	inode->i_version++;
>  	inode->i_mtime = inode->i_ctime = CURRENT_TIME;
> +	inode->i_version = 1;
>  	ext4_mark_inode_dirty(handle, inode);
>  	mutex_unlock(&inode->i_mutex);
>  	return len - towrite;


Is this correct ? . Why do we set the qutoa file inodes version to 1  during write ?


- aneesh

Re: [EXT4 set 4][PATCH 4/5] i_version:ext4 inode version update

[Andreas Dilger]

On Jul 03, 2007  12:19 +0530, Aneesh Kumar K.V wrote:
> Mingming Cao wrote:
> >Index: linux-2.6.22-rc4/fs/ext4/super.c
> >===================================================================
> >--- linux-2.6.22-rc4.orig/fs/ext4/super.c	2007-06-13 
> >17:19:11.000000000 -0700
> >+++ linux-2.6.22-rc4/fs/ext4/super.c	2007-06-13 17:24:45.000000000 -0700
> >@@ -2846,8 +2846,8 @@ out:
> > 		i_size_write(inode, off+len-towrite);
> > 		EXT4_I(inode)->i_disksize = inode->i_size;
> > 	}
> >-	inode->i_version++;
> > 	inode->i_mtime = inode->i_ctime = CURRENT_TIME;
> >+	inode->i_version = 1;
> > 	ext4_mark_inode_dirty(handle, inode);
> > 	mutex_unlock(&inode->i_mutex);
> > 	return len - towrite;
> 
> 
> Is this correct ? . Why do we set the qutoa file inodes version to 1  
> during write ?

Hmm, I thought we had previously fixed this?

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

Re: [EXT4 set 4][PATCH 4/5] i_version:ext4 inode version update

[Andrew Morton]

On Sun, 01 Jul 2007 03:37:45 -0400
Mingming Cao <cmm@us.ibm.com> wrote:

> This patch is on top of i_version_update_vfs.
> The i_version field of the inode is set on inode creation and incremented when
> the inode is being modified.
> 

Again, I don't think I've ever seen this patch before.  It is at least a
month old.

> 
> Index: linux-2.6.22-rc4/fs/ext4/ialloc.c
> ===================================================================
> --- linux-2.6.22-rc4.orig/fs/ext4/ialloc.c	2007-06-13 17:16:28.000000000 -0700
> +++ linux-2.6.22-rc4/fs/ext4/ialloc.c	2007-06-13 17:24:45.000000000 -0700
> @@ -565,6 +565,7 @@ got:
>  	inode->i_blocks = 0;
>  	inode->i_mtime = inode->i_atime = inode->i_ctime = ei->i_crtime =
>  						       ext4_current_time(inode);
> +	inode->i_version = 1;
>  
>  	memset(ei->i_data, 0, sizeof(ei->i_data));
>  	ei->i_dir_start_lookup = 0;
> Index: linux-2.6.22-rc4/fs/ext4/inode.c
> ===================================================================
> --- linux-2.6.22-rc4.orig/fs/ext4/inode.c	2007-06-13 17:21:29.000000000 -0700
> +++ linux-2.6.22-rc4/fs/ext4/inode.c	2007-06-13 17:24:45.000000000 -0700
> @@ -3082,6 +3082,7 @@ int ext4_mark_iloc_dirty(handle_t *handl
>  {
>  	int err = 0;
>  
> +	inode->i_version++;
>  	/* the do_update_inode consumes one bh->b_count */
>  	get_bh(iloc->bh);
>  
> Index: linux-2.6.22-rc4/fs/ext4/super.c
> ===================================================================
> --- linux-2.6.22-rc4.orig/fs/ext4/super.c	2007-06-13 17:19:11.000000000 -0700
> +++ linux-2.6.22-rc4/fs/ext4/super.c	2007-06-13 17:24:45.000000000 -0700
> @@ -2846,8 +2846,8 @@ out:
>  		i_size_write(inode, off+len-towrite);
>  		EXT4_I(inode)->i_disksize = inode->i_size;
>  	}
> -	inode->i_version++;
>  	inode->i_mtime = inode->i_ctime = CURRENT_TIME;
> +	inode->i_version = 1;
>  	ext4_mark_inode_dirty(handle, inode);
>  	mutex_unlock(&inode->i_mutex);
>  	return len - towrite;

ext4 already has code to update i_version on directories.  Here we appear
to be udpating it on regular files?

But for what reason?  The changelog doesn't say?

AFAICT the code forgets to update i_version during file overwrites (unless
the overwrite was over a hole).  But without a decent description of this
change I cannot tell whether this was a bug.

Re: [EXT4 set 4][PATCH 4/5] i_version:ext4 inode version update

[Christoph Hellwig]

On Sun, Jul 01, 2007 at 03:37:45AM -0400, Mingming Cao wrote:
> This patch is on top of i_version_update_vfs.
> The i_version field of the inode is set on inode creation and incremented when
> the inode is being modified.

Which is not what i_version is supposed to do.  It'll get you tons of misses
for NFSv3 filehandles that rely on the generation staying the same for the
same file.  Please add a new field for the NFSv4 sequence counter instead
of making i_version unuseable.

Re: [EXT4 set 4][PATCH 4/5] i_version:ext4 inode version update

[Andreas Dilger]

On Jul 11, 2007  09:47 +0100, Christoph Hellwig wrote:
> On Sun, Jul 01, 2007 at 03:37:45AM -0400, Mingming Cao wrote:
> > This patch is on top of i_version_update_vfs.
> > The i_version field of the inode is set on inode creation and incremented
> > when the inode is being modified.
> 
> Which is not what i_version is supposed to do.  It'll get you tons of misses
> for NFSv3 filehandles that rely on the generation staying the same for the
> same file.  Please add a new field for the NFSv4 sequence counter instead
> of making i_version unuseable.

You are confusing i_generation (the instance of this inode number) with
i_version (whether this file has been modified)?

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

Re: [EXT4 set 4][PATCH 4/5] i_version:ext4 inode version update

[Trond Myklebust]

On Wed, 2007-07-11 at 09:47 +0100, Christoph Hellwig wrote:
> On Sun, Jul 01, 2007 at 03:37:45AM -0400, Mingming Cao wrote:
> > This patch is on top of i_version_update_vfs.
> > The i_version field of the inode is set on inode creation and incremented when
> > the inode is being modified.
> 
> Which is not what i_version is supposed to do.  It'll get you tons of misses
> for NFSv3 filehandles that rely on the generation staying the same for the
> same file.  Please add a new field for the NFSv4 sequence counter instead
> of making i_version unuseable.

Aren't you confusing i_version and i_generation here? Those are two
separate inode fields.

Cheers
  Trond

Re: [EXT4 set 4][PATCH 4/5] i_version:ext4 inode version update

[Christoph Hellwig]

On Wed, Jul 11, 2007 at 05:52:24AM -0600, Andreas Dilger wrote:
> On Jul 11, 2007  09:47 +0100, Christoph Hellwig wrote:
> > On Sun, Jul 01, 2007 at 03:37:45AM -0400, Mingming Cao wrote:
> > > This patch is on top of i_version_update_vfs.
> > > The i_version field of the inode is set on inode creation and incremented
> > > when the inode is being modified.
> > 
> > Which is not what i_version is supposed to do.  It'll get you tons of misses
> > for NFSv3 filehandles that rely on the generation staying the same for the
> > same file.  Please add a new field for the NFSv4 sequence counter instead
> > of making i_version unuseable.
> 
> You are confusing i_generation (the instance of this inode number) with
> i_version (whether this file has been modified)?

Yes, sorry.  Objection dropped.