[PATCH] f2fs: remove default option setting in remount

[PATCH] f2fs: remove default option setting in remount

[Chengguang Xu]

Currently we set default value to options before parsing remount options,
it will cause unexpected change of options which were specified in first
mount.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
---
 fs/f2fs/super.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 00d512cd4bf2..89ea19466e80 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1499,8 +1499,6 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
 			clear_sbi_flag(sbi, SBI_NEED_SB_WRITE);
 	}
 
-	default_options(sbi);
-
 	/* parse mount options */
 	err = parse_options(sb, data);
 	if (err)
-- 
2.17.1

Re: [PATCH] f2fs: remove default option setting in remount

[Chao Yu]

On 2018/9/18 14:23, Chengguang Xu wrote:
> Currently we set default value to options before parsing remount options,
> it will cause unexpected change of options which were specified in first
> mount.

Can you check below commit? It looks w/o it we may lose default option after
remount.

498c5e9fcd10 ("f2fs: add default mount options to remount")

Thanks,

> 
> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> ---
>  fs/f2fs/super.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 00d512cd4bf2..89ea19466e80 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1499,8 +1499,6 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
>  			clear_sbi_flag(sbi, SBI_NEED_SB_WRITE);
>  	}
>  
> -	default_options(sbi);
> -
>  	/* parse mount options */
>  	err = parse_options(sb, data);
>  	if (err)
> 

Re: [PATCH] f2fs: remove default option setting in remount

[cgxu519]

On 09/18/2018 09:20 PM, Chao Yu wrote:
> On 2018/9/18 14:23, Chengguang Xu wrote:
>> Currently we set default value to options before parsing remount options,
>> it will cause unexpected change of options which were specified in first
>> mount.
> Can you check below commit? It looks w/o it we may lose default option after
> remount.
>
> 498c5e9fcd10 ("f2fs: add default mount options to remount")

Hi Chao,

It looks like there was a bug in remount at that time, but I think the 
fix above is not correct.

from the patch '498c5e9fcd10', I think it was caused by clearing 
sbi->mount_opt.opt before

parsing. I think remount should not change the options which were 
specified by user in

previous mount unless user specifies in remount.

Thanks,
Chengguang

Re: [PATCH] f2fs: remove default option setting in remount

[Chao Yu]

On 2018/9/18 21:47, cgxu519 wrote:
> On 09/18/2018 09:20 PM, Chao Yu wrote:
>> On 2018/9/18 14:23, Chengguang Xu wrote:
>>> Currently we set default value to options before parsing remount options,
>>> it will cause unexpected change of options which were specified in first
>>> mount.
>> Can you check below commit? It looks w/o it we may lose default option after
>> remount.
>>
>> 498c5e9fcd10 ("f2fs: add default mount options to remount")
> 
> Hi Chao,

Hi Chengguang,

> 
> It looks like there was a bug in remount at that time, but I think the fix above
> is not correct.
> 
> from the patch '498c5e9fcd10', I think it was caused by clearing
> sbi->mount_opt.opt before
> 
> parsing. I think remount should not change the options which were specified by
> user in
> 
> previous mount unless user specifies in remount.

Can you check description in manual of mount.

IIRC, old mount option will be record into /etc/mtab or /proc/mounts (for
adapting namespace feature), with command of "mount -o remount,rw  /dir", old
mount options can be loaded from above config file, and merge them with new
specified options.

Even we kill old mount options by call default_options() in ->remount, user's
old mount option can still be set through parameters.

But the problem here is, some old parameter can be configured via sysfs, if we
reset them in default_options(), we will lose them forever after remount.

If you have no other opinion about this, could you adapt your commit log?

Thanks,

> 
> Thanks,
> Chengguang
> 
> 
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: remove default option setting in remount

[cgxu519]

