btrfs check --check-data-csum enhancement for
cover btrfs check --check-data-csum enhancement for
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(-)
Signature · 1 line
2.16.2
PATCH 1/4 btrfs-progs: check: Check data csum for all copies
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.
--- check/main.c | 65 ++++++++++++++++++++++++++++++------------------------------ 1 file changed, 32 insertions(+), 33 deletions(-)
check/main.c +32 -33
@@ -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) {
Signature · 1 line
2.16.2
Re: [PATCH 1/4] btrfs-progs: check: Check data csum for all copies
Quoted 37 lines · 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 <[email protected]>--- check/main.c | 65 ++++++++++++++++++++++++++++++------------------------------ 1 file changed, 32 insertions(+), 33 deletions(-)check/main.c +1
-16@@ -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);
check/main.c static int check_extent_csums(struct btrfs_root *root, u64 bytenr, · line 5386
Quoted 10 lines
check/main.c +1
-9-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
Quoted 52 lines
check/main.c +30
-8+ 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
Quoted 49 lines · Su Yue
Quoted 46 lines · 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 <[email protected]>---
check/main.c| 65
++++++++++++++++++++++++++++++------------------------------1 file changed, 32 insertions(+), 33 deletions(-)check/main.c +0
-24@@ -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) {
Quoted 8 lines
check/main.c static int check_extent_csums(struct · line 5387
Quoted 3 lines
check/main.c +1
-1- 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 influencesbtrfs_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
Quoted 61 lines
Thanks,
SuQuoted 52 lines
check/main.c +25
-7+ 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;
}check/main.c +5
-1@@ -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) {Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] btrfs-progs: check: Check data csum for all copies
Quoted 69 lines · Qu Wenruo
Quoted 49 lines · Su Yue
Quoted 46 lines · 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 <[email protected]>---
check/main.c| 65
++++++++++++++++++++++++++++++------------------------------1 file changed, 32 insertions(+), 33 deletions(-)check/main.c +0
-24@@ -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) {Quoted 8 lines
check/main.c static int check_extent_csums(struct · line 5387
Quoted 3 lines
check/main.c +1
-1- 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 influencesbtrfs_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.
Quoted 7 lines
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?
Quoted 65 lines
Thanks,
QuQuoted 61 lines
Thanks,
SuQuoted 52 lines
check/main.c +25
-7+ 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;
}check/main.c +5
-1@@ -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) {Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] btrfs-progs: check: Check data csum for all copies
Quoted 75 lines · Nikolay Borisov
Quoted 72 lines · Qu Wenruo
Quoted 49 lines · Su Yue
Quoted 46 lines · 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 <[email protected]>---
check/main.c| 65
++++++++++++++++++++++++++++++------------------------------1 file changed, 32 insertions(+), 33 deletions(-)check/main.c +0
-24@@ -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) {Quoted 8 lines
check/main.c static int check_extent_csums(struct · line 5387
Quoted 3 lines
check/main.c +1
-1- 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 influencesbtrfs_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.
Quoted 6 lines
Quoted 4 lines
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
Quoted 66 lines
Quoted 66 lines
Thanks,
QuQuoted 61 lines
Thanks,
SuQuoted 52 lines
check/main.c +25
-7+ 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;
}check/main.c +5
-1@@ -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) {Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] btrfs-progs: check: Check data csum for all copies
In reply to Qu WenruoQuoted 69 lines · Qu Wenruo
Quoted 49 lines · Su Yue
Quoted 46 lines · 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 <[email protected]>--- check/main.c | 65 ++++++++++++++++++++++++++++++------------------------------ 1 file changed, 32 insertions(+), 33 deletions(-)check/main.c +0
-24@@ -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) {Quoted 8 lines
check/main.c static int check_extent_csums(struct · line 5387
Quoted 3 lines
check/main.c +1
-1- 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 influencesbtrfs_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.
Quoted 8 lines
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
Quoted 64 lines
Thanks,
QuQuoted 61 lines
Thanks,
SuQuoted 52 lines
check/main.c +30
-8+ 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) {Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 2/4 btrfs-progs: check: Fix data csum check return value
In reply to Qu WenruoWhen --check-data-csum option found csum mismatch, btrfs check still return 0.
Fix it so log-replay could automatically pause when found csum error.
--- check/main.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
check/main.c +15 -1
@@ -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;
Signature · 1 line
2.16.2
PATCH 3/4 btrfs-progs: check: Continue check even csum error is found
In reply to Qu WenruoSince data csum is not a fatal error compared to fs/extent trees, continue check.
--- check/main.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
check/main.c +6 -4
@@ -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 */
Signature · 1 line
2.16.2
PATCH 4/4 btrfs-progs: check: Distingusih csum checking output for --check-data-csum
In reply to Qu WenruoNo 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.
--- check/main.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
check/main.c +5 -1
@@ -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
Signature · 1 line
2.16.2
Quoted 6 lines · 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.
Quoted 6 lines
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.
Gentle ping.
Or do I need to rebase the patchset and resend?
Thanks,
Qu
Quoted 16 lines · 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-csumcheck/main.c | 93 ++++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 56 insertions(+), 37 deletions(-)
Quoted 1 line · Qu Wenruo
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.