On Mon, Feb 25, 2019 at 02:24:15PM +0100, 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>
> Reviewed-by: Qu Wenruo <wqu@xxxxxxxx>
> Reviewed-by: Nikolay Borisov <nborisov@xxxxxxxx>
> Signed-off-by: Johannes Thumshirn <jthumshirn@xxxxxxx>
>
> ---
>
> Changes to v3:
> - Move WARN_ON() from csum_dirty_buffer() into csum_tree_buffer()
>
> Changes to v2:
> - Directly return -EINVAL instead of EUCLEAN
>
> Changes to v1:
> - return error from csum_tree_buffer() in csum_dirty_buffer() instead of
> EUCLEAN (Nikolay)
> ---
> 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..8090e8f4ccc2 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.
The comment does not match the function anymore
> */
> -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)
Formatting
> {
> - 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 (csum_tree_block(eb, result))
> + return -EINVAL;
> +
> + 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];
This should be 'u8'
All of the above fixed and added to 5.2 queue, thanks.