On 9/19/18 10:02 PM, Chao Yu wrote:
> On 2018/9/18 21:47, cgxu519 wrote:
>> On 09/18/2018 09:20 PM, Chao Yu wrote:
>>> On 2018/9/18 14:23, Chengguang Xu wrote:
>>>> Currently we set default value to options before parsing remount options,
>>>> it will cause unexpected change of options which were specified in first
>>>> mount.
>>> Can you check below commit? It looks w/o it we may lose default option after
>>> remount.
>>>
>>> 498c5e9fcd10 ("f2fs: add default mount options to remount")
>> Hi Chao,
> Hi Chengguang,
>
>> It looks like there was a bug in remount at that time, but I think the fix above
>> is not correct.
>>
>> from the patch '498c5e9fcd10', I think it was caused by clearing
>> sbi->mount_opt.opt before
>>
>> parsing. I think remount should not change the options which were specified by
>> user in
>>
>> previous mount unless user specifies in remount.
> Can you check description in manual of mount.
>
> IIRC, old mount option will be record into /etc/mtab or /proc/mounts (for
> adapting namespace feature), with command of "mount -o remount,rw  /dir", old
> mount options can be loaded from above config file, and merge them with new
> specified options.
>
> Even we kill old mount options by call default_options() in ->remount, user's
> old mount option can still be set through parameters.

Please let me show you different behaviors before/after this patch.


[root@x201 cgxu]# uname -a
Linux x201 4.19.0-rc2+ #51 SMP Wed Sep 19 22:41:20 CST 2018 x86_64 
x86_64 x86_64 GNU/Linux


Before:

[root@x201 cgxu]# mount -o fsync_mode=strict /dev/mapper/test-test1 
/mnt/test/test1
[root@x201 cgxu]# mount | grep test1
/dev/mapper/test-test1 on /mnt/test/test1 type f2fs 
(rw,relatime,lazytime,background_gc=on,discard,no_heap,user_xattr,inline_xattr,acl,inline_data,inline_dentry,flush_merge,extent_cache,mode=adaptive,active_logs=6,alloc_mode=reuse,fsync_mode=strict)

[root@x201 cgxu]# mount -o remount,background_gc=sync 
/dev/mapper/test-test1 /mnt/test/test1
[root@x201 cgxu]# mount | grep test1
/dev/mapper/test-test1 on /mnt/test/test1 type f2fs 
(rw,relatime,background_gc=sync,discard,no_heap,user_xattr,inline_xattr,acl,inline_data,inline_dentry,flush_merge,extent_cache,mode=adaptive,active_logs=6,alloc_mode=default,fsync_mode=posix)


I only changed background_gc in ->remount, but fsync_mode is implicitly 
set to default value 'posix'.

After:

[root@x201 linus]# mount -o fsync_mode=strict /dev/mapper/test-test1 
/mnt/test/test1
[root@x201 linus]# mount | grep test1
/dev/mapper/test-test1 on /mnt/test/test1 type f2fs 
(rw,relatime,lazytime,background_gc=on,discard,no_heap,user_xattr,inline_xattr,acl,inline_data,inline_dentry,flush_merge,extent_cache,mode=adaptive,active_logs=6,alloc_mode=reuse,fsync_mode=strict)

[root@x201 linus]# mount -o remount,background_gc=sync 
/dev/mapper/test-test1 /mnt/test/test1
[root@x201 linus]# mount | grep test1
/dev/mapper/test-test1 on /mnt/test/test1 type f2fs 
(rw,relatime,background_gc=sync,discard,no_heap,user_xattr,inline_xattr,acl,inline_data,inline_dentry,flush_merge,extent_cache,mode=adaptive,active_logs=6,alloc_mode=reuse,fsync_mode=strict)

This time fysnc_mode looks fine.

>
> But the problem here is, some old parameter can be configured via sysfs, if we
> reset them in default_options(), we will lose them forever after remount.
>
> If you have no other opinion about this, could you adapt your commit log?
>

Re: [PATCH] f2fs: remove default option setting in remount

[Chao Yu]

