btrfs: factor our read/write stage off csum_tree_block() into its callers
diff mbox series

Message ID 20190219090202.9715-1-jthumshirn@suse.de
State New
Headers show
Series
  • btrfs: factor our read/write stage off csum_tree_block() into its callers
Related show

Commit Message

Johannes Thumshirn Feb. 19, 2019, 9:02 a.m. UTC
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>
Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 fs/btrfs/disk-io.c | 51 +++++++++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 24 deletions(-)

Comments

Nikolay Borisov Feb. 19, 2019, 9:31 a.m. UTC | #1
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@suse.com>
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>, 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
>
Qu Wenruo Feb. 19, 2019, 9:31 a.m. UTC | #2
On 2019/2/19 下午5: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@suse.com>
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>

Looks good.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>  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;
> +
> +	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
>
Johannes Thumshirn Feb. 19, 2019, 10:04 a.m. UTC | #3
On 19/02/2019 10:31, Nikolay Borisov wrote:
>>  
>> -	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?
> 

I choose -EUCLEAN as it is used in csum_dirty_buffer() before, but yes
agreed, returning the error from csum_tree_block() would be better here.

Patch
diff mbox series

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