[PATCH] Revert "f2fs: remove unreachable lazytime mount option parsing"

[PATCH] Revert "f2fs: remove unreachable lazytime mount option parsing"

[Jaegeuk Kim]

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

Re: [PATCH] Revert "f2fs: remove unreachable lazytime mount option parsing"

[Jaegeuk Kim]

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

Re: [f2fs-dev] [PATCH] Revert "f2fs: remove unreachable lazytime mount option parsing"

[patchwork-bot+f2fs]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [f2fs-dev] [PATCH] Revert "f2fs: remove unreachable lazytime mount option parsing"

[Chao Yu]

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

Re: [f2fs-dev] [PATCH] Revert "f2fs: remove unreachable lazytime mount option parsing"

[Jaegeuk Kim]

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

Re: [f2fs-dev] [PATCH] Revert "f2fs: remove unreachable lazytime mount option parsing"

[Chao Yu]

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

Re: [PATCH] Revert "f2fs: remove unreachable lazytime mount option parsing"

[Eric Sandeen]

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
> 

Re: [PATCH] Revert "f2fs: remove unreachable lazytime mount option parsing"

[Eric Sandeen]

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
>>
> 

Re: [f2fs-dev] [PATCH] Revert "f2fs: remove unreachable lazytime mount option parsing"

[Jaegeuk Kim]

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

Re: [PATCH] Revert "f2fs: remove unreachable lazytime mount option parsing"

[Jaegeuk Kim]

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
> >>
> > 

Re: [f2fs-dev] [PATCH] Revert "f2fs: remove unreachable lazytime mount option parsing"

[Chao Yu]

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,