| Message ID | cb7312a3d6c88100df88dc61c911e6d5e8455070.1665565866.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: make open_ctree() init/exit sequence strictly matched | expand |
On Wed, Oct 12, 2022 at 05:13:11PM +0800, Qu Wenruo wrote: > With the new open_ctree_seq[] array, we can afford a debug mount option > to do all the error inject at different stages to have a much better > coverage for the error path. > > The new "fail_mount=%u" mount option will be hidden behind > CONFIG_BTRFS_DEBUG option, and when enabled it will cause mount failure > just after the init function of specified stage. > > This can be verified by the following script: > > mkfs.btrfs -f $dev > for (( i=0;; i++ )) do > mount -o fail_mount=$i $dev $mnt > ret=$? > if [ $ret -eq 0 ]; then > umount $mnt > exit > fi > done > umount $mnt > > Signed-off-by: Qu Wenruo <wqu@suse.com> Death to all mount options, especially for this. I'd rather see something like this inserted in the main loop bool btrfs_mail_fail_init(struct btrfs_fs_info *fs_info, int seq) { return false; } ALLOW_ERROR_INJECTION(btrfs_may_fail_init); and then we can error inject that way. Alternatively you could just use ALLOW_ERROR_INJECTION for every one of the init/exit functions and acheive the same thing. Thanks, Josef
On 2022/10/14 20:43, Josef Bacik wrote: > On Wed, Oct 12, 2022 at 05:13:11PM +0800, Qu Wenruo wrote: >> With the new open_ctree_seq[] array, we can afford a debug mount option >> to do all the error inject at different stages to have a much better >> coverage for the error path. >> >> The new "fail_mount=%u" mount option will be hidden behind >> CONFIG_BTRFS_DEBUG option, and when enabled it will cause mount failure >> just after the init function of specified stage. >> >> This can be verified by the following script: >> >> mkfs.btrfs -f $dev >> for (( i=0;; i++ )) do >> mount -o fail_mount=$i $dev $mnt >> ret=$? >> if [ $ret -eq 0 ]; then >> umount $mnt >> exit >> fi >> done >> umount $mnt >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> > > Death to all mount options, especially for this. I'd rather see something like > this inserted in the main loop > > bool btrfs_mail_fail_init(struct btrfs_fs_info *fs_info, int seq) > { > return false; > } > ALLOW_ERROR_INJECTION(btrfs_may_fail_init); IIRC the error injection system can not do a proper conditional injection according to the @seq parameter. > > and then we can error inject that way. Alternatively you could just use > ALLOW_ERROR_INJECTION for every one of the init/exit functions and acheive the > same thing. Thanks, That is even worse. The problem of injecting error directly into init functions is - No exit will be called if init failed As we all expect the init function to cleanup itself if something wrong happened Another problem is, it's much harder to test, we need to inject errors to different functions, while a debug only mount option can easily test the whole thing just with different fail stage number. Thanks, Qu > > Josef
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index a4557075b5c2..6e0cd5b5bc61 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -854,6 +854,7 @@ struct btrfs_fs_info { spinlock_t eb_leak_lock; struct list_head allocated_ebs; + int fail_stage; #endif }; diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 8e49a6dee207..065d13891866 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -3019,6 +3019,7 @@ void btrfs_init_fs_info(struct btrfs_fs_info *fs_info) INIT_LIST_HEAD(&fs_info->allocated_roots); INIT_LIST_HEAD(&fs_info->allocated_ebs); spin_lock_init(&fs_info->eb_leak_lock); + fs_info->fail_stage = -1; #endif extent_map_tree_init(&fs_info->mapping_tree); btrfs_init_block_rsv(&fs_info->global_block_rsv, @@ -3971,6 +3972,19 @@ int __cold open_ctree(struct super_block *sb, char *options) if (ret < 0) goto fail; open_ctree_res[i] = true; +#ifdef CONFIG_BTRFS_DEBUG + /* + * This is not the best timing, as fail_stage will only be + * initialized after open_ctree_features_init(). + * But this is still better to cover more error paths. + */ + if (fs_info->fail_stage >= 0 && i >= fs_info->fail_stage) { + btrfs_info(fs_info, + "error injected at open ctree stage %u", i); + ret = -ECANCELED; + goto fail; + } +#endif } if (btrfs_build_ref_tree(fs_info)) diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index bbdbd2a6e3bc..c25220bae232 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -447,6 +447,7 @@ enum { Opt_enospc_debug, Opt_noenospc_debug, #ifdef CONFIG_BTRFS_DEBUG Opt_fragment_data, Opt_fragment_metadata, Opt_fragment_all, + Opt_fail_mount, #endif #ifdef CONFIG_BTRFS_FS_REF_VERIFY Opt_ref_verify, @@ -521,6 +522,7 @@ static const match_table_t tokens = { {Opt_fragment_data, "fragment=data"}, {Opt_fragment_metadata, "fragment=metadata"}, {Opt_fragment_all, "fragment=all"}, + {Opt_fail_mount, "fail_mount=%u"}, #endif #ifdef CONFIG_BTRFS_FS_REF_VERIFY {Opt_ref_verify, "ref_verify"}, @@ -1106,6 +1108,17 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, btrfs_info(info, "fragmenting data"); btrfs_set_opt(info->mount_opt, FRAGMENT_DATA); break; + case Opt_fail_mount: + ret = match_int(&args[0], &intarg); + if (ret) { + btrfs_err(info, "unrecognized fail_mount value %s", + args[0].from); + goto out; + } + btrfs_info(info, "fail mount at open_ctree() stage %u", + intarg); + info->fail_stage = intarg; + break; #endif #ifdef CONFIG_BTRFS_FS_REF_VERIFY case Opt_ref_verify:
With the new open_ctree_seq[] array, we can afford a debug mount option to do all the error inject at different stages to have a much better coverage for the error path. The new "fail_mount=%u" mount option will be hidden behind CONFIG_BTRFS_DEBUG option, and when enabled it will cause mount failure just after the init function of specified stage. This can be verified by the following script: mkfs.btrfs -f $dev for (( i=0;; i++ )) do mount -o fail_mount=$i $dev $mnt ret=$? if [ $ret -eq 0 ]; then umount $mnt exit fi done umount $mnt Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/ctree.h | 1 + fs/btrfs/disk-io.c | 14 ++++++++++++++ fs/btrfs/super.c | 13 +++++++++++++ 3 files changed, 28 insertions(+)