[PATCH 1/2] btrfs-progs: Fix a bug in __btrfs_map_block() always maps wrong stripe for DUP/RAID1

[PATCH 1/2] btrfs-progs: Fix a bug in __btrfs_map_block() always maps wrong stripe for DUP/RAID1

[Qu Wenruo]

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:
 		if (rw == WRITE)
 			multi->num_stripes = map->num_stripes;
 		else if (mirror_num)
-			stripe_index = mirror_num - 1;
+			stripe_index = mirror_num;
 		else
 			stripe_index = stripe_nr % map->num_stripes;
 	} else if (map->type & BTRFS_BLOCK_GROUP_RAID10) {
@@ -1416,7 +1416,7 @@ again:
 		if (rw == WRITE)
 			multi->num_stripes = map->num_stripes;
 		else if (mirror_num)
-			stripe_index = mirror_num - 1;
+			stripe_index = mirror_num;
 	} else if (map->type & (BTRFS_BLOCK_GROUP_RAID5 |
 				BTRFS_BLOCK_GROUP_RAID6)) {
 
-- 
2.4.0

[PATCH 2/2] btrfs-progs: Cleanup logic of btrfs_read_tree_block().

[Qu Wenruo]

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

Re: [PATCH 1/2] btrfs-progs: Fix a bug in __btrfs_map_block() always maps wrong stripe for DUP/RAID1

[David Sterba]

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.

Re: [PATCH 1/2] btrfs-progs: Fix a bug in __btrfs_map_block() always maps wrong stripe for DUP/RAID1

[Qu Wenruo]

-------- Original Message  --------
Subject: Re: [PATCH 1/2] btrfs-progs: Fix a bug in __btrfs_map_block() 
always maps wrong stripe for DUP/RAID1
From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2015年05月22日 00:06

> 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.
>
It seems that I was confused about mirror_num of btrfs_map_block function.

When mirror mirror_num 0, it will map all stripes and if larger than 0,
it will map the <mirror_num>th stripe.

So when mirror_num is given 1, it should return the first stripe.
And it's the correct behavior.
If I am wrong again, please correct me.

Please ignore the patch.

Thanks,
Qu