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 <jungyeon@xxxxxxxxxx> >>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=202751 >>> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx> >>> --- >>> 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; >>> + } >>> + } >>> + } >
Attachment:
signature.asc
Description: OpenPGP digital signature
