diff mbox series

[f2fs-dev,3/9] f2fs: factor out an f2fs_default_check function

Message ID 20250303172127.298602-4-sandeen@redhat.com (mailing list archive)
State New
Headers show
Series f2fs: first steps towards mount API conversion | expand

Commit Message

Eric Sandeen March 3, 2025, 5:12 p.m. UTC 
From: Eric Sandeen <sandeen@sandeen.net>

The current options parsing function both parses options and validates
them - factor the validation out to reduce the size of the function and
make transition to the new mount API possible, because under the new mount
API, options are parsed one at a time, and cannot all be tested at the end
of the parsing function.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/f2fs/super.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Comments

Chao Yu March 12, 2025, 3:10 a.m. UTC | #1 
On 3/4/25 01:12, Eric Sandeen wrote:
> From: Eric Sandeen <sandeen@sandeen.net>
> 
> The current options parsing function both parses options and validates
> them - factor the validation out to reduce the size of the function and
> make transition to the new mount API possible, because under the new mount
> API, options are parsed one at a time, and cannot all be tested at the end
> of the parsing function.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  fs/f2fs/super.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 29b3aa1ee99c..7cfd5e4e806e 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -687,7 +687,7 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
>  	int ret;
>  
>  	if (!options)
> -		goto default_check;

Eric, do you know in which condition options can be NULL, mount w/o any
specified options?

If so, maybe we'd keep this in order to chech whether default options
generated from default_options() is conflicted or not? What do you think?

Thanks,

> +		return 0;
>  
>  	while ((p = strsep(&options, ",")) != NULL) {
>  		int token;
> @@ -1318,7 +1318,11 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
>  			return -EINVAL;
>  		}
>  	}
> -default_check:
> +	return 0;
> +}
> +
> +static int f2fs_default_check(struct f2fs_sb_info *sbi)
> +{
>  #ifdef CONFIG_QUOTA
>  	if (f2fs_check_quota_options(sbi))
>  		return -EINVAL;
> @@ -2364,6 +2368,10 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
>  	}
>  #endif
>  
> +	err = f2fs_default_check(sbi);
> +	if (err)
> +		goto restore_opts;
> +
>  	/* flush outstanding errors before changing fs state */
>  	flush_work(&sbi->s_error_work);
>  
> @@ -4489,6 +4497,10 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>  	if (err)
>  		goto free_options;
>  
> +	err = f2fs_default_check(sbi);
> +	if (err)
> +		goto free_options;
> +
>  	sb->s_maxbytes = max_file_blocks(NULL) <<
>  				le32_to_cpu(raw_super->log_blocksize);
>  	sb->s_max_links = F2FS_LINK_MAX;
Eric Sandeen March 12, 2025, 1:29 p.m. UTC | #2 
On 3/11/25 10:10 PM, Chao Yu wrote:
> On 3/4/25 01:12, Eric Sandeen wrote:
>> From: Eric Sandeen <sandeen@sandeen.net>
>>
>> The current options parsing function both parses options and validates
>> them - factor the validation out to reduce the size of the function and
>> make transition to the new mount API possible, because under the new mount
>> API, options are parsed one at a time, and cannot all be tested at the end
>> of the parsing function.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>  fs/f2fs/super.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>> index 29b3aa1ee99c..7cfd5e4e806e 100644
>> --- a/fs/f2fs/super.c
>> +++ b/fs/f2fs/super.c
>> @@ -687,7 +687,7 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
>>  	int ret;
>>  
>>  	if (!options)
>> -		goto default_check;
> 
> Eric, do you know in which condition options can be NULL, mount w/o any
> specified options?
> 
> If so, maybe we'd keep this in order to chech whether default options
> generated from default_options() is conflicted or not? What do you think?
> 
> Thanks,

Yes, that's I think that is correct - (!options) is true when no f2fs-specific
options are present for parsing.

However, I think that we do still check default options with my patch in this
case, because both calls to parse_options() still call f2fs_default_check()
when parse_options() completes.

Or am I misunderstanding your question?

I added printks to check:

# mount -o loop,ro  f2fsfile.img mnt
[root@fedora-rawhide f2fs-test]# dmesg 
[847946.326384] loop2: detected capacity change from 0 to 819200
[847946.337625] parse_options: (!options) is true
[847946.337637] enter f2fs_default_check()

Thank you for reviewing this series. I think at least the first 2 or 3 patches
are suitable for merge now, if you agree. I am fairly certain that the rest of
them are proper steps towards mount API conversion as well, but as I have not
yet finished the work, I can't guarantee it yet. :)

