Re: [PATCH] ext4: handle option set by mount flags correctly

[Ritesh Harjani <riteshh@linux.ibm.com>]

On 7/23/20 8:35 PM, Lukas Czerner wrote:
> Currently there is a problem with mount options that can be both set by
> vfs using mount flags or by a string parsing in ext4.
> 
> i_version/iversion options gets lost after remount, for example
> 
> $ mount -o i_version /dev/pmem0 /mnt
> $ grep pmem0 /proc/self/mountinfo | grep i_version
> 310 95 259:0 / /mnt rw,relatime shared:163 - ext4 /dev/pmem0 rw,seclabel,i_version
> $ mount -o remount,ro /mnt
> $ grep pmem0 /proc/self/mountinfo | grep i_version
> 
> nolazytime gets ignored by ext4 on remount, for example
> 
> $ mount -o lazytime /dev/pmem0 /mnt
> $ grep pmem0 /proc/self/mountinfo | grep lazytime
> 310 95 259:0 / /mnt rw,relatime shared:163 - ext4 /dev/pmem0 rw,lazytime,seclabel
> $ mount -o remount,nolazytime /mnt
> $ grep pmem0 /proc/self/mountinfo | grep lazytime
> 310 95 259:0 / /mnt rw,relatime shared:163 - ext4 /dev/pmem0 rw,lazytime,seclabel
> 
> Fix it by applying the SB_LAZYTIME and SB_I_VERSION flags from *flags to
> s_flags before we parse the option and use the resulting state of the
> same flags in *flags at the end of successful remount.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Looks good to me. I did some small unit testing to observe the
behavior. Below logs are *with your patch applied* for nolazytime e.g.
which you referred above. But without your patch, as you mentioned
nolazytime was not taking into effect.

           mount  8635 [007]  5477.595642:      probe:ext4_remount: 
(ffffffff813bd270) s_flags=0x72810000 flags=0x0 data_string="i_version"
            mount  8635 [007]  5477.595672:  probe:ext4_remount_L55: 
(ffffffff813bd3d1) s_flags=0x70010000 flags=0x0 data_string="i_version"
            mount  8635 [007]  5477.595758: probe:ext4_remount_L217: 
(ffffffff813bd787) s_flags=0x70810000 flags=0x0
            mount  8635 [007]  5477.595772: probe:ext4_remount_L246: 
(ffffffff813bd820) s_flags=0x70810000 flags=0x800000



Please feel free to add.
Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>


> ---
>   fs/ext4/super.c | 21 ++++++++++++++++-----
>   1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 330957ed1f05..caab4c57f909 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5445,7 +5445,7 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>   {
>   	struct ext4_super_block *es;
>   	struct ext4_sb_info *sbi = EXT4_SB(sb);
> -	unsigned long old_sb_flags;
> +	unsigned long old_sb_flags, vfs_flags;
>   	struct ext4_mount_options old_opts;
>   	int enable_quota = 0;
>   	ext4_group_t g;
> @@ -5488,6 +5488,14 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>   	if (sbi->s_journal && sbi->s_journal->j_task->io_context)
>   		journal_ioprio = sbi->s_journal->j_task->io_context->ioprio;
> 
> +	/*
> +	 * Some options can be enabled by ext4 and/or by VFS mount flag
> +	 * either way we need to make sure it matches in both *flags and
> +	 * s_flags. Copy those selected flags from *flags to s_flags
> +	 */
> +	vfs_flags = SB_LAZYTIME | SB_I_VERSION;
> +	sb->s_flags = (sb->s_flags & ~vfs_flags) | (*flags & vfs_flags);
> +
>   	if (!parse_options(data, sb, NULL, &journal_ioprio, 1)) {
>   		err = -EINVAL;
>   		goto restore_opts;
> @@ -5541,9 +5549,6 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>   		set_task_ioprio(sbi->s_journal->j_task, journal_ioprio);
>   	}
> 
> -	if (*flags & SB_LAZYTIME)
> -		sb->s_flags |= SB_LAZYTIME;
> -
>   	if ((bool)(*flags & SB_RDONLY) != sb_rdonly(sb)) {
>   		if (sbi->s_mount_flags & EXT4_MF_FS_ABORTED) {
>   			err = -EROFS;
> @@ -5675,7 +5680,13 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>   	}
>   #endif
> 
> -	*flags = (*flags & ~SB_LAZYTIME) | (sb->s_flags & SB_LAZYTIME);
> +	/*
> +	 * Some options can be enabled by ext4 and/or by VFS mount flag
> +	 * either way we need to make sure it matches in both *flags and
> +	 * s_flags. Copy those selected flags from s_flags to *flags
> +	 */
> +	*flags = (*flags & ~vfs_flags) | (sb->s_flags & vfs_flags);
> +
>   	ext4_msg(sb, KERN_INFO, "re-mounted. Opts: %s", orig_data);
>   	kfree(orig_data);
>   	return 0;
>