On 2018/9/20 7:54, cgxu519 wrote:
> On 9/19/18 10:02 PM, Chao Yu wrote:
>> On 2018/9/18 21:47, cgxu519 wrote:
>>> On 09/18/2018 09:20 PM, Chao Yu wrote:
>>>> On 2018/9/18 14:23, Chengguang Xu wrote:
>>>>> Currently we set default value to options before parsing remount options,
>>>>> it will cause unexpected change of options which were specified in first
>>>>> mount.
>>>> Can you check below commit? It looks w/o it we may lose default option after
>>>> remount.
>>>>
>>>> 498c5e9fcd10 ("f2fs: add default mount options to remount")
>>> Hi Chao,
>> Hi Chengguang,
>>
>>> It looks like there was a bug in remount at that time, but I think the fix above
>>> is not correct.
>>>
>>> from the patch '498c5e9fcd10', I think it was caused by clearing
>>> sbi->mount_opt.opt before
>>>
>>> parsing. I think remount should not change the options which were specified by
>>> user in
>>>
>>> previous mount unless user specifies in remount.
>> Can you check description in manual of mount.
>>
>> IIRC, old mount option will be record into /etc/mtab or /proc/mounts (for
>> adapting namespace feature), with command of "mount -o remount,rw  /dir", old
>> mount options can be loaded from above config file, and merge them with new
>> specified options.
>>
>> Even we kill old mount options by call default_options() in ->remount, user's
>> old mount option can still be set through parameters.
> 
> Please let me show you different behaviors before/after this patch.
> 
> 
> [root@x201 cgxu]# uname -a
> Linux x201 4.19.0-rc2+ #51 SMP Wed Sep 19 22:41:20 CST 2018 x86_64 
> x86_64 x86_64 GNU/Linux
> 
> 
> Before:
> 
> [root@x201 cgxu]# mount -o fsync_mode=strict /dev/mapper/test-test1 
> /mnt/test/test1
> [root@x201 cgxu]# mount | grep test1
> /dev/mapper/test-test1 on /mnt/test/test1 type f2fs 
> (rw,relatime,lazytime,background_gc=on,discard,no_heap,user_xattr,inline_xattr,acl,inline_data,inline_dentry,flush_merge,extent_cache,mode=adaptive,active_logs=6,alloc_mode=reuse,fsync_mode=strict)
> 
> [root@x201 cgxu]# mount -o remount,background_gc=sync 
> /dev/mapper/test-test1 /mnt/test/test1
> [root@x201 cgxu]# mount | grep test1
> /dev/mapper/test-test1 on /mnt/test/test1 type f2fs 
> (rw,relatime,background_gc=sync,discard,no_heap,user_xattr,inline_xattr,acl,inline_data,inline_dentry,flush_merge,extent_cache,mode=adaptive,active_logs=6,alloc_mode=default,fsync_mode=posix)
> 
> 
> I only changed background_gc in ->remount, but fsync_mode is implicitly 
> set to default value 'posix'.

IMO, the resul is as expected, let's referenced description in manual of mount:

              The  remount  functionality  follows the standard way how the
mount command works with options from fstab. It means the mount command
doesn't read fstab (or mtab) only when a device and
              dir are fully specified.

              mount -o remount,rw /dev/foo /dir

              After this call all old mount options are replaced and
arbitrary stuff from fstab is ignored, except the loop= option which is
internally generated and maintained by the mount command.

              mount -o remount,rw  /dir

              After this call mount reads fstab (or mtab) and merges these
options with options from command line ( -o ).


So if you want to keep old mount option, you should use:

mount -o remount,background_gc=sync /mnt/test/test1

instead of

mount -o remount,background_gc=sync /dev/mapper/test-test1 /mnt/test/test1

Thanks,

> 
> After:
> 
> [root@x201 linus]# mount -o fsync_mode=strict /dev/mapper/test-test1 
> /mnt/test/test1
> [root@x201 linus]# mount | grep test1
> /dev/mapper/test-test1 on /mnt/test/test1 type f2fs 
> (rw,relatime,lazytime,background_gc=on,discard,no_heap,user_xattr,inline_xattr,acl,inline_data,inline_dentry,flush_merge,extent_cache,mode=adaptive,active_logs=6,alloc_mode=reuse,fsync_mode=strict)
> 
> [root@x201 linus]# mount -o remount,background_gc=sync 
> /dev/mapper/test-test1 /mnt/test/test1
> [root@x201 linus]# mount | grep test1
> /dev/mapper/test-test1 on /mnt/test/test1 type f2fs 
> (rw,relatime,background_gc=sync,discard,no_heap,user_xattr,inline_xattr,acl,inline_data,inline_dentry,flush_merge,extent_cache,mode=adaptive,active_logs=6,alloc_mode=reuse,fsync_mode=strict)
> 
> This time fysnc_mode looks fine.
> 
>>
>> But the problem here is, some old parameter can be configured via sysfs, if we
>> reset them in default_options(), we will lose them forever after remount.
>>
>> If you have no other opinion about this, could you adapt your commit log?
>>
> 
> .
> 

