| Message ID | 20241112010820.2788822-1-jaegeuk@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f8bed73c6c52b726ed3dff200cfeadcfbdfe88d6 |
| Headers | show |
| Series | [f2fs-dev] Revert "f2fs: remove unreachable lazytime mount option parsing" | expand |
Hi Eric, Could you please check this revert as it breaks the mount()? It seems F2FS needs to implement new mount support. Thanks, On 11/12, Jaegeuk Kim wrote: > This reverts commit 54f43a10fa257ad4af02a1d157fefef6ebcfa7dc. > > The above commit broke the lazytime mount, given > > mount("/dev/vdb", "/mnt/test", "f2fs", 0, "lazytime"); > > CC: stable@vger.kernel.org # 6.11+ > Signed-off-by: Daniel Rosenberg <drosen@google.com> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > --- > fs/f2fs/super.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > index 49519439b770..35c4394e4fc6 100644 > --- a/fs/f2fs/super.c > +++ b/fs/f2fs/super.c > @@ -150,6 +150,8 @@ enum { > Opt_mode, > Opt_fault_injection, > Opt_fault_type, > + Opt_lazytime, > + Opt_nolazytime, > Opt_quota, > Opt_noquota, > Opt_usrquota, > @@ -226,6 +228,8 @@ static match_table_t f2fs_tokens = { > {Opt_mode, "mode=%s"}, > {Opt_fault_injection, "fault_injection=%u"}, > {Opt_fault_type, "fault_type=%u"}, > + {Opt_lazytime, "lazytime"}, > + {Opt_nolazytime, "nolazytime"}, > {Opt_quota, "quota"}, > {Opt_noquota, "noquota"}, > {Opt_usrquota, "usrquota"}, > @@ -922,6 +926,12 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) > f2fs_info(sbi, "fault_type options not supported"); > break; > #endif > + case Opt_lazytime: > + sb->s_flags |= SB_LAZYTIME; > + break; > + case Opt_nolazytime: > + sb->s_flags &= ~SB_LAZYTIME; > + break; > #ifdef CONFIG_QUOTA > case Opt_quota: > case Opt_usrquota: > -- > 2.47.0.277.g8800431eea-goog
Hello: This patch was applied to jaegeuk/f2fs.git (dev) by Jaegeuk Kim <jaegeuk@kernel.org>: On Tue, 12 Nov 2024 01:08:20 +0000 you wrote: > This reverts commit 54f43a10fa257ad4af02a1d157fefef6ebcfa7dc. > > The above commit broke the lazytime mount, given > > mount("/dev/vdb", "/mnt/test", "f2fs", 0, "lazytime"); > > CC: stable@vger.kernel.org # 6.11+ > Signed-off-by: Daniel Rosenberg <drosen@google.com> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > [...] Here is the summary with links: - [f2fs-dev] Revert "f2fs: remove unreachable lazytime mount option parsing" https://git.kernel.org/jaegeuk/f2fs/c/f8bed73c6c52 You are awesome, thank you!
On 2024/11/13 5:39, Jaegeuk Kim via Linux-f2fs-devel wrote: > Hi Eric, > > Could you please check this revert as it breaks the mount()? > It seems F2FS needs to implement new mount support. Hi all, Actually, if we want to enable lazytime option, we can use mount syscall as: mount("/dev/vdb", "/mnt/test", "f2fs", MS_LAZYTIME, NULL); or use shell script as: mount -t f2fs -o lazytime /dev/vdb /mnt/test IIUC, the reason why mount command can handle lazytime is, after 8c7f073aaeaa ("libmount: add support for MS_LAZYTIME"), mount command supports to map "lazytime" to MS_LAZYTIME, and use MS_LAZYTIME in parameter @mountflags of mount(2). So, it looks we have alternative way to enable/disable lazytime feature after removing Opt_{no,}lazytime parsing in f2fs, do we really need this revert patch? Thanks, > > Thanks, > > On 11/12, Jaegeuk Kim wrote: >> This reverts commit 54f43a10fa257ad4af02a1d157fefef6ebcfa7dc. >> >> The above commit broke the lazytime mount, given >> >> mount("/dev/vdb", "/mnt/test", "f2fs", 0, "lazytime"); >> >> CC: stable@vger.kernel.org # 6.11+ >> Signed-off-by: Daniel Rosenberg <drosen@google.com> >> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> >> --- >> fs/f2fs/super.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >> index 49519439b770..35c4394e4fc6 100644 >> --- a/fs/f2fs/super.c >> +++ b/fs/f2fs/super.c >> @@ -150,6 +150,8 @@ enum { >> Opt_mode, >> Opt_fault_injection, >> Opt_fault_type, >> + Opt_lazytime, >> + Opt_nolazytime, >> Opt_quota, >> Opt_noquota, >> Opt_usrquota, >> @@ -226,6 +228,8 @@ static match_table_t f2fs_tokens = { >> {Opt_mode, "mode=%s"}, >> {Opt_fault_injection, "fault_injection=%u"}, >> {Opt_fault_type, "fault_type=%u"}, >> + {Opt_lazytime, "lazytime"}, >> + {Opt_nolazytime, "nolazytime"}, >> {Opt_quota, "quota"}, >> {Opt_noquota, "noquota"}, >> {Opt_usrquota, "usrquota"}, >> @@ -922,6 +926,12 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) >> f2fs_info(sbi, "fault_type options not supported"); >> break; >> #endif >> + case Opt_lazytime: >> + sb->s_flags |= SB_LAZYTIME; >> + break; >> + case Opt_nolazytime: >> + sb->s_flags &= ~SB_LAZYTIME; >> + break; >> #ifdef CONFIG_QUOTA >> case Opt_quota: >> case Opt_usrquota: >> -- >> 2.47.0.277.g8800431eea-goog > > > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On 11/19, Chao Yu wrote: > On 2024/11/13 5:39, Jaegeuk Kim via Linux-f2fs-devel wrote: > > Hi Eric, > > > > Could you please check this revert as it breaks the mount()? > > It seems F2FS needs to implement new mount support. > > Hi all, > > Actually, if we want to enable lazytime option, we can use mount > syscall as: > > mount("/dev/vdb", "/mnt/test", "f2fs", MS_LAZYTIME, NULL); > > or use shell script as: > > mount -t f2fs -o lazytime /dev/vdb /mnt/test > > IIUC, the reason why mount command can handle lazytime is, after > 8c7f073aaeaa ("libmount: add support for MS_LAZYTIME"), mount command > supports to map "lazytime" to MS_LAZYTIME, and use MS_LAZYTIME in > parameter @mountflags of mount(2). > > So, it looks we have alternative way to enable/disable lazytime feature > after removing Opt_{no,}lazytime parsing in f2fs, do we really need this > revert patch? This is a regression of the below command. I don't think offering others are feasible. mount("/dev/vdb", "/mnt/test", "f2fs", 0, "lazytime"); > > Thanks, > > > > > Thanks, > > > > On 11/12, Jaegeuk Kim wrote: > > > This reverts commit 54f43a10fa257ad4af02a1d157fefef6ebcfa7dc. > > > > > > The above commit broke the lazytime mount, given > > > > > > mount("/dev/vdb", "/mnt/test", "f2fs", 0, "lazytime"); > > > > > > CC: stable@vger.kernel.org # 6.11+ > > > Signed-off-by: Daniel Rosenberg <drosen@google.com> > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > --- > > > fs/f2fs/super.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > > > index 49519439b770..35c4394e4fc6 100644 > > > --- a/fs/f2fs/super.c > > > +++ b/fs/f2fs/super.c > > > @@ -150,6 +150,8 @@ enum { > > > Opt_mode, > > > Opt_fault_injection, > > > Opt_fault_type, > > > + Opt_lazytime, > > > + Opt_nolazytime, > > > Opt_quota, > > > Opt_noquota, > > > Opt_usrquota, > > > @@ -226,6 +228,8 @@ static match_table_t f2fs_tokens = { > > > {Opt_mode, "mode=%s"}, > > > {Opt_fault_injection, "fault_injection=%u"}, > > > {Opt_fault_type, "fault_type=%u"}, > > > + {Opt_lazytime, "lazytime"}, > > > + {Opt_nolazytime, "nolazytime"}, > > > {Opt_quota, "quota"}, > > > {Opt_noquota, "noquota"}, > > > {Opt_usrquota, "usrquota"}, > > > @@ -922,6 +926,12 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) > > > f2fs_info(sbi, "fault_type options not supported"); > > > break; > > > #endif > > > + case Opt_lazytime: > > > + sb->s_flags |= SB_LAZYTIME; > > > + break; > > > + case Opt_nolazytime: > > > + sb->s_flags &= ~SB_LAZYTIME; > > > + break; > > > #ifdef CONFIG_QUOTA > > > case Opt_quota: > > > case Opt_usrquota: > > > -- > > > 2.47.0.277.g8800431eea-goog > > > > > > _______________________________________________ > > Linux-f2fs-devel mailing list > > Linux-f2fs-devel@lists.sourceforge.net > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On 2024/11/20 4:48, Jaegeuk Kim wrote: > On 11/19, Chao Yu wrote: >> On 2024/11/13 5:39, Jaegeuk Kim via Linux-f2fs-devel wrote: >>> Hi Eric, >>> >>> Could you please check this revert as it breaks the mount()? >>> It seems F2FS needs to implement new mount support. >> >> Hi all, >> >> Actually, if we want to enable lazytime option, we can use mount >> syscall as: >> >> mount("/dev/vdb", "/mnt/test", "f2fs", MS_LAZYTIME, NULL); >> >> or use shell script as: >> >> mount -t f2fs -o lazytime /dev/vdb /mnt/test >> >> IIUC, the reason why mount command can handle lazytime is, after >> 8c7f073aaeaa ("libmount: add support for MS_LAZYTIME"), mount command >> supports to map "lazytime" to MS_LAZYTIME, and use MS_LAZYTIME in >> parameter @mountflags of mount(2). >> >> So, it looks we have alternative way to enable/disable lazytime feature >> after removing Opt_{no,}lazytime parsing in f2fs, do we really need this >> revert patch? > > This is a regression of the below command. I don't think offering others are > feasible. > > mount("/dev/vdb", "/mnt/test", "f2fs", 0, "lazytime"); Alright, there are other options were removed along w/ removal of related feature. e.g. 1. io_bits=%u by commit 87161a2b0aed ("f2fs: deprecate io_bits") 2. whint_mode=%s by commit 930e2607638d ("f2fs: remove obsolete whint_mode") Do we need to add these options handling back, and print "xxx options were deprecated" as we did in ("f2fs: kill heap-based allocation"), in order to avoid mount(......, "io_bits=%u" or "whint_mode=%s") command regression? diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index 867b147eb957..329f317e6f09 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -733,10 +733,8 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) clear_opt(sbi, DISCARD); break; case Opt_noheap: - set_opt(sbi, NOHEAP); - break; case Opt_heap: - clear_opt(sbi, NOHEAP); + f2fs_warn(sbi, "heap/no_heap options were deprecated"); break; Thanks, > >> >> Thanks, >> >>> >>> Thanks, >>> >>> On 11/12, Jaegeuk Kim wrote: >>>> This reverts commit 54f43a10fa257ad4af02a1d157fefef6ebcfa7dc. >>>> >>>> The above commit broke the lazytime mount, given >>>> >>>> mount("/dev/vdb", "/mnt/test", "f2fs", 0, "lazytime"); >>>> >>>> CC: stable@vger.kernel.org # 6.11+ >>>> Signed-off-by: Daniel Rosenberg <drosen@google.com> >>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> >>>> --- >>>> fs/f2fs/super.c | 10 ++++++++++ >>>> 1 file changed, 10 insertions(+) >>>> >>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>>> index 49519439b770..35c4394e4fc6 100644 >>>> --- a/fs/f2fs/super.c >>>> +++ b/fs/f2fs/super.c >>>> @@ -150,6 +150,8 @@ enum { >>>> Opt_mode, >>>> Opt_fault_injection, >>>> Opt_fault_type, >>>> + Opt_lazytime, >>>> + Opt_nolazytime, >>>> Opt_quota, >>>> Opt_noquota, >>>> Opt_usrquota, >>>> @@ -226,6 +228,8 @@ static match_table_t f2fs_tokens = { >>>> {Opt_mode, "mode=%s"}, >>>> {Opt_fault_injection, "fault_injection=%u"}, >>>> {Opt_fault_type, "fault_type=%u"}, >>>> + {Opt_lazytime, "lazytime"}, >>>> + {Opt_nolazytime, "nolazytime"}, >>>> {Opt_quota, "quota"}, >>>> {Opt_noquota, "noquota"}, >>>> {Opt_usrquota, "usrquota"}, >>>> @@ -922,6 +926,12 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) >>>> f2fs_info(sbi, "fault_type options not supported"); >>>> break; >>>> #endif >>>> + case Opt_lazytime: >>>> + sb->s_flags |= SB_LAZYTIME; >>>> + break; >>>> + case Opt_nolazytime: >>>> + sb->s_flags &= ~SB_LAZYTIME; >>>> + break; >>>> #ifdef CONFIG_QUOTA >>>> case Opt_quota: >>>> case Opt_usrquota: >>>> -- >>>> 2.47.0.277.g8800431eea-goog >>> >>> >>> _______________________________________________ >>> Linux-f2fs-devel mailing list >>> Linux-f2fs-devel@lists.sourceforge.net >>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On 11/12/24 3:39 PM, Jaegeuk Kim wrote: > Hi Eric, > > Could you please check this revert as it breaks the mount()? > It seems F2FS needs to implement new mount support. > > Thanks, I'm sorry, I missed this email. I will look into it more today. As for f2fs new mount API support, I have been struggling with it for a long time, f2fs has been uniquely complex. The assumption that the superblock and on-disk features are known at option parsing time makes it much more difficult than most other filesystems. But if there's a problem/regression with this commit, I have no objection to reverting the commit for now, and I'm sorry for the error. -Eric > On 11/12, Jaegeuk Kim wrote: >> This reverts commit 54f43a10fa257ad4af02a1d157fefef6ebcfa7dc. >> >> The above commit broke the lazytime mount, given >> >> mount("/dev/vdb", "/mnt/test", "f2fs", 0, "lazytime"); >> >> CC: stable@vger.kernel.org # 6.11+ >> Signed-off-by: Daniel Rosenberg <drosen@google.com> >> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> >> --- >> fs/f2fs/super.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >> index 49519439b770..35c4394e4fc6 100644 >> --- a/fs/f2fs/super.c >> +++ b/fs/f2fs/super.c >> @@ -150,6 +150,8 @@ enum { >> Opt_mode, >> Opt_fault_injection, >> Opt_fault_type, >> + Opt_lazytime, >> + Opt_nolazytime, >> Opt_quota, >> Opt_noquota, >> Opt_usrquota, >> @@ -226,6 +228,8 @@ static match_table_t f2fs_tokens = { >> {Opt_mode, "mode=%s"}, >> {Opt_fault_injection, "fault_injection=%u"}, >> {Opt_fault_type, "fault_type=%u"}, >> + {Opt_lazytime, "lazytime"}, >> + {Opt_nolazytime, "nolazytime"}, >> {Opt_quota, "quota"}, >> {Opt_noquota, "noquota"}, >> {Opt_usrquota, "usrquota"}, >> @@ -922,6 +926,12 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) >> f2fs_info(sbi, "fault_type options not supported"); >> break; >> #endif >> + case Opt_lazytime: >> + sb->s_flags |= SB_LAZYTIME; >> + break; >> + case Opt_nolazytime: >> + sb->s_flags &= ~SB_LAZYTIME; >> + break; >> #ifdef CONFIG_QUOTA >> case Opt_quota: >> case Opt_usrquota: >> -- >> 2.47.0.277.g8800431eea-goog >
On 11/20/24 8:27 AM, Eric Sandeen wrote: > On 11/12/24 3:39 PM, Jaegeuk Kim wrote: >> Hi Eric, >> >> Could you please check this revert as it breaks the mount()? >> It seems F2FS needs to implement new mount support. >> >> Thanks, > > I'm sorry, I missed this email. I will look into it more today. Ok, I see that I had not considered a direct mount call passing the lazytime option strings. :( Using mount(8), "lazytime" is never passed as an option all the way to f2fs, nor is "nolazytime" - # mount -o loop,nolazytime f2fsfile.img mnt # mount | grep lazytime /root/f2fs-test/f2fsfile.img on /root/f2fs-test/mnt type f2fs (rw,relatime,lazytime,seclabel,background_gc=on,nogc_merge,discard,discard_unit=block,user_xattr,inline_xattr,acl,inline_data,inline_dentry,flush_merge,barrier,extent_cache,mode=adaptive,active_logs=6,alloc_mode=reuse,checkpoint_merge,fsync_mode=posix,memory=normal,errors=continue) (note that lazytime is still set despite -o nolazytime) when mount(8) is using the new mount API, it does do fsconfig for (no)lazytime: fsconfig(3, FSCONFIG_SET_FLAG, "nolazytime", NULL, 0) = 0 but that is consumed by the VFS and never sent into f2fs for parsing. And because default_options() does: sbi->sb->s_flags |= SB_LAZYTIME; by default, it overrides the "nolazytime" that the vfs had previously handled. I'm fairly sure that when mount(8) was using the old mount API (long ago) it also did not send in the lazytime option string - it sent it as a flag instead. However - a direct call to mount(2) /will/ pass those options all the way to f2fs, and parse_options() does need to handle them there or it will be rejected as an invalid option. (Note that f2fs is the only filesystem that attempts to handle lazytime within the filesystem itself): [linux]# grep -r \"lazytime\" fs/*/ fs/f2fs/super.c: {Opt_lazytime, "lazytime"}, [linux]# I'm not entirely sure how to untangle all this, but regressions are not acceptable, so please revert my commit for now. Thanks, -Eric > As for f2fs new mount API support, I have been struggling with it for a > long time, f2fs has been uniquely complex. The assumption that the superblock > and on-disk features are known at option parsing time makes it much more > difficult than most other filesystems. > > But if there's a problem/regression with this commit, I have no objection to > reverting the commit for now, and I'm sorry for the error. > > -Eric > >> On 11/12, Jaegeuk Kim wrote: >>> This reverts commit 54f43a10fa257ad4af02a1d157fefef6ebcfa7dc. >>> >>> The above commit broke the lazytime mount, given >>> >>> mount("/dev/vdb", "/mnt/test", "f2fs", 0, "lazytime"); >>> >>> CC: stable@vger.kernel.org # 6.11+ >>> Signed-off-by: Daniel Rosenberg <drosen@google.com> >>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> >>> --- >>> fs/f2fs/super.c | 10 ++++++++++ >>> 1 file changed, 10 insertions(+) >>> >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>> index 49519439b770..35c4394e4fc6 100644 >>> --- a/fs/f2fs/super.c >>> +++ b/fs/f2fs/super.c >>> @@ -150,6 +150,8 @@ enum { >>> Opt_mode, >>> Opt_fault_injection, >>> Opt_fault_type, >>> + Opt_lazytime, >>> + Opt_nolazytime, >>> Opt_quota, >>> Opt_noquota, >>> Opt_usrquota, >>> @@ -226,6 +228,8 @@ static match_table_t f2fs_tokens = { >>> {Opt_mode, "mode=%s"}, >>> {Opt_fault_injection, "fault_injection=%u"}, >>> {Opt_fault_type, "fault_type=%u"}, >>> + {Opt_lazytime, "lazytime"}, >>> + {Opt_nolazytime, "nolazytime"}, >>> {Opt_quota, "quota"}, >>> {Opt_noquota, "noquota"}, >>> {Opt_usrquota, "usrquota"}, >>> @@ -922,6 +926,12 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) >>> f2fs_info(sbi, "fault_type options not supported"); >>> break; >>> #endif >>> + case Opt_lazytime: >>> + sb->s_flags |= SB_LAZYTIME; >>> + break; >>> + case Opt_nolazytime: >>> + sb->s_flags &= ~SB_LAZYTIME; >>> + break; >>> #ifdef CONFIG_QUOTA >>> case Opt_quota: >>> case Opt_usrquota: >>> -- >>> 2.47.0.277.g8800431eea-goog >> >
On 11/20, Chao Yu wrote: > On 2024/11/20 4:48, Jaegeuk Kim wrote: > > On 11/19, Chao Yu wrote: > > > On 2024/11/13 5:39, Jaegeuk Kim via Linux-f2fs-devel wrote: > > > > Hi Eric, > > > > > > > > Could you please check this revert as it breaks the mount()? > > > > It seems F2FS needs to implement new mount support. > > > > > > Hi all, > > > > > > Actually, if we want to enable lazytime option, we can use mount > > > syscall as: > > > > > > mount("/dev/vdb", "/mnt/test", "f2fs", MS_LAZYTIME, NULL); > > > > > > or use shell script as: > > > > > > mount -t f2fs -o lazytime /dev/vdb /mnt/test > > > > > > IIUC, the reason why mount command can handle lazytime is, after > > > 8c7f073aaeaa ("libmount: add support for MS_LAZYTIME"), mount command > > > supports to map "lazytime" to MS_LAZYTIME, and use MS_LAZYTIME in > > > parameter @mountflags of mount(2). > > > > > > So, it looks we have alternative way to enable/disable lazytime feature > > > after removing Opt_{no,}lazytime parsing in f2fs, do we really need this > > > revert patch? > > > > This is a regression of the below command. I don't think offering others are > > feasible. > > > > mount("/dev/vdb", "/mnt/test", "f2fs", 0, "lazytime"); > > Alright, there are other options were removed along w/ removal of > related feature. e.g. > > 1. io_bits=%u by commit 87161a2b0aed ("f2fs: deprecate io_bits") > 2. whint_mode=%s by commit 930e2607638d ("f2fs: remove obsolete whint_mode") > > Do we need to add these options handling back, and print "xxx options were > deprecated" as we did in ("f2fs: kill heap-based allocation"), in order to > avoid mount(......, "io_bits=%u" or "whint_mode=%s") command regression? These are different from the generic lazytime option, and I haven't got any complaint on this removal. > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > index 867b147eb957..329f317e6f09 100644 > --- a/fs/f2fs/super.c > +++ b/fs/f2fs/super.c > @@ -733,10 +733,8 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) > clear_opt(sbi, DISCARD); > break; > case Opt_noheap: > - set_opt(sbi, NOHEAP); > - break; > case Opt_heap: > - clear_opt(sbi, NOHEAP); > + f2fs_warn(sbi, "heap/no_heap options were deprecated"); > break; > > Thanks, > > > > > > > > > Thanks, > > > > > > > > > > > Thanks, > > > > > > > > On 11/12, Jaegeuk Kim wrote: > > > > > This reverts commit 54f43a10fa257ad4af02a1d157fefef6ebcfa7dc. > > > > > > > > > > The above commit broke the lazytime mount, given > > > > > > > > > > mount("/dev/vdb", "/mnt/test", "f2fs", 0, "lazytime"); > > > > > > > > > > CC: stable@vger.kernel.org # 6.11+ > > > > > Signed-off-by: Daniel Rosenberg <drosen@google.com> > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > > > --- > > > > > fs/f2fs/super.c | 10 ++++++++++ > > > > > 1 file changed, 10 insertions(+) > > > > > > > > > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > > > > > index 49519439b770..35c4394e4fc6 100644 > > > > > --- a/fs/f2fs/super.c > > > > > +++ b/fs/f2fs/super.c > > > > > @@ -150,6 +150,8 @@ enum { > > > > > Opt_mode, > > > > > Opt_fault_injection, > > > > > Opt_fault_type, > > > > > + Opt_lazytime, > > > > > + Opt_nolazytime, > > > > > Opt_quota, > > > > > Opt_noquota, > > > > > Opt_usrquota, > > > > > @@ -226,6 +228,8 @@ static match_table_t f2fs_tokens = { > > > > > {Opt_mode, "mode=%s"}, > > > > > {Opt_fault_injection, "fault_injection=%u"}, > > > > > {Opt_fault_type, "fault_type=%u"}, > > > > > + {Opt_lazytime, "lazytime"}, > > > > > + {Opt_nolazytime, "nolazytime"}, > > > > > {Opt_quota, "quota"}, > > > > > {Opt_noquota, "noquota"}, > > > > > {Opt_usrquota, "usrquota"}, > > > > > @@ -922,6 +926,12 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) > > > > > f2fs_info(sbi, "fault_type options not supported"); > > > > > break; > > > > > #endif > > > > > + case Opt_lazytime: > > > > > + sb->s_flags |= SB_LAZYTIME; > > > > > + break; > > > > > + case Opt_nolazytime: > > > > > + sb->s_flags &= ~SB_LAZYTIME; > > > > > + break; > > > > > #ifdef CONFIG_QUOTA > > > > > case Opt_quota: > > > > > case Opt_usrquota: > > > > > -- > > > > > 2.47.0.277.g8800431eea-goog > > > > > > > > > > > > _______________________________________________ > > > > Linux-f2fs-devel mailing list > > > > Linux-f2fs-devel@lists.sourceforge.net > > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On 11/20, Eric Sandeen wrote: > On 11/20/24 8:27 AM, Eric Sandeen wrote: > > On 11/12/24 3:39 PM, Jaegeuk Kim wrote: > >> Hi Eric, > >> > >> Could you please check this revert as it breaks the mount()? > >> It seems F2FS needs to implement new mount support. > >> > >> Thanks, > > > > I'm sorry, I missed this email. I will look into it more today. > > Ok, I see that I had not considered a direct mount call passing > the lazytime option strings. :( > > Using mount(8), "lazytime" is never passed as an option all the way to f2fs, > nor is "nolazytime" - > > # mount -o loop,nolazytime f2fsfile.img mnt > # mount | grep lazytime > /root/f2fs-test/f2fsfile.img on /root/f2fs-test/mnt type f2fs (rw,relatime,lazytime,seclabel,background_gc=on,nogc_merge,discard,discard_unit=block,user_xattr,inline_xattr,acl,inline_data,inline_dentry,flush_merge,barrier,extent_cache,mode=adaptive,active_logs=6,alloc_mode=reuse,checkpoint_merge,fsync_mode=posix,memory=normal,errors=continue) > > (note that lazytime is still set despite -o nolazytime) > > when mount(8) is using the new mount API, it does do fsconfig for (no)lazytime: > > fsconfig(3, FSCONFIG_SET_FLAG, "nolazytime", NULL, 0) = 0 > > but that is consumed by the VFS and never sent into f2fs for parsing. > > And because default_options() does: > > sbi->sb->s_flags |= SB_LAZYTIME; > > by default, it overrides the "nolazytime" that the vfs had previously handled. > > I'm fairly sure that when mount(8) was using the old mount API (long ago) it also > did not send in the lazytime option string - it sent it as a flag instead. > > However - a direct call to mount(2) /will/ pass those options all the way > to f2fs, and parse_options() does need to handle them there or it will be rejected > as an invalid option. > > (Note that f2fs is the only filesystem that attempts to handle lazytime within > the filesystem itself): > > [linux]# grep -r \"lazytime\" fs/*/ > fs/f2fs/super.c: {Opt_lazytime, "lazytime"}, > [linux]# > > I'm not entirely sure how to untangle all this, but regressions are not acceptable, > so please revert my commit for now. Thanks for the explanation. At a glance, I thought it's caused that f2fs doesn't implement fs_context_operations. We'll take a look at how to support it. > > Thanks, > -Eric > > > > As for f2fs new mount API support, I have been struggling with it for a > > long time, f2fs has been uniquely complex. The assumption that the superblock > > and on-disk features are known at option parsing time makes it much more > > difficult than most other filesystems. > > > > But if there's a problem/regression with this commit, I have no objection to > > reverting the commit for now, and I'm sorry for the error. > > > > -Eric > > > >> On 11/12, Jaegeuk Kim wrote: > >>> This reverts commit 54f43a10fa257ad4af02a1d157fefef6ebcfa7dc. > >>> > >>> The above commit broke the lazytime mount, given > >>> > >>> mount("/dev/vdb", "/mnt/test", "f2fs", 0, "lazytime"); > >>> > >>> CC: stable@vger.kernel.org # 6.11+ > >>> Signed-off-by: Daniel Rosenberg <drosen@google.com> > >>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > >>> --- > >>> fs/f2fs/super.c | 10 ++++++++++ > >>> 1 file changed, 10 insertions(+) > >>> > >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > >>> index 49519439b770..35c4394e4fc6 100644 > >>> --- a/fs/f2fs/super.c > >>> +++ b/fs/f2fs/super.c > >>> @@ -150,6 +150,8 @@ enum { > >>> Opt_mode, > >>> Opt_fault_injection, > >>> Opt_fault_type, > >>> + Opt_lazytime, > >>> + Opt_nolazytime, > >>> Opt_quota, > >>> Opt_noquota, > >>> Opt_usrquota, > >>> @@ -226,6 +228,8 @@ static match_table_t f2fs_tokens = { > >>> {Opt_mode, "mode=%s"}, > >>> {Opt_fault_injection, "fault_injection=%u"}, > >>> {Opt_fault_type, "fault_type=%u"}, > >>> + {Opt_lazytime, "lazytime"}, > >>> + {Opt_nolazytime, "nolazytime"}, > >>> {Opt_quota, "quota"}, > >>> {Opt_noquota, "noquota"}, > >>> {Opt_usrquota, "usrquota"}, > >>> @@ -922,6 +926,12 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) > >>> f2fs_info(sbi, "fault_type options not supported"); > >>> break; > >>> #endif > >>> + case Opt_lazytime: > >>> + sb->s_flags |= SB_LAZYTIME; > >>> + break; > >>> + case Opt_nolazytime: > >>> + sb->s_flags &= ~SB_LAZYTIME; > >>> + break; > >>> #ifdef CONFIG_QUOTA > >>> case Opt_quota: > >>> case Opt_usrquota: > >>> -- > >>> 2.47.0.277.g8800431eea-goog > >> > >
On 2024/11/12 9:08, Jaegeuk Kim via Linux-f2fs-devel wrote: > This reverts commit 54f43a10fa257ad4af02a1d157fefef6ebcfa7dc. > > The above commit broke the lazytime mount, given > > mount("/dev/vdb", "/mnt/test", "f2fs", 0, "lazytime"); > > CC: stable@vger.kernel.org # 6.11+ > Signed-off-by: Daniel Rosenberg <drosen@google.com> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> Reviewed-by: Chao Yu <chao@kernel.org> Thanks,
On 11/20/24 2:38 PM, Jaegeuk Kim wrote: > On 11/20, Eric Sandeen wrote: ... >> (Note that f2fs is the only filesystem that attempts to handle lazytime within >> the filesystem itself): >> >> [linux]# grep -r \"lazytime\" fs/*/ >> fs/f2fs/super.c: {Opt_lazytime, "lazytime"}, >> [linux]# >> >> I'm not entirely sure how to untangle all this, but regressions are not acceptable, >> so please revert my commit for now. > > Thanks for the explanation. At a glance, I thought it's caused that f2fs doesn't > implement fs_context_operations. We'll take a look at how to support it. (cc: list trimmed) I had thought the conversion would resolve this too, but had not considered direct mount(2) calls passing the string in, which is something that probably needs to be supported even after the conversion, sadly. As a reminder, this might be a start / sketch of how to convert to the new mount API: https://git.kernel.org/pub/scm/linux/kernel/git/sandeen/linux.git/log/?h=f2fs-mount-api It's not entirely correct, but at least the first several patches are probably the right idea - getting sb / sbi out of the parsing path, and deferring option-vs-disk-feature checks until after the superblock is read, etc. The final patch is probably not the way to go - it allocates an entire f2fs_sb_info in f2fs_init_fs_context - it probably makes more sense to create a new context structure which holds only mount options, which is then transferred into the sbi after option parsing during mount or remount. I was doing these conversions as a side project, and given the f2fs conversion complexity, I have yet to get to a series that I'm happy with. Perhaps expert eyes can help! Thanks, -Eric
On 11/21, Eric Sandeen wrote: > On 11/20/24 2:38 PM, Jaegeuk Kim wrote: > > On 11/20, Eric Sandeen wrote: > > ... > > >> (Note that f2fs is the only filesystem that attempts to handle lazytime within > >> the filesystem itself): > >> > >> [linux]# grep -r \"lazytime\" fs/*/ > >> fs/f2fs/super.c: {Opt_lazytime, "lazytime"}, > >> [linux]# > >> > >> I'm not entirely sure how to untangle all this, but regressions are not acceptable, > >> so please revert my commit for now. > > > > Thanks for the explanation. At a glance, I thought it's caused that f2fs doesn't > > implement fs_context_operations. We'll take a look at how to support it. > > (cc: list trimmed) > > I had thought the conversion would resolve this too, but had not considered direct > mount(2) calls passing the string in, which is something that probably needs to be > supported even after the conversion, sadly. > > As a reminder, this might be a start / sketch of how to convert to the new mount API: > > https://git.kernel.org/pub/scm/linux/kernel/git/sandeen/linux.git/log/?h=f2fs-mount-api > > It's not entirely correct, but at least the first several patches are probably the right > idea - getting sb / sbi out of the parsing path, and deferring option-vs-disk-feature > checks until after the superblock is read, etc. > > The final patch is probably not the way to go - it allocates an entire f2fs_sb_info > in f2fs_init_fs_context - it probably makes more sense to create a new context > structure which holds only mount options, which is then transferred into the > sbi after option parsing during mount or remount. Cool. Let us take a look at them soon. Thank you! > > I was doing these conversions as a side project, and given the f2fs conversion > complexity, I have yet to get to a series that I'm happy with. Perhaps expert > eyes can help! > > Thanks, > -Eric
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index 49519439b770..35c4394e4fc6 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -150,6 +150,8 @@ enum { Opt_mode, Opt_fault_injection, Opt_fault_type, + Opt_lazytime, + Opt_nolazytime, Opt_quota, Opt_noquota, Opt_usrquota, @@ -226,6 +228,8 @@ static match_table_t f2fs_tokens = { {Opt_mode, "mode=%s"}, {Opt_fault_injection, "fault_injection=%u"}, {Opt_fault_type, "fault_type=%u"}, + {Opt_lazytime, "lazytime"}, + {Opt_nolazytime, "nolazytime"}, {Opt_quota, "quota"}, {Opt_noquota, "noquota"}, {Opt_usrquota, "usrquota"}, @@ -922,6 +926,12 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) f2fs_info(sbi, "fault_type options not supported"); break; #endif + case Opt_lazytime: + sb->s_flags |= SB_LAZYTIME; + break; + case Opt_nolazytime: + sb->s_flags &= ~SB_LAZYTIME; + break; #ifdef CONFIG_QUOTA case Opt_quota: case Opt_usrquota:
This reverts commit 54f43a10fa257ad4af02a1d157fefef6ebcfa7dc. The above commit broke the lazytime mount, given mount("/dev/vdb", "/mnt/test", "f2fs", 0, "lazytime"); CC: stable@vger.kernel.org # 6.11+ Signed-off-by: Daniel Rosenberg <drosen@google.com> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/super.c | 10 ++++++++++ 1 file changed, 10 insertions(+)