On 10/17/19 12:33 PM, Qu Wenruo wrote:
On 2019/10/17 上午11:23, Anand Jain wrote:
On 10/8/19 12:49 PM, Qu Wenruo wrote:
This patch does the following refactor:
- Refactor parameter from @root to @fs_info
- Refactor the large loop body into another function
Now we have a helper function, read_one_block_group(), to handle
block group cache and space info related routine.
- Refactor the return value
Even we have the code handling ret > 0 from find_first_block_group(),
it never works, as when there is no more block group,
find_first_block_group() just return -ENOENT other than 1.
Can it be separated into patches? My concern is as it alters the return
value of the rescue command. So we shall have clarity of a discrete
patch to blame. Otherwise I agree its a good change.
No problem.
What about 3 patches split by the mentioned 3 refactors?
This is super confusing, it's almost a mircle it even works.
Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
---
ctree.h | 2 +-
disk-io.c | 9 ++-
extent-tree.c | 160 ++++++++++++++++++++++++++++----------------------
3 files changed, 97 insertions(+), 74 deletions(-)
diff --git a/ctree.h b/ctree.h
index 8c7b3cb40151..2899de358613 100644
--- a/ctree.h
+++ b/ctree.h
@@ -2550,7 +2550,7 @@ int update_space_info(struct btrfs_fs_info
*info, u64 flags,
u64 total_bytes, u64 bytes_used,
struct btrfs_space_info **space_info);
int btrfs_free_block_groups(struct btrfs_fs_info *info);
-int btrfs_read_block_groups(struct btrfs_root *root);
+int btrfs_read_block_groups(struct btrfs_fs_info *info);
struct btrfs_block_group_cache *
btrfs_add_block_group(struct btrfs_fs_info *fs_info, u64 bytes_used,
u64 type,
u64 chunk_offset, u64 size);
diff --git a/disk-io.c b/disk-io.c
index be44eead5cef..8978f0cb60c7 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -983,14 +983,17 @@ int btrfs_setup_all_roots(struct btrfs_fs_info
*fs_info, u64 root_tree_bytenr,
fs_info->last_trans_committed = generation;
if (extent_buffer_uptodate(fs_info->extent_root->node) &&
!(flags & OPEN_CTREE_NO_BLOCK_GROUPS)) {
- ret = btrfs_read_block_groups(fs_info->tree_root);
+ ret = btrfs_read_block_groups(fs_info);
/*
* If we don't find any blockgroups (ENOENT) we're either
* restoring or creating the filesystem, where it's expected,
* anything else is error
*/
- if (ret != -ENOENT)
- return -EIO;
+ if (ret < 0 && ret != -ENOENT) {
+ errno = -ret;
+ error("failed to read block groups: %m");
+ return ret;
+ }
}
As mentioned this alters the rescue command semantics as show below.
Earlier we had only -EIO as error now its much more and accurate
which is good. fstests is fine but anything else?
cmd_rescue_chunk_recover()
btrfs_recover_chunk_tree()
open_ctree_with_broken_chunk()
btrfs_setup_all_roots()
I'm not sure if I got the point.
Although btrfs_setup_all_roots() get called in above call chain, it
doesn't have any special handling of -EIO or others.
It just reads the extent tree root.
Would you mind to explain a little more?
sure.
The above thread is in the call chain of the command
btrfs rescue chunk-recover [options] <device>"
And as the its return error code is being changed for the same problem,
so a separate patch not part of the bg-tree changes would make sense.
Thanks, Anand
