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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- 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 --]