| Message ID | 20200211214042.4645-2-josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Error condition failure fixes | expand |
On 2020/2/12 上午5:40, Josef Bacik wrote: > While running my error injection script I hit a panic when we tried to > clean up the fs_root when free'ing the fs_root. This is because > fs_info->fs_root == PTR_ERR(-EIO), which isn't great. Fix this by > setting fs_info->fs_root = NULL; if we fail to read the root. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Just one off-topic idea, can we have test cases in fstests to do specific error injection test? For your fix, we can inject ENOMEM error with call chain btrfs_read_fs_root_no_name()->open_ctree() to get a 100% reproducible test, which looks to be a solid test case. Thanks, Qu > --- > fs/btrfs/disk-io.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index eb441fa3711b..5b6140482cef 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -3260,6 +3260,7 @@ int __cold open_ctree(struct super_block *sb, > if (IS_ERR(fs_info->fs_root)) { > err = PTR_ERR(fs_info->fs_root); > btrfs_warn(fs_info, "failed to read fs tree: %d", err); > + fs_info->fs_root = NULL; > goto fail_qgroup; > } > >
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On 11.02.20 г. 23:40 ч., Josef Bacik wrote: > While running my error injection script I hit a panic when we tried to > clean up the fs_root when free'ing the fs_root. This is because > fs_info->fs_root == PTR_ERR(-EIO), which isn't great. Fix this by > setting fs_info->fs_root = NULL; if we fail to read the root. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> While looking to see how ->fs_root (git grep "\->fs_root\W" fs/btrfs) is used I realized we almost never query it through that member. It's cleaned up via the btrfs_free_fs_roots which queries the root radix. Given this I fail to see how the presence of a bogus value in fs_info->fs_root would cause a crash (it's certainly wrong so your patch per-se is fine). Can you provide an example call trace? In any case : Reviewed-by: Nikolay Borisov <nborisov@suse.com>
On 2/13/20 5:48 AM, Nikolay Borisov wrote: > > > On 11.02.20 г. 23:40 ч., Josef Bacik wrote: >> While running my error injection script I hit a panic when we tried to >> clean up the fs_root when free'ing the fs_root. This is because >> fs_info->fs_root == PTR_ERR(-EIO), which isn't great. Fix this by >> setting fs_info->fs_root = NULL; if we fail to read the root. >> >> Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > > While looking to see how ->fs_root (git grep "\->fs_root\W" fs/btrfs) is > used I realized we almost never query it through that member. It's > cleaned up via the btrfs_free_fs_roots which queries the root radix. > Given this I fail to see how the presence of a bogus value in > fs_info->fs_root would cause a crash (it's certainly wrong so your patch > per-se is fine). Can you provide an example call trace? > We do a btrfs_put_root(fs_info->fs_root); in btrfs_free_fs_info. There's for sure an argument to be made for getting rid of fs_info->fs_root, and just using the radix lookup. Once all of my root ref patches are merged I'll take a run at that. Thanks, Josef
On 13.02.20 г. 17:31 ч., Josef Bacik wrote: > > We do a btrfs_put_root(fs_info->fs_root); in btrfs_free_fs_info. > There's for sure an argument to be made for getting rid of > fs_info->fs_root, and just using the radix lookup. Once all of my root > ref patches are merged I'll take a run at that. Thanks, This makes sense, I was looking at linux master which don't have your patches, hence I wasn't seeing this. Fair point.
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index eb441fa3711b..5b6140482cef 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -3260,6 +3260,7 @@ int __cold open_ctree(struct super_block *sb, if (IS_ERR(fs_info->fs_root)) { err = PTR_ERR(fs_info->fs_root); btrfs_warn(fs_info, "failed to read fs tree: %d", err); + fs_info->fs_root = NULL; goto fail_qgroup; }
While running my error injection script I hit a panic when we tried to clean up the fs_root when free'ing the fs_root. This is because fs_info->fs_root == PTR_ERR(-EIO), which isn't great. Fix this by setting fs_info->fs_root = NULL; if we fail to read the root. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/disk-io.c | 1 + 1 file changed, 1 insertion(+)