On 05/07, Eric Sandeen wrote:
> 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?
I'm worrying about any missing case to check options enabled by default_options.
For example, in the case of device_aliasing, we rely on enabling extent_cache
by default_options, which was not caught by f2fs_check_opt_consistency.
I was thinking that we'd need a post sanity check.
>
> 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;
