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

["Chengguang Xu" <cgxu519@gmx.com>]

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