| Message ID | 20211021114508.21407-13-lczerner@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ext4: new mount API conversion | expand |
Hi Lukas, On 21/10/2021 12:45, Lukas Czerner wrote: > Add the necessary functions for the fs_context_operations. Convert and > rename ext4_remount() and ext4_fill_super() to ext4_get_tree() and > ext4_reconfigure() respectively and switch the ext4 to use the new api. > > One user facing change is the fact that we no longer have access to the > entire string of mount options provided by mount(2) since the mount api > does not store it anywhere. As a result we can't print the options to > the log as we did in the past after the successful mount. > > Signed-off-by: Lukas Czerner <lczerner@redhat.com> I have noticed the following error on -next on various ARM64 platforms that we have ... ERR KERN /dev/mmcblk1: Can't open blockdev I have bisected this, to see where this was introduced and bisect is pointing to this commit. I have not looked any further so far, but wanted to see if you had any ideas/suggestions? Cheers Jon
On Thu, Jan 13, 2022 at 11:29:24AM +0000, Jon Hunter wrote: > Hi Lukas, > > On 21/10/2021 12:45, Lukas Czerner wrote: > > Add the necessary functions for the fs_context_operations. Convert and > > rename ext4_remount() and ext4_fill_super() to ext4_get_tree() and > > ext4_reconfigure() respectively and switch the ext4 to use the new api. > > > > One user facing change is the fact that we no longer have access to the > > entire string of mount options provided by mount(2) since the mount api > > does not store it anywhere. As a result we can't print the options to > > the log as we did in the past after the successful mount. > > > > Signed-off-by: Lukas Czerner <lczerner@redhat.com> > > > I have noticed the following error on -next on various ARM64 platforms that > we have ... > > ERR KERN /dev/mmcblk1: Can't open blockdev > > I have bisected this, to see where this was introduced and bisect is > pointing to this commit. I have not looked any further so far, but wanted to > see if you had any ideas/suggestions? Hi, this error does not come from the ext4, but probably rather from vfs. More specifically from get_tree_bdev() bdev = blkdev_get_by_path(fc->source, mode, fc->fs_type); if (IS_ERR(bdev)) { errorf(fc, "%s: Can't open blockdev", fc->source); return PTR_ERR(bdev); } I have no idea why this fails in your case. Do you know what kind of error it fails with? Any oher error or warning messages preceding the one you point out in the logs? I assume that this happens on mount and the device that you're trying to mount contains ext4 file system? Ext4 is not the only file system utilizing the new mount api, can you try the same with xfs on the device? Does this happen only on some specific devices? I see that the error is mentioning /dev/mmcblk1. Is it the case that it only affects MMC ? Does this happen when you try to mount a different type of block device with ext4 on it? Any specific mount options you're using? Is it rw mount? If so, any chance the device is read only? Do you have any way of reliably reproducing this? Thanks! -Lukas > > Cheers > Jon > > -- > nvpublic >
On 13/01/2022 12:08, Lukas Czerner wrote: > On Thu, Jan 13, 2022 at 11:29:24AM +0000, Jon Hunter wrote: >> Hi Lukas, >> >> On 21/10/2021 12:45, Lukas Czerner wrote: >>> Add the necessary functions for the fs_context_operations. Convert and >>> rename ext4_remount() and ext4_fill_super() to ext4_get_tree() and >>> ext4_reconfigure() respectively and switch the ext4 to use the new api. >>> >>> One user facing change is the fact that we no longer have access to the >>> entire string of mount options provided by mount(2) since the mount api >>> does not store it anywhere. As a result we can't print the options to >>> the log as we did in the past after the successful mount. >>> >>> Signed-off-by: Lukas Czerner <lczerner@redhat.com> >> >> >> I have noticed the following error on -next on various ARM64 platforms that >> we have ... >> >> ERR KERN /dev/mmcblk1: Can't open blockdev >> >> I have bisected this, to see where this was introduced and bisect is >> pointing to this commit. I have not looked any further so far, but wanted to >> see if you had any ideas/suggestions? > > Hi, > > this error does not come from the ext4, but probably rather from vfs. More > specifically from get_tree_bdev() > > bdev = blkdev_get_by_path(fc->source, mode, fc->fs_type); > if (IS_ERR(bdev)) { > errorf(fc, "%s: Can't open blockdev", fc->source); > return PTR_ERR(bdev); > } Yes, obviously this warning has been there for a while but only seen after this change was made. > I have no idea why this fails in your case. Do you know what kind of > error it fails with? Any oher error or warning messages preceding the one you > point out in the logs? No only this one. > I assume that this happens on mount and the device that you're trying to > mount contains ext4 file system? Ext4 is not the only file system > utilizing the new mount api, can you try the same with xfs on the device? This is happening on a board in the test farm and so not easy to reformat. Looking some more /dev/mmcblk1 is not a valid device, I only see /dev/mmcblk0 from the bootlogs on this board. Hmmm, OK I will have to take a closer look to see where this is coming from. > Does this happen only on some specific devices? I see that the error > is mentioning /dev/mmcblk1. Is it the case that it only affects MMC ? > Does this happen when you try to mount a different type of block device > with ext4 on it? So far I have only seen this with the MMC, but I have not tried others. > Any specific mount options you're using? Is it rw mount? If so, any > chance the device is read only? Interestingly we are booting with NFS and so not mounting any MMC by default. > Do you have any way of reliably reproducing this? I see it on every boot and this is causing a warning test to fail. This is a new failure and I have not seen this before. I don't see it on the mainline with the same hardware, only on -next. Cheers Jon
On 13/01/2022 15:06, Jon Hunter wrote: > > On 13/01/2022 12:08, Lukas Czerner wrote: >> On Thu, Jan 13, 2022 at 11:29:24AM +0000, Jon Hunter wrote: >>> Hi Lukas, >>> >>> On 21/10/2021 12:45, Lukas Czerner wrote: >>>> Add the necessary functions for the fs_context_operations. Convert and >>>> rename ext4_remount() and ext4_fill_super() to ext4_get_tree() and >>>> ext4_reconfigure() respectively and switch the ext4 to use the new api. >>>> >>>> One user facing change is the fact that we no longer have access to the >>>> entire string of mount options provided by mount(2) since the mount api >>>> does not store it anywhere. As a result we can't print the options to >>>> the log as we did in the past after the successful mount. >>>> >>>> Signed-off-by: Lukas Czerner <lczerner@redhat.com> >>> >>> >>> I have noticed the following error on -next on various ARM64 >>> platforms that >>> we have ... >>> >>> ERR KERN /dev/mmcblk1: Can't open blockdev >>> >>> I have bisected this, to see where this was introduced and bisect is >>> pointing to this commit. I have not looked any further so far, but >>> wanted to >>> see if you had any ideas/suggestions? >> >> Hi, >> >> this error does not come from the ext4, but probably rather from vfs. >> More >> specifically from get_tree_bdev() >> >> bdev = blkdev_get_by_path(fc->source, mode, fc->fs_type); >> if (IS_ERR(bdev)) { >> errorf(fc, "%s: Can't open blockdev", fc->source); >> return PTR_ERR(bdev); >> } > > Yes, obviously this warning has been there for a while but only seen > after this change was made. > >> I have no idea why this fails in your case. Do you know what kind of >> error it fails with? Any oher error or warning messages preceding the >> one you >> point out in the logs? > > No only this one. > >> I assume that this happens on mount and the device that you're trying to >> mount contains ext4 file system? Ext4 is not the only file system >> utilizing the new mount api, can you try the same with xfs on the device? > > This is happening on a board in the test farm and so not easy to > reformat. Looking some more /dev/mmcblk1 is not a valid device, I only > see /dev/mmcblk0 from the bootlogs on this board. Hmmm, OK I will have > to take a closer look to see where this is coming from. OK, I see what is happening. It appears that our test harness always tries to mount a device called /dev/mmcblk1. Prior to this change there was not kernel error generated and looking at the logs I would see ... mount: /mnt: special device /dev/mmcblk1 does not exist. Following this change, now a kernel warning is generated and I see ... [ 137.078994] /dev/mmcblk1: Can't open blockdev mount: /mnt: special device /dev/mmcblk1 does not exist. So there is a change in behaviour but at the same time the error looks correct. So sorry for the false-positive. Cheers Jon
On 13/01/2022 16:10, Jon Hunter wrote: ... > OK, I see what is happening. It appears that our test harness always > tries to mount a device called /dev/mmcblk1. Prior to this change there > was not kernel error generated and looking at the logs I would see ... > > mount: /mnt: special device /dev/mmcblk1 does not exist. > > Following this change, now a kernel warning is generated and I see ... > > [ 137.078994] /dev/mmcblk1: Can't open blockdev > mount: /mnt: special device /dev/mmcblk1 does not exist. > > So there is a change in behaviour but at the same time the error looks > correct. So sorry for the false-positive. Looking some more, previously, mount_bdev was being called and this has ... bdev = blkdev_get_by_path(dev_name, mode, fs_type); if (IS_ERR(bdev)) return ERR_CAST(bdev); And now we are calling get_tree_bdev() and this has ... bdev = blkdev_get_by_path(fc->source, mode, fc->fs_type); if (IS_ERR(bdev)) { errorf(fc, "%s: Can't open blockdev", fc->source); return PTR_ERR(bdev); } Hence, the difference. I was interested to know what had changed. Cheers Jon
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 0ccd47f3fa91..16d434a512d8 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -74,12 +74,9 @@ static int ext4_mark_recovery_complete(struct super_block *sb, static int ext4_clear_journal_err(struct super_block *sb, struct ext4_super_block *es); static int ext4_sync_fs(struct super_block *sb, int wait); -static int ext4_remount(struct super_block *sb, int *flags, char *data); static int ext4_statfs(struct dentry *dentry, struct kstatfs *buf); static int ext4_unfreeze(struct super_block *sb); static int ext4_freeze(struct super_block *sb); -static struct dentry *ext4_mount(struct file_system_type *fs_type, int flags, - const char *dev_name, void *data); static inline int ext2_feature_set_ok(struct super_block *sb); static inline int ext3_feature_set_ok(struct super_block *sb); static void ext4_destroy_lazyinit_thread(void); @@ -92,6 +89,11 @@ static int ext4_check_opt_consistency(struct fs_context *fc, struct super_block *sb); static int ext4_apply_options(struct fs_context *fc, struct super_block *sb); static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param); +static int ext4_get_tree(struct fs_context *fc); +static int ext4_reconfigure(struct fs_context *fc); +static void ext4_fc_free(struct fs_context *fc); +static int ext4_init_fs_context(struct fs_context *fc); +static const struct fs_parameter_spec ext4_param_specs[]; /* * Lock ordering @@ -121,16 +123,20 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param); static const struct fs_context_operations ext4_context_ops = { .parse_param = ext4_parse_param, + .get_tree = ext4_get_tree, + .reconfigure = ext4_reconfigure, + .free = ext4_fc_free, }; #if !defined(CONFIG_EXT2_FS) && !defined(CONFIG_EXT2_FS_MODULE) && defined(CONFIG_EXT4_USE_FOR_EXT2) static struct file_system_type ext2_fs_type = { - .owner = THIS_MODULE, - .name = "ext2", - .mount = ext4_mount, - .kill_sb = kill_block_super, - .fs_flags = FS_REQUIRES_DEV, + .owner = THIS_MODULE, + .name = "ext2", + .init_fs_context = ext4_init_fs_context, + .parameters = ext4_param_specs, + .kill_sb = kill_block_super, + .fs_flags = FS_REQUIRES_DEV, }; MODULE_ALIAS_FS("ext2"); MODULE_ALIAS("ext2"); @@ -141,11 +147,12 @@ MODULE_ALIAS("ext2"); static struct file_system_type ext3_fs_type = { - .owner = THIS_MODULE, - .name = "ext3", - .mount = ext4_mount, - .kill_sb = kill_block_super, - .fs_flags = FS_REQUIRES_DEV, + .owner = THIS_MODULE, + .name = "ext3", + .init_fs_context = ext4_init_fs_context, + .parameters = ext4_param_specs, + .kill_sb = kill_block_super, + .fs_flags = FS_REQUIRES_DEV, }; MODULE_ALIAS_FS("ext3"); MODULE_ALIAS("ext3"); @@ -1658,7 +1665,6 @@ static const struct super_operations ext4_sops = { .freeze_fs = ext4_freeze, .unfreeze_fs = ext4_unfreeze, .statfs = ext4_statfs, - .remount_fs = ext4_remount, .show_options = ext4_show_options, #ifdef CONFIG_QUOTA .quota_read = ext4_quota_read, @@ -2196,6 +2202,35 @@ struct ext4_fs_context { ext4_fsblk_t s_sb_block; }; +static void ext4_fc_free(struct fs_context *fc) +{ + struct ext4_fs_context *ctx = fc->fs_private; + int i; + + if (!ctx) + return; + + for (i = 0; i < EXT4_MAXQUOTAS; i++) + kfree(ctx->s_qf_names[i]); + + kfree(ctx->test_dummy_enc_arg); + kfree(ctx); +} + +int ext4_init_fs_context(struct fs_context *fc) +{ + struct xfs_fs_context *ctx; + + ctx = kzalloc(sizeof(struct ext4_fs_context), GFP_KERNEL); + if (!ctx) + return -ENOMEM; + + fc->fs_private = ctx; + fc->ops = &ext4_context_ops; + + return 0; +} + #ifdef CONFIG_QUOTA /* * Note the name of the specified quota file. @@ -5594,61 +5629,31 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb, return err ? err : ret; } -static void cleanup_ctx(struct ext4_fs_context *ctx) +static int ext4_fill_super(struct super_block *sb, struct fs_context *fc) { - int i; - - if (!ctx) - return; - - for (i = 0; i < EXT4_MAXQUOTAS; i++) { - kfree(ctx->s_qf_names[i]); - } - - kfree(ctx->test_dummy_enc_arg); -} - -static int ext4_fill_super(struct super_block *sb, void *data, int silent) -{ - struct ext4_fs_context ctx; + struct ext4_fs_context *ctx = fc->fs_private; struct ext4_sb_info *sbi; - struct fs_context fc; const char *descr; - char *orig_data; - int ret = -ENOMEM; - - orig_data = kstrdup(data, GFP_KERNEL); - if (data && !orig_data) - return -ENOMEM; - - /* Cleanup superblock name */ - strreplace(sb->s_id, '/', '!'); - - memset(&fc, 0, sizeof(fc)); - memset(&ctx, 0, sizeof(ctx)); - fc.fs_private = &ctx; - - ret = parse_options(&fc, (char *) data); - if (ret < 0) - goto free_data; + int ret; sbi = ext4_alloc_sbi(sb); - if (!sbi) { + if (!sbi) ret = -ENOMEM; - goto free_data; - } - fc.s_fs_info = sbi; + fc->s_fs_info = sbi; + + /* Cleanup superblock name */ + strreplace(sb->s_id, '/', '!'); sbi->s_sb_block = 1; /* Default super block location */ - if (ctx.spec & EXT4_SPEC_s_sb_block) - sbi->s_sb_block = ctx.s_sb_block; + if (ctx->spec & EXT4_SPEC_s_sb_block) + sbi->s_sb_block = ctx->s_sb_block; - ret = __ext4_fill_super(&fc, sb, silent); + ret = __ext4_fill_super(fc, sb, fc->sb_flags & SB_SILENT); if (ret < 0) goto free_sbi; - if (EXT4_SB(sb)->s_journal) { + if (sbi->s_journal) { if (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA) descr = " journalled data mode"; else if (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_ORDERED_DATA) @@ -5660,23 +5665,21 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) if (___ratelimit(&ext4_mount_msg_ratelimit, "EXT4-fs mount")) ext4_msg(sb, KERN_INFO, "mounted filesystem with%s. " - "Opts: %.*s%s%s. Quota mode: %s.", descr, - (int) sizeof(sbi->s_es->s_mount_opts), - sbi->s_es->s_mount_opts, - *sbi->s_es->s_mount_opts ? "; " : "", orig_data, - ext4_quota_mode(sb)); - - kfree(orig_data); - cleanup_ctx(&ctx); + "Quota mode: %s.", descr, ext4_quota_mode(sb)); + return 0; + free_sbi: ext4_free_sbi(sbi); -free_data: - kfree(orig_data); - cleanup_ctx(&ctx); + fc->s_fs_info = NULL; return ret; } +static int ext4_get_tree(struct fs_context *fc) +{ + return get_tree_bdev(fc, ext4_fill_super); +} + /* * Setup any per-fs journal parameters now. We'll do this both on * initial mount, once the journal has been initialised but before we've @@ -6599,47 +6602,26 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb, return err; } -static int ext4_remount(struct super_block *sb, int *flags, char *data) +static int ext4_reconfigure(struct fs_context *fc) { - struct ext4_sb_info *sbi = EXT4_SB(sb); - struct ext4_fs_context ctx; - struct fs_context fc; - char *orig_data; + struct super_block *sb = fc->root->d_sb; + int flags = fc->sb_flags; int ret; - orig_data = kstrdup(data, GFP_KERNEL); - if (data && !orig_data) - return -ENOMEM; - - memset(&fc, 0, sizeof(fc)); - memset(&ctx, 0, sizeof(ctx)); + fc->s_fs_info = EXT4_SB(sb); - fc.fs_private = &ctx; - fc.purpose = FS_CONTEXT_FOR_RECONFIGURE; - fc.s_fs_info = sbi; - - ret = parse_options(&fc, (char *) data); + ret = ext4_check_opt_consistency(fc, sb); if (ret < 0) - goto err_out; + return ret; - ret = ext4_check_opt_consistency(&fc, sb); + ret = __ext4_remount(fc, sb, &flags); if (ret < 0) - goto err_out; + return ret; - ret = __ext4_remount(&fc, sb, flags); - if (ret < 0) - goto err_out; + ext4_msg(sb, KERN_INFO, "re-mounted. Quota mode: %s.", + ext4_quota_mode(sb)); - ext4_msg(sb, KERN_INFO, "re-mounted. Opts: %s. Quota mode: %s.", - orig_data, ext4_quota_mode(sb)); - cleanup_ctx(&ctx); - kfree(orig_data); return 0; - -err_out: - cleanup_ctx(&ctx); - kfree(orig_data); - return ret; } #ifdef CONFIG_QUOTA @@ -7124,12 +7106,6 @@ static ssize_t ext4_quota_write(struct super_block *sb, int type, } #endif -static struct dentry *ext4_mount(struct file_system_type *fs_type, int flags, - const char *dev_name, void *data) -{ - return mount_bdev(fs_type, flags, dev_name, data, ext4_fill_super); -} - #if !defined(CONFIG_EXT2_FS) && !defined(CONFIG_EXT2_FS_MODULE) && defined(CONFIG_EXT4_USE_FOR_EXT2) static inline void register_as_ext2(void) { @@ -7187,11 +7163,12 @@ static inline int ext3_feature_set_ok(struct super_block *sb) } static struct file_system_type ext4_fs_type = { - .owner = THIS_MODULE, - .name = "ext4", - .mount = ext4_mount, - .kill_sb = kill_block_super, - .fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP, + .owner = THIS_MODULE, + .name = "ext4", + .init_fs_context = ext4_init_fs_context, + .parameters = ext4_param_specs, + .kill_sb = kill_block_super, + .fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP, }; MODULE_ALIAS_FS("ext4");
Add the necessary functions for the fs_context_operations. Convert and rename ext4_remount() and ext4_fill_super() to ext4_get_tree() and ext4_reconfigure() respectively and switch the ext4 to use the new api. One user facing change is the fact that we no longer have access to the entire string of mount options provided by mount(2) since the mount api does not store it anywhere. As a result we can't print the options to the log as we did in the past after the successful mount. Signed-off-by: Lukas Czerner <lczerner@redhat.com> --- fs/ext4/super.c | 195 +++++++++++++++++++++--------------------------- 1 file changed, 86 insertions(+), 109 deletions(-)