Re: [PATCH] tmpfs: preserve SB_I_VERSION on remount

[Hugh Dickins]

On Tue, 19 Aug 2025, Jeff Layton wrote:
> On Tue, 2025-08-19 at 14:18 +0800, libaokun@xxxxxxxxxxxxxxx wrote:
> > From: Baokun Li <libaokun1@xxxxxxxxxx>
> > 
> > Now tmpfs enables i_version by default and tmpfs does not modify it. But
> > SB_I_VERSION can also be modified via sb_flags, and reconfigure_super()
> > always overwrites the existing flags with the latest ones. This means that
> > if tmpfs is remounted without specifying iversion, the default i_version
> > will be unexpectedly disabled.

Wow, what a surprise! Thank you so much for finding and fixing this.

> > 
> > To ensure iversion remains enabled, SB_I_VERSION is now always set for
> > fc->sb_flags in shmem_init_fs_context(), instead of for sb->s_flags in
> > shmem_fill_super().

I have to say that your patch looks to me like a hacky workaround. But 
after spending ages trying to work out how this came about, have concluded
that it's an artifact of "iversion" and/or "noiversion" being or having
been a mount option in some filesystems, with MS_I_VERSION in MS_RMT_MASK
getting propagated to sb_flags_mask, implying that the remounter is
changing the option when they have no such intention.  And any attempt
to fix this in a better way would be too likely to cause more trouble
than it's worth - unless other filesystems are also still surprised.

I had to worry, does the same weird disappearance-on-remount happen to
tmpfs's SB_POSIXACL too?  But it looks like not, because MS_POSIXACL is
not in MS_RMT_MASK - a relic of history why one in but not the other.

But I've added linux-fsdevel to the Ccs, mainly as a protest at this
unexpected interface (though no work for Christian to do: Andrew has
already taken the patch, thanks).

> > 
> > Fixes: 36f05cab0a2c ("tmpfs: add support for an i_version counter")
> > Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
> > ---
> >  mm/shmem.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index e2c76a30802b..eebe12ff5bc6 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -5081,7 +5081,7 @@ static int shmem_fill_super(struct super_block *sb, struct fs_context *fc)
> >  		sb->s_flags |= SB_NOUSER;
> >  	}
> >  	sb->s_export_op = &shmem_export_ops;
> > -	sb->s_flags |= SB_NOSEC | SB_I_VERSION;
> > +	sb->s_flags |= SB_NOSEC;
> >  
> >  #if IS_ENABLED(CONFIG_UNICODE)
> >  	if (!ctx->encoding && ctx->strict_encoding) {
> > @@ -5385,6 +5385,9 @@ int shmem_init_fs_context(struct fs_context *fc)
> >  
> >  	fc->fs_private = ctx;
> >  	fc->ops = &shmem_fs_context_ops;
> > +#ifdef CONFIG_TMPFS

Ah, you're being very punctilious with that #ifdef: yes, the original
code happened not to set it in the #ifndef CONFIG_TMPFS case (when the
i_version would be invisible anyway).  But I bet that if we had done it
this way originally, we would have preferred not to clutter the source
with #ifdef and #else here.  Oh well, perhaps they will vanish in the
night sometime, it's a nit not worth you resending.

> > +	fc->sb_flags |= SB_I_VERSION;
> > +#endif
> >  	return 0;
> >  }
> >  
> 
> Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>

Acked-by: Hugh Dickins <hughd@xxxxxxxxxx>