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>
