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 <[email protected]>
Signed-off-by: Mingming Cao <[email protected]>
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;
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
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.
> ta));
> 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);
>
If we bump i_version in ext4_mark_iloc_dity then we should be removing the i_version update
at other places. A simple grep of ext4 dir shows i_version being updated followed by
ext4_mark_inode_dirty.
-aneesh
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.
Aneesh Kumar K.V wrote:
>
> If we bump i_version in ext4_mark_iloc_dity then we should be removing
> the i_version update
> at other places. A simple grep of ext4 dir shows i_version being updated
> followed by ext4_mark_inode_dirty.
>
>
How about the below patch. I also removed the i_version = 1 from
ext4_new_inode because we do the same in ext4_alloc_inode. I also added
the From: line.
-aneesh
>From ccdfc4a6a8edbdbf5d21d32fd2350f36399a89f8 Mon Sep 17 00:00:00 2001
From: Jean Noel Cordenner <[email protected]>
Date: Wed, 4 Jul 2007 10:51:50 +0530
Subject: [PATCH] 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 <[email protected]>
Signed-off-by: Aneesh Kumar K.V <[email protected]>
Signed-off-by: Mingming Cao <[email protected]>
---
fs/ext4/inode.c | 1 +
fs/ext4/namei.c | 4 ----
fs/ext4/super.c | 1 -
3 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index cb9f1f4..cd3e886 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3084,6 +3084,7 @@ int ext4_mark_iloc_dirty(handle_t *handle,
{
int err = 0;
+ inode->i_version++;
/* the do_update_inode consumes one bh->b_count */
get_bh(iloc->bh);
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 477b6d2..a1dc5ef 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1287,7 +1287,6 @@ static int add_dirent_to_buf(handle_t *handle, struct dentry *dentry,
*/
dir->i_mtime = dir->i_ctime = ext4_current_time(dir);
ext4_update_dx_flag(dir);
- dir->i_version++;
ext4_mark_inode_dirty(handle, dir);
BUFFER_TRACE(bh, "call ext4_journal_dirty_metadata");
err = ext4_journal_dirty_metadata(handle, bh);
@@ -1606,7 +1605,6 @@ static int ext4_delete_entry (handle_t *handle,
le16_to_cpu(de->rec_len));
else
de->inode = 0;
- dir->i_version++;
BUFFER_TRACE(bh, "call ext4_journal_dirty_metadata");
ext4_journal_dirty_metadata(handle, bh);
return 0;
@@ -2039,7 +2037,6 @@ static int ext4_rmdir (struct inode * dir, struct dentry *dentry)
ext4_warning (inode->i_sb, "ext4_rmdir",
"empty directory has nlink!=2 (%d)",
inode->i_nlink);
- inode->i_version++;
clear_nlink(inode);
/* There's no need to set i_disksize: the fact that i_nlink is
* zero will ensure that the right thing happens during any
@@ -2284,7 +2281,6 @@ static int ext4_rename (struct inode * old_dir, struct dentry *old_dentry,
if (EXT4_HAS_INCOMPAT_FEATURE(new_dir->i_sb,
EXT4_FEATURE_INCOMPAT_FILETYPE))
new_de->file_type = old_de->file_type;
- new_dir->i_version++;
BUFFER_TRACE(new_bh, "call ext4_journal_dirty_metadata");
ext4_journal_dirty_metadata(handle, new_bh);
brelse(new_bh);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 914c273..55b99b3 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2846,7 +2846,6 @@ 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;
ext4_mark_inode_dirty(handle, inode);
mutex_unlock(&inode->i_mutex);
--
1.5.3.rc0-dirty
Aneesh Kumar K.V wrote:
>
> If we bump i_version in ext4_mark_iloc_dity then we should be removing
> the i_version update
> at other places. A simple grep of ext4 dir shows i_version being updated
> followed by ext4_mark_inode_dirty.
>
>
How about the below patch. I also removed the i_version = 1 from
ext4_new_inode because we do the same in ext4_alloc_inode. I also added
the From: line.
-aneesh
>From ccdfc4a6a8edbdbf5d21d32fd2350f36399a89f8 Mon Sep 17 00:00:00 2001
From: Jean Noel Cordenner <[email protected]>
Date: Wed, 4 Jul 2007 10:51:50 +0530
Subject: [PATCH] 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 <[email protected]>
Signed-off-by: Aneesh Kumar K.V <[email protected]>
Signed-off-by: Mingming Cao <[email protected]>
---
fs/ext4/inode.c | 1 +
fs/ext4/namei.c | 4 ----
fs/ext4/super.c | 1 -
3 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index cb9f1f4..cd3e886 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3084,6 +3084,7 @@ int ext4_mark_iloc_dirty(handle_t *handle,
{
int err = 0;
+ inode->i_version++;
/* the do_update_inode consumes one bh->b_count */
get_bh(iloc->bh);
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 477b6d2..a1dc5ef 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1287,7 +1287,6 @@ static int add_dirent_to_buf(handle_t *handle, struct dentry *dentry,
*/
dir->i_mtime = dir->i_ctime = ext4_current_time(dir);
ext4_update_dx_flag(dir);
- dir->i_version++;
ext4_mark_inode_dirty(handle, dir);
BUFFER_TRACE(bh, "call ext4_journal_dirty_metadata");
err = ext4_journal_dirty_metadata(handle, bh);
@@ -1606,7 +1605,6 @@ static int ext4_delete_entry (handle_t *handle,
le16_to_cpu(de->rec_len));
else
de->inode = 0;
- dir->i_version++;
BUFFER_TRACE(bh, "call ext4_journal_dirty_metadata");
ext4_journal_dirty_metadata(handle, bh);
return 0;
@@ -2039,7 +2037,6 @@ static int ext4_rmdir (struct inode * dir, struct dentry *dentry)
ext4_warning (inode->i_sb, "ext4_rmdir",
"empty directory has nlink!=2 (%d)",
inode->i_nlink);
- inode->i_version++;
clear_nlink(inode);
/* There's no need to set i_disksize: the fact that i_nlink is
* zero will ensure that the right thing happens during any
@@ -2284,7 +2281,6 @@ static int ext4_rename (struct inode * old_dir, struct dentry *old_dentry,
if (EXT4_HAS_INCOMPAT_FEATURE(new_dir->i_sb,
EXT4_FEATURE_INCOMPAT_FILETYPE))
new_de->file_type = old_de->file_type;
- new_dir->i_version++;
BUFFER_TRACE(new_bh, "call ext4_journal_dirty_metadata");
ext4_journal_dirty_metadata(handle, new_bh);
brelse(new_bh);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 914c273..55b99b3 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2846,7 +2846,6 @@ 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;
ext4_mark_inode_dirty(handle, inode);
mutex_unlock(&inode->i_mutex);
--
1.5.3.rc0-dirty
On Wed, 2007-07-04 at 10:56 +0530, Aneesh Kumar K.V wrote:
>
> Aneesh Kumar K.V wrote:
>
> >
> > If we bump i_version in ext4_mark_iloc_dity then we should be removing
> > the i_version update
> > at other places. A simple grep of ext4 dir shows i_version being updated
> > followed by ext4_mark_inode_dirty.
> >
> >
>
> How about the below patch. I also removed the i_version = 1 from
> ext4_new_inode because we do the same in ext4_alloc_inode. I also added
> the From: line.
>
Acked.
Ha, I actually just finished on a incremental patch to cleanup redundant
i_version update.:) But I missed the ext4_new_inode().
Thanks. I will replace this patch in the ext4 patch queue
> -aneesh
>
> From ccdfc4a6a8edbdbf5d21d32fd2350f36399a89f8 Mon Sep 17 00:00:00 2001
> From: Jean Noel Cordenner <[email protected]>
> Date: Wed, 4 Jul 2007 10:51:50 +0530
> Subject: [PATCH] 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 <[email protected]>
> Signed-off-by: Aneesh Kumar K.V <[email protected]>
> Signed-off-by: Mingming Cao <[email protected]>
> ---
> fs/ext4/inode.c | 1 +
> fs/ext4/namei.c | 4 ----
> fs/ext4/super.c | 1 -
> 3 files changed, 1 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index cb9f1f4..cd3e886 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3084,6 +3084,7 @@ int ext4_mark_iloc_dirty(handle_t *handle,
> {
> int err = 0;
>
> + inode->i_version++;
> /* the do_update_inode consumes one bh->b_count */
> get_bh(iloc->bh);
>
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 477b6d2..a1dc5ef 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -1287,7 +1287,6 @@ static int add_dirent_to_buf(handle_t *handle, struct dentry *dentry,
> */
> dir->i_mtime = dir->i_ctime = ext4_current_time(dir);
> ext4_update_dx_flag(dir);
> - dir->i_version++;
> ext4_mark_inode_dirty(handle, dir);
> BUFFER_TRACE(bh, "call ext4_journal_dirty_metadata");
> err = ext4_journal_dirty_metadata(handle, bh);
> @@ -1606,7 +1605,6 @@ static int ext4_delete_entry (handle_t *handle,
> le16_to_cpu(de->rec_len));
> else
> de->inode = 0;
> - dir->i_version++;
> BUFFER_TRACE(bh, "call ext4_journal_dirty_metadata");
> ext4_journal_dirty_metadata(handle, bh);
> return 0;
> @@ -2039,7 +2037,6 @@ static int ext4_rmdir (struct inode * dir, struct dentry *dentry)
> ext4_warning (inode->i_sb, "ext4_rmdir",
> "empty directory has nlink!=2 (%d)",
> inode->i_nlink);
> - inode->i_version++;
> clear_nlink(inode);
> /* There's no need to set i_disksize: the fact that i_nlink is
> * zero will ensure that the right thing happens during any
> @@ -2284,7 +2281,6 @@ static int ext4_rename (struct inode * old_dir, struct dentry *old_dentry,
> if (EXT4_HAS_INCOMPAT_FEATURE(new_dir->i_sb,
> EXT4_FEATURE_INCOMPAT_FILETYPE))
> new_de->file_type = old_de->file_type;
> - new_dir->i_version++;
> BUFFER_TRACE(new_bh, "call ext4_journal_dirty_metadata");
> ext4_journal_dirty_metadata(handle, new_bh);
> brelse(new_bh);
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 914c273..55b99b3 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2846,7 +2846,6 @@ 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;
> ext4_mark_inode_dirty(handle, inode);
> mutex_unlock(&inode->i_mutex);
On Sun, 01 Jul 2007 03:37:45 -0400
Mingming Cao <[email protected]> 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.
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.
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.
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
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.