| Message ID | 20191008044936.157873-3-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: Support for BG_TREE feature | expand |
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. > This is super confusing, it's almost a mircle it even works. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > 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() Thanks, Anand > key.objectid = BTRFS_FS_TREE_OBJECTID; > diff --git a/extent-tree.c b/extent-tree.c > index 19d1ea0df570..9713d627764c 100644 > --- a/extent-tree.c > +++ b/extent-tree.c > @@ -2607,6 +2607,13 @@ int btrfs_free_block_groups(struct btrfs_fs_info *info) > return 0; > } > > +/* > + * Find a block group which starts >= @key->objectid in extent tree. > + * > + * Return 0 for found > + * Retrun >0 for not found > + * Return <0 for error > + */ > static int find_first_block_group(struct btrfs_root *root, > struct btrfs_path *path, struct btrfs_key *key) > { > @@ -2636,36 +2643,95 @@ static int find_first_block_group(struct btrfs_root *root, > return 0; > path->slots[0]++; > } > - ret = -ENOENT; > + ret = 1; > error: > return ret; > } > > -int btrfs_read_block_groups(struct btrfs_root *root) > +/* > + * Helper function to read out one BLOCK_GROUP_ITEM and insert it into > + * block group cache. > + * > + * Return 0 if nothing wrong (either insert the bg cache or skip 0 sized bg) > + * Return <0 for error. > + */ > +static int read_one_block_group(struct btrfs_fs_info *fs_info, > + struct btrfs_path *path) > { > - struct btrfs_path *path; > - int ret; > - int bit; > - struct btrfs_block_group_cache *cache; > - struct btrfs_fs_info *info = root->fs_info; > + struct extent_io_tree *block_group_cache = &fs_info->block_group_cache; > + struct extent_buffer *leaf = path->nodes[0]; > struct btrfs_space_info *space_info; > - struct extent_io_tree *block_group_cache; > + struct btrfs_block_group_cache *cache; > struct btrfs_key key; > - struct btrfs_key found_key; > - struct extent_buffer *leaf; > + int slot = path->slots[0]; > + int bit = 0; > + int ret; > > - block_group_cache = &info->block_group_cache; > + btrfs_item_key_to_cpu(leaf, &key, slot); > + ASSERT(key.type == BTRFS_BLOCK_GROUP_ITEM_KEY); > + > + /* > + * Skip 0 sized block group, don't insert them into block > + * group cache tree, as its length is 0, it won't get > + * freed at close_ctree() time. > + */ > + if (key.offset == 0) > + return 0; > + > + cache = kzalloc(sizeof(*cache), GFP_NOFS); > + if (!cache) > + return -ENOMEM; > + read_extent_buffer(leaf, &cache->item, > + btrfs_item_ptr_offset(leaf, slot), > + sizeof(cache->item)); > + memcpy(&cache->key, &key, sizeof(key)); > + cache->cached = 0; > + cache->pinned = 0; > + cache->flags = btrfs_block_group_flags(&cache->item); > + if (cache->flags & BTRFS_BLOCK_GROUP_DATA) { > + bit = BLOCK_GROUP_DATA; > + } else if (cache->flags & BTRFS_BLOCK_GROUP_SYSTEM) { > + bit = BLOCK_GROUP_SYSTEM; > + } else if (cache->flags & BTRFS_BLOCK_GROUP_METADATA) { > + bit = BLOCK_GROUP_METADATA; > + } > + set_avail_alloc_bits(fs_info, cache->flags); > + if (btrfs_chunk_readonly(fs_info, cache->key.objectid)) > + cache->ro = 1; > + exclude_super_stripes(fs_info, cache); > + > + ret = update_space_info(fs_info, cache->flags, cache->key.offset, > + btrfs_block_group_used(&cache->item), > + &space_info); > + if (ret < 0) { > + free(cache); > + return ret; > + } > + cache->space_info = space_info; > + > + set_extent_bits(block_group_cache, cache->key.objectid, > + cache->key.objectid + cache->key.offset - 1, > + bit | EXTENT_LOCKED); > + set_state_private(block_group_cache, cache->key.objectid, > + (unsigned long)cache); > + return 0; > +} > > - root = info->extent_root; > +int btrfs_read_block_groups(struct btrfs_fs_info *fs_info) > +{ > + struct btrfs_path path; > + struct btrfs_root *root; > + int ret; > + struct btrfs_key key; > + > + root = fs_info->extent_root; > key.objectid = 0; > key.offset = 0; > key.type = BTRFS_BLOCK_GROUP_ITEM_KEY; > - path = btrfs_alloc_path(); > - if (!path) > - return -ENOMEM; > + btrfs_init_path(&path); > > while(1) { > - ret = find_first_block_group(root, path, &key); > + ret = find_first_block_group(root, &path, &key); > if (ret > 0) { > ret = 0; > goto error; > @@ -2673,67 +2739,21 @@ int btrfs_read_block_groups(struct btrfs_root *root) > if (ret != 0) { > goto error; > } > - leaf = path->nodes[0]; > - btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]); > + btrfs_item_key_to_cpu(path.nodes[0], &key, path.slots[0]); > > - cache = kzalloc(sizeof(*cache), GFP_NOFS); > - if (!cache) { > - ret = -ENOMEM; > + ret = read_one_block_group(fs_info, &path); > + if (ret < 0) > goto error; > - } > > - read_extent_buffer(leaf, &cache->item, > - btrfs_item_ptr_offset(leaf, path->slots[0]), > - sizeof(cache->item)); > - memcpy(&cache->key, &found_key, sizeof(found_key)); > - cache->cached = 0; > - cache->pinned = 0; > - key.objectid = found_key.objectid + found_key.offset; > - if (found_key.offset == 0) > + if (key.offset == 0) > key.objectid++; > - btrfs_release_path(path); > - > - /* > - * Skip 0 sized block group, don't insert them into block > - * group cache tree, as its length is 0, it won't get > - * freed at close_ctree() time. > - */ > - if (found_key.offset == 0) { > - free(cache); > - continue; > - } > - > - cache->flags = btrfs_block_group_flags(&cache->item); > - bit = 0; > - if (cache->flags & BTRFS_BLOCK_GROUP_DATA) { > - bit = BLOCK_GROUP_DATA; > - } else if (cache->flags & BTRFS_BLOCK_GROUP_SYSTEM) { > - bit = BLOCK_GROUP_SYSTEM; > - } else if (cache->flags & BTRFS_BLOCK_GROUP_METADATA) { > - bit = BLOCK_GROUP_METADATA; > - } > - set_avail_alloc_bits(info, cache->flags); > - if (btrfs_chunk_readonly(info, cache->key.objectid)) > - cache->ro = 1; > - > - exclude_super_stripes(info, cache); > - > - ret = update_space_info(info, cache->flags, found_key.offset, > - btrfs_block_group_used(&cache->item), > - &space_info); > - BUG_ON(ret); > - cache->space_info = space_info; > - > - /* use EXTENT_LOCKED to prevent merging */ > - set_extent_bits(block_group_cache, found_key.objectid, > - found_key.objectid + found_key.offset - 1, > - bit | EXTENT_LOCKED); > - set_state_private(block_group_cache, found_key.objectid, > - (unsigned long)cache); > + else > + key.objectid = key.objectid + key.offset; > + btrfs_release_path(&path); > } > ret = 0; > error: > - btrfs_free_path(path); > + btrfs_release_path(&path); > return ret; > } > >
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@suse.com> >> --- >> 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? Thanks, Qu > > Thanks, Anand > >> key.objectid = BTRFS_FS_TREE_OBJECTID; >> diff --git a/extent-tree.c b/extent-tree.c >> index 19d1ea0df570..9713d627764c 100644 >> --- a/extent-tree.c >> +++ b/extent-tree.c >> @@ -2607,6 +2607,13 @@ int btrfs_free_block_groups(struct >> btrfs_fs_info *info) >> return 0; >> } >> +/* >> + * Find a block group which starts >= @key->objectid in extent tree. >> + * >> + * Return 0 for found >> + * Retrun >0 for not found >> + * Return <0 for error >> + */ >> static int find_first_block_group(struct btrfs_root *root, >> struct btrfs_path *path, struct btrfs_key *key) >> { >> @@ -2636,36 +2643,95 @@ static int find_first_block_group(struct >> btrfs_root *root, >> return 0; >> path->slots[0]++; >> } >> - ret = -ENOENT; >> + ret = 1; >> error: >> return ret; >> } >> -int btrfs_read_block_groups(struct btrfs_root *root) >> +/* >> + * Helper function to read out one BLOCK_GROUP_ITEM and insert it into >> + * block group cache. >> + * >> + * Return 0 if nothing wrong (either insert the bg cache or skip 0 >> sized bg) >> + * Return <0 for error. >> + */ >> +static int read_one_block_group(struct btrfs_fs_info *fs_info, >> + struct btrfs_path *path) >> { >> - struct btrfs_path *path; >> - int ret; >> - int bit; >> - struct btrfs_block_group_cache *cache; >> - struct btrfs_fs_info *info = root->fs_info; >> + struct extent_io_tree *block_group_cache = >> &fs_info->block_group_cache; >> + struct extent_buffer *leaf = path->nodes[0]; >> struct btrfs_space_info *space_info; >> - struct extent_io_tree *block_group_cache; >> + struct btrfs_block_group_cache *cache; >> struct btrfs_key key; >> - struct btrfs_key found_key; >> - struct extent_buffer *leaf; >> + int slot = path->slots[0]; >> + int bit = 0; >> + int ret; >> - block_group_cache = &info->block_group_cache; >> + btrfs_item_key_to_cpu(leaf, &key, slot); >> + ASSERT(key.type == BTRFS_BLOCK_GROUP_ITEM_KEY); >> + >> + /* >> + * Skip 0 sized block group, don't insert them into block >> + * group cache tree, as its length is 0, it won't get >> + * freed at close_ctree() time. >> + */ >> + if (key.offset == 0) >> + return 0; >> + >> + cache = kzalloc(sizeof(*cache), GFP_NOFS); >> + if (!cache) >> + return -ENOMEM; >> + read_extent_buffer(leaf, &cache->item, >> + btrfs_item_ptr_offset(leaf, slot), >> + sizeof(cache->item)); >> + memcpy(&cache->key, &key, sizeof(key)); >> + cache->cached = 0; >> + cache->pinned = 0; >> + cache->flags = btrfs_block_group_flags(&cache->item); >> + if (cache->flags & BTRFS_BLOCK_GROUP_DATA) { >> + bit = BLOCK_GROUP_DATA; >> + } else if (cache->flags & BTRFS_BLOCK_GROUP_SYSTEM) { >> + bit = BLOCK_GROUP_SYSTEM; >> + } else if (cache->flags & BTRFS_BLOCK_GROUP_METADATA) { >> + bit = BLOCK_GROUP_METADATA; >> + } >> + set_avail_alloc_bits(fs_info, cache->flags); >> + if (btrfs_chunk_readonly(fs_info, cache->key.objectid)) >> + cache->ro = 1; >> + exclude_super_stripes(fs_info, cache); >> + >> + ret = update_space_info(fs_info, cache->flags, cache->key.offset, >> + btrfs_block_group_used(&cache->item), >> + &space_info); >> + if (ret < 0) { >> + free(cache); >> + return ret; >> + } >> + cache->space_info = space_info; >> + >> + set_extent_bits(block_group_cache, cache->key.objectid, >> + cache->key.objectid + cache->key.offset - 1, >> + bit | EXTENT_LOCKED); >> + set_state_private(block_group_cache, cache->key.objectid, >> + (unsigned long)cache); >> + return 0; >> +} >> - root = info->extent_root; >> +int btrfs_read_block_groups(struct btrfs_fs_info *fs_info) >> +{ >> + struct btrfs_path path; >> + struct btrfs_root *root; >> + int ret; >> + struct btrfs_key key; >> + >> + root = fs_info->extent_root; >> key.objectid = 0; >> key.offset = 0; >> key.type = BTRFS_BLOCK_GROUP_ITEM_KEY; >> - path = btrfs_alloc_path(); >> - if (!path) >> - return -ENOMEM; >> + btrfs_init_path(&path); >> while(1) { >> - ret = find_first_block_group(root, path, &key); >> + ret = find_first_block_group(root, &path, &key); >> if (ret > 0) { >> ret = 0; >> goto error; >> @@ -2673,67 +2739,21 @@ int btrfs_read_block_groups(struct btrfs_root >> *root) >> if (ret != 0) { >> goto error; >> } >> - leaf = path->nodes[0]; >> - btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]); >> + btrfs_item_key_to_cpu(path.nodes[0], &key, path.slots[0]); >> - cache = kzalloc(sizeof(*cache), GFP_NOFS); >> - if (!cache) { >> - ret = -ENOMEM; >> + ret = read_one_block_group(fs_info, &path); >> + if (ret < 0) >> goto error; >> - } >> - read_extent_buffer(leaf, &cache->item, >> - btrfs_item_ptr_offset(leaf, path->slots[0]), >> - sizeof(cache->item)); >> - memcpy(&cache->key, &found_key, sizeof(found_key)); >> - cache->cached = 0; >> - cache->pinned = 0; >> - key.objectid = found_key.objectid + found_key.offset; >> - if (found_key.offset == 0) >> + if (key.offset == 0) >> key.objectid++; >> - btrfs_release_path(path); >> - >> - /* >> - * Skip 0 sized block group, don't insert them into block >> - * group cache tree, as its length is 0, it won't get >> - * freed at close_ctree() time. >> - */ >> - if (found_key.offset == 0) { >> - free(cache); >> - continue; >> - } >> - >> - cache->flags = btrfs_block_group_flags(&cache->item); >> - bit = 0; >> - if (cache->flags & BTRFS_BLOCK_GROUP_DATA) { >> - bit = BLOCK_GROUP_DATA; >> - } else if (cache->flags & BTRFS_BLOCK_GROUP_SYSTEM) { >> - bit = BLOCK_GROUP_SYSTEM; >> - } else if (cache->flags & BTRFS_BLOCK_GROUP_METADATA) { >> - bit = BLOCK_GROUP_METADATA; >> - } >> - set_avail_alloc_bits(info, cache->flags); >> - if (btrfs_chunk_readonly(info, cache->key.objectid)) >> - cache->ro = 1; >> - >> - exclude_super_stripes(info, cache); >> - >> - ret = update_space_info(info, cache->flags, found_key.offset, >> - btrfs_block_group_used(&cache->item), >> - &space_info); >> - BUG_ON(ret); >> - cache->space_info = space_info; >> - >> - /* use EXTENT_LOCKED to prevent merging */ >> - set_extent_bits(block_group_cache, found_key.objectid, >> - found_key.objectid + found_key.offset - 1, >> - bit | EXTENT_LOCKED); >> - set_state_private(block_group_cache, found_key.objectid, >> - (unsigned long)cache); >> + else >> + key.objectid = key.objectid + key.offset; >> + btrfs_release_path(&path); >> } >> ret = 0; >> error: >> - btrfs_free_path(path); >> + btrfs_release_path(&path); >> return ret; >> } >> >
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@suse.com> >>> --- >>> 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
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; + } } key.objectid = BTRFS_FS_TREE_OBJECTID; diff --git a/extent-tree.c b/extent-tree.c index 19d1ea0df570..9713d627764c 100644 --- a/extent-tree.c +++ b/extent-tree.c @@ -2607,6 +2607,13 @@ int btrfs_free_block_groups(struct btrfs_fs_info *info) return 0; } +/* + * Find a block group which starts >= @key->objectid in extent tree. + * + * Return 0 for found + * Retrun >0 for not found + * Return <0 for error + */ static int find_first_block_group(struct btrfs_root *root, struct btrfs_path *path, struct btrfs_key *key) { @@ -2636,36 +2643,95 @@ static int find_first_block_group(struct btrfs_root *root, return 0; path->slots[0]++; } - ret = -ENOENT; + ret = 1; error: return ret; } -int btrfs_read_block_groups(struct btrfs_root *root) +/* + * Helper function to read out one BLOCK_GROUP_ITEM and insert it into + * block group cache. + * + * Return 0 if nothing wrong (either insert the bg cache or skip 0 sized bg) + * Return <0 for error. + */ +static int read_one_block_group(struct btrfs_fs_info *fs_info, + struct btrfs_path *path) { - struct btrfs_path *path; - int ret; - int bit; - struct btrfs_block_group_cache *cache; - struct btrfs_fs_info *info = root->fs_info; + struct extent_io_tree *block_group_cache = &fs_info->block_group_cache; + struct extent_buffer *leaf = path->nodes[0]; struct btrfs_space_info *space_info; - struct extent_io_tree *block_group_cache; + struct btrfs_block_group_cache *cache; struct btrfs_key key; - struct btrfs_key found_key; - struct extent_buffer *leaf; + int slot = path->slots[0]; + int bit = 0; + int ret; - block_group_cache = &info->block_group_cache; + btrfs_item_key_to_cpu(leaf, &key, slot); + ASSERT(key.type == BTRFS_BLOCK_GROUP_ITEM_KEY); + + /* + * Skip 0 sized block group, don't insert them into block + * group cache tree, as its length is 0, it won't get + * freed at close_ctree() time. + */ + if (key.offset == 0) + return 0; + + cache = kzalloc(sizeof(*cache), GFP_NOFS); + if (!cache) + return -ENOMEM; + read_extent_buffer(leaf, &cache->item, + btrfs_item_ptr_offset(leaf, slot), + sizeof(cache->item)); + memcpy(&cache->key, &key, sizeof(key)); + cache->cached = 0; + cache->pinned = 0; + cache->flags = btrfs_block_group_flags(&cache->item); + if (cache->flags & BTRFS_BLOCK_GROUP_DATA) { + bit = BLOCK_GROUP_DATA; + } else if (cache->flags & BTRFS_BLOCK_GROUP_SYSTEM) { + bit = BLOCK_GROUP_SYSTEM; + } else if (cache->flags & BTRFS_BLOCK_GROUP_METADATA) { + bit = BLOCK_GROUP_METADATA; + } + set_avail_alloc_bits(fs_info, cache->flags); + if (btrfs_chunk_readonly(fs_info, cache->key.objectid)) + cache->ro = 1; + exclude_super_stripes(fs_info, cache); + + ret = update_space_info(fs_info, cache->flags, cache->key.offset, + btrfs_block_group_used(&cache->item), + &space_info); + if (ret < 0) { + free(cache); + return ret; + } + cache->space_info = space_info; + + set_extent_bits(block_group_cache, cache->key.objectid, + cache->key.objectid + cache->key.offset - 1, + bit | EXTENT_LOCKED); + set_state_private(block_group_cache, cache->key.objectid, + (unsigned long)cache); + return 0; +} - root = info->extent_root; +int btrfs_read_block_groups(struct btrfs_fs_info *fs_info) +{ + struct btrfs_path path; + struct btrfs_root *root; + int ret; + struct btrfs_key key; + + root = fs_info->extent_root; key.objectid = 0; key.offset = 0; key.type = BTRFS_BLOCK_GROUP_ITEM_KEY; - path = btrfs_alloc_path(); - if (!path) - return -ENOMEM; + btrfs_init_path(&path); while(1) { - ret = find_first_block_group(root, path, &key); + ret = find_first_block_group(root, &path, &key); if (ret > 0) { ret = 0; goto error; @@ -2673,67 +2739,21 @@ int btrfs_read_block_groups(struct btrfs_root *root) if (ret != 0) { goto error; } - leaf = path->nodes[0]; - btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]); + btrfs_item_key_to_cpu(path.nodes[0], &key, path.slots[0]); - cache = kzalloc(sizeof(*cache), GFP_NOFS); - if (!cache) { - ret = -ENOMEM; + ret = read_one_block_group(fs_info, &path); + if (ret < 0) goto error; - } - read_extent_buffer(leaf, &cache->item, - btrfs_item_ptr_offset(leaf, path->slots[0]), - sizeof(cache->item)); - memcpy(&cache->key, &found_key, sizeof(found_key)); - cache->cached = 0; - cache->pinned = 0; - key.objectid = found_key.objectid + found_key.offset; - if (found_key.offset == 0) + if (key.offset == 0) key.objectid++; - btrfs_release_path(path); - - /* - * Skip 0 sized block group, don't insert them into block - * group cache tree, as its length is 0, it won't get - * freed at close_ctree() time. - */ - if (found_key.offset == 0) { - free(cache); - continue; - } - - cache->flags = btrfs_block_group_flags(&cache->item); - bit = 0; - if (cache->flags & BTRFS_BLOCK_GROUP_DATA) { - bit = BLOCK_GROUP_DATA; - } else if (cache->flags & BTRFS_BLOCK_GROUP_SYSTEM) { - bit = BLOCK_GROUP_SYSTEM; - } else if (cache->flags & BTRFS_BLOCK_GROUP_METADATA) { - bit = BLOCK_GROUP_METADATA; - } - set_avail_alloc_bits(info, cache->flags); - if (btrfs_chunk_readonly(info, cache->key.objectid)) - cache->ro = 1; - - exclude_super_stripes(info, cache); - - ret = update_space_info(info, cache->flags, found_key.offset, - btrfs_block_group_used(&cache->item), - &space_info); - BUG_ON(ret); - cache->space_info = space_info; - - /* use EXTENT_LOCKED to prevent merging */ - set_extent_bits(block_group_cache, found_key.objectid, - found_key.objectid + found_key.offset - 1, - bit | EXTENT_LOCKED); - set_state_private(block_group_cache, found_key.objectid, - (unsigned long)cache); + else + key.objectid = key.objectid + key.offset; + btrfs_release_path(&path); } ret = 0; error: - btrfs_free_path(path); + btrfs_release_path(&path); return ret; }
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. This is super confusing, it's almost a mircle it even works. Signed-off-by: Qu Wenruo <wqu@suse.com> --- ctree.h | 2 +- disk-io.c | 9 ++- extent-tree.c | 160 ++++++++++++++++++++++++++++---------------------- 3 files changed, 97 insertions(+), 74 deletions(-)