On 19.02.19 г. 11:02 ч., Johannes Thumshirn wrote:
> Currently csum_tree_block() does two things, first it as it's name
> suggests it calculates the checksum for a tree-block. But it also writes
> this checksum to disk or reads an extent_buffer from disk and compares the
> checksum with the calculated checksum, depending on the verify argument.
>
> Furthermore one of the two callers passes in '1' for the verify argument,
> the other one passes in '0'.
>
> For clarity and less layering violations, factor out the second stage in
> csum_tree_block()'s callers.
>
> Suggested-by: Nikolay Borisov <nborisov@xxxxxxxx>
> Signed-off-by: Johannes Thumshirn <jthumshirn@xxxxxxx>
Reviewed-by: Nikolay Borisov <nborisov@xxxxxxxx>, albeit see my remarks
below.
> ---
> fs/btrfs/disk-io.c | 51 +++++++++++++++++++++++++++------------------------
> 1 file changed, 27 insertions(+), 24 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 5216e7b3f9ad..5330123b0fe3 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -263,12 +263,9 @@ void btrfs_csum_final(u32 crc, u8 *result)
> * compute the csum for a btree block, and either verify it or write it
> * into the csum field of the block.
> */
> -static int csum_tree_block(struct btrfs_fs_info *fs_info,
> - struct extent_buffer *buf,
> - int verify)
> +static int csum_tree_block(struct extent_buffer *buf,
> + u8 *result)
> {
> - u16 csum_size = btrfs_super_csum_size(fs_info->super_copy);
> - char result[BTRFS_CSUM_SIZE];
> unsigned long len;
> unsigned long cur_len;
> unsigned long offset = BTRFS_CSUM_SIZE;
> @@ -300,23 +297,6 @@ static int csum_tree_block(struct btrfs_fs_info *fs_info,
>
> btrfs_csum_final(crc, result);
>
> - if (verify) {
> - if (memcmp_extent_buffer(buf, result, 0, csum_size)) {
> - u32 val;
> - u32 found = 0;
> - memcpy(&found, result, csum_size);
> -
> - read_extent_buffer(buf, &val, 0, csum_size);
> - btrfs_warn_rl(fs_info,
> - "%s checksum verify failed on %llu wanted %X found %X level %d",
> - fs_info->sb->s_id, buf->start,
> - val, found, btrfs_header_level(buf));
> - return -EUCLEAN;
> - }
> - } else {
> - write_extent_buffer(buf, result, 0, csum_size);
> - }
> -
> return 0;
> }
>
> @@ -533,6 +513,8 @@ static int csum_dirty_buffer(struct btrfs_fs_info *fs_info, struct page *page)
> {
> u64 start = page_offset(page);
> u64 found_start;
> + u8 result[BTRFS_CSUM_SIZE];
> + u16 csum_size = btrfs_super_csum_size(fs_info->super_copy);
> struct extent_buffer *eb;
>
> eb = (struct extent_buffer *)page->private;
> @@ -552,7 +534,11 @@ static int csum_dirty_buffer(struct btrfs_fs_info *fs_info, struct page *page)
> ASSERT(memcmp_extent_buffer(eb, fs_info->fs_devices->metadata_uuid,
> btrfs_header_fsid(), BTRFS_FSID_SIZE) == 0);
>
> - return csum_tree_block(fs_info, eb, 0);
> + if (WARN_ON(csum_tree_block(eb, result)))
> + return -EUCLEAN;
I meant to discuss this with you yesterday but the WARN_ON here could
trigger only if map_private_extent_buffer fails. And that could be only
due to two things:
1. If we want to map more than the buffer's length
2. If the item spans two pages.
Initially I wondered whether WARN_ON is really appropriate but the more
I think about it the more I'm inclined to say yes. However, I wonder if
EUCLEAN is the correct error code and shouldn't it be, perhaps, -EINVAL
(as returned from map_private_extent_buffer), that fits both with the
return code in case of invalid mapping length as well as item spanning
multiple pages. What are your thoughts?
> +
> + write_extent_buffer(eb, result, 0, csum_size);
> + return 0;
> }
>
> static int check_tree_block_fsid(struct btrfs_fs_info *fs_info,
> @@ -595,7 +581,9 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
> struct extent_buffer *eb;
> struct btrfs_root *root = BTRFS_I(page->mapping->host)->root;
> struct btrfs_fs_info *fs_info = root->fs_info;
> + u16 csum_size = btrfs_super_csum_size(fs_info->super_copy);
> int ret = 0;
> + char result[BTRFS_CSUM_SIZE];
> int reads_done;
>
> if (!page->private)
> @@ -642,10 +630,25 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
> btrfs_set_buffer_lockdep_class(btrfs_header_owner(eb),
> eb, found_level);
>
> - ret = csum_tree_block(fs_info, eb, 1);
> + ret = csum_tree_block(eb, result);
> if (ret)
> goto err;
>
> + if (memcmp_extent_buffer(eb, result, 0, csum_size)) {
> + u32 val;
> + u32 found = 0;
> +
> + memcpy(&found, result, csum_size);
> +
> + read_extent_buffer(eb, &val, 0, csum_size);
> + btrfs_warn_rl(fs_info,
> + "%s checksum verify failed on %llu wanted %x found %x level %d",
> + fs_info->sb->s_id, eb->start,
> + val, found, btrfs_header_level(eb));
> + ret = -EUCLEAN;
> + goto err;
> + }
> +
> /*
> * If this is a leaf block and it is corrupt, set the corrupt bit so
> * that we don't try and read the other copies of this block, just
>
