[PATCH] ext4: unconditionally enable the i_version counter

[PATCH] ext4: unconditionally enable the i_version counter

[Jeff Layton]

The original i_version implementation was pretty expensive, requiring a
log flush on every change. Because of this, it was gated behind a mount
option (implemented via the MS_I_VERSION mountoption flag).

Commit ae5e165d855d (fs: new API for handling inode->i_version) made the
i_version flag much less expensive, so there is no longer a performance
penalty from enabling it.

Have ext4 ignore the SB_I_VERSION flag, and just enable it
unconditionally.

Cc: Dave Chinner <david@fromorbit.com>
Cc: Lukas Czerner <lczerner@redhat.com>
Cc: Benjamin Coddington <bcodding@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ext4/inode.c | 5 ++---
 fs/ext4/super.c | 8 ++++----
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 84c0eb55071d..c785c0b72116 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5411,7 +5411,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 			return -EINVAL;
 		}
 
-		if (IS_I_VERSION(inode) && attr->ia_size != inode->i_size)
+		if (attr->ia_size != inode->i_size)
 			inode_inc_iversion(inode);
 
 		if (shrink) {
@@ -5717,8 +5717,7 @@ int ext4_mark_iloc_dirty(handle_t *handle,
 	}
 	ext4_fc_track_inode(handle, inode);
 
-	if (IS_I_VERSION(inode))
-		inode_inc_iversion(inode);
+	inode_inc_iversion(inode);
 
 	/* the do_update_inode consumes one bh->b_count */
 	get_bh(iloc->bh);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 845f2f8aee5f..30645d4343b6 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2142,8 +2142,7 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
 		return 0;
 	case Opt_i_version:
 		ext4_msg(NULL, KERN_WARNING, deprecated_msg, param->key, "5.20");
-		ext4_msg(NULL, KERN_WARNING, "Use iversion instead\n");
-		ctx_set_flags(ctx, SB_I_VERSION);
+		ext4_msg(NULL, KERN_WARNING, "i_version counter is always enabled.\n");
 		return 0;
 	case Opt_inlinecrypt:
 #ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
@@ -2970,8 +2969,6 @@ static int _ext4_show_options(struct seq_file *seq, struct super_block *sb,
 		SEQ_OPTS_PRINT("min_batch_time=%u", sbi->s_min_batch_time);
 	if (nodefs || sbi->s_max_batch_time != EXT4_DEF_MAX_BATCH_TIME)
 		SEQ_OPTS_PRINT("max_batch_time=%u", sbi->s_max_batch_time);
-	if (sb->s_flags & SB_I_VERSION)
-		SEQ_OPTS_PUTS("i_version");
 	if (nodefs || sbi->s_stripe)
 		SEQ_OPTS_PRINT("stripe=%lu", sbi->s_stripe);
 	if (nodefs || EXT4_MOUNT_DATA_FLAGS &
@@ -4630,6 +4627,9 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	sb->s_flags = (sb->s_flags & ~SB_POSIXACL) |
 		(test_opt(sb, POSIX_ACL) ? SB_POSIXACL : 0);
 
+	/* i_version is always enabled now */
+	sb->s_flags |= SB_I_VERSION;
+
 	if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV &&
 	    (ext4_has_compat_features(sb) ||
 	     ext4_has_ro_compat_features(sb) ||
-- 
2.37.1

Re: [PATCH] ext4: unconditionally enable the i_version counter

[Darrick J. Wong]

On Mon, Jul 25, 2022 at 03:29:46PM -0400, Jeff Layton wrote:
> The original i_version implementation was pretty expensive, requiring a
> log flush on every change. Because of this, it was gated behind a mount
> option (implemented via the MS_I_VERSION mountoption flag).
> 
> Commit ae5e165d855d (fs: new API for handling inode->i_version) made the
> i_version flag much less expensive, so there is no longer a performance
> penalty from enabling it.
> 
> Have ext4 ignore the SB_I_VERSION flag, and just enable it
> unconditionally.
> 
> Cc: Dave Chinner <david@fromorbit.com>
> Cc: Lukas Czerner <lczerner@redhat.com>
> Cc: Benjamin Coddington <bcodding@redhat.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ext4/inode.c | 5 ++---
>  fs/ext4/super.c | 8 ++++----
>  2 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 84c0eb55071d..c785c0b72116 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5411,7 +5411,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
>  			return -EINVAL;
>  		}
>  
> -		if (IS_I_VERSION(inode) && attr->ia_size != inode->i_size)
> +		if (attr->ia_size != inode->i_size)
>  			inode_inc_iversion(inode);
>  
>  		if (shrink) {
> @@ -5717,8 +5717,7 @@ int ext4_mark_iloc_dirty(handle_t *handle,
>  	}
>  	ext4_fc_track_inode(handle, inode);
>  
> -	if (IS_I_VERSION(inode))
> -		inode_inc_iversion(inode);
> +	inode_inc_iversion(inode);
>  
>  	/* the do_update_inode consumes one bh->b_count */
>  	get_bh(iloc->bh);
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 845f2f8aee5f..30645d4343b6 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2142,8 +2142,7 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
>  		return 0;
>  	case Opt_i_version:
>  		ext4_msg(NULL, KERN_WARNING, deprecated_msg, param->key, "5.20");

Perhaps it's time to kill off Opt_i_version, since we're now at 5.20?

(For that matter, noacl/nouser_xattr were supposed to be gone by 3.5 and
they're clearly still there, so either they ought to go as well?)

--D

> -		ext4_msg(NULL, KERN_WARNING, "Use iversion instead\n");
> -		ctx_set_flags(ctx, SB_I_VERSION);
> +		ext4_msg(NULL, KERN_WARNING, "i_version counter is always enabled.\n");
>  		return 0;
>  	case Opt_inlinecrypt:
>  #ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
> @@ -2970,8 +2969,6 @@ static int _ext4_show_options(struct seq_file *seq, struct super_block *sb,
>  		SEQ_OPTS_PRINT("min_batch_time=%u", sbi->s_min_batch_time);
>  	if (nodefs || sbi->s_max_batch_time != EXT4_DEF_MAX_BATCH_TIME)
>  		SEQ_OPTS_PRINT("max_batch_time=%u", sbi->s_max_batch_time);
> -	if (sb->s_flags & SB_I_VERSION)
> -		SEQ_OPTS_PUTS("i_version");
>  	if (nodefs || sbi->s_stripe)
>  		SEQ_OPTS_PRINT("stripe=%lu", sbi->s_stripe);
>  	if (nodefs || EXT4_MOUNT_DATA_FLAGS &
> @@ -4630,6 +4627,9 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  	sb->s_flags = (sb->s_flags & ~SB_POSIXACL) |
>  		(test_opt(sb, POSIX_ACL) ? SB_POSIXACL : 0);
>  
> +	/* i_version is always enabled now */
> +	sb->s_flags |= SB_I_VERSION;
> +
>  	if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV &&
>  	    (ext4_has_compat_features(sb) ||
>  	     ext4_has_ro_compat_features(sb) ||
> -- 
> 2.37.1
> 

Re: [PATCH] ext4: unconditionally enable the i_version counter

[xuyang2018.jy]

on 2022/07/26 5:49, Darrick J. Wong wrote:
> On Mon, Jul 25, 2022 at 03:29:46PM -0400, Jeff Layton wrote:
>> The original i_version implementation was pretty expensive, requiring a
>> log flush on every change. Because of this, it was gated behind a mount
>> option (implemented via the MS_I_VERSION mountoption flag).
>>
>> Commit ae5e165d855d (fs: new API for handling inode->i_version) made the
>> i_version flag much less expensive, so there is no longer a performance
>> penalty from enabling it.
>>
>> Have ext4 ignore the SB_I_VERSION flag, and just enable it
>> unconditionally.
>>
>> Cc: Dave Chinner <david@fromorbit.com>
>> Cc: Lukas Czerner <lczerner@redhat.com>
>> Cc: Benjamin Coddington <bcodding@redhat.com>
>> Cc: Christoph Hellwig <hch@infradead.org>
>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>> ---
>>   fs/ext4/inode.c | 5 ++---
>>   fs/ext4/super.c | 8 ++++----
>>   2 files changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 84c0eb55071d..c785c0b72116 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -5411,7 +5411,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
>>   			return -EINVAL;
>>   		}
>>   
>> -		if (IS_I_VERSION(inode) && attr->ia_size != inode->i_size)
>> +		if (attr->ia_size != inode->i_size)
>>   			inode_inc_iversion(inode);
>>   
>>   		if (shrink) {
>> @@ -5717,8 +5717,7 @@ int ext4_mark_iloc_dirty(handle_t *handle,
>>   	}
>>   	ext4_fc_track_inode(handle, inode);
>>   
>> -	if (IS_I_VERSION(inode))
>> -		inode_inc_iversion(inode);
>> +	inode_inc_iversion(inode);
>>   
>>   	/* the do_update_inode consumes one bh->b_count */
>>   	get_bh(iloc->bh);
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 845f2f8aee5f..30645d4343b6 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -2142,8 +2142,7 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
>>   		return 0;
>>   	case Opt_i_version:
>>   		ext4_msg(NULL, KERN_WARNING, deprecated_msg, param->key, "5.20");
> 
> Perhaps it's time to kill off Opt_i_version, since we're now at 5.20?
> 
> (For that matter, noacl/nouser_xattr were supposed to be gone by 3.5 and
> they're clearly still there, so either they ought to go as well?)

For noacl/nouser_xattr, have some disscussion in here[1].

Also, I want to remove deprecated flag[2] but seems removing these 
deprecated options is better.

I just use noacl mount option in ltp/xfstests test case[3][4], but seems 
none use them in application because none comlain about these deprecated 
flags over the years.

[1]https://www.spinics.net/lists/linux-ext4/msg82507.html
[2]https://patchwork.ozlabs.org/project/linux-ext4/patch/1654164099-2164-1-git-send-email-xuyang2018.jy@fujitsu.com/
[3]https://patchwork.ozlabs.org/project/ltp/patch/1658485640-2188-2-git-send-email-xuyang2018.jy@fujitsu.com/
[4]https://www.spinics.net/lists/fstests/msg19554.html

Best Regards
Yang Xu
> 
> --D
> 
>> -		ext4_msg(NULL, KERN_WARNING, "Use iversion instead\n");
>> -		ctx_set_flags(ctx, SB_I_VERSION);
>> +		ext4_msg(NULL, KERN_WARNING, "i_version counter is always enabled.\n");
>>   		return 0;
>>   	case Opt_inlinecrypt:
>>   #ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
>> @@ -2970,8 +2969,6 @@ static int _ext4_show_options(struct seq_file *seq, struct super_block *sb,
>>   		SEQ_OPTS_PRINT("min_batch_time=%u", sbi->s_min_batch_time);
>>   	if (nodefs || sbi->s_max_batch_time != EXT4_DEF_MAX_BATCH_TIME)
>>   		SEQ_OPTS_PRINT("max_batch_time=%u", sbi->s_max_batch_time);
>> -	if (sb->s_flags & SB_I_VERSION)
>> -		SEQ_OPTS_PUTS("i_version");
>>   	if (nodefs || sbi->s_stripe)
>>   		SEQ_OPTS_PRINT("stripe=%lu", sbi->s_stripe);
>>   	if (nodefs || EXT4_MOUNT_DATA_FLAGS &
>> @@ -4630,6 +4627,9 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>>   	sb->s_flags = (sb->s_flags & ~SB_POSIXACL) |
>>   		(test_opt(sb, POSIX_ACL) ? SB_POSIXACL : 0);
>>   
>> +	/* i_version is always enabled now */
>> +	sb->s_flags |= SB_I_VERSION;
>> +
>>   	if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV &&
>>   	    (ext4_has_compat_features(sb) ||
>>   	     ext4_has_ro_compat_features(sb) ||
>> -- 
>> 2.37.1
>>

Re: [PATCH] ext4: unconditionally enable the i_version counter

[Lukas Czerner]

On Mon, Jul 25, 2022 at 03:29:46PM -0400, Jeff Layton wrote:
> The original i_version implementation was pretty expensive, requiring a
> log flush on every change. Because of this, it was gated behind a mount
> option (implemented via the MS_I_VERSION mountoption flag).
> 
> Commit ae5e165d855d (fs: new API for handling inode->i_version) made the
> i_version flag much less expensive, so there is no longer a performance
> penalty from enabling it.
> 
> Have ext4 ignore the SB_I_VERSION flag, and just enable it
> unconditionally.

Hi Jeff,

the ext4 specific i_version option is deprecated and it's perfect time
to remove it as well.

As for enabling iversion by default, as I said before I am fine with
that. With this patch we're left with no means of disabling it, which
might be ok, I don't have a strong opinion.

However I don't like the fact that the noiversion mount option is going
to be just silently ignored and only on some file systems. The
inconsistency bugs me and will create confusion when things go wrong.

Shouldn't we introduce something like SB_NOIVERSION to give fs a clean
way to inform user we're ignoring it, or even change the default? Or
perhaps 'noinversion' should be removed?

-Lukas

> 
> Cc: Dave Chinner <david@fromorbit.com>
> Cc: Lukas Czerner <lczerner@redhat.com>
> Cc: Benjamin Coddington <bcodding@redhat.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ext4/inode.c | 5 ++---
>  fs/ext4/super.c | 8 ++++----
>  2 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 84c0eb55071d..c785c0b72116 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5411,7 +5411,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
>  			return -EINVAL;
>  		}
>  
> -		if (IS_I_VERSION(inode) && attr->ia_size != inode->i_size)
> +		if (attr->ia_size != inode->i_size)
>  			inode_inc_iversion(inode);
>  
>  		if (shrink) {
> @@ -5717,8 +5717,7 @@ int ext4_mark_iloc_dirty(handle_t *handle,
>  	}
>  	ext4_fc_track_inode(handle, inode);
>  
> -	if (IS_I_VERSION(inode))
> -		inode_inc_iversion(inode);
> +	inode_inc_iversion(inode);
>  
>  	/* the do_update_inode consumes one bh->b_count */
>  	get_bh(iloc->bh);
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 845f2f8aee5f..30645d4343b6 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2142,8 +2142,7 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
>  		return 0;
>  	case Opt_i_version:
>  		ext4_msg(NULL, KERN_WARNING, deprecated_msg, param->key, "5.20");
> -		ext4_msg(NULL, KERN_WARNING, "Use iversion instead\n");
> -		ctx_set_flags(ctx, SB_I_VERSION);
> +		ext4_msg(NULL, KERN_WARNING, "i_version counter is always enabled.\n");
>  		return 0;
>  	case Opt_inlinecrypt:
>  #ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
> @@ -2970,8 +2969,6 @@ static int _ext4_show_options(struct seq_file *seq, struct super_block *sb,
>  		SEQ_OPTS_PRINT("min_batch_time=%u", sbi->s_min_batch_time);
>  	if (nodefs || sbi->s_max_batch_time != EXT4_DEF_MAX_BATCH_TIME)
>  		SEQ_OPTS_PRINT("max_batch_time=%u", sbi->s_max_batch_time);
> -	if (sb->s_flags & SB_I_VERSION)
> -		SEQ_OPTS_PUTS("i_version");
>  	if (nodefs || sbi->s_stripe)
>  		SEQ_OPTS_PRINT("stripe=%lu", sbi->s_stripe);
>  	if (nodefs || EXT4_MOUNT_DATA_FLAGS &
> @@ -4630,6 +4627,9 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  	sb->s_flags = (sb->s_flags & ~SB_POSIXACL) |
>  		(test_opt(sb, POSIX_ACL) ? SB_POSIXACL : 0);
>  
> +	/* i_version is always enabled now */
> +	sb->s_flags |= SB_I_VERSION;
> +
>  	if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV &&
>  	    (ext4_has_compat_features(sb) ||
>  	     ext4_has_ro_compat_features(sb) ||
> -- 
> 2.37.1
> 

Re: [PATCH] ext4: unconditionally enable the i_version counter

[Jeff Layton]

On Mon, 2022-07-25 at 14:49 -0700, Darrick J. Wong wrote:
> On Mon, Jul 25, 2022 at 03:29:46PM -0400, Jeff Layton wrote:
> > The original i_version implementation was pretty expensive, requiring a
> > log flush on every change. Because of this, it was gated behind a mount
> > option (implemented via the MS_I_VERSION mountoption flag).
> > 
> > Commit ae5e165d855d (fs: new API for handling inode->i_version) made the
> > i_version flag much less expensive, so there is no longer a performance
> > penalty from enabling it.
> > 
> > Have ext4 ignore the SB_I_VERSION flag, and just enable it
> > unconditionally.
> > 
> > Cc: Dave Chinner <david@fromorbit.com>
> > Cc: Lukas Czerner <lczerner@redhat.com>
> > Cc: Benjamin Coddington <bcodding@redhat.com>
> > Cc: Christoph Hellwig <hch@infradead.org>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ext4/inode.c | 5 ++---
> >  fs/ext4/super.c | 8 ++++----
> >  2 files changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index 84c0eb55071d..c785c0b72116 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -5411,7 +5411,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
> >  			return -EINVAL;
> >  		}
> >  
> > -		if (IS_I_VERSION(inode) && attr->ia_size != inode->i_size)
> > +		if (attr->ia_size != inode->i_size)
> >  			inode_inc_iversion(inode);
> >  
> >  		if (shrink) {
> > @@ -5717,8 +5717,7 @@ int ext4_mark_iloc_dirty(handle_t *handle,
> >  	}
> >  	ext4_fc_track_inode(handle, inode);
> >  
> > -	if (IS_I_VERSION(inode))
> > -		inode_inc_iversion(inode);
> > +	inode_inc_iversion(inode);
> >  
> >  	/* the do_update_inode consumes one bh->b_count */
> >  	get_bh(iloc->bh);
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index 845f2f8aee5f..30645d4343b6 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -2142,8 +2142,7 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
> >  		return 0;
> >  	case Opt_i_version:
> >  		ext4_msg(NULL, KERN_WARNING, deprecated_msg, param->key, "5.20");
> 
> Perhaps it's time to kill off Opt_i_version, since we're now at 5.20?
> 

Ok. I'll plan to remove that instead of altering it like this.

> (For that matter, noacl/nouser_xattr were supposed to be gone by 3.5 and
> they're clearly still there, so either they ought to go as well?)
> 

I'll leave that for a separate patch.

> --D
> 
> > -		ext4_msg(NULL, KERN_WARNING, "Use iversion instead\n");
> > -		ctx_set_flags(ctx, SB_I_VERSION);
> > +		ext4_msg(NULL, KERN_WARNING, "i_version counter is always enabled.\n");
> >  		return 0;
> >  	case Opt_inlinecrypt:
> >  #ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
> > @@ -2970,8 +2969,6 @@ static int _ext4_show_options(struct seq_file *seq, struct super_block *sb,
> >  		SEQ_OPTS_PRINT("min_batch_time=%u", sbi->s_min_batch_time);
> >  	if (nodefs || sbi->s_max_batch_time != EXT4_DEF_MAX_BATCH_TIME)
> >  		SEQ_OPTS_PRINT("max_batch_time=%u", sbi->s_max_batch_time);
> > -	if (sb->s_flags & SB_I_VERSION)
> > -		SEQ_OPTS_PUTS("i_version");
> >  	if (nodefs || sbi->s_stripe)
> >  		SEQ_OPTS_PRINT("stripe=%lu", sbi->s_stripe);
> >  	if (nodefs || EXT4_MOUNT_DATA_FLAGS &
> > @@ -4630,6 +4627,9 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
> >  	sb->s_flags = (sb->s_flags & ~SB_POSIXACL) |
> >  		(test_opt(sb, POSIX_ACL) ? SB_POSIXACL : 0);
> >  
> > +	/* i_version is always enabled now */
> > +	sb->s_flags |= SB_I_VERSION;
> > +
> >  	if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV &&
> >  	    (ext4_has_compat_features(sb) ||
> >  	     ext4_has_ro_compat_features(sb) ||
> > -- 
> > 2.37.1
> > 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] ext4: unconditionally enable the i_version counter

[Jeff Layton]

On Tue, 2022-07-26 at 09:10 +0200, Lukas Czerner wrote:
> On Mon, Jul 25, 2022 at 03:29:46PM -0400, Jeff Layton wrote:
> > The original i_version implementation was pretty expensive, requiring a
> > log flush on every change. Because of this, it was gated behind a mount
> > option (implemented via the MS_I_VERSION mountoption flag).
> > 
> > Commit ae5e165d855d (fs: new API for handling inode->i_version) made the
> > i_version flag much less expensive, so there is no longer a performance
> > penalty from enabling it.
> > 
> > Have ext4 ignore the SB_I_VERSION flag, and just enable it
> > unconditionally.
> 
> Hi Jeff,
> 
> the ext4 specific i_version option is deprecated and it's perfect time
> to remove it as well.
> 
> As for enabling iversion by default, as I said before I am fine with
> that. With this patch we're left with no means of disabling it, which
> might be ok, I don't have a strong opinion.
> 
> However I don't like the fact that the noiversion mount option is going
> to be just silently ignored and only on some file systems. The
> inconsistency bugs me and will create confusion when things go wrong.
> 
> Shouldn't we introduce something like SB_NOIVERSION to give fs a clean
> way to inform user we're ignoring it, or even change the default? Or
> perhaps 'noinversion' should be removed?
> 

Ordinarily, I'd agree here. I like leaving some mechanism to revert an
unconditional change like this.

Unfortunately, the problem here is that the original mount option was
implemented via a mountflag and was parsed by the userland helper.
Trying to rework how "-o {no}iversion" options work is going to be
messy. We can't rely on /bin/mount and the kernel being in sync.

If we did want to add something like this, then we might need to add a
new mount option that doesn't match the old one (e.g. -o no_i_version).
Alternately, could that be made settable via mkfs/tune2fs?

> 
> > 
> > Cc: Dave Chinner <david@fromorbit.com>
> > Cc: Lukas Czerner <lczerner@redhat.com>
> > Cc: Benjamin Coddington <bcodding@redhat.com>
> > Cc: Christoph Hellwig <hch@infradead.org>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ext4/inode.c | 5 ++---
> >  fs/ext4/super.c | 8 ++++----
> >  2 files changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index 84c0eb55071d..c785c0b72116 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -5411,7 +5411,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
> >  			return -EINVAL;
> >  		}
> >  
> > -		if (IS_I_VERSION(inode) && attr->ia_size != inode->i_size)
> > +		if (attr->ia_size != inode->i_size)
> >  			inode_inc_iversion(inode);
> >  
> >  		if (shrink) {
> > @@ -5717,8 +5717,7 @@ int ext4_mark_iloc_dirty(handle_t *handle,
> >  	}
> >  	ext4_fc_track_inode(handle, inode);
> >  
> > -	if (IS_I_VERSION(inode))
> > -		inode_inc_iversion(inode);
> > +	inode_inc_iversion(inode);
> >  
> >  	/* the do_update_inode consumes one bh->b_count */
> >  	get_bh(iloc->bh);
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index 845f2f8aee5f..30645d4343b6 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -2142,8 +2142,7 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
> >  		return 0;
> >  	case Opt_i_version:
> >  		ext4_msg(NULL, KERN_WARNING, deprecated_msg, param->key, "5.20");
> > -		ext4_msg(NULL, KERN_WARNING, "Use iversion instead\n");
> > -		ctx_set_flags(ctx, SB_I_VERSION);
> > +		ext4_msg(NULL, KERN_WARNING, "i_version counter is always enabled.\n");
> >  		return 0;
> >  	case Opt_inlinecrypt:
> >  #ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
> > @@ -2970,8 +2969,6 @@ static int _ext4_show_options(struct seq_file *seq, struct super_block *sb,
> >  		SEQ_OPTS_PRINT("min_batch_time=%u", sbi->s_min_batch_time);
> >  	if (nodefs || sbi->s_max_batch_time != EXT4_DEF_MAX_BATCH_TIME)
> >  		SEQ_OPTS_PRINT("max_batch_time=%u", sbi->s_max_batch_time);
> > -	if (sb->s_flags & SB_I_VERSION)
> > -		SEQ_OPTS_PUTS("i_version");
> >  	if (nodefs || sbi->s_stripe)
> >  		SEQ_OPTS_PRINT("stripe=%lu", sbi->s_stripe);
> >  	if (nodefs || EXT4_MOUNT_DATA_FLAGS &
> > @@ -4630,6 +4627,9 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
> >  	sb->s_flags = (sb->s_flags & ~SB_POSIXACL) |
> >  		(test_opt(sb, POSIX_ACL) ? SB_POSIXACL : 0);
> >  
> > +	/* i_version is always enabled now */
> > +	sb->s_flags |= SB_I_VERSION;
> > +
> >  	if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV &&
> >  	    (ext4_has_compat_features(sb) ||
> >  	     ext4_has_ro_compat_features(sb) ||
> > -- 
> > 2.37.1
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>