Re: [PATCH v2] btrfs: factor our read/write stage off csum_tree_block() into its callers

[Nikolay Borisov <nborisov@suse.com>]

On 19.02.19 г. 12:58 ч., 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@suse.com>
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> 
> ---
> Changes to v1:
> - return error from csum_tree_buffer() in csum_dirty_buffer() instead of
>   EUCLEAN (Nikolay)
> ---
>  fs/btrfs/disk-io.c | 53 +++++++++++++++++++++++++++++------------------------
>  1 file changed, 29 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 5216e7b3f9ad..56f5fe732be5 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,7 +513,10 @@ 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;
> +	int ret = 0;
>  
>  	eb = (struct extent_buffer *)page->private;
>  	if (page != eb->pages[0])
> @@ -552,7 +535,12 @@ 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);
> +	ret = csum_tree_block(eb, result);
> +	if (WARN_ON(ret))
> +		return ret;

You are not handling the case where csum-tree_block returns a positive
number. It should be translated to a negative value.

> +
> +	write_extent_buffer(eb, result, 0, csum_size);
> +	return ret;
>  }
>  
>  static int check_tree_block_fsid(struct btrfs_fs_info *fs_info,
> @@ -595,7 +583,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 +632,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
>