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

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

[Johannes Thumshirn]

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;
+
+	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
-- 
2.16.4

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

[Nikolay Borisov]

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
> 

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

[Johannes Thumshirn]

On 19/02/2019 13:00, Nikolay Borisov wrote:
>> -	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.

Correct, my bad.

But then regarding -EINVAL vs. -EUNCLEAN. In btree_csum_one_bio() the
return of csum_dirty_buffer() gets fed into errno_to_blk_status(), which
translates the errno into the block layer's blk_status_t values. As
neither EINVAL nor EUCLEAN are in the translation list, both would just
be translated to the default BLK_STS_IOERR which is the equivalent of EIO.

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850