Re: [PATCH] f2fs: remove default option setting in remount

[cgxu519]

On 09/20/2018 02:29 PM, Chao Yu wrote:
> On 2018/9/20 7:54, cgxu519 wrote:
>> On 9/19/18 10:02 PM, Chao Yu wrote:
>>> On 2018/9/18 21:47, cgxu519 wrote:
>>>> On 09/18/2018 09:20 PM, Chao Yu wrote:
>>>>> On 2018/9/18 14:23, Chengguang Xu wrote:
>>>>>> Currently we set default value to options before parsing remount options,
>>>>>> it will cause unexpected change of options which were specified in first
>>>>>> mount.
>>>>> Can you check below commit? It looks w/o it we may lose default option after
>>>>> remount.
>>>>>
>>>>> 498c5e9fcd10 ("f2fs: add default mount options to remount")
>>>> Hi Chao,
>>> Hi Chengguang,
>>>
>>>> It looks like there was a bug in remount at that time, but I think the fix above
>>>> is not correct.
>>>>
>>>> from the patch '498c5e9fcd10', I think it was caused by clearing
>>>> sbi->mount_opt.opt before
>>>>
>>>> parsing. I think remount should not change the options which were specified by
>>>> user in
>>>>
>>>> previous mount unless user specifies in remount.
>>> Can you check description in manual of mount.
>>>
>>> IIRC, old mount option will be record into /etc/mtab or /proc/mounts (for
>>> adapting namespace feature), with command of "mount -o remount,rw  /dir", old
>>> mount options can be loaded from above config file, and merge them with new
>>> specified options.
>>>
>>> Even we kill old mount options by call default_options() in ->remount, user's
>>> old mount option can still be set through parameters.
>> Please let me show you different behaviors before/after this patch.
>>
>>
>> [root@x201 cgxu]# uname -a
>> Linux x201 4.19.0-rc2+ #51 SMP Wed Sep 19 22:41:20 CST 2018 x86_64
>> x86_64 x86_64 GNU/Linux
>>
>>
>> Before:
>>
>> [root@x201 cgxu]# mount -o fsync_mode=strict /dev/mapper/test-test1
>> /mnt/test/test1
>> [root@x201 cgxu]# mount | grep test1
>> /dev/mapper/test-test1 on /mnt/test/test1 type f2fs
>> (rw,relatime,lazytime,background_gc=on,discard,no_heap,user_xattr,inline_xattr,acl,inline_data,inline_dentry,flush_merge,extent_cache,mode=adaptive,active_logs=6,alloc_mode=reuse,fsync_mode=strict)
>>
>> [root@x201 cgxu]# mount -o remount,background_gc=sync
>> /dev/mapper/test-test1 /mnt/test/test1
>> [root@x201 cgxu]# mount | grep test1
>> /dev/mapper/test-test1 on /mnt/test/test1 type f2fs
>> (rw,relatime,background_gc=sync,discard,no_heap,user_xattr,inline_xattr,acl,inline_data,inline_dentry,flush_merge,extent_cache,mode=adaptive,active_logs=6,alloc_mode=default,fsync_mode=posix)
>>
>>
>> I only changed background_gc in ->remount, but fsync_mode is implicitly
>> set to default value 'posix'.
> IMO, the resul is as expected, let's referenced description in manual of mount:
>
>                The  remount  functionality  follows the standard way how the
> mount command works with options from fstab. It means the mount command
> doesn't read fstab (or mtab) only when a device and
>                dir are fully specified.
>
>                mount -o remount,rw /dev/foo /dir
>
>                After this call all old mount options are replaced and
> arbitrary stuff from fstab is ignored, except the loop= option which is
> internally generated and maintained by the mount command.
>
>                mount -o remount,rw  /dir
>
>                After this call mount reads fstab (or mtab) and merges these
> options with options from command line ( -o ).
>
>
> So if you want to keep old mount option, you should use:
>
> mount -o remount,background_gc=sync /mnt/test/test1
>
> instead of
>
> mount -o remount,background_gc=sync /dev/mapper/test-test1 /mnt/test/test1

