[PATCH] ext4: fix i_version handling in ext4

[PATCH] ext4: fix i_version handling in ext4

[Jeff Layton]

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 <lczerner@redhat.com>
Cc: Jan Kara <jack@suse.cz>
Cc: Christian Brauner <brauner@kernel.org>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 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

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

[Christian Brauner]

On Tue, Aug 16, 2022 at 09:15:22AM -0400, Jeff Layton wrote:
> 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 <lczerner@redhat.com>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Christian Brauner <brauner@kernel.org>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---

Seems good to me. But it seems that the xfs patch you sent does have
inode_inc_version() right after setattr_copy() as well. So I wonder if
we couldn't just try and move inode_inc_version() into setattr_copy()
itself.

>  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
> 

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

[Jeff Layton]

On Tue, 2022-08-16 at 15:33 +0200, Christian Brauner wrote:
> On Tue, Aug 16, 2022 at 09:15:22AM -0400, Jeff Layton wrote:
> > 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 <lczerner@redhat.com>
> > Cc: Jan Kara <jack@suse.cz>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> 
> Seems good to me. But it seems that the xfs patch you sent does have
> inode_inc_version() right after setattr_copy() as well. So I wonder if
> we couldn't just try and move inode_inc_version() into setattr_copy()
> itself.
> 

We probably could, but setattr_copy has a lot of callers and most
filesystems don't need this.  Also, there are some cases where we don't
want to update the i_version after a setattr.

In particular, if you do a truncate and the size doesn't change, then
you really don't want to update the timestamps (and therefore the
i_version shouldn't change either).


> >  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 <jlayton@kernel.org>

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

[Christian Brauner]

On Tue, Aug 16, 2022 at 09:43:16AM -0400, Jeff Layton wrote:
> On Tue, 2022-08-16 at 15:33 +0200, Christian Brauner wrote:
> > On Tue, Aug 16, 2022 at 09:15:22AM -0400, Jeff Layton wrote:
> > > 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 <lczerner@redhat.com>
> > > Cc: Jan Kara <jack@suse.cz>
> > > Cc: Christian Brauner <brauner@kernel.org>
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > 
> > Seems good to me. But it seems that the xfs patch you sent does have
> > inode_inc_version() right after setattr_copy() as well. So I wonder if
> > we couldn't just try and move inode_inc_version() into setattr_copy()
> > itself.
> > 
> 
> We probably could, but setattr_copy has a lot of callers and most
> filesystems don't need this.  Also, there are some cases where we don't
> want to update the i_version after a setattr.
> 
> In particular, if you do a truncate and the size doesn't change, then
> you really don't want to update the timestamps (and therefore the
> i_version shouldn't change either).

We could probably all handle that with some massaging but I'm also fine
with doing it just for ext4 and xfs if these are the only ones where
this is relevant:

Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>

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

[Jan Kara]

On Tue 16-08-22 09:15:22, Jeff Layton wrote:
> 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 <lczerner@redhat.com>
> 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

> ---
>  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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Jeff Layton]

On Wed, 2022-08-17 at 15:04 +0200, Jan Kara wrote:
> On Tue 16-08-22 09:15:22, Jeff Layton wrote:
> > 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 <lczerner@redhat.com>
> > 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. 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.


> > ---
> >  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 <jlayton@kernel.org>

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

[Jan Kara]

On Wed 17-08-22 09:09:58, Jeff Layton wrote:
> On Wed, 2022-08-17 at 15:04 +0200, Jan Kara wrote:
> > On Tue 16-08-22 09:15:22, Jeff Layton wrote:
> > > 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 <lczerner@redhat.com>
> > > 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

> 
> 
> > > ---
> > >  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 <jlayton@kernel.org>
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Jeff Layton]

On Wed, 2022-08-17 at 15:25 +0200, Jan Kara wrote:
> On Wed 17-08-22 09:09:58, Jeff Layton wrote:
> > On Wed, 2022-08-17 at 15:04 +0200, Jan Kara wrote:
> > > On Tue 16-08-22 09:15:22, Jeff Layton wrote:
> > > > 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 <lczerner@redhat.com>
> > > > 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.
> 

