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 <jungy...@gatech.edu> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=202751 >> Signed-off-by: Qu Wenruo <w...@suse.com> >> --- >> 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.
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
