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 <lihongb...@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 <lihongb...@huawei.com> > > > > > [sandeen: forward port] > > > > > Signed-off-by: Eric Sandeen <sand...@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, ¶m); > > > > > - 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