Oh, ok! I mostly noticed the places I was "missing" by inspection. It's
possible that I don't need those changes after all. If you think this
patch is sufficient, then I'll plan to just go with this one.

> > 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
> 
> > 
> > 
> > > > ---
> > > >  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 <jlayton@kernel.org>

-- 
Jeff Layton <jlayton@kernel.org>

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

[Jan Kara]

On Wed 17-08-22 09:28:52, Jeff Layton wrote:
> On Wed, 2022-08-17 at 15:25 +0200, Jan Kara wrote:
> > On Wed 17-08-22 09:09:58, Jeff Layton wrote:
> > > On Wed, 2022-08-17 at 15:04 +0200, Jan Kara wrote:
> > > > On Tue 16-08-22 09:15:22, Jeff Layton wrote:
> > > > > 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 <lczerner@redhat.com>
> > > > > 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.
> > 
> 
> Oh, ok! I mostly noticed the places I was "missing" by inspection. It's
> possible that I don't need those changes after all. If you think this
> patch is sufficient, then I'll plan to just go with this one.

So I did some more grepping and I think we are missing i_version increment
in ext4 ioctl handling. In particular stuff like swap_inode_boot_loader(),
ext4_ioctl_setflags(), ext4_ioctl_setproject(), and EXT4_IOC_SETVERSION
handling in __ext4_ioctl() need i_version increment. Similarly for defrag
ioctl implemented in ext4_move_extents() (that even seems to miss a ctime
update).

 								Honza

> > > > > ---
> > > > >  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 <jlayton@kernel.org>
> 
> -- 
> Jeff Layton <jlayton@kernel.org>
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH] ext4: fix i_version handling in ext4

[Jeff Layton]

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 and unnecessary remeasurements for IMA. 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, setxattr and
ioctl 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.

In ext4_move_extents, increment the i_version on both inodes, and also
add in missing ctime updates.

