On 5/7/25 2:48 PM, Jaegeuk Kim wrote:
> On 05/07, Eric Sandeen wrote:
>> On 5/7/25 9:46 AM, Jaegeuk Kim wrote:
>>
>>> I meant:
>>>
>>> # mkfs/mkfs.f2fs -c /dev/vdc@xxxxxxxx /dev/vdb
>>> # mount /dev/vdb mnt
>>>
>>> It's supposed to be successful, since extent_cache is enabled by default.
>>
>> I'm sorry, clearly I was too sleepy last night. This fixes it for me.
>>
>> We have to test the mask to see if the option was explisitly set (either
>> extent_cache or noextent_cache) at mount time.
>>
>> If it was not specified at all, it will be set by the default f'n and
>> remain in the sbi, and it will pass this consistency check.
>>
>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>> index d89b9ede221e..e178796ce9a7 100644
>> --- a/fs/f2fs/super.c
>> +++ b/fs/f2fs/super.c
>> @@ -1412,7 +1414,8 @@ static int f2fs_check_opt_consistency(struct fs_context *fc,
>> }
>>
>> if (f2fs_sb_has_device_alias(sbi) &&
>> - !ctx_test_opt(ctx, F2FS_MOUNT_READ_EXTENT_CACHE)) {
>> + (ctx->opt_mask & F2FS_MOUNT_READ_EXTENT_CACHE) &&
>> + !ctx_test_opt(ctx, F2FS_MOUNT_READ_EXTENT_CACHE)) {
>> f2fs_err(sbi, "device aliasing requires extent cache");
>> return -EINVAL;
>> }
>
> I think that will cover the user-given options only, but we'd better check the
> final options as well. Can we apply like this?
I'm sorry, I'm not sure I understand what situation this additional
changes will cover...
It looks like this adds the f2fs_sanity_check_options() to the remount
path to explicitly (re-)check a few things.
But as far as I can tell, at least for the extent cache, remount is handled
properly already (with the hunk above):
# mkfs/mkfs.f2fs -c /dev/vdc@xxxxxxxx /dev/vdb
# mount /dev/vdb mnt
# mount -o remount,noextent_cache mnt
mount: /root/mnt: mount point not mounted or bad option.
dmesg(1) may have more information after failed mount system call.
# dmesg | tail -n 1
[60012.364941] F2FS-fs (vdb): device aliasing requires extent cache
#
I haven't tested with i.e. blkzoned devices though, is there a testcase
that fails for you?
Thanks,
-Eric
> ---
> fs/f2fs/super.c | 50 ++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 33 insertions(+), 17 deletions(-)
>
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index d89b9ede221e..270a9bf9773d 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1412,6 +1412,7 @@ static int f2fs_check_opt_consistency(struct fs_context *fc,
> }
>
> if (f2fs_sb_has_device_alias(sbi) &&
> + (ctx->opt_mask & F2FS_MOUNT_READ_EXTENT_CACHE) &&
> !ctx_test_opt(ctx, F2FS_MOUNT_READ_EXTENT_CACHE)) {
> f2fs_err(sbi, "device aliasing requires extent cache");
> return -EINVAL;
> @@ -1657,6 +1658,29 @@ static void f2fs_apply_options(struct fs_context *fc, struct super_block *sb)
> f2fs_apply_quota_options(fc, sb);
> }
>
> +static int f2fs_sanity_check_options(struct f2fs_sb_info *sbi)
> +{
> + if (f2fs_sb_has_device_alias(sbi) &&
> + !test_opt(sbi, READ_EXTENT_CACHE)) {
> + f2fs_err(sbi, "device aliasing requires extent cache");
> + return -EINVAL;
> + }
> +#ifdef CONFIG_BLK_DEV_ZONED
> + if (f2fs_sb_has_blkzoned(sbi) &&
> + sbi->max_open_zones < F2FS_OPTION(sbi).active_logs) {
> + f2fs_err(sbi,
> + "zoned: max open zones %u is too small, need at least %u open zones",
> + sbi->max_open_zones, F2FS_OPTION(sbi).active_logs);
> + return -EINVAL;
> + }
> +#endif
> + if (f2fs_lfs_mode(sbi) && !IS_F2FS_IPU_DISABLE(sbi)) {
> + f2fs_warn(sbi, "LFS is not compatible with IPU");
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> static struct inode *f2fs_alloc_inode(struct super_block *sb)
> {
> struct f2fs_inode_info *fi;
> @@ -2616,21 +2640,15 @@ static int __f2fs_remount(struct fs_context *fc, struct super_block *sb)
> default_options(sbi, true);
>
> err = f2fs_check_opt_consistency(fc, sb);
> - if (err < 0)
> + if (err)
> goto restore_opts;
>
> f2fs_apply_options(fc, sb);
>
> -#ifdef CONFIG_BLK_DEV_ZONED
> - if (f2fs_sb_has_blkzoned(sbi) &&
> - sbi->max_open_zones < F2FS_OPTION(sbi).active_logs) {
> - f2fs_err(sbi,
> - "zoned: max open zones %u is too small, need at least %u open zones",
> - sbi->max_open_zones, F2FS_OPTION(sbi).active_logs);
> - err = -EINVAL;
> + err = f2fs_sanity_check_options(sbi);
> + if (err)
> goto restore_opts;
> - }
> -#endif
> +
> /* flush outstanding errors before changing fs state */
> flush_work(&sbi->s_error_work);
>
> @@ -2663,12 +2681,6 @@ static int __f2fs_remount(struct fs_context *fc, struct super_block *sb)
> }
> }
> #endif
> - if (f2fs_lfs_mode(sbi) && !IS_F2FS_IPU_DISABLE(sbi)) {
> - err = -EINVAL;
> - f2fs_warn(sbi, "LFS is not compatible with IPU");
> - goto restore_opts;
> - }
> -
> /* disallow enable atgc dynamically */
> if (no_atgc == !!test_opt(sbi, ATGC)) {
> err = -EINVAL;
> @@ -4808,11 +4820,15 @@ static int f2fs_fill_super(struct super_block *sb, struct fs_context *fc)
> default_options(sbi, false);
>
> err = f2fs_check_opt_consistency(fc, sb);
> - if (err < 0)
> + if (err)
> goto free_sb_buf;
>
> f2fs_apply_options(fc, sb);
>
> + err = f2fs_sanity_check_options(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;
