On 2018年07月16日 21:16, David Sterba wrote: > On Fri, Jul 06, 2018 at 07:44:37AM +0800, Qu Wenruo wrote: >> >> >> On 2018年07月05日 23:18, David Sterba wrote: >>> On Tue, Jul 03, 2018 at 05:10:09PM +0800, Qu Wenruo wrote: >>>> If a crafted btrfs has missing block group items, it could cause >>>> unexpected behavior and breaks our expectation on 1:1 >>>> chunk<->block group mapping. >>>> >>>> Although we added block group -> chunk mapping check, we still need >>>> chunk -> block group mapping check. >>>> >>>> This patch will do extra check to ensure each chunk has its >>>> corresponding block group. >>>> >>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=199847 >>>> Reported-by: Xu Wen <wen...@gatech.edu> >>>> Signed-off-by: Qu Wenruo <w...@suse.com> >>>> --- >>>> fs/btrfs/extent-tree.c | 52 +++++++++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 51 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c >>>> index 82b446f014b9..746095034ca2 100644 >>>> --- a/fs/btrfs/extent-tree.c >>>> +++ b/fs/btrfs/extent-tree.c >>>> @@ -10038,6 +10038,56 @@ static int check_exist_chunk(struct btrfs_fs_info >>>> *fs_info, u64 start, u64 len, >>>> return ret; >>>> } >>>> >>>> +/* >>>> + * Iterate all chunks and verify each of them has corresponding block >>>> group >>>> + */ >>>> +static int check_chunk_block_group_mappings(struct btrfs_fs_info *fs_info) >>>> +{ >>>> + struct btrfs_mapping_tree *map_tree = &fs_info->mapping_tree; >>>> + struct extent_map *em; >>>> + struct btrfs_block_group_cache *bg; >>>> + u64 start = 0; >>>> + int ret = 0; >>>> + >>>> + while (1) { >>>> + read_lock(&map_tree->map_tree.lock); >>>> + em = lookup_extent_mapping(&map_tree->map_tree, start, >>>> + (u64)-1 - start); >>> >>> This needs a comment. >> >> For the @len part? > > Yes, for the expression how it's calculated. > >>> >>>> + read_unlock(&map_tree->map_tree.lock); >>>> + if (!em) >>>> + break; >>>> + >>>> + bg = btrfs_lookup_block_group(fs_info, em->start); >>>> + if (!bg) { >>>> + btrfs_err_rl(fs_info, >>>> + "chunk start=%llu len=%llu doesn't have corresponding block group", >>>> + em->start, em->len); >>>> + ret = -ENOENT; >>> >>> -EUCLEAN ? >> >> Either works for me. > > That's not just a cosmetic change, there's a semantic difference between > the error codes, I maybe make that more explicit and not expect that this > is obvious. > > ENOENT does not make much sense in this context, the caller (mount in > this case) cannot do anything about a code that says 'some internal > structure not found'.
The point here is, if every self-checker should only return -EUCLEAN, it won't really indicate what's going wrong, except points to some self-checker (and such self-checkers are growing larger than our expectation already). My practice here is, put some human readable and meaningful error message. No matter what we choose to return, the error message should tell us what's going wrong. In this case, I don't really care the return value. If it's explicitly needed to return -EUCLEAN, I could make all existing checker (from tree-checker to chunk/bg/dev-extent checker) to return -EUCLEAN if anything is wrong (and save several "ret = -EUCLEAN" lines). The return value doesn't really have much meaning nowadays, it's the error message important now. Thanks, Qu > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
signature.asc
Description: OpenPGP digital signature