Thanks,
-Eric
Chao Yu March 13, 2025, 1:40 a.m. UTC | #3 
On 3/12/25 21:29, Eric Sandeen wrote:
> On 3/11/25 10:10 PM, Chao Yu wrote:
>> On 3/4/25 01:12, Eric Sandeen wrote:
>>> From: Eric Sandeen <sandeen@sandeen.net>
>>>
>>> The current options parsing function both parses options and validates
>>> them - factor the validation out to reduce the size of the function and
>>> make transition to the new mount API possible, because under the new mount
>>> API, options are parsed one at a time, and cannot all be tested at the end
>>> of the parsing function.
>>>
>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>> ---
>>>  fs/f2fs/super.c | 16 ++++++++++++++--
>>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>> index 29b3aa1ee99c..7cfd5e4e806e 100644
>>> --- a/fs/f2fs/super.c
>>> +++ b/fs/f2fs/super.c
>>> @@ -687,7 +687,7 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
>>>  	int ret;
>>>  
>>>  	if (!options)
>>> -		goto default_check;
>>
>> Eric, do you know in which condition options can be NULL, mount w/o any
>> specified options?
>>
>> If so, maybe we'd keep this in order to chech whether default options
>> generated from default_options() is conflicted or not? What do you think?
>>
>> Thanks,
> 
> Yes, that's I think that is correct - (!options) is true when no f2fs-specific
> options are present for parsing.
> 
> However, I think that we do still check default options with my patch in this
> case, because both calls to parse_options() still call f2fs_default_check()
> when parse_options() completes.
> 
> Or am I misunderstanding your question?
> 
> I added printks to check:
> 
> # mount -o loop,ro  f2fsfile.img mnt
> [root@fedora-rawhide f2fs-test]# dmesg 
> [847946.326384] loop2: detected capacity change from 0 to 819200
> [847946.337625] parse_options: (!options) is true
> [847946.337637] enter f2fs_default_check()

Oh, I missed that we will call f2fs_default_check() after default_options()
anyway, so I guess it's fine here.

Thanks for your confirmation.

> 
> Thank you for reviewing this series. I think at least the first 2 or 3 patches
> are suitable for merge now, if you agree. I am fairly certain that the rest of

I've reviewed other patches, they look clean to me, Jaegeuk has merged them into
dev-test branch, let's test them for a while.

> them are proper steps towards mount API conversion as well, but as I have not
> yet finished the work, I can't guarantee it yet. :)

Thanks for the effect, and good to see progress on changing to use new
mount API. :)

Thanks,

> 
> Thanks,
> -Eric
>
Chao Yu March 13, 2025, 1:40 a.m. UTC | #4 
On 3/4/25 01:12, Eric Sandeen wrote:
> From: Eric Sandeen <sandeen@sandeen.net>
> 
> The current options parsing function both parses options and validates
> them - factor the validation out to reduce the size of the function and
> make transition to the new mount API possible, because under the new mount
> API, options are parsed one at a time, and cannot all be tested at the end
> of the parsing function.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,
diff mbox series

Patch

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 29b3aa1ee99c..7cfd5e4e806e 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -687,7 +687,7 @@  static int parse_options(struct super_block *sb, char *options, bool is_remount)
 	int ret;
 
 	if (!options)
-		goto default_check;
+		return 0;
 
 	while ((p = strsep(&options, ",")) != NULL) {
 		int token;
@@ -1318,7 +1318,11 @@  static int parse_options(struct super_block *sb, char *options, bool is_remount)
 			return -EINVAL;
 		}
 	}
-default_check:
+	return 0;
+}
+
+static int f2fs_default_check(struct f2fs_sb_info *sbi)
+{
 #ifdef CONFIG_QUOTA
 	if (f2fs_check_quota_options(sbi))
 		return -EINVAL;
@@ -2364,6 +2368,10 @@  static int f2fs_remount(struct super_block *sb, int *flags, char *data)
 	}
 #endif
 
+	err = f2fs_default_check(sbi);
+	if (err)
+		goto restore_opts;
+
 	/* flush outstanding errors before changing fs state */
 	flush_work(&sbi->s_error_work);
 
@@ -4489,6 +4497,10 @@  static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	if (err)
 		goto free_options;
 
+	err = f2fs_default_check(sbi);
+	if (err)
+		goto free_options;
+
 	sb->s_maxbytes = max_file_blocks(NULL) <<
 				le32_to_cpu(raw_super->log_blocksize);
 	sb->s_max_links = F2FS_LINK_MAX;