[PATCH 0/4] btrfs check --check-data-csum enhancement for

[PATCH 0/4] btrfs check --check-data-csum enhancement for

[Qu Wenruo]

Log-writes tool from Josef exposed btrfs data corruption.

Although still digging, at least enhance --check-data-csum option so
that log-replay --check can stop exactly at the point where btrfs data
get corrupted, instead of manually checking the output.

Qu Wenruo (4):
  btrfs-progs: check: Check data csum for all copies
  btrfs-progs: check: Fix data csum check return value
  btrfs-progs: check: Continue check even csum error is found
  btrfs-progs: check: Distingusih csum checking output for
    --check-data-csum

 check/main.c | 93 ++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 56 insertions(+), 37 deletions(-)

-- 
2.16.2

[PATCH 1/4] btrfs-progs: check: Check data csum for all copies

[Qu Wenruo]

Original --check-data-csum option will skip the extra copy if the first
copy matches csum.

Since offline scrub (with recoverability report) is still out-of-tree, at
least enhance --check-data-csum option to handle multiple copies.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c | 65 ++++++++++++++++++++++++++++++------------------------------
 1 file changed, 32 insertions(+), 33 deletions(-)

diff --git a/check/main.c b/check/main.c
index 97baae583f04..f25fdc765d63 100644
--- a/check/main.c
+++ b/check/main.c
@@ -5381,42 +5381,37 @@ static int check_extent_csums(struct btrfs_root *root, u64 bytenr,
 	if (!data)
 		return -ENOMEM;
 
+	num_copies = btrfs_num_copies(root->fs_info, bytenr, num_bytes);
 	while (offset < num_bytes) {
-		mirror = 0;
-again:
-		read_len = num_bytes - offset;
-		/* read as much space once a time */
-		ret = read_extent_data(fs_info, data + offset,
-				bytenr + offset, &read_len, mirror);
-		if (ret)
-			goto out;
-		data_checked = 0;
-		/* verify every 4k data's checksum */
-		while (data_checked < read_len) {
-			csum = ~(u32)0;
-			tmp = offset + data_checked;
-
-			csum = btrfs_csum_data((char *)data + tmp,
-					       csum, fs_info->sectorsize);
-			btrfs_csum_final(csum, (u8 *)&csum);
-
-			csum_offset = leaf_offset +
-				 tmp / fs_info->sectorsize * csum_size;
-			read_extent_buffer(eb, (char *)&csum_expected,
-					   csum_offset, csum_size);
-			/* try another mirror */
-			if (csum != csum_expected) {
-				fprintf(stderr, "mirror %d bytenr %llu csum %u expected csum %u\n",
+		for (mirror = 1; mirror <= num_copies; mirror++) {
+			read_len = num_bytes - offset;
+			/* read as much space once a time */
+			ret = read_extent_data(fs_info, data + offset,
+					bytenr + offset, &read_len, mirror);
+			if (ret)
+				goto out;
+
+			data_checked = 0;
+			/* verify every 4k data's checksum */
+			while (data_checked < read_len) {
+				csum = ~(u32)0;
+				tmp = offset + data_checked;
+
+				csum = btrfs_csum_data((char *)data + tmp,
+						csum, fs_info->sectorsize);
+				btrfs_csum_final(csum, (u8 *)&csum);
+
+				csum_offset = leaf_offset +
+					 tmp / fs_info->sectorsize * csum_size;
+				read_extent_buffer(eb, (char *)&csum_expected,
+						   csum_offset, csum_size);
+				if (csum != csum_expected)
+					fprintf(stderr,
+			"mirror %d bytenr %llu csum %u expected csum %u\n",
 						mirror, bytenr + tmp,
 						csum, csum_expected);
-				num_copies = btrfs_num_copies(root->fs_info,
-						bytenr, num_bytes);
-				if (mirror < num_copies - 1) {
-					mirror += 1;
-					goto again;
-				}
+				data_checked += fs_info->sectorsize;
 			}
-			data_checked += fs_info->sectorsize;
 		}
 		offset += read_len;
 	}
@@ -5624,7 +5619,11 @@ static int check_csums(struct btrfs_root *root)
 		leaf_offset = btrfs_item_ptr_offset(leaf, path.slots[0]);
 		ret = check_extent_csums(root, key.offset, data_len,
 					 leaf_offset, leaf);
-		if (ret)
+		/*
+		 * Only break for fatal errors, if mismatch is found,
+		 * continue checking until all extents are checked.
+		 */
+		if (ret < 0)
 			break;
 skip_csum_check:
 		if (!num_bytes) {
-- 
2.16.2

[PATCH 2/4] btrfs-progs: check: Fix data csum check return value

[Qu Wenruo]

When --check-data-csum option found csum mismatch, btrfs check still
return 0.

Fix it so log-replay could automatically pause when found csum error.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/check/main.c b/check/main.c
index f25fdc765d63..15b3c402c9f5 100644
--- a/check/main.c
+++ b/check/main.c
@@ -5356,6 +5356,13 @@ static int check_space_cache(struct btrfs_root *root)
 	return error ? -EINVAL : 0;
 }
 
+/*
+ * Check data checksum for [@bytenr, @bytenr + @num_bytes).
+ *
+ * Return <0 for fatal error (fails to read checksum/data or allocate memory).
+ * Return >0 for csum mismatch for *ANY* copy.
+ * Return 0 if everything is OK.
+ */
 static int check_extent_csums(struct btrfs_root *root, u64 bytenr,
 			u64 num_bytes, unsigned long leaf_offset,
 			struct extent_buffer *eb)
@@ -5373,6 +5380,7 @@ static int check_extent_csums(struct btrfs_root *root, u64 bytenr,
 	int ret = 0;
 	int mirror;
 	int num_copies;
+	bool csum_mismatch = false;
 
 	if (num_bytes % fs_info->sectorsize)
 		return -EINVAL;
@@ -5405,11 +5413,13 @@ static int check_extent_csums(struct btrfs_root *root, u64 bytenr,
 					 tmp / fs_info->sectorsize * csum_size;
 				read_extent_buffer(eb, (char *)&csum_expected,
 						   csum_offset, csum_size);
-				if (csum != csum_expected)
+				if (csum != csum_expected) {
+					csum_mismatch = true;
 					fprintf(stderr,
 			"mirror %d bytenr %llu csum %u expected csum %u\n",
 						mirror, bytenr + tmp,
 						csum, csum_expected);
+				}
 				data_checked += fs_info->sectorsize;
 			}
 		}
@@ -5417,6 +5427,8 @@ static int check_extent_csums(struct btrfs_root *root, u64 bytenr,
 	}
 out:
 	free(data);
+	if (!ret && csum_mismatch)
+		ret = 1;
 	return ret;
 }
 
@@ -5625,6 +5637,8 @@ static int check_csums(struct btrfs_root *root)
 		 */
 		if (ret < 0)
 			break;
+		if (ret > 0)
+			errors++;
 skip_csum_check:
 		if (!num_bytes) {
 			offset = key.offset;
-- 
2.16.2

[PATCH 3/4] btrfs-progs: check: Continue check even csum error is found

[Qu Wenruo]

Since data csum is not a fatal error compared to fs/extent trees,
continue check.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/check/main.c b/check/main.c
index 15b3c402c9f5..22a78273be15 100644
--- a/check/main.c
+++ b/check/main.c
@@ -9847,11 +9847,13 @@ int cmd_check(int argc, char **argv)
 
 	fprintf(stderr, "checking csums\n");
 	ret = check_csums(root);
-	err |= !!ret;
-	if (ret) {
+	/*
+	 * Data csum error is not fatal, and it may indicates more serious
+	 * corruption, continue checking.
+	 */
+	if (ret)
 		error("errors found in csum tree");
-		goto out;
-	}
+	err |= !!ret;
 
 	fprintf(stderr, "checking root refs\n");
 	/* For low memory mode, check_fs_roots_v2 handles root refs */
-- 
2.16.2

[PATCH 4/4] btrfs-progs: check: Distingusih csum checking output for --check-data-csum

[Qu Wenruo]

No matter --check-data-csum is passed or not, btrfs check will always
output message like "checking csum".

This message could be a little confusing, change it according to
--check-data-csum option.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/check/main.c b/check/main.c
index 22a78273be15..3f4a6bcac4df 100644
--- a/check/main.c
+++ b/check/main.c
@@ -9845,7 +9845,11 @@ int cmd_check(int argc, char **argv)
 		goto out;
 	}
 
-	fprintf(stderr, "checking csums\n");
+	if (check_data_csum)
+		fprintf(stderr, "checking csums against data\n");
+	else
+		fprintf(stderr,
+			"checking csum items alone (without verifying data)\n");
 	ret = check_csums(root);
 	/*
 	 * Data csum error is not fatal, and it may indicates more serious
-- 
2.16.2

Re: [PATCH 1/4] btrfs-progs: check: Check data csum for all copies

[Su Yue]

On 02/27/2018 05:12 PM, Qu Wenruo wrote:
> Original --check-data-csum option will skip the extra copy if the first
> copy matches csum.
> 
> Since offline scrub (with recoverability report) is still out-of-tree, at
> least enhance --check-data-csum option to handle multiple copies.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   check/main.c | 65 ++++++++++++++++++++++++++++++------------------------------
>   1 file changed, 32 insertions(+), 33 deletions(-)
> 
> diff --git a/check/main.c b/check/main.c
> index 97baae583f04..f25fdc765d63 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -5381,42 +5381,37 @@ static int check_extent_csums(struct btrfs_root *root, u64 bytenr,
>   	if (!data)
>   		return -ENOMEM;
>   
> +	num_copies = btrfs_num_copies(root->fs_info, bytenr, num_bytes);
>   	while (offset < num_bytes) {
> -		mirror = 0;
> -again:
> -		read_len = num_bytes - offset;
> -		/* read as much space once a time */
> -		ret = read_extent_data(fs_info, data + offset,
> -				bytenr + offset, &read_len, mirror);
> -		if (ret)
> -			goto out;
> -		data_checked = 0;
> -		/* verify every 4k data's checksum */
> -		while (data_checked < read_len) {
> -			csum = ~(u32)0;
> -			tmp = offset + data_checked;
> -
> -			csum = btrfs_csum_data((char *)data + tmp,
> -					       csum, fs_info->sectorsize);
> -			btrfs_csum_final(csum, (u8 *)&csum);
> -
> -			csum_offset = leaf_offset +
> -				 tmp / fs_info->sectorsize * csum_size;
> -			read_extent_buffer(eb, (char *)&csum_expected,
> -					   csum_offset, csum_size);
> -			/* try another mirror */
> -			if (csum != csum_expected) {
> -				fprintf(stderr, "mirror %d bytenr %llu csum %u expected csum %u\n",
> +		for (mirror = 1; mirror <= num_copies; mirror++) {

Got your point.
But what confuses me is that why mirror starts from 1 here.
The mirror influences btrfs_map_block() which is not related
to this patch though.

Thanks,
Su
> +			read_len = num_bytes - offset;
> +			/* read as much space once a time */
> +			ret = read_extent_data(fs_info, data + offset,
> +					bytenr + offset, &read_len, mirror);
> +			if (ret)
> +				goto out;
> +
> +			data_checked = 0;
> +			/* verify every 4k data's checksum */
> +			while (data_checked < read_len) {
> +				csum = ~(u32)0;
> +				tmp = offset + data_checked;
> +
> +				csum = btrfs_csum_data((char *)data + tmp,
> +						csum, fs_info->sectorsize);
> +				btrfs_csum_final(csum, (u8 *)&csum);
> +
> +				csum_offset = leaf_offset +
> +					 tmp / fs_info->sectorsize * csum_size;
> +				read_extent_buffer(eb, (char *)&csum_expected,
> +						   csum_offset, csum_size);
> +				if (csum != csum_expected)
> +					fprintf(stderr,
> +			"mirror %d bytenr %llu csum %u expected csum %u\n",
>   						mirror, bytenr + tmp,
>   						csum, csum_expected);
> -				num_copies = btrfs_num_copies(root->fs_info,
> -						bytenr, num_bytes);
> -				if (mirror < num_copies - 1) {
> -					mirror += 1;
> -					goto again;
> -				}
> +				data_checked += fs_info->sectorsize;
>   			}
> -			data_checked += fs_info->sectorsize;
>   		}
>   		offset += read_len;
>   	}
> @@ -5624,7 +5619,11 @@ static int check_csums(struct btrfs_root *root)
>   		leaf_offset = btrfs_item_ptr_offset(leaf, path.slots[0]);
>   		ret = check_extent_csums(root, key.offset, data_len,
>   					 leaf_offset, leaf);
> -		if (ret)
> +		/*
> +		 * Only break for fatal errors, if mismatch is found,
> +		 * continue checking until all extents are checked.
> +		 */
> +		if (ret < 0)
>   			break;
>   skip_csum_check:
>   		if (!num_bytes) {
> 

Re: [PATCH 1/4] btrfs-progs: check: Check data csum for all copies

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 6265 bytes --]



On 2018年02月27日 18:01, Su Yue wrote:
> 
> 
> On 02/27/2018 05:12 PM, Qu Wenruo wrote:
>> Original --check-data-csum option will skip the extra copy if the first
>> copy matches csum.
>>
>> Since offline scrub (with recoverability report) is still out-of-tree, at
>> least enhance --check-data-csum option to handle multiple copies.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   check/main.c | 65
>> ++++++++++++++++++++++++++++++------------------------------
>>   1 file changed, 32 insertions(+), 33 deletions(-)
>>
>> diff --git a/check/main.c b/check/main.c
>> index 97baae583f04..f25fdc765d63 100644
>> --- a/check/main.c
>> +++ b/check/main.c
>> @@ -5381,42 +5381,37 @@ static int check_extent_csums(struct
>> btrfs_root *root, u64 bytenr,
>>       if (!data)
>>           return -ENOMEM;
>>   +    num_copies = btrfs_num_copies(root->fs_info, bytenr, num_bytes);
>>       while (offset < num_bytes) {
>> -        mirror = 0;
>> -again:
>> -        read_len = num_bytes - offset;
>> -        /* read as much space once a time */
>> -        ret = read_extent_data(fs_info, data + offset,
>> -                bytenr + offset, &read_len, mirror);
>> -        if (ret)
>> -            goto out;
>> -        data_checked = 0;
>> -        /* verify every 4k data's checksum */
>> -        while (data_checked < read_len) {
>> -            csum = ~(u32)0;
>> -            tmp = offset + data_checked;
>> -
>> -            csum = btrfs_csum_data((char *)data + tmp,
>> -                           csum, fs_info->sectorsize);
>> -            btrfs_csum_final(csum, (u8 *)&csum);
>> -
>> -            csum_offset = leaf_offset +
>> -                 tmp / fs_info->sectorsize * csum_size;
>> -            read_extent_buffer(eb, (char *)&csum_expected,
>> -                       csum_offset, csum_size);
>> -            /* try another mirror */
>> -            if (csum != csum_expected) {
>> -                fprintf(stderr, "mirror %d bytenr %llu csum %u
>> expected csum %u\n",
>> +        for (mirror = 1; mirror <= num_copies; mirror++) {
> 
> Got your point.
> But what confuses me is that why mirror starts from 1 here.
> The mirror influences btrfs_map_block() which is not related
> to this patch though.

mirror number has different meanings in fact.

For 0, it means try to read *ANY* valid copy, it can be the copy of a
duplication, or even rebuilt data from RAID5/6.

For 1, it means the fist copy. Either the only copy of
SINGLE/RAID0/RAID5/6, or the first copy of RAID1/10.

For 2, it means the 2nd copy, it can be real copy for RAID1/RAID10, or
rebuilt data from RAID5/6.

And in fact Liu Bo is adding new copy numbers for RAID5/6 to allow us to
specify how to build the missing data for RAID6.

So here starting from mirror 1 is the correct behavior.

Thanks,
Qu
> 
> Thanks,
> Su
>> +            read_len = num_bytes - offset;
>> +            /* read as much space once a time */
>> +            ret = read_extent_data(fs_info, data + offset,
>> +                    bytenr + offset, &read_len, mirror);
>> +            if (ret)
>> +                goto out;
>> +
>> +            data_checked = 0;
>> +            /* verify every 4k data's checksum */
>> +            while (data_checked < read_len) {
>> +                csum = ~(u32)0;
>> +                tmp = offset + data_checked;
>> +
>> +                csum = btrfs_csum_data((char *)data + tmp,
>> +                        csum, fs_info->sectorsize);
>> +                btrfs_csum_final(csum, (u8 *)&csum);
>> +
>> +                csum_offset = leaf_offset +
>> +                     tmp / fs_info->sectorsize * csum_size;
>> +                read_extent_buffer(eb, (char *)&csum_expected,
>> +                           csum_offset, csum_size);
>> +                if (csum != csum_expected)
>> +                    fprintf(stderr,
>> +            "mirror %d bytenr %llu csum %u expected csum %u\n",
>>                           mirror, bytenr + tmp,
>>                           csum, csum_expected);
>> -                num_copies = btrfs_num_copies(root->fs_info,
>> -                        bytenr, num_bytes);
>> -                if (mirror < num_copies - 1) {
>> -                    mirror += 1;
>> -                    goto again;
>> -                }
>> +                data_checked += fs_info->sectorsize;
>>               }
>> -            data_checked += fs_info->sectorsize;
>>           }
>>           offset += read_len;
>>       }
>> @@ -5624,7 +5619,11 @@ static int check_csums(struct btrfs_root *root)
>>           leaf_offset = btrfs_item_ptr_offset(leaf, path.slots[0]);
>>           ret = check_extent_csums(root, key.offset, data_len,
>>                        leaf_offset, leaf);
>> -        if (ret)
>> +        /*
>> +         * Only break for fatal errors, if mismatch is found,
>> +         * continue checking until all extents are checked.
>> +         */
>> +        if (ret < 0)
>>               break;
>>   skip_csum_check:
>>           if (!num_bytes) {
>>
> 
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 1/4] btrfs-progs: check: Check data csum for all copies

[Nikolay Borisov]

On 27.02.2018 12:31, Qu Wenruo wrote:
> 
> 
> On 2018年02月27日 18:01, Su Yue wrote:
>>
>>
>> On 02/27/2018 05:12 PM, Qu Wenruo wrote:
>>> Original --check-data-csum option will skip the extra copy if the first
>>> copy matches csum.
>>>
>>> Since offline scrub (with recoverability report) is still out-of-tree, at
>>> least enhance --check-data-csum option to handle multiple copies.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>   check/main.c | 65
>>> ++++++++++++++++++++++++++++++------------------------------
>>>   1 file changed, 32 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/check/main.c b/check/main.c
>>> index 97baae583f04..f25fdc765d63 100644
>>> --- a/check/main.c
>>> +++ b/check/main.c
>>> @@ -5381,42 +5381,37 @@ static int check_extent_csums(struct
>>> btrfs_root *root, u64 bytenr,
>>>       if (!data)
>>>           return -ENOMEM;
>>>   +    num_copies = btrfs_num_copies(root->fs_info, bytenr, num_bytes);
>>>       while (offset < num_bytes) {
>>> -        mirror = 0;
>>> -again:
>>> -        read_len = num_bytes - offset;
>>> -        /* read as much space once a time */
>>> -        ret = read_extent_data(fs_info, data + offset,
>>> -                bytenr + offset, &read_len, mirror);
>>> -        if (ret)
>>> -            goto out;
>>> -        data_checked = 0;
>>> -        /* verify every 4k data's checksum */
>>> -        while (data_checked < read_len) {
>>> -            csum = ~(u32)0;
>>> -            tmp = offset + data_checked;
>>> -
>>> -            csum = btrfs_csum_data((char *)data + tmp,
>>> -                           csum, fs_info->sectorsize);
>>> -            btrfs_csum_final(csum, (u8 *)&csum);
>>> -
>>> -            csum_offset = leaf_offset +
>>> -                 tmp / fs_info->sectorsize * csum_size;
>>> -            read_extent_buffer(eb, (char *)&csum_expected,
>>> -                       csum_offset, csum_size);
>>> -            /* try another mirror */
>>> -            if (csum != csum_expected) {
>>> -                fprintf(stderr, "mirror %d bytenr %llu csum %u
>>> expected csum %u\n",
>>> +        for (mirror = 1; mirror <= num_copies; mirror++) {
>>
>> Got your point.
>> But what confuses me is that why mirror starts from 1 here.
>> The mirror influences btrfs_map_block() which is not related
>> to this patch though.
> 
> mirror number has different meanings in fact.
> 
> For 0, it means try to read *ANY* valid copy, it can be the copy of a
> duplication, or even rebuilt data from RAID5/6.
> 
> For 1, it means the fist copy. Either the only copy of
> SINGLE/RAID0/RAID5/6, or the first copy of RAID1/10.
> 
> For 2, it means the 2nd copy, it can be real copy for RAID1/RAID10, or
> rebuilt data from RAID5/6.
> 
> And in fact Liu Bo is adding new copy numbers for RAID5/6 to allow us to
> specify how to build the missing data for RAID6.
> 
> So here starting from mirror 1 is the correct behavior.

Shouldn't those be documented?

> 
> Thanks,
> Qu
>>
>> Thanks,
>> Su
>>> +            read_len = num_bytes - offset;
>>> +            /* read as much space once a time */
>>> +            ret = read_extent_data(fs_info, data + offset,
>>> +                    bytenr + offset, &read_len, mirror);
>>> +            if (ret)
>>> +                goto out;
>>> +
>>> +            data_checked = 0;
>>> +            /* verify every 4k data's checksum */
>>> +            while (data_checked < read_len) {
>>> +                csum = ~(u32)0;
>>> +                tmp = offset + data_checked;
>>> +
>>> +                csum = btrfs_csum_data((char *)data + tmp,
>>> +                        csum, fs_info->sectorsize);
>>> +                btrfs_csum_final(csum, (u8 *)&csum);
>>> +
>>> +                csum_offset = leaf_offset +
>>> +                     tmp / fs_info->sectorsize * csum_size;
>>> +                read_extent_buffer(eb, (char *)&csum_expected,
>>> +                           csum_offset, csum_size);
>>> +                if (csum != csum_expected)
>>> +                    fprintf(stderr,
>>> +            "mirror %d bytenr %llu csum %u expected csum %u\n",
>>>                           mirror, bytenr + tmp,
>>>                           csum, csum_expected);
>>> -                num_copies = btrfs_num_copies(root->fs_info,
>>> -                        bytenr, num_bytes);
>>> -                if (mirror < num_copies - 1) {
>>> -                    mirror += 1;
>>> -                    goto again;
>>> -                }
>>> +                data_checked += fs_info->sectorsize;
>>>               }
>>> -            data_checked += fs_info->sectorsize;
>>>           }
>>>           offset += read_len;
>>>       }
>>> @@ -5624,7 +5619,11 @@ static int check_csums(struct btrfs_root *root)
>>>           leaf_offset = btrfs_item_ptr_offset(leaf, path.slots[0]);
>>>           ret = check_extent_csums(root, key.offset, data_len,
>>>                        leaf_offset, leaf);
>>> -        if (ret)
>>> +        /*
>>> +         * Only break for fatal errors, if mismatch is found,
>>> +         * continue checking until all extents are checked.
>>> +         */
>>> +        if (ret < 0)
>>>               break;
>>>   skip_csum_check:
>>>           if (!num_bytes) {
>>>
>>
>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/4] btrfs-progs: check: Check data csum for all copies

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 6796 bytes --]



On 2018年02月27日 19:09, Nikolay Borisov wrote:
> 
> 
> On 27.02.2018 12:31, Qu Wenruo wrote:
>>
>>
>> On 2018年02月27日 18:01, Su Yue wrote:
>>>
>>>
>>> On 02/27/2018 05:12 PM, Qu Wenruo wrote:
>>>> Original --check-data-csum option will skip the extra copy if the first
>>>> copy matches csum.
>>>>
>>>> Since offline scrub (with recoverability report) is still out-of-tree, at
>>>> least enhance --check-data-csum option to handle multiple copies.
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>   check/main.c | 65
>>>> ++++++++++++++++++++++++++++++------------------------------
>>>>   1 file changed, 32 insertions(+), 33 deletions(-)
>>>>
>>>> diff --git a/check/main.c b/check/main.c
>>>> index 97baae583f04..f25fdc765d63 100644
>>>> --- a/check/main.c
>>>> +++ b/check/main.c
>>>> @@ -5381,42 +5381,37 @@ static int check_extent_csums(struct
>>>> btrfs_root *root, u64 bytenr,
>>>>       if (!data)
>>>>           return -ENOMEM;
>>>>   +    num_copies = btrfs_num_copies(root->fs_info, bytenr, num_bytes);
>>>>       while (offset < num_bytes) {
>>>> -        mirror = 0;
>>>> -again:
>>>> -        read_len = num_bytes - offset;
>>>> -        /* read as much space once a time */
>>>> -        ret = read_extent_data(fs_info, data + offset,
>>>> -                bytenr + offset, &read_len, mirror);
>>>> -        if (ret)
>>>> -            goto out;
>>>> -        data_checked = 0;
>>>> -        /* verify every 4k data's checksum */
>>>> -        while (data_checked < read_len) {
>>>> -            csum = ~(u32)0;
>>>> -            tmp = offset + data_checked;
>>>> -
>>>> -            csum = btrfs_csum_data((char *)data + tmp,
>>>> -                           csum, fs_info->sectorsize);
>>>> -            btrfs_csum_final(csum, (u8 *)&csum);
>>>> -
>>>> -            csum_offset = leaf_offset +
>>>> -                 tmp / fs_info->sectorsize * csum_size;
>>>> -            read_extent_buffer(eb, (char *)&csum_expected,
>>>> -                       csum_offset, csum_size);
>>>> -            /* try another mirror */
>>>> -            if (csum != csum_expected) {
>>>> -                fprintf(stderr, "mirror %d bytenr %llu csum %u
>>>> expected csum %u\n",
>>>> +        for (mirror = 1; mirror <= num_copies; mirror++) {
>>>
>>> Got your point.
>>> But what confuses me is that why mirror starts from 1 here.
>>> The mirror influences btrfs_map_block() which is not related
>>> to this patch though.
>>
>> mirror number has different meanings in fact.
>>
>> For 0, it means try to read *ANY* valid copy, it can be the copy of a
>> duplication, or even rebuilt data from RAID5/6.
>>
>> For 1, it means the fist copy. Either the only copy of
>> SINGLE/RAID0/RAID5/6, or the first copy of RAID1/10.
>>
>> For 2, it means the 2nd copy, it can be real copy for RAID1/RAID10, or
>> rebuilt data from RAID5/6.
>>
>> And in fact Liu Bo is adding new copy numbers for RAID5/6 to allow us to
>> specify how to build the missing data for RAID6.
>>
>> So here starting from mirror 1 is the correct behavior.
> 
> Shouldn't those be documented?

Makes sense.

I'll add such comment for both kernel and btrfs-progs.

Thanks,
Quuu

> 
>>
>> Thanks,
>> Qu
>>>
>>> Thanks,
>>> Su
>>>> +            read_len = num_bytes - offset;
>>>> +            /* read as much space once a time */
>>>> +            ret = read_extent_data(fs_info, data + offset,
>>>> +                    bytenr + offset, &read_len, mirror);
>>>> +            if (ret)
>>>> +                goto out;
>>>> +
>>>> +            data_checked = 0;
>>>> +            /* verify every 4k data's checksum */
>>>> +            while (data_checked < read_len) {
>>>> +                csum = ~(u32)0;
>>>> +                tmp = offset + data_checked;
>>>> +
>>>> +                csum = btrfs_csum_data((char *)data + tmp,
>>>> +                        csum, fs_info->sectorsize);
>>>> +                btrfs_csum_final(csum, (u8 *)&csum);
>>>> +
>>>> +                csum_offset = leaf_offset +
>>>> +                     tmp / fs_info->sectorsize * csum_size;
>>>> +                read_extent_buffer(eb, (char *)&csum_expected,
>>>> +                           csum_offset, csum_size);
>>>> +                if (csum != csum_expected)
>>>> +                    fprintf(stderr,
>>>> +            "mirror %d bytenr %llu csum %u expected csum %u\n",
>>>>                           mirror, bytenr + tmp,
>>>>                           csum, csum_expected);
>>>> -                num_copies = btrfs_num_copies(root->fs_info,
>>>> -                        bytenr, num_bytes);
>>>> -                if (mirror < num_copies - 1) {
>>>> -                    mirror += 1;
>>>> -                    goto again;
>>>> -                }
>>>> +                data_checked += fs_info->sectorsize;
>>>>               }
>>>> -            data_checked += fs_info->sectorsize;
>>>>           }
>>>>           offset += read_len;
>>>>       }
>>>> @@ -5624,7 +5619,11 @@ static int check_csums(struct btrfs_root *root)
>>>>           leaf_offset = btrfs_item_ptr_offset(leaf, path.slots[0]);
>>>>           ret = check_extent_csums(root, key.offset, data_len,
>>>>                        leaf_offset, leaf);
>>>> -        if (ret)
>>>> +        /*
>>>> +         * Only break for fatal errors, if mismatch is found,
>>>> +         * continue checking until all extents are checked.
>>>> +         */
>>>> +        if (ret < 0)
>>>>               break;
>>>>   skip_csum_check:
>>>>           if (!num_bytes) {
>>>>
>>>
>>>
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 1/4] btrfs-progs: check: Check data csum for all copies

[Su Yue]

On 02/27/2018 06:31 PM, Qu Wenruo wrote:
> 
> 
> On 2018年02月27日 18:01, Su Yue wrote:
>>
>>
>> On 02/27/2018 05:12 PM, Qu Wenruo wrote:
>>> Original --check-data-csum option will skip the extra copy if the first
>>> copy matches csum.
>>>
>>> Since offline scrub (with recoverability report) is still out-of-tree, at
>>> least enhance --check-data-csum option to handle multiple copies.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>    check/main.c | 65
>>> ++++++++++++++++++++++++++++++------------------------------
>>>    1 file changed, 32 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/check/main.c b/check/main.c
>>> index 97baae583f04..f25fdc765d63 100644
>>> --- a/check/main.c
>>> +++ b/check/main.c
>>> @@ -5381,42 +5381,37 @@ static int check_extent_csums(struct
>>> btrfs_root *root, u64 bytenr,
>>>        if (!data)
>>>            return -ENOMEM;
>>>    +    num_copies = btrfs_num_copies(root->fs_info, bytenr, num_bytes);
>>>        while (offset < num_bytes) {
>>> -        mirror = 0;
>>> -again:
>>> -        read_len = num_bytes - offset;
>>> -        /* read as much space once a time */
>>> -        ret = read_extent_data(fs_info, data + offset,
>>> -                bytenr + offset, &read_len, mirror);
>>> -        if (ret)
>>> -            goto out;
>>> -        data_checked = 0;
>>> -        /* verify every 4k data's checksum */
>>> -        while (data_checked < read_len) {
>>> -            csum = ~(u32)0;
>>> -            tmp = offset + data_checked;
>>> -
>>> -            csum = btrfs_csum_data((char *)data + tmp,
>>> -                           csum, fs_info->sectorsize);
>>> -            btrfs_csum_final(csum, (u8 *)&csum);
>>> -
>>> -            csum_offset = leaf_offset +
>>> -                 tmp / fs_info->sectorsize * csum_size;
>>> -            read_extent_buffer(eb, (char *)&csum_expected,
>>> -                       csum_offset, csum_size);
>>> -            /* try another mirror */
>>> -            if (csum != csum_expected) {
>>> -                fprintf(stderr, "mirror %d bytenr %llu csum %u
>>> expected csum %u\n",
>>> +        for (mirror = 1; mirror <= num_copies; mirror++) {
>>
>> Got your point.
>> But what confuses me is that why mirror starts from 1 here.
>> The mirror influences btrfs_map_block() which is not related
>> to this patch though.
> 
> mirror number has different meanings in fact.
> 
> For 0, it means try to read *ANY* valid copy, it can be the copy of a
> duplication, or even rebuilt data from RAID5/6.
> 
> For 1, it means the fist copy. Either the only copy of
> SINGLE/RAID0/RAID5/6, or the first copy of RAID1/10.
> 
> For 2, it means the 2nd copy, it can be real copy for RAID1/RAID10, or
> rebuilt data from RAID5/6.
> 
> And in fact Liu Bo is adding new copy numbers for RAID5/6 to allow us to
> specify how to build the missing data for RAID6.
> 
> So here starting from mirror 1 is the correct behavior.
> 
Make sense.

Thanks for your explanation！
Su

> Thanks,
> Qu
>>
>> Thanks,
>> Su
>>> +            read_len = num_bytes - offset;
>>> +            /* read as much space once a time */
>>> +            ret = read_extent_data(fs_info, data + offset,
>>> +                    bytenr + offset, &read_len, mirror);
>>> +            if (ret)
>>> +                goto out;
>>> +
>>> +            data_checked = 0;
>>> +            /* verify every 4k data's checksum */
>>> +            while (data_checked < read_len) {
>>> +                csum = ~(u32)0;
>>> +                tmp = offset + data_checked;
>>> +
>>> +                csum = btrfs_csum_data((char *)data + tmp,
>>> +                        csum, fs_info->sectorsize);
>>> +                btrfs_csum_final(csum, (u8 *)&csum);
>>> +
>>> +                csum_offset = leaf_offset +
>>> +                     tmp / fs_info->sectorsize * csum_size;
>>> +                read_extent_buffer(eb, (char *)&csum_expected,
>>> +                           csum_offset, csum_size);
>>> +                if (csum != csum_expected)
>>> +                    fprintf(stderr,
>>> +            "mirror %d bytenr %llu csum %u expected csum %u\n",
>>>                            mirror, bytenr + tmp,
>>>                            csum, csum_expected);
>>> -                num_copies = btrfs_num_copies(root->fs_info,
>>> -                        bytenr, num_bytes);
>>> -                if (mirror < num_copies - 1) {
>>> -                    mirror += 1;
>>> -                    goto again;
>>> -                }
>>> +                data_checked += fs_info->sectorsize;
>>>                }
>>> -            data_checked += fs_info->sectorsize;
>>>            }
>>>            offset += read_len;
>>>        }
>>> @@ -5624,7 +5619,11 @@ static int check_csums(struct btrfs_root *root)
>>>            leaf_offset = btrfs_item_ptr_offset(leaf, path.slots[0]);
>>>            ret = check_extent_csums(root, key.offset, data_len,
>>>                         leaf_offset, leaf);
>>> -        if (ret)
>>> +        /*
>>> +         * Only break for fatal errors, if mismatch is found,
>>> +         * continue checking until all extents are checked.
>>> +         */
>>> +        if (ret < 0)
>>>                break;
>>>    skip_csum_check:
>>>            if (!num_bytes) {
>>>
>>
>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 0/4] btrfs check --check-data-csum enhancement for

[David Sterba]

On Tue, Feb 27, 2018 at 05:12:55PM +0800, Qu Wenruo wrote:
> Log-writes tool from Josef exposed btrfs data corruption.
> 
> Although still digging, at least enhance --check-data-csum option so
> that log-replay --check can stop exactly at the point where btrfs data
> get corrupted, instead of manually checking the output.
> 
> Qu Wenruo (4):
>   btrfs-progs: check: Check data csum for all copies
>   btrfs-progs: check: Fix data csum check return value
>   btrfs-progs: check: Continue check even csum error is found
>   btrfs-progs: check: Distingusih csum checking output for
>     --check-data-csum

Added to devel, thanks.

Re: [PATCH 0/4] btrfs check --check-data-csum enhancement for

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 833 bytes --]

Gentle ping.

Or do I need to rebase the patchset and resend?

Thanks,
Qu

On 2018年02月27日 17:12, Qu Wenruo wrote:
> Log-writes tool from Josef exposed btrfs data corruption.
> 
> Although still digging, at least enhance --check-data-csum option so
> that log-replay --check can stop exactly at the point where btrfs data
> get corrupted, instead of manually checking the output.
> 
> Qu Wenruo (4):
>   btrfs-progs: check: Check data csum for all copies
>   btrfs-progs: check: Fix data csum check return value
>   btrfs-progs: check: Continue check even csum error is found
>   btrfs-progs: check: Distingusih csum checking output for
>     --check-data-csum
> 
>  check/main.c | 93 ++++++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 56 insertions(+), 37 deletions(-)
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/4] btrfs check --check-data-csum enhancement for

[David Sterba]

On Mon, May 28, 2018 at 08:20:35PM +0800, Qu Wenruo wrote:
> Or do I need to rebase the patchset and resend?

The patches are in devel for a long time. If you have any updates,
please send as incremental patches, thanks.