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
>
>
> .
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
