On 2025-08-22 10:49, Hugh Dickins wrote:
> 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.
Exactly!
> 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.
Other filesystems supporting i_version (ext4, xfs, btrfs) have encountered
similar issues. The solution adopted was either resetting SB_I_VERSION
during remount operations or setting SB_I_VERSION in init_fs_context().
Given that the overhead of iversion is now minimal, all supported
filesystems in the kernel enable it by default. I previously considered
converting SB_I_VERSION to FS_I_VERSION and setting it in
file_system_type->fs_flags, but since XFS only supports iversion in v5,
this idea was ultimately abandoned. Alternatively, removing MS_I_VERSION
from MS_RMT_MASK might also be a viable approach.
> 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.
Yes.
> 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).
Okay.
>
>>> 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.
Yes, I kept this macro to maintain consistency with shmem_fill_super().
If i_version is not supported but SB_I_VERSION is set, it may cause
confusion for IMA or NFS.
>
>>> + fc->sb_flags |= SB_I_VERSION;
>>> +#endif
>>> return 0;
>>> }
>>>
>> Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
> Acked-by: Hugh Dickins <hughd@xxxxxxxxxx>
>
Thanks,
Baokun
