On 2019/3/20 上午8:46, Qu Wenruo wrote: > > > On 2019/3/19 下午10:50, David Sterba wrote: >> On Wed, Mar 13, 2019 at 04:55:06PM +0800, Qu Wenruo wrote: >>> We already have btrfs_check_chunk_valid() to verify each chunk before >>> tree-checker. >>> >>> Merge that function into tree-checker, and update its error message to >>> be more readable. >>> >>> Old error message would be something like: >>> BTRFS error (device dm-3): invalid chunk num_stipres: 0 >>> >>> New error message would be: >>> Btrfs critical (device dm-3): corrupt superblock syschunk array: >>> chunk_start=2097152, invalid chunk num_stripes: 0 >>> Or >>> Btrfs critical (device dm-3): corrupt leaf: root=3 block=8388608 slot=3 >>> chunk_start=2097152, invalid chunk num_stripes: 0 >>> >>> Btrfs_check_chunk_valid() is exported for super block syschunk array >>> verification. >>> >>> Also make tree-checker to verify chunk items, this makes chunk item >>> checker covers all chunks and avoid fuzzed image. >>> >>> Reported-by: Yoon Jungyeon <[email protected]> >>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=202751 >>> Signed-off-by: Qu Wenruo <[email protected]> >>> --- >>> fs/btrfs/tree-checker.c | 152 ++++++++++++++++++++++++++++++++++++++++ >>> fs/btrfs/tree-checker.h | 3 + >>> fs/btrfs/volumes.c | 94 +------------------------ >>> 3 files changed, 156 insertions(+), 93 deletions(-) >> >> Please split the patch to part where you just move the code and where >> the logic is changed. btrfs_check_chunk_valid is not the same, old has >> -EIO and new -EUCLEAN. Moving a function is ok in the same patch if >> there's no change. > > There is change in the verification timing by just moving the code to > tree checker.
My bad, I though the code move and verification timing must be done in
one patch, and that's definitely wrong.
I'll split the code move and logic change.
Thanks,
Qu
>
> The new timing of chunk verification will make btrfs more robust by
> trying the other copy when content doesn't make sense.
>
> In fact the code move itself would solve the bug in the kernel bugzilla.
>
> So It doesn't make that much sense to split the patch.
>
> Thanks,
> Qu
>
>>
>>>
>>> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
>>> index b8cdaf472031..fe3f37c23c29 100644
>>> --- a/fs/btrfs/tree-checker.c
>>> +++ b/fs/btrfs/tree-checker.c
>>> @@ -448,6 +448,152 @@ static int check_block_group_item(struct
>>> btrfs_fs_info *fs_info,
>>> return 0;
>>> }
>>>
>>> +__printf(5, 6)
>>> +__cold
>>> +static void chunk_err(const struct btrfs_fs_info *fs_info,
>>> + const struct extent_buffer *leaf,
>>> + const struct btrfs_chunk *chunk, u64 logical,
>>> + const char *fmt, ...)
>>> +{
>>> + /* Only superblock eb is able to have such small offset */
>>> + bool is_sb = (leaf->start == BTRFS_SUPER_INFO_OFFSET);
>>
>> Please move the initialization and comment out of the declaration block
>>
>>> + struct va_format vaf;
>>> + va_list args;
>>> + int i;
>>> + int slot = -1;
>>> +
>>> + if (!is_sb) {
>>> + /*
>>> + * Get the slot number by iterating through all slots, this
>>> + * would provide better readability.
>>> + */
>>> + for (i = 0; i < btrfs_header_nritems(leaf); i++) {
>>> + if (btrfs_item_ptr_offset(leaf, i) ==
>>> + (unsigned long) chunk) {
>>> + slot = i;
>>> + break;
>>> + }
>>> + }
>>> + }
>
signature.asc
Description: OpenPGP digital signature
