* [PATCH 2/2] btrfs-progs: Cleanup logic of btrfs_read_tree_block().
2015-05-11 0:45 [PATCH 1/2] btrfs-progs: Fix a bug in __btrfs_map_block() always maps wrong stripe for DUP/RAID1 Qu Wenruo
@ 2015-05-11 0:45 ` Qu Wenruo
2015-05-21 16:06 ` [PATCH 1/2] btrfs-progs: Fix a bug in __btrfs_map_block() always maps wrong stripe for DUP/RAID1 David Sterba
1 sibling, 0 replies; 4+ messages in thread
From: Qu Wenruo @ 2015-05-11 0:45 UTC (permalink / raw)
To: linux-btrfs
Make the logic of btrfs_read_tree_block() more clean.
Now it will do as the following flow:
1) Iterate all copies of the tree block
If one can pass all tests(csum, check tree block, transid), then
return it.
2) If all above failed, use the copy with highest transid
If that copy can pass checksum and tree block check, then ignore its
transid error
3) If none passed above, print out error.
This provides the basis for later all tree block copies check.
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
disk-io.c | 73 +++++++++++++++++++++++++++++++++------------------------------
1 file changed, 38 insertions(+), 35 deletions(-)
diff --git a/disk-io.c b/disk-io.c
index c1cf146..c8302a8 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -280,9 +280,8 @@ struct extent_buffer *read_tree_block(struct btrfs_root *root, u64 bytenr,
struct extent_buffer *eb;
u64 best_transid = 0;
int mirror_num = 0;
- int good_mirror = 0;
+ int best_mirror = -1;
int num_copies;
- int ignore = 0;
eb = btrfs_find_create_tree_block(root, bytenr, blocksize);
if (!eb)
@@ -291,50 +290,54 @@ struct extent_buffer *read_tree_block(struct btrfs_root *root, u64 bytenr,
if (btrfs_buffer_uptodate(eb, parent_transid))
return eb;
- while (1) {
+ num_copies = btrfs_num_copies(&root->fs_info->mapping_tree, eb->start,
+ eb->len);
+
+ /* Iterate all copies with restrict check */
+ for (mirror_num = 0; mirror_num < num_copies; mirror_num++) {
ret = read_whole_eb(root->fs_info, eb, mirror_num);
if (ret == 0 && csum_tree_block(root, eb, 1) == 0 &&
check_tree_block(root, eb) == 0 &&
- verify_parent_transid(eb->tree, eb, parent_transid, ignore)
+ verify_parent_transid(eb->tree, eb, parent_transid, 0)
== 0) {
- if (eb->flags & EXTENT_BAD_TRANSID &&
- list_empty(&eb->recow)) {
- list_add_tail(&eb->recow,
- &root->fs_info->recow_ebs);
- eb->refs++;
- }
btrfs_set_buffer_uptodate(eb);
return eb;
}
- if (ignore) {
- if (check_tree_block(root, eb)) {
- if (!root->fs_info->suppress_check_block_errors)
- print_tree_block_error(root, eb,
- check_tree_block(root, eb));
- } else {
- if (!root->fs_info->suppress_check_block_errors)
- fprintf(stderr, "Csum didn't match\n");
- }
- ret = -EIO;
- break;
- }
- num_copies = btrfs_num_copies(&root->fs_info->mapping_tree,
- eb->start, eb->len);
- if (num_copies == 1) {
- ignore = 1;
- continue;
- }
- if (btrfs_header_generation(eb) > best_transid && mirror_num) {
+ if (btrfs_header_generation(eb) > best_transid) {
best_transid = btrfs_header_generation(eb);
- good_mirror = mirror_num;
+ best_mirror = mirror_num;
}
- mirror_num++;
- if (mirror_num > num_copies) {
- mirror_num = good_mirror;
- ignore = 1;
- continue;
+ }
+ /* No valid eb found, use one with best transid or the last mirror */
+ if (best_mirror != -1)
+ mirror_num = best_mirror;
+ ret = read_whole_eb(root->fs_info, eb, mirror_num);
+ if (ret < 0)
+ goto out;
+
+ /* ignore transid error */
+ if (csum_tree_block(root, eb, 1) == 0 &&
+ check_tree_block(root, eb) ==0 &&
+ verify_parent_transid(eb->tree, eb, parent_transid, 1) == 0) {
+ if (eb->flags & EXTENT_BAD_TRANSID && list_empty(&eb->recow)) {
+ list_add_tail(&eb->recow, &root->fs_info->recow_ebs);
+ eb->refs++;
}
+ btrfs_set_buffer_uptodate(eb);
+ return eb;
+ }
+
+ /* print other errors */
+ if (check_tree_block(root, eb)) {
+ if (!root->fs_info->suppress_check_block_errors)
+ print_tree_block_error(root, eb,
+ check_tree_block(root, eb));
+ } else {
+ if (!root->fs_info->suppress_check_block_errors)
+ fprintf(stderr, "Csum didn't match\n");
}
+ ret = -EIO;
+out:
free_extent_buffer(eb);
return ERR_PTR(ret);
}
--
2.4.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH 1/2] btrfs-progs: Fix a bug in __btrfs_map_block() always maps wrong stripe for DUP/RAID1
2015-05-11 0:45 [PATCH 1/2] btrfs-progs: Fix a bug in __btrfs_map_block() always maps wrong stripe for DUP/RAID1 Qu Wenruo
2015-05-11 0:45 ` [PATCH 2/2] btrfs-progs: Cleanup logic of btrfs_read_tree_block() Qu Wenruo
@ 2015-05-21 16:06 ` David Sterba
2015-05-22 2:53 ` Qu Wenruo
1 sibling, 1 reply; 4+ messages in thread
From: David Sterba @ 2015-05-21 16:06 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs
On Mon, May 11, 2015 at 08:45:47AM +0800, Qu Wenruo wrote:
> In __btrfs_map_block(), stripe_index is always wrong if mirror_num is
> given.
>
> For DUP/RAID1 case, if mirror_num is given, e.g. 1, it should return the
> second stripe.
> But codes consider the mirror_num is start from 1 and always minus 1,
> causing even mirror number is given as 1, __btrfs_map_block() will still
> map the first stripe not the second.
>
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> ---
> volumes.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/volumes.c b/volumes.c
> index 16dbf64..c66c02a 100644
> --- a/volumes.c
> +++ b/volumes.c
> @@ -1397,7 +1397,7 @@ again:
> - stripe_index = mirror_num - 1;
> + stripe_index = mirror_num;
> @@ -1416,7 +1416,7 @@ again:
> - stripe_index = mirror_num - 1;
> + stripe_index = mirror_num;
The original code is same in kernel, is the logic different in the
userspace code? If yes I don't see it.
^ permalink raw reply [flat|nested] 4+ messages in thread