I get your point about how mount command organizes options ,
but it seems other filesystem do not initialize mount option in remount.
What do you think for that? and if we keep initialization in f2fs,
shouldn't we set default value to all mount options for remount?

Thanks,
Chengguang

Re: [PATCH] f2fs: remove default option setting in remount

[Chao Yu]

On 2018/9/22 22:40, cgxu519 wrote:
> On 09/20/2018 02:29 PM, Chao Yu wrote:
>> On 2018/9/20 7:54, cgxu519 wrote:
>>> On 9/19/18 10:02 PM, Chao Yu wrote:
>>>> On 2018/9/18 21:47, cgxu519 wrote:
>>>>> On 09/18/2018 09:20 PM, Chao Yu wrote:
>>>>>> On 2018/9/18 14:23, Chengguang Xu wrote:
>>>>>>> Currently we set default value to options before parsing remount options,
>>>>>>> it will cause unexpected change of options which were specified in first
>>>>>>> mount.
>>>>>> Can you check below commit? It looks w/o it we may lose default option after
>>>>>> remount.
>>>>>>
>>>>>> 498c5e9fcd10 ("f2fs: add default mount options to remount")
>>>>> Hi Chao,
>>>> Hi Chengguang,
>>>>
>>>>> It looks like there was a bug in remount at that time, but I think the fix above
>>>>> is not correct.
>>>>>
>>>>> from the patch '498c5e9fcd10', I think it was caused by clearing
>>>>> sbi->mount_opt.opt before
>>>>>
>>>>> parsing. I think remount should not change the options which were specified by
>>>>> user in
>>>>>
>>>>> previous mount unless user specifies in remount.
>>>> Can you check description in manual of mount.
>>>>
>>>> IIRC, old mount option will be record into /etc/mtab or /proc/mounts (for
>>>> adapting namespace feature), with command of "mount -o remount,rw  /dir", old
>>>> mount options can be loaded from above config file, and merge them with new
>>>> specified options.
>>>>
>>>> Even we kill old mount options by call default_options() in ->remount, user's
>>>> old mount option can still be set through parameters.
>>> Please let me show you different behaviors before/after this patch.
>>>
>>>
>>> [root@x201 cgxu]# uname -a
>>> Linux x201 4.19.0-rc2+ #51 SMP Wed Sep 19 22:41:20 CST 2018 x86_64
>>> x86_64 x86_64 GNU/Linux
>>>
>>>
>>> Before:
>>>
>>> [root@x201 cgxu]# mount -o fsync_mode=strict /dev/mapper/test-test1
>>> /mnt/test/test1
>>> [root@x201 cgxu]# mount | grep test1
>>> /dev/mapper/test-test1 on /mnt/test/test1 type f2fs
>>> (rw,relatime,lazytime,background_gc=on,discard,no_heap,user_xattr,inline_xattr,acl,inline_data,inline_dentry,flush_merge,extent_cache,mode=adaptive,active_logs=6,alloc_mode=reuse,fsync_mode=strict)
>>>
>>> [root@x201 cgxu]# mount -o remount,background_gc=sync
>>> /dev/mapper/test-test1 /mnt/test/test1
>>> [root@x201 cgxu]# mount | grep test1
>>> /dev/mapper/test-test1 on /mnt/test/test1 type f2fs
>>> (rw,relatime,background_gc=sync,discard,no_heap,user_xattr,inline_xattr,acl,inline_data,inline_dentry,flush_merge,extent_cache,mode=adaptive,active_logs=6,alloc_mode=default,fsync_mode=posix)
>>>
>>>
>>> I only changed background_gc in ->remount, but fsync_mode is implicitly
>>> set to default value 'posix'.
>> IMO, the resul is as expected, let's referenced description in manual of mount:
>>
>>                The  remount  functionality  follows the standard way how the
>> mount command works with options from fstab. It means the mount command
>> doesn't read fstab (or mtab) only when a device and
>>                dir are fully specified.
>>
>>                mount -o remount,rw /dev/foo /dir
>>
>>                After this call all old mount options are replaced and
>> arbitrary stuff from fstab is ignored, except the loop= option which is
>> internally generated and maintained by the mount command.
>>
>>                mount -o remount,rw  /dir
>>
>>                After this call mount reads fstab (or mtab) and merges these
>> options with options from command line ( -o ).
>>
>>
>> So if you want to keep old mount option, you should use:
>>
>> mount -o remount,background_gc=sync /mnt/test/test1
>>
>> instead of
>>
>> mount -o remount,background_gc=sync /dev/mapper/test-test1 /mnt/test/test1
> 
> I get your point about how mount command organizes options ,
> but it seems other filesystem do not initialize mount option in remount.
> What do you think for that? and if we keep initialization in f2fs,
> shouldn't we set default value to all mount options for remount?

