Re: [PATCH] ext4: fix i_version handling in ext4

On Wed 17-08-22 09:09:58, Jeff Layton wrote:

On Wed, 2022-08-17 at 15:04 +0200, Jan Kara wrote:

quoted

On Tue 16-08-22 09:15:22, Jeff Layton wrote:

quoted

ext4 currently updates the i_version counter when the atime is updated
during a read. This is less than ideal as it can cause unnecessary cache
invalidations with NFSv4. The increment in ext4_mark_iloc_dirty is also
problematic since it can also corrupt the i_version counter for
ea_inodes.

We aren't bumping the file times in ext4_mark_iloc_dirty, so changing
the i_version there seems wrong, and is the cause of both problems.
Remove that callsite and add increments to the setattr and setxattr
codepaths (at the same time that we update the ctime). The i_version
bump that already happens during timestamp updates should take care of
the rest.

Cc: Lukas Czerner <redacted>
Cc: Jan Kara <jack@suse.cz>
Cc: Christian Brauner <brauner@kernel.org>
Signed-off-by: Jeff Layton <jlayton@kernel.org>

After some verification (which was not completely trivial e.g. for
directories) I agree all cases should be covered. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

Thanks.

I think this covers the typical cases, but there are some places I
missed:

The setacl codepath, for one, and there are a number of places that set
the ctime explicitly for hole punching and the like.

Hum, why is setacl() not covered by your change to ext4_xattr_set_handle()?
ext4_set_acl() ends up calling it... I have checked hole punching (whole
ext4_fallocate()) and it seems to be incrementing iversion where needed.

I'm planning to
send a v2 once I do a bit more testing. I'll hold off on adding your
Reviewed-by just yet, since the final patch may be quite a bit
different.

OK, thanks!

								Honza

quoted

quoted

---
 fs/ext4/inode.c | 10 +++++-----
 fs/ext4/xattr.c |  2 ++
 2 files changed, 7 insertions(+), 5 deletions(-)

I think this patch should probably supersede Lukas' patch entitled:

    ext4: don't increase iversion counter for ea_inodes

This will also mean that we'll need to respin the patch to turn on the
i_version counter unconditionally in ext4 (though that should be
trivial).

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 601214453c3a..a70921df89a5 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c

@@ -5342,6 +5342,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	int error, rc = 0;
 	int orphan = 0;
 	const unsigned int ia_valid = attr->ia_valid;
+	bool inc_ivers = IS_IVERSION(inode);
 
 	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
 		return -EIO;

@@ -5425,8 +5426,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 			return -EINVAL;
 		}
 
-		if (IS_I_VERSION(inode) && attr->ia_size != inode->i_size)
-			inode_inc_iversion(inode);
+		if (attr->ia_size == inode->i_size)
+			inc_ivers = false;
 
 		if (shrink) {
 			if (ext4_should_order_data(inode)) {

@@ -5528,6 +5529,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	}
 
 	if (!error) {
+		if (inc_ivers)
+			inode_inc_iversion(inode);
 		setattr_copy(mnt_userns, inode, attr);
 		mark_inode_dirty(inode);
 	}

@@ -5731,9 +5734,6 @@ int ext4_mark_iloc_dirty(handle_t *handle,
 	}
 	ext4_fc_track_inode(handle, inode);
 
-	if (IS_I_VERSION(inode))
-		inode_inc_iversion(inode);
-
 	/* the do_update_inode consumes one bh->b_count */
 	get_bh(iloc->bh);
 

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 533216e80fa2..4d84919d1c9c 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c

@@ -2412,6 +2412,8 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,
 	if (!error) {
 		ext4_xattr_update_super_block(handle, inode->i_sb);
 		inode->i_ctime = current_time(inode);
+		if (IS_IVERSION(inode))
+			inode_inc_iversion(inode);
 		if (!value)
 			no_expand = 0;
 		error = ext4_mark_iloc_dirty(handle, inode, &is.iloc);

-- 
2.37.2

-- 
Jeff Layton [off-list ref]

-- 
Jan Kara [off-list ref]
SUSE Labs, CR