Cc: Lukas Czerner <lczerner@redhat.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ext4/inode.c       | 10 +++++-----
 fs/ext4/ioctl.c       |  4 ++++
 fs/ext4/move_extent.c |  6 ++++++
 fs/ext4/xattr.c       |  2 ++
 4 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 601214453c3a..aa37bce4c541 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_I_VERSION(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/ioctl.c b/fs/ext4/ioctl.c
index 3cf3ec4b1c21..ad3a294a88eb 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -452,6 +452,7 @@ static long swap_inode_boot_loader(struct super_block *sb,
 	swap_inode_data(inode, inode_bl);
 
 	inode->i_ctime = inode_bl->i_ctime = current_time(inode);
+	inode_inc_iversion(inode);
 
 	inode->i_generation = prandom_u32();
 	inode_bl->i_generation = prandom_u32();
@@ -665,6 +666,7 @@ static int ext4_ioctl_setflags(struct inode *inode,
 	ext4_set_inode_flags(inode, false);
 
 	inode->i_ctime = current_time(inode);
+	inode_inc_iversion(inode);
 
 	err = ext4_mark_iloc_dirty(handle, inode, &iloc);
 flags_err:
@@ -775,6 +777,7 @@ static int ext4_ioctl_setproject(struct inode *inode, __u32 projid)
 
 	EXT4_I(inode)->i_projid = kprojid;
 	inode->i_ctime = current_time(inode);
+	inode_inc_iversion(inode);
 out_dirty:
 	rc = ext4_mark_iloc_dirty(handle, inode, &iloc);
 	if (!err)
@@ -1257,6 +1260,7 @@ static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		err = ext4_reserve_inode_write(handle, inode, &iloc);
 		if (err == 0) {
 			inode->i_ctime = current_time(inode);
+			inode_inc_iversion(inode);
 			inode->i_generation = generation;
 			err = ext4_mark_iloc_dirty(handle, inode, &iloc);
 		}
diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
index 701f1d6a217f..285700b00d38 100644
--- a/fs/ext4/move_extent.c
+++ b/fs/ext4/move_extent.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/fs.h>
+#include <linux/iversion.h>
 #include <linux/quotaops.h>
 #include <linux/slab.h>
 #include <linux/sched/mm.h>
@@ -683,6 +684,11 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk,
 			break;
 		o_start += cur_len;
 		d_start += cur_len;
+
+		orig_inode->i_ctime = current_time(orig_inode);
+		donor_inode->i_ctime = current_time(donor_inode);
+		inode_inc_iversion(orig_inode);
+		inode_inc_iversion(donor_inode);
 	}
 	*moved_len = o_start - orig_blk;
 	if (*moved_len > len)
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 533216e80fa2..e975442e4ab2 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_I_VERSION(inode))
+			inode_inc_iversion(inode);
 		if (!value)
 			no_expand = 0;
 		error = ext4_mark_iloc_dirty(handle, inode, &is.iloc);
-- 
2.37.2

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

[Jeff Layton]

Oops, I should have labeled this "PATCH v3". In any case, I think we've
covered all of the places where it should get bumped now.

On Fri, 2022-08-19 at 07:36 -0400, Jeff Layton wrote:
> 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 and unnecessary remeasurements for IMA. 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, setxattr and
> ioctl 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.
> 
> In ext4_move_extents, increment the i_version on both inodes, and also
> add in missing ctime updates.
> 
> Cc: Lukas Czerner <lczerner@redhat.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ext4/inode.c       | 10 +++++-----
>  fs/ext4/ioctl.c       |  4 ++++
>  fs/ext4/move_extent.c |  6 ++++++
>  fs/ext4/xattr.c       |  2 ++
>  4 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 601214453c3a..aa37bce4c541 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_I_VERSION(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/ioctl.c b/fs/ext4/ioctl.c
> index 3cf3ec4b1c21..ad3a294a88eb 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -452,6 +452,7 @@ static long swap_inode_boot_loader(struct super_block *sb,
>  	swap_inode_data(inode, inode_bl);
>  
>  	inode->i_ctime = inode_bl->i_ctime = current_time(inode);
> +	inode_inc_iversion(inode);
>  
>  	inode->i_generation = prandom_u32();
>  	inode_bl->i_generation = prandom_u32();
> @@ -665,6 +666,7 @@ static int ext4_ioctl_setflags(struct inode *inode,
>  	ext4_set_inode_flags(inode, false);
>  
>  	inode->i_ctime = current_time(inode);
> +	inode_inc_iversion(inode);
>  
>  	err = ext4_mark_iloc_dirty(handle, inode, &iloc);
>  flags_err:
> @@ -775,6 +777,7 @@ static int ext4_ioctl_setproject(struct inode *inode, __u32 projid)
>  
>  	EXT4_I(inode)->i_projid = kprojid;
>  	inode->i_ctime = current_time(inode);
> +	inode_inc_iversion(inode);
>  out_dirty:
>  	rc = ext4_mark_iloc_dirty(handle, inode, &iloc);
>  	if (!err)
> @@ -1257,6 +1260,7 @@ static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		err = ext4_reserve_inode_write(handle, inode, &iloc);
>  		if (err == 0) {
>  			inode->i_ctime = current_time(inode);
> +			inode_inc_iversion(inode);
>  			inode->i_generation = generation;
>  			err = ext4_mark_iloc_dirty(handle, inode, &iloc);
>  		}
> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
> index 701f1d6a217f..285700b00d38 100644
> --- a/fs/ext4/move_extent.c
> +++ b/fs/ext4/move_extent.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/fs.h>
> +#include <linux/iversion.h>
>  #include <linux/quotaops.h>
>  #include <linux/slab.h>
>  #include <linux/sched/mm.h>
> @@ -683,6 +684,11 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk,
>  			break;
>  		o_start += cur_len;
>  		d_start += cur_len;
> +
> +		orig_inode->i_ctime = current_time(orig_inode);
> +		donor_inode->i_ctime = current_time(donor_inode);
> +		inode_inc_iversion(orig_inode);
> +		inode_inc_iversion(donor_inode);
>  	}
>  	*moved_len = o_start - orig_blk;
>  	if (*moved_len > len)
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 533216e80fa2..e975442e4ab2 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_I_VERSION(inode))
> +			inode_inc_iversion(inode);
>  		if (!value)
>  			no_expand = 0;
>  		error = ext4_mark_iloc_dirty(handle, inode, &is.iloc);

-- 
Jeff Layton <jlayton@kernel.org>