Re: [f2fs-dev] [PATCH V3 7/7] f2fs: switch to the new mount api

[Jaegeuk Kim via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>]

Hi, Hongbo,

It seems we're getting more issues in the patch set. May I ask for some
help sending the new patch series having all the fixes that I made as well
as addressing the concerns? You can get the patches from [1].

[1] https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/log/?h=dev-test

On 05/14, Hongbo Li wrote:
> 
> 
> On 2025/5/14 12:03, Chao Yu wrote:
> > On 5/14/25 10:33, Hongbo Li wrote:
> > > 
> > > 
> > > On 2025/5/13 16:59, Chao Yu wrote:
> > > > On 4/24/25 01:08, Eric Sandeen wrote:
> > > > > From: Hongbo Li <lihongbo22@huawei.com>
> > > > > 
> > > > > The new mount api will execute .parse_param, .init_fs_context, .get_tree
> > > > > and will call .remount if remount happened. So we add the necessary
> > > > > functions for the fs_context_operations. If .init_fs_context is added,
> > > > > the old .mount should remove.
> > > > > 
> > > > > See Documentation/filesystems/mount_api.rst for more information.
> > > > 
> > > > mkfs.f2fs -f -O extra_attr,flexible_inline_xattr /dev/vdb
> > > > mount -o inline_xattr_size=512 /dev/vdb /mnt/f2fs
> > > > mount: /mnt/f2fs: wrong fs type, bad option, bad superblock on /dev/vdb, missing codepage or helper program, or other error.
> > > >          dmesg(1) may have more information after failed mount system call.
> > > > dmesg
> > > > [ 1758.202282] F2FS-fs (vdb): Image doesn't support compression
> > > > [ 1758.202286] F2FS-fs (vdb): inline_xattr_size option should be set with inline_xattr option
> > > > 
> > > > Eric, Hongbo, can you please take a look at this issue?
> > > > 
> > > Sorry, we only check the option hold in ctx, we should do the double check in sbi. Or other elegant approaches.
> > > 
> > > For the "support compression", is it also the error in this testcase?
> > 
> > I think so, I checked this w/ additional logs, and found this:
> > 
> > #define F2FS_MOUNT_INLINE_XATTR_SIZE	0x00080000
> > #define F2FS_SPEC_compress_chksum		(1 << 19) /* equal to 0x00080000)
> > 
> > 	if (!f2fs_sb_has_compression(sbi)) {
> > 		if (test_compression_spec(ctx->opt_mask) ||
> > 			ctx_test_opt(ctx, F2FS_MOUNT_COMPRESS_CACHE))
> > 			f2fs_info(sbi, "Image doesn't support compression");
> > 		clear_compression_spec(ctx);
> > 		ctx->opt_mask &= ~F2FS_MOUNT_COMPRESS_CACHE;
> > 		return 0;
> > 	}
> > 
> > We should use test_compression_spec(ctx->spec_mask) instead of
> > test_compression_spec(ctx->opt_mask) to check special mask of compression
> > option?
> > 
> 
> Yeah, you're right. test_compression_spec is used to check spec_mask, and we
> got it wrong.
> 
> Thanks,
> Hongbo
> 
> > Thanks,
> > 
> > > 
> > > Thanks,
> > > Hongbo
> > > 
> > > > Thanks,
> > > > 
> > > > > 
> > > > > Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
> > > > > [sandeen: forward port]
> > > > > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > > > > ---
> > > > >    fs/f2fs/super.c | 156 +++++++++++++++++++-----------------------------
> > > > >    1 file changed, 62 insertions(+), 94 deletions(-)
> > > > > 
> > > > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > > > > index 37497fd80bb9..041bd6c482a0 100644
> > > > > --- a/fs/f2fs/super.c
> > > > > +++ b/fs/f2fs/super.c
> > > > > @@ -1141,47 +1141,6 @@ static int f2fs_parse_param(struct fs_context *fc, struct fs_parameter *param)
> > > > >        return 0;
> > > > >    }
> > > > >    -static int parse_options(struct fs_context *fc, char *options)
> > > > > -{
> > > > > -    struct fs_parameter param;
> > > > > -    char *key;
> > > > > -    int ret;
> > > > > -
> > > > > -    if (!options)
> > > > > -        return 0;
> > > > > -
> > > > > -    while ((key = strsep(&options, ",")) != NULL) {
> > > > > -        if (*key) {
> > > > > -            size_t v_len = 0;
> > > > > -            char *value = strchr(key, '=');
> > > > > -
> > > > > -            param.type = fs_value_is_flag;
> > > > > -            param.string = NULL;
> > > > > -
> > > > > -            if (value) {
> > > > > -                if (value == key)
> > > > > -                    continue;
> > > > > -
> > > > > -                *value++ = 0;
> > > > > -                v_len = strlen(value);
> > > > > -                param.string = kmemdup_nul(value, v_len, GFP_KERNEL);
> > > > > -                if (!param.string)
> > > > > -                    return -ENOMEM;
> > > > > -                param.type = fs_value_is_string;
> > > > > -            }
> > > > > -
> > > > > -            param.key = key;
> > > > > -            param.size = v_len;
> > > > > -
> > > > > -            ret = f2fs_parse_param(fc, &param);
> > > > > -            kfree(param.string);
> > > > > -            if (ret < 0)
> > > > > -                return ret;
> > > > > -        }
> > > > > -    }
> > > > > -    return 0;
> > > > > -}
> > > > > -
> > > > >    /*
> > > > >     * Check quota settings consistency.
> > > > >     */
> > > > > @@ -2583,13 +2542,12 @@ static void f2fs_enable_checkpoint(struct f2fs_sb_info *sbi)
> > > > >        f2fs_flush_ckpt_thread(sbi);
> > > > >    }
> > > > >    -static int f2fs_remount(struct super_block *sb, int *flags, char *data)
> > > > > +static int __f2fs_remount(struct fs_context *fc, struct super_block *sb)
> > > > >    {
> > > > >        struct f2fs_sb_info *sbi = F2FS_SB(sb);
> > > > >        struct f2fs_mount_info org_mount_opt;
> > > > > -    struct f2fs_fs_context ctx;
> > > > > -    struct fs_context fc;
> > > > >        unsigned long old_sb_flags;
> > > > > +    unsigned int flags = fc->sb_flags;
> > > > >        int err;
> > > > >        bool need_restart_gc = false, need_stop_gc = false;
> > > > >        bool need_restart_flush = false, need_stop_flush = false;
> > > > > @@ -2635,7 +2593,7 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
> > > > >    #endif
> > > > >          /* recover superblocks we couldn't write due to previous RO mount */
> > > > > -    if (!(*flags & SB_RDONLY) && is_sbi_flag_set(sbi, SBI_NEED_SB_WRITE)) {
> > > > > +    if (!(flags & SB_RDONLY) && is_sbi_flag_set(sbi, SBI_NEED_SB_WRITE)) {
> > > > >            err = f2fs_commit_super(sbi, false);
> > > > >            f2fs_info(sbi, "Try to recover all the superblocks, ret: %d",
> > > > >                  err);
> > > > > @@ -2645,21 +2603,11 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
> > > > >          default_options(sbi, true);
> > > > >    -    memset(&fc, 0, sizeof(fc));
> > > > > -    memset(&ctx, 0, sizeof(ctx));
> > > > > -    fc.fs_private = &ctx;
> > > > > -    fc.purpose = FS_CONTEXT_FOR_RECONFIGURE;
> > > > > -
> > > > > -    /* parse mount options */
> > > > > -    err = parse_options(&fc, data);
> > > > > -    if (err)
> > > > > -        goto restore_opts;
> > > > > -
> > > > > -    err = f2fs_check_opt_consistency(&fc, sb);
> > > > > +    err = f2fs_check_opt_consistency(fc, sb);
> > > > >        if (err < 0)
> > > > >            goto restore_opts;
> > > > >    -    f2fs_apply_options(&fc, sb);
> > > > > +    f2fs_apply_options(fc, sb);
> > > > >      #ifdef CONFIG_BLK_DEV_ZONED
> > > > >        if (f2fs_sb_has_blkzoned(sbi) &&
> > > > > @@ -2678,20 +2626,20 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
> > > > >         * Previous and new state of filesystem is RO,
> > > > >         * so skip checking GC and FLUSH_MERGE conditions.
> > > > >         */
> > > > > -    if (f2fs_readonly(sb) && (*flags & SB_RDONLY))
> > > > > +    if (f2fs_readonly(sb) && (flags & SB_RDONLY))
> > > > >            goto skip;
> > > > >    -    if (f2fs_dev_is_readonly(sbi) && !(*flags & SB_RDONLY)) {
> > > > > +    if (f2fs_dev_is_readonly(sbi) && !(flags & SB_RDONLY)) {
> > > > >            err = -EROFS;
> > > > >            goto restore_opts;
> > > > >        }
> > > > >      #ifdef CONFIG_QUOTA
> > > > > -    if (!f2fs_readonly(sb) && (*flags & SB_RDONLY)) {
> > > > > +    if (!f2fs_readonly(sb) && (flags & SB_RDONLY)) {
> > > > >            err = dquot_suspend(sb, -1);
> > > > >            if (err < 0)
> > > > >                goto restore_opts;
> > > > > -    } else if (f2fs_readonly(sb) && !(*flags & SB_RDONLY)) {
> > > > > +    } else if (f2fs_readonly(sb) && !(flags & SB_RDONLY)) {
> > > > >            /* dquot_resume needs RW */
> > > > >            sb->s_flags &= ~SB_RDONLY;
> > > > >            if (sb_any_quota_suspended(sb)) {
> > > > > @@ -2747,7 +2695,7 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
> > > > >            goto restore_opts;
> > > > >        }
> > > > >    -    if ((*flags & SB_RDONLY) && test_opt(sbi, DISABLE_CHECKPOINT)) {
> > > > > +    if ((flags & SB_RDONLY) && test_opt(sbi, DISABLE_CHECKPOINT)) {
> > > > >            err = -EINVAL;
> > > > >            f2fs_warn(sbi, "disabling checkpoint not compatible with read-only");
> > > > >            goto restore_opts;
> > > > > @@ -2758,7 +2706,7 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
> > > > >         * or if background_gc = off is passed in mount
> > > > >         * option. Also sync the filesystem.
> > > > >         */
> > > > > -    if ((*flags & SB_RDONLY) ||
> > > > > +    if ((flags & SB_RDONLY) ||
> > > > >                (F2FS_OPTION(sbi).bggc_mode == BGGC_MODE_OFF &&
> > > > >                !test_opt(sbi, GC_MERGE))) {
> > > > >            if (sbi->gc_thread) {
> > > > > @@ -2772,7 +2720,7 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
> > > > >            need_stop_gc = true;
> > > > >        }
> > > > >    -    if (*flags & SB_RDONLY) {
> > > > > +    if (flags & SB_RDONLY) {
> > > > >            sync_inodes_sb(sb);
> > > > >              set_sbi_flag(sbi, SBI_IS_DIRTY);
> > > > > @@ -2785,7 +2733,7 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
> > > > >         * We stop issue flush thread if FS is mounted as RO
> > > > >         * or if flush_merge is not passed in mount option.
> > > > >         */
> > > > > -    if ((*flags & SB_RDONLY) || !test_opt(sbi, FLUSH_MERGE)) {
> > > > > +    if ((flags & SB_RDONLY) || !test_opt(sbi, FLUSH_MERGE)) {
> > > > >            clear_opt(sbi, FLUSH_MERGE);
> > > > >            f2fs_destroy_flush_cmd_control(sbi, false);
> > > > >            need_restart_flush = true;
> > > > > @@ -2827,7 +2775,7 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
> > > > >         * triggered while remount and we need to take care of it before
> > > > >         * returning from remount.
> > > > >         */
> > > > > -    if ((*flags & SB_RDONLY) || test_opt(sbi, DISABLE_CHECKPOINT) ||
> > > > > +    if ((flags & SB_RDONLY) || test_opt(sbi, DISABLE_CHECKPOINT) ||
> > > > >                !test_opt(sbi, MERGE_CHECKPOINT)) {
> > > > >            f2fs_stop_ckpt_thread(sbi);
> > > > >        } else {
> > > > > @@ -2854,7 +2802,7 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
> > > > >            (test_opt(sbi, POSIX_ACL) ? SB_POSIXACL : 0);
> > > > >          limit_reserve_root(sbi);
> > > > > -    *flags = (*flags & ~SB_LAZYTIME) | (sb->s_flags & SB_LAZYTIME);
> > > > > +    fc->sb_flags = (flags & ~SB_LAZYTIME) | (sb->s_flags & SB_LAZYTIME);
> > > > >          sbi->umount_lock_holder = NULL;
> > > > >        return 0;
> > > > > @@ -3523,7 +3471,6 @@ static const struct super_operations f2fs_sops = {
> > > > >        .freeze_fs    = f2fs_freeze,
> > > > >        .unfreeze_fs    = f2fs_unfreeze,
> > > > >        .statfs        = f2fs_statfs,
> > > > > -    .remount_fs    = f2fs_remount,
> > > > >        .shutdown    = f2fs_shutdown,
> > > > >    };
> > > > >    @@ -4745,16 +4692,13 @@ static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi)
> > > > >        sbi->readdir_ra = true;
> > > > >    }
> > > > >    -static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> > > > > +static int f2fs_fill_super(struct super_block *sb, struct fs_context *fc)
> > > > >    {
> > > > >        struct f2fs_sb_info *sbi;
> > > > >        struct f2fs_super_block *raw_super;
> > > > > -    struct f2fs_fs_context ctx;
> > > > > -    struct fs_context fc;
> > > > >        struct inode *root;
> > > > >        int err;
> > > > >        bool skip_recovery = false, need_fsck = false;
> > > > > -    char *options = NULL;
> > > > >        int recovery, i, valid_super_block;
> > > > >        struct curseg_info *seg_i;
> > > > >        int retry_cnt = 1;
> > > > > @@ -4767,9 +4711,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> > > > >        raw_super = NULL;
> > > > >        valid_super_block = -1;
> > > > >        recovery = 0;
> > > > > -    memset(&fc, 0, sizeof(fc));
> > > > > -    memset(&ctx, 0, sizeof(ctx));
> > > > > -    fc.fs_private = &ctx;
> > > > >          /* allocate memory for f2fs-specific super block info */
> > > > >        sbi = kzalloc(sizeof(struct f2fs_sb_info), GFP_KERNEL);
> > > > > @@ -4820,22 +4761,12 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> > > > >                            sizeof(raw_super->uuid));
> > > > >          default_options(sbi, false);
> > > > > -    /* parse mount options */
> > > > > -    options = kstrdup((const char *)data, GFP_KERNEL);
> > > > > -    if (data && !options) {
> > > > > -        err = -ENOMEM;
> > > > > -        goto free_sb_buf;
> > > > > -    }
> > > > > -
> > > > > -    err = parse_options(&fc, options);
> > > > > -    if (err)
> > > > > -        goto free_options;
> > > > >    -    err = f2fs_check_opt_consistency(&fc, sb);
> > > > > +    err = f2fs_check_opt_consistency(fc, sb);
> > > > >        if (err < 0)
> > > > > -        goto free_options;
> > > > > +        goto free_sb_buf;
> > > > >    -    f2fs_apply_options(&fc, sb);
> > > > > +    f2fs_apply_options(fc, sb);
> > > > >          sb->s_maxbytes = max_file_blocks(NULL) <<
> > > > >                    le32_to_cpu(raw_super->log_blocksize);
> > > > > @@ -5160,7 +5091,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> > > > >            if (err)
> > > > >                goto sync_free_meta;
> > > > >        }
> > > > > -    kvfree(options);
> > > > >          /* recover broken superblock */
> > > > >        if (recovery) {
> > > > > @@ -5255,7 +5185,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> > > > >            kfree(F2FS_OPTION(sbi).s_qf_names[i]);
> > > > >    #endif
> > > > >        fscrypt_free_dummy_policy(&F2FS_OPTION(sbi).dummy_enc_policy);
> > > > > -    kvfree(options);
> > > > >    free_sb_buf:
> > > > >        kfree(raw_super);
> > > > >    free_sbi:
> > > > > @@ -5271,14 +5200,39 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> > > > >        return err;
> > > > >    }
> > > > >    -static struct dentry *f2fs_mount(struct file_system_type *fs_type, int flags,
> > > > > -            const char *dev_name, void *data)
> > > > > +static int f2fs_get_tree(struct fs_context *fc)
> > > > >    {
> > > > > -    return mount_bdev(fs_type, flags, dev_name, data, f2fs_fill_super);
> > > > > +    return get_tree_bdev(fc, f2fs_fill_super);
> > > > > +}
> > > > > +
> > > > > +static int f2fs_reconfigure(struct fs_context *fc)
> > > > > +{
> > > > > +    struct super_block *sb = fc->root->d_sb;
> > > > > +
> > > > > +    return __f2fs_remount(fc, sb);
> > > > > +}
> > > > > +
> > > > > +static void f2fs_fc_free(struct fs_context *fc)
> > > > > +{
> > > > > +    struct f2fs_fs_context *ctx = fc->fs_private;
> > > > > +    int i;
> > > > > +
> > > > > +    if (!ctx)
> > > > > +        return;
> > > > > +
> > > > > +#ifdef CONFIG_QUOTA
> > > > > +    for (i = 0; i < MAXQUOTAS; i++)
> > > > > +        kfree(F2FS_CTX_INFO(ctx).s_qf_names[i]);
> > > > > +#endif
> > > > > +    fscrypt_free_dummy_policy(&F2FS_CTX_INFO(ctx).dummy_enc_policy);
> > > > > +    kfree(ctx);
> > > > >    }
> > > > >      static const struct fs_context_operations f2fs_context_ops = {
> > > > >        .parse_param    = f2fs_parse_param,
> > > > > +    .get_tree    = f2fs_get_tree,
> > > > > +    .reconfigure = f2fs_reconfigure,
> > > > > +    .free    = f2fs_fc_free,
> > > > >    };
> > > > >      static void kill_f2fs_super(struct super_block *sb)
> > > > > @@ -5322,10 +5276,24 @@ static void kill_f2fs_super(struct super_block *sb)
> > > > >        }
> > > > >    }
> > > > >    +static int f2fs_init_fs_context(struct fs_context *fc)
> > > > > +{
> > > > > +    struct f2fs_fs_context *ctx;
> > > > > +
> > > > > +    ctx = kzalloc(sizeof(struct f2fs_fs_context), GFP_KERNEL);
> > > > > +    if (!ctx)
> > > > > +        return -ENOMEM;
> > > > > +
> > > > > +    fc->fs_private = ctx;
> > > > > +    fc->ops = &f2fs_context_ops;
> > > > > +
> > > > > +    return 0;
> > > > > +}
> > > > > +
> > > > >    static struct file_system_type f2fs_fs_type = {
> > > > >        .owner        = THIS_MODULE,
> > > > >        .name        = "f2fs",
> > > > > -    .mount        = f2fs_mount,
> > > > > +    .init_fs_context = f2fs_init_fs_context,
> > > > >        .kill_sb    = kill_f2fs_super,
> > > > >        .fs_flags    = FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
> > > > >    };
> > > > 
> > 
> > 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel