| Message ID | cf1b90302fa68b6ea40aa86d37153294bd4717f9.1606938211.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanup error handling in relocation | expand |
On 2020/12/3 上午3:50, Josef Bacik wrote: > Generally speaking this shouldn't ever fail, the corresponding fs root > for the reloc root will already be in memory, so we won't get -ENOMEM > here. > > However if there is no corresponding root for the reloc root then we > could get -ENOMEM when we try to allocate it or we could get -ENOENT > when we look it up and see that it doesn't exist. > > Convert these BUG_ON()'s into ASSERT()'s + proper error handling for the > case of corruption. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > fs/btrfs/relocation.c | 26 ++++++++++++++++++++++++-- > 1 file changed, 24 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c > index d663d8fc085d..5a4b44857522 100644 > --- a/fs/btrfs/relocation.c > +++ b/fs/btrfs/relocation.c > @@ -1973,8 +1973,30 @@ static int record_reloc_root_in_trans(struct btrfs_trans_handle *trans, > return 0; > > root = btrfs_get_fs_root(fs_info, reloc_root->root_key.offset, false); > - BUG_ON(IS_ERR(root)); > - BUG_ON(root->reloc_root != reloc_root); > + > + /* > + * This should succeed, since we can't have a reloc root without having > + * already looked up the actual root and created the reloc root for this > + * root. > + * > + * However if there's some sort of corruption where we have a ref to a > + * reloc root without a corresponding root this could return -ENOENT. > + * > + * The ASSERT()'s are to catch this case in testing, because it could > + * indicate a bug, but for non-developers it indicates corruption and we > + * should error out. The same mention of ASSERT() now looks really overkilled. > + */ > + ASSERT(!IS_ERR(root)); > + ASSERT(root->reloc_root == reloc_root); > + if (IS_ERR(root)) > + return PTR_ERR(root); > + if (root->reloc_root != reloc_root) { ASSERT(0) would be easier to read here IMHO. Despite that looks good to me. Thanks, Qu > + btrfs_err(fs_info, > + "root %llu has two reloc roots associated with it", > + reloc_root->root_key.offset); > + btrfs_put_root(root); > + return -EUCLEAN; > + } > ret = btrfs_record_root_in_trans(trans, root); > btrfs_put_root(root); > >
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c index d663d8fc085d..5a4b44857522 100644 --- a/fs/btrfs/relocation.c +++ b/fs/btrfs/relocation.c @@ -1973,8 +1973,30 @@ static int record_reloc_root_in_trans(struct btrfs_trans_handle *trans, return 0; root = btrfs_get_fs_root(fs_info, reloc_root->root_key.offset, false); - BUG_ON(IS_ERR(root)); - BUG_ON(root->reloc_root != reloc_root); + + /* + * This should succeed, since we can't have a reloc root without having + * already looked up the actual root and created the reloc root for this + * root. + * + * However if there's some sort of corruption where we have a ref to a + * reloc root without a corresponding root this could return -ENOENT. + * + * The ASSERT()'s are to catch this case in testing, because it could + * indicate a bug, but for non-developers it indicates corruption and we + * should error out. + */ + ASSERT(!IS_ERR(root)); + ASSERT(root->reloc_root == reloc_root); + if (IS_ERR(root)) + return PTR_ERR(root); + if (root->reloc_root != reloc_root) { + btrfs_err(fs_info, + "root %llu has two reloc roots associated with it", + reloc_root->root_key.offset); + btrfs_put_root(root); + return -EUCLEAN; + } ret = btrfs_record_root_in_trans(trans, root); btrfs_put_root(root);
Generally speaking this shouldn't ever fail, the corresponding fs root for the reloc root will already be in memory, so we won't get -ENOMEM here. However if there is no corresponding root for the reloc root then we could get -ENOMEM when we try to allocate it or we could get -ENOENT when we look it up and see that it doesn't exist. Convert these BUG_ON()'s into ASSERT()'s + proper error handling for the case of corruption. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/relocation.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-)