| Message ID | 6e41922428a5b89b5ef0d7d47f8274e11468ee2c.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:51, Josef Bacik wrote: > This probably can't happen even with a corrupt file system, because we > would have failed much earlier on than here. However there's no reason > we can't just check and bail out as appropriate, so do that and convert > the correctness BUG_ON() to an ASSERT(). > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> The handling it self is kinda OK. Reviewed-by: Qu Wenruo <wqu@suse.com> But still some (maybe unrelated) question inlined below. > --- > fs/btrfs/relocation.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c > index 695a52cd07b0..d4656a8f507d 100644 > --- a/fs/btrfs/relocation.c > +++ b/fs/btrfs/relocation.c > @@ -1870,8 +1870,14 @@ int prepare_to_merge(struct reloc_control *rc, int err) > > 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); > + if (IS_ERR(root)) { > + list_add(&reloc_root->root_list, &reloc_roots); I found it pretty strange that even if prepare_to_merge() failed, we still go merge_reloc_roots(). I guess we'd better handle that first? Thanks, Qu > + btrfs_abort_transaction(trans, (int)PTR_ERR(root)); > + if (!err) > + err = PTR_ERR(root); > + break; > + } > + ASSERT(root->reloc_root == reloc_root); > > /* > * set reference count to 1, so btrfs_recover_relocation >
On 12/3/20 12:39 AM, Qu Wenruo wrote: > > > On 2020/12/3 上午3:51, Josef Bacik wrote: >> This probably can't happen even with a corrupt file system, because we >> would have failed much earlier on than here. However there's no reason >> we can't just check and bail out as appropriate, so do that and convert >> the correctness BUG_ON() to an ASSERT(). >> >> Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > The handling it self is kinda OK. > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > But still some (maybe unrelated) question inlined below. >> --- >> fs/btrfs/relocation.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c >> index 695a52cd07b0..d4656a8f507d 100644 >> --- a/fs/btrfs/relocation.c >> +++ b/fs/btrfs/relocation.c >> @@ -1870,8 +1870,14 @@ int prepare_to_merge(struct reloc_control *rc, int err) >> >> 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); >> + if (IS_ERR(root)) { >> + list_add(&reloc_root->root_list, &reloc_roots); > > I found it pretty strange that even if prepare_to_merge() failed, we > still go merge_reloc_roots(). > > I guess we'd better handle that first? > This is because the cleaning up of the rc->reloc_roots is dealt with in merge_reloc_roots(). It's kinda shitty, but something I'm going to address later when I rework all of this code. I tried to limit the scope of this patchset to purely the error handling, and then I'll clean up the awfulness in a more complicated follow up patchset. Thanks, Josef
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c index 695a52cd07b0..d4656a8f507d 100644 --- a/fs/btrfs/relocation.c +++ b/fs/btrfs/relocation.c @@ -1870,8 +1870,14 @@ int prepare_to_merge(struct reloc_control *rc, int err) 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); + if (IS_ERR(root)) { + list_add(&reloc_root->root_list, &reloc_roots); + btrfs_abort_transaction(trans, (int)PTR_ERR(root)); + if (!err) + err = PTR_ERR(root); + break; + } + ASSERT(root->reloc_root == reloc_root); /* * set reference count to 1, so btrfs_recover_relocation
This probably can't happen even with a corrupt file system, because we would have failed much earlier on than here. However there's no reason we can't just check and bail out as appropriate, so do that and convert the correctness BUG_ON() to an ASSERT(). Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/relocation.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)