I'm okay with this change to keep line with other filesystems.

+Cc Yunlei to check whether we are missing something.

Thanks,

> 
> Thanks,
> Chengguang
> 
> 
> 
> 
> 
> 
> 
> 
> .
> 

Re: [PATCH] f2fs: remove default option setting in remount

[Chengguang Xu]

On 2018/9/25 at 3:41 PM, Chao Yu wrote:

> On 2018/9/22 22:40, cgxu519 wrote:
> > On 09/20/2018 02:29 PM, Chao Yu wrote:
> >> On 2018/9/20 7:54, cgxu519 wrote:
> >>> On 9/19/18 10:02 PM, Chao Yu wrote:
> >>>> On 2018/9/18 21:47, cgxu519 wrote:
> >>>>> On 09/18/2018 09:20 PM, Chao Yu wrote:
> >>>>>> On 2018/9/18 14:23, Chengguang Xu wrote:
> >>>>>>> Currently we set default value to options before parsing remount options,
> >>>>>>> it will cause unexpected change of options which were specified in first
> >>>>>>> mount.
> >>>>>> Can you check below commit? It looks w/o it we may lose default option after
> >>>>>> remount.
> >>>>>>
> >>>>>> 498c5e9fcd10 ("f2fs: add default mount options to remount")
> >>>>> Hi Chao,
> >>>> Hi Chengguang,
> >>>>
> >>>>> It looks like there was a bug in remount at that time, but I think the fix above
> >>>>> is not correct.
> >>>>>
> >>>>> from the patch '498c5e9fcd10', I think it was caused by clearing
> >>>>> sbi->mount_opt.opt before
> >>>>>
> >>>>> parsing. I think remount should not change the options which were specified by
> >>>>> user in
> >>>>>
> >>>>> previous mount unless user specifies in remount.
> >>>> Can you check description in manual of mount.
> >>>>
> >>>> IIRC, old mount option will be record into /etc/mtab or /proc/mounts (for
> >>>> adapting namespace feature), with command of "mount -o remount,rw  /dir", old
> >>>> mount options can be loaded from above config file, and merge them with new
> >>>> specified options.
> >>>>
> >>>> Even we kill old mount options by call default_options() in ->remount, user's
> >>>> old mount option can still be set through parameters.
> >>> Please let me show you different behaviors before/after this patch.
> >>>
> >>>
> >>> [root@x201 cgxu]# uname -a
> >>> Linux x201 4.19.0-rc2+ #51 SMP Wed Sep 19 22:41:20 CST 2018 x86_64
> >>> x86_64 x86_64 GNU/Linux
> >>>
> >>>
> >>> Before:
> >>>
> >>> [root@x201 cgxu]# mount -o fsync_mode=strict /dev/mapper/test-test1
> >>> /mnt/test/test1
> >>> [root@x201 cgxu]# mount | grep test1
> >>> /dev/mapper/test-test1 on /mnt/test/test1 type f2fs
> >>> (rw,relatime,lazytime,background_gc=on,discard,no_heap,user_xattr,inline_xattr,acl,inline_data,inline_dentry,flush_merge,extent_cache,mode=adaptive,active_logs=6,alloc_mode=reuse,fsync_mode=strict)
> >>>
> >>> [root@x201 cgxu]# mount -o remount,background_gc=sync
> >>> /dev/mapper/test-test1 /mnt/test/test1
> >>> [root@x201 cgxu]# mount | grep test1
> >>> /dev/mapper/test-test1 on /mnt/test/test1 type f2fs
> >>> (rw,relatime,background_gc=sync,discard,no_heap,user_xattr,inline_xattr,acl,inline_data,inline_dentry,flush_merge,extent_cache,mode=adaptive,active_logs=6,alloc_mode=default,fsync_mode=posix)
> >>>
> >>>
> >>> I only changed background_gc in ->remount, but fsync_mode is implicitly
> >>> set to default value 'posix'.
> >> IMO, the resul is as expected, let's referenced description in manual of mount:
> >>
> >>                The  remount  functionality  follows the standard way how the
> >> mount command works with options from fstab. It means the mount command
> >> doesn't read fstab (or mtab) only when a device and
> >>                dir are fully specified.
> >>
> >>                mount -o remount,rw /dev/foo /dir
> >>
> >>                After this call all old mount options are replaced and
> >> arbitrary stuff from fstab is ignored, except the loop= option which is
> >> internally generated and maintained by the mount command.
> >>
> >>                mount -o remount,rw  /dir
> >>
> >>                After this call mount reads fstab (or mtab) and merges these
> >> options with options from command line ( -o ).
> >>
> >>
> >> So if you want to keep old mount option, you should use:
> >>
> >> mount -o remount,background_gc=sync /mnt/test/test1
> >>
> >> instead of
> >>
> >> mount -o remount,background_gc=sync /dev/mapper/test-test1 /mnt/test/test1
> > 
> > I get your point about how mount command organizes options ,
> > but it seems other filesystem do not initialize mount option in remount.
> > What do you think for that? and if we keep initialization in f2fs,
> > shouldn't we set default value to all mount options for remount?
> 
> I'm okay with this change to keep line with other filesystems.

