| Message ID | 20220606110819.3943-1-dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: add error messages to all unrecognized mount options | expand |
On 2022/6/6 19:08, David Sterba wrote: > Almost none of the errors stemming from a valid mount option but wrong > value prints a descriptive message which would help to identify why > mount failed. Like in the linked report: > > $ uname -r > v4.19 > $ mount -o compress=zstd /dev/sdb /mnt > mount: /mnt: wrong fs type, bad option, bad superblock on > /dev/sdb, missing codepage or helper program, or other error. > $ dmesg > ... > BTRFS error (device sdb): open_ctree failed > > Errors caused by memory allocation failures are left out as it's not a > user error so reporting that would be confusing. > > Link: https://lore.kernel.org/linux-btrfs/9c3fec36-fc61-3a33-4977-a7e207c3fa4e@gmx.de/ > CC: stable@vger.kernel.org # 4.9+ > Signed-off-by: David Sterba <dsterba@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > fs/btrfs/super.c | 39 ++++++++++++++++++++++++++++++++------- > 1 file changed, 32 insertions(+), 7 deletions(-) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index d8e2eac0417e..719dda57dc7a 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -764,6 +764,8 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > compress_force = false; > no_compress++; > } else { > + btrfs_err(info, "unrecognized compression value %s", > + args[0].from); > ret = -EINVAL; > goto out; > } > @@ -822,8 +824,11 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > case Opt_thread_pool: > ret = match_int(&args[0], &intarg); > if (ret) { > + btrfs_err(info, "unrecognized thread_pool value %s", > + args[0].from); > goto out; > } else if (intarg == 0) { > + btrfs_err(info, "invalid value 0 for thread_pool"); > ret = -EINVAL; > goto out; > } > @@ -884,8 +889,11 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > break; > case Opt_ratio: > ret = match_int(&args[0], &intarg); > - if (ret) > + if (ret) { > + btrfs_err(info, "unrecognized metadata_ratio value %s", > + args[0].from); > goto out; > + } > info->metadata_ratio = intarg; > btrfs_info(info, "metadata ratio %u", > info->metadata_ratio); > @@ -902,6 +910,8 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > btrfs_set_and_info(info, DISCARD_ASYNC, > "turning on async discard"); > } else { > + btrfs_err(info, "unrecognized discard mode value %s", > + args[0].from); > ret = -EINVAL; > goto out; > } > @@ -934,6 +944,8 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > btrfs_set_and_info(info, FREE_SPACE_TREE, > "enabling free space tree"); > } else { > + btrfs_err(info, "unrecognized space_cache value %s", > + args[0].from); > ret = -EINVAL; > goto out; > } > @@ -1015,8 +1027,12 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > break; > case Opt_check_integrity_print_mask: > ret = match_int(&args[0], &intarg); > - if (ret) > + if (ret) { > + btrfs_err(info, > + "unrecognized check_integrity_print_mask value %s", > + args[0].from); > goto out; > + } > info->check_integrity_print_mask = intarg; > btrfs_info(info, "check_integrity_print_mask 0x%x", > info->check_integrity_print_mask); > @@ -1031,13 +1047,15 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > goto out; > #endif > case Opt_fatal_errors: > - if (strcmp(args[0].from, "panic") == 0) > + if (strcmp(args[0].from, "panic") == 0) { > btrfs_set_opt(info->mount_opt, > PANIC_ON_FATAL_ERROR); > - else if (strcmp(args[0].from, "bug") == 0) > + } else if (strcmp(args[0].from, "bug") == 0) { > btrfs_clear_opt(info->mount_opt, > PANIC_ON_FATAL_ERROR); > - else { > + } else { > + btrfs_err(info, "unrecognized fatal_errors value %s", > + args[0].from); > ret = -EINVAL; > goto out; > } > @@ -1045,8 +1063,12 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > case Opt_commit_interval: > intarg = 0; > ret = match_int(&args[0], &intarg); > - if (ret) > + if (ret) { > + btrfs_err(info, "unrecognized commit_interval value %s", > + args[0].from); > + ret = -EINVAL; > goto out; > + } > if (intarg == 0) { > btrfs_info(info, > "using default commit interval %us", > @@ -1060,8 +1082,11 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > break; > case Opt_rescue: > ret = parse_rescue_options(info, args[0].from); > - if (ret < 0) > + if (ret < 0) { > + btrfs_err(info, "unrecognized rescue value %s", > + args[0].from); > goto out; > + } > break; > #ifdef CONFIG_BTRFS_DEBUG > case Opt_fragment_all:
On 6.06.22 г. 14:08 ч., David Sterba wrote: > Almost none of the errors stemming from a valid mount option but wrong > value prints a descriptive message which would help to identify why > mount failed. Like in the linked report: > > $ uname -r > v4.19 > $ mount -o compress=zstd /dev/sdb /mnt > mount: /mnt: wrong fs type, bad option, bad superblock on > /dev/sdb, missing codepage or helper program, or other error. > $ dmesg > ... > BTRFS error (device sdb): open_ctree failed > > Errors caused by memory allocation failures are left out as it's not a > user error so reporting that would be confusing. > > Link: https://lore.kernel.org/linux-btrfs/9c3fec36-fc61-3a33-4977-a7e207c3fa4e@gmx.de/ > CC: stable@vger.kernel.org # 4.9+ > Signed-off-by: David Sterba <dsterba@suse.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
LGTM. Reviewed-by: Anand Jain <anand.jain@oracle.com> While we are on this topic- Not all valid mount options get printed either. I sent a patch a long time back [1] to fix it. If there is enough interest, I could revive it. [1] [PATCH v2] btrfs: add mount umount logs On 6/6/22 16:38, David Sterba wrote: > Almost none of the errors stemming from a valid mount option but wrong > value prints a descriptive message which would help to identify why > mount failed. Like in the linked report: > > $ uname -r > v4.19 > $ mount -o compress=zstd /dev/sdb /mnt > mount: /mnt: wrong fs type, bad option, bad superblock on > /dev/sdb, missing codepage or helper program, or other error. > $ dmesg > ... > BTRFS error (device sdb): open_ctree failed > > Errors caused by memory allocation failures are left out as it's not a > user error so reporting that would be confusing. > > Link: https://lore.kernel.org/linux-btrfs/9c3fec36-fc61-3a33-4977-a7e207c3fa4e@gmx.de/ > CC: stable@vger.kernel.org # 4.9+ > Signed-off-by: David Sterba <dsterba@suse.com> > --- > fs/btrfs/super.c | 39 ++++++++++++++++++++++++++++++++------- > 1 file changed, 32 insertions(+), 7 deletions(-) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index d8e2eac0417e..719dda57dc7a 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -764,6 +764,8 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > compress_force = false; > no_compress++; > } else { > + btrfs_err(info, "unrecognized compression value %s", > + args[0].from); > ret = -EINVAL; > goto out; > } > @@ -822,8 +824,11 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > case Opt_thread_pool: > ret = match_int(&args[0], &intarg); > if (ret) { > + btrfs_err(info, "unrecognized thread_pool value %s", > + args[0].from); > goto out; > } else if (intarg == 0) { > + btrfs_err(info, "invalid value 0 for thread_pool"); > ret = -EINVAL; > goto out; > } > @@ -884,8 +889,11 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > break; > case Opt_ratio: > ret = match_int(&args[0], &intarg); > - if (ret) > + if (ret) { > + btrfs_err(info, "unrecognized metadata_ratio value %s", > + args[0].from); > goto out; > + } > info->metadata_ratio = intarg; > btrfs_info(info, "metadata ratio %u", > info->metadata_ratio); > @@ -902,6 +910,8 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > btrfs_set_and_info(info, DISCARD_ASYNC, > "turning on async discard"); > } else { > + btrfs_err(info, "unrecognized discard mode value %s", > + args[0].from); > ret = -EINVAL; > goto out; > } > @@ -934,6 +944,8 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > btrfs_set_and_info(info, FREE_SPACE_TREE, > "enabling free space tree"); > } else { > + btrfs_err(info, "unrecognized space_cache value %s", > + args[0].from); > ret = -EINVAL; > goto out; > } > @@ -1015,8 +1027,12 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > break; > case Opt_check_integrity_print_mask: > ret = match_int(&args[0], &intarg); > - if (ret) > + if (ret) { > + btrfs_err(info, > + "unrecognized check_integrity_print_mask value %s", > + args[0].from); > goto out; > + } > info->check_integrity_print_mask = intarg; > btrfs_info(info, "check_integrity_print_mask 0x%x", > info->check_integrity_print_mask); > @@ -1031,13 +1047,15 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > goto out; > #endif > case Opt_fatal_errors: > - if (strcmp(args[0].from, "panic") == 0) > + if (strcmp(args[0].from, "panic") == 0) { > btrfs_set_opt(info->mount_opt, > PANIC_ON_FATAL_ERROR); > - else if (strcmp(args[0].from, "bug") == 0) > + } else if (strcmp(args[0].from, "bug") == 0) { > btrfs_clear_opt(info->mount_opt, > PANIC_ON_FATAL_ERROR); > - else { > + } else { > + btrfs_err(info, "unrecognized fatal_errors value %s", > + args[0].from); > ret = -EINVAL; > goto out; > } > @@ -1045,8 +1063,12 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > case Opt_commit_interval: > intarg = 0; > ret = match_int(&args[0], &intarg); > - if (ret) > + if (ret) { > + btrfs_err(info, "unrecognized commit_interval value %s", > + args[0].from); > + ret = -EINVAL; > goto out; > + } > if (intarg == 0) { > btrfs_info(info, > "using default commit interval %us", > @@ -1060,8 +1082,11 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > break; > case Opt_rescue: > ret = parse_rescue_options(info, args[0].from); > - if (ret < 0) > + if (ret < 0) { > + btrfs_err(info, "unrecognized rescue value %s", > + args[0].from); > goto out; > + } > break; > #ifdef CONFIG_BTRFS_DEBUG > case Opt_fragment_all:
On Tue, Jun 07, 2022 at 05:07:55PM +0530, Anand Jain wrote: > > LGTM. > Reviewed-by: Anand Jain <anand.jain@oracle.com> > > While we are on this topic- > Not all valid mount options get printed either. Looking at the amount of other informative messages we already print, I think it won't be that bad to add the mount/umount messages, but it depends on the system, there was an argument that bind mounts can make a loot of noise in the logs. > I sent a patch a long > time back [1] to fix it. If there is enough interest, I could revive it. As was mentioned in the VFS version, other filesystems also print that so let's do it for btrfs too. Please update and resend the patch, we may then discuss what exactly to print, I'm not convinced we need all the internal stats like the refcounts but it would be better to have something for start.
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index d8e2eac0417e..719dda57dc7a 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -764,6 +764,8 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, compress_force = false; no_compress++; } else { + btrfs_err(info, "unrecognized compression value %s", + args[0].from); ret = -EINVAL; goto out; } @@ -822,8 +824,11 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, case Opt_thread_pool: ret = match_int(&args[0], &intarg); if (ret) { + btrfs_err(info, "unrecognized thread_pool value %s", + args[0].from); goto out; } else if (intarg == 0) { + btrfs_err(info, "invalid value 0 for thread_pool"); ret = -EINVAL; goto out; } @@ -884,8 +889,11 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, break; case Opt_ratio: ret = match_int(&args[0], &intarg); - if (ret) + if (ret) { + btrfs_err(info, "unrecognized metadata_ratio value %s", + args[0].from); goto out; + } info->metadata_ratio = intarg; btrfs_info(info, "metadata ratio %u", info->metadata_ratio); @@ -902,6 +910,8 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, btrfs_set_and_info(info, DISCARD_ASYNC, "turning on async discard"); } else { + btrfs_err(info, "unrecognized discard mode value %s", + args[0].from); ret = -EINVAL; goto out; } @@ -934,6 +944,8 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, btrfs_set_and_info(info, FREE_SPACE_TREE, "enabling free space tree"); } else { + btrfs_err(info, "unrecognized space_cache value %s", + args[0].from); ret = -EINVAL; goto out; } @@ -1015,8 +1027,12 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, break; case Opt_check_integrity_print_mask: ret = match_int(&args[0], &intarg); - if (ret) + if (ret) { + btrfs_err(info, + "unrecognized check_integrity_print_mask value %s", + args[0].from); goto out; + } info->check_integrity_print_mask = intarg; btrfs_info(info, "check_integrity_print_mask 0x%x", info->check_integrity_print_mask); @@ -1031,13 +1047,15 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, goto out; #endif case Opt_fatal_errors: - if (strcmp(args[0].from, "panic") == 0) + if (strcmp(args[0].from, "panic") == 0) { btrfs_set_opt(info->mount_opt, PANIC_ON_FATAL_ERROR); - else if (strcmp(args[0].from, "bug") == 0) + } else if (strcmp(args[0].from, "bug") == 0) { btrfs_clear_opt(info->mount_opt, PANIC_ON_FATAL_ERROR); - else { + } else { + btrfs_err(info, "unrecognized fatal_errors value %s", + args[0].from); ret = -EINVAL; goto out; } @@ -1045,8 +1063,12 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, case Opt_commit_interval: intarg = 0; ret = match_int(&args[0], &intarg); - if (ret) + if (ret) { + btrfs_err(info, "unrecognized commit_interval value %s", + args[0].from); + ret = -EINVAL; goto out; + } if (intarg == 0) { btrfs_info(info, "using default commit interval %us", @@ -1060,8 +1082,11 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, break; case Opt_rescue: ret = parse_rescue_options(info, args[0].from); - if (ret < 0) + if (ret < 0) { + btrfs_err(info, "unrecognized rescue value %s", + args[0].from); goto out; + } break; #ifdef CONFIG_BTRFS_DEBUG case Opt_fragment_all:
Almost none of the errors stemming from a valid mount option but wrong value prints a descriptive message which would help to identify why mount failed. Like in the linked report: $ uname -r v4.19 $ mount -o compress=zstd /dev/sdb /mnt mount: /mnt: wrong fs type, bad option, bad superblock on /dev/sdb, missing codepage or helper program, or other error. $ dmesg ... BTRFS error (device sdb): open_ctree failed Errors caused by memory allocation failures are left out as it's not a user error so reporting that would be confusing. Link: https://lore.kernel.org/linux-btrfs/9c3fec36-fc61-3a33-4977-a7e207c3fa4e@gmx.de/ CC: stable@vger.kernel.org # 4.9+ Signed-off-by: David Sterba <dsterba@suse.com> --- fs/btrfs/super.c | 39 ++++++++++++++++++++++++++++++++------- 1 file changed, 32 insertions(+), 7 deletions(-)