Hi Chao

Do you agree this patch without change of commit log?

Thanks,
Chengguang

Re: [PATCH] f2fs: remove default option setting in remount

[Chao Yu]

On 2018/10/18 13:28, Chengguang Xu wrote:
> On 2018/9/25 at 3:41 PM, Chao Yu wrote:
> 
>> On 2018/9/22 22:40, cgxu519 wrote:
>>> On 09/20/2018 02:29 PM, Chao Yu wrote:
>>>> On 2018/9/20 7:54, cgxu519 wrote:
>>>>> On 9/19/18 10:02 PM, Chao Yu wrote:
>>>>>> On 2018/9/18 21:47, cgxu519 wrote:
>>>>>>> On 09/18/2018 09:20 PM, Chao Yu wrote:
>>>>>>>> On 2018/9/18 14:23, Chengguang Xu wrote:
>>>>>>>>> Currently we set default value to options before parsing remount options,
>>>>>>>>> it will cause unexpected change of options which were specified in first
>>>>>>>>> mount.
>>>>>>>> Can you check below commit? It looks w/o it we may lose default option after
>>>>>>>> remount.
>>>>>>>>
>>>>>>>> 498c5e9fcd10 ("f2fs: add default mount options to remount")
>>>>>>> Hi Chao,
>>>>>> Hi Chengguang,
>>>>>>
>>>>>>> It looks like there was a bug in remount at that time, but I think the fix above
>>>>>>> is not correct.
>>>>>>>
>>>>>>> from the patch '498c5e9fcd10', I think it was caused by clearing
>>>>>>> sbi->mount_opt.opt before
>>>>>>>
>>>>>>> parsing. I think remount should not change the options which were specified by
>>>>>>> user in
>>>>>>>
>>>>>>> previous mount unless user specifies in remount.
>>>>>> Can you check description in manual of mount.
>>>>>>
>>>>>> IIRC, old mount option will be record into /etc/mtab or /proc/mounts (for
>>>>>> adapting namespace feature), with command of "mount -o remount,rw  /dir", old
>>>>>> mount options can be loaded from above config file, and merge them with new
>>>>>> specified options.
>>>>>>
>>>>>> Even we kill old mount options by call default_options() in ->remount, user's
>>>>>> old mount option can still be set through parameters.
>>>>> Please let me show you different behaviors before/after this patch.
>>>>>
>>>>>
>>>>> [root@x201 cgxu]# uname -a
>>>>> Linux x201 4.19.0-rc2+ #51 SMP Wed Sep 19 22:41:20 CST 2018 x86_64
>>>>> x86_64 x86_64 GNU/Linux
>>>>>
>>>>>
>>>>> Before:
>>>>>
>>>>> [root@x201 cgxu]# mount -o fsync_mode=strict /dev/mapper/test-test1
>>>>> /mnt/test/test1
>>>>> [root@x201 cgxu]# mount | grep test1
>>>>> /dev/mapper/test-test1 on /mnt/test/test1 type f2fs
>>>>> (rw,relatime,lazytime,background_gc=on,discard,no_heap,user_xattr,inline_xattr,acl,inline_data,inline_dentry,flush_merge,extent_cache,mode=adaptive,active_logs=6,alloc_mode=reuse,fsync_mode=strict)
>>>>>
>>>>> [root@x201 cgxu]# mount -o remount,background_gc=sync
>>>>> /dev/mapper/test-test1 /mnt/test/test1
>>>>> [root@x201 cgxu]# mount | grep test1
>>>>> /dev/mapper/test-test1 on /mnt/test/test1 type f2fs
>>>>> (rw,relatime,background_gc=sync,discard,no_heap,user_xattr,inline_xattr,acl,inline_data,inline_dentry,flush_merge,extent_cache,mode=adaptive,active_logs=6,alloc_mode=default,fsync_mode=posix)
>>>>>
>>>>>
>>>>> I only changed background_gc in ->remount, but fsync_mode is implicitly
>>>>> set to default value 'posix'.
>>>> IMO, the resul is as expected, let's referenced description in manual of mount:
>>>>
>>>>                The  remount  functionality  follows the standard way how the
>>>> mount command works with options from fstab. It means the mount command
>>>> doesn't read fstab (or mtab) only when a device and
>>>>                dir are fully specified.
>>>>
>>>>                mount -o remount,rw /dev/foo /dir
>>>>
>>>>                After this call all old mount options are replaced and
>>>> arbitrary stuff from fstab is ignored, except the loop= option which is
>>>> internally generated and maintained by the mount command.
>>>>
>>>>                mount -o remount,rw  /dir
>>>>
>>>>                After this call mount reads fstab (or mtab) and merges these
>>>> options with options from command line ( -o ).
>>>>
>>>>
>>>> So if you want to keep old mount option, you should use:
>>>>
>>>> mount -o remount,background_gc=sync /mnt/test/test1
>>>>
>>>> instead of
>>>>
>>>> mount -o remount,background_gc=sync /dev/mapper/test-test1 /mnt/test/test1
>>>
>>> I get your point about how mount command organizes options ,
>>> but it seems other filesystem do not initialize mount option in remount.
>>> What do you think for that? and if we keep initialization in f2fs,
>>> shouldn't we set default value to all mount options for remount?
>>
>> I'm okay with this change to keep line with other filesystems.
> 
> Hi Chao
> 
> Do you agree this patch without change of commit log?

Hi Chengguang,

I didn't see unexpected result caused by calling default_options() in
f2fs_remount(), if there is, we'd better add such example in commit
message. Otherwise, it will be better to say 'keep line with other
filesystem'...

Thanks,

> 
> Thanks,
> Chengguang
> 
> 
> .
>