[PATCH 0/4] Misc cleanups

[PATCH 0/4] Misc cleanups

[David Sterba]

A few more cleanups that needed more testing.

David Sterba (4):
  btrfs: inline checksum name and driver definitions
  btrfs: simplify tree block checksumming loop
  btrfs: return void from csum_tree_block
  btrfs: balance: factor out convert profile validation

 fs/btrfs/ctree.c   |  7 ++++---
 fs/btrfs/disk-io.c | 45 +++++++++++----------------------------------
 fs/btrfs/volumes.c | 45 +++++++++++++++++++++------------------------
 3 files changed, 36 insertions(+), 61 deletions(-)

-- 
2.25.0

[PATCH 1/4] btrfs: inline checksum name and driver definitions

[David Sterba]

There's an unnecessary indirection in the checksum definition table,
pointer and the string itself. The strings are short and the overall
size of one entry is now 24 bytes.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index f948435e87df..bfedbbe2311f 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -31,8 +31,8 @@ static void del_ptr(struct btrfs_root *root, struct btrfs_path *path,
 
 static const struct btrfs_csums {
 	u16		size;
-	const char	*name;
-	const char	*driver;
+	const char	name[10];
+	const char	driver[12];
 } btrfs_csums[] = {
 	[BTRFS_CSUM_TYPE_CRC32] = { .size = 4, .name = "crc32c" },
 	[BTRFS_CSUM_TYPE_XXHASH] = { .size = 8, .name = "xxhash64" },
@@ -63,7 +63,8 @@ const char *btrfs_super_csum_name(u16 csum_type)
 const char *btrfs_super_csum_driver(u16 csum_type)
 {
 	/* csum type is validated at mount time */
-	return btrfs_csums[csum_type].driver ?:
+	return btrfs_csums[csum_type].driver[0] ?
+		btrfs_csums[csum_type].driver :
 		btrfs_csums[csum_type].name;
 }
 
-- 
2.25.0

Re: [PATCH 1/4] btrfs: inline checksum name and driver definitions

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 1/4] btrfs: inline checksum name and driver definitions

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1509 bytes --]



On 2020/2/28 上午4:00, David Sterba wrote:
> There's an unnecessary indirection in the checksum definition table,
> pointer and the string itself. The strings are short and the overall
> size of one entry is now 24 bytes.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/ctree.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index f948435e87df..bfedbbe2311f 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -31,8 +31,8 @@ static void del_ptr(struct btrfs_root *root, struct btrfs_path *path,
>  
>  static const struct btrfs_csums {
>  	u16		size;
> -	const char	*name;
> -	const char	*driver;
> +	const char	name[10];

Just a nitpick, the longest name I haven seen is "xxhash64" which is
only 8 chars, +1 for '\n'.
Thus we can save one extra byte here.

Despite that.

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

Thanks,
Qu
> +	const char	driver[12];
>  } btrfs_csums[] = {
>  	[BTRFS_CSUM_TYPE_CRC32] = { .size = 4, .name = "crc32c" },
>  	[BTRFS_CSUM_TYPE_XXHASH] = { .size = 8, .name = "xxhash64" },
> @@ -63,7 +63,8 @@ const char *btrfs_super_csum_name(u16 csum_type)
>  const char *btrfs_super_csum_driver(u16 csum_type)
>  {
>  	/* csum type is validated at mount time */
> -	return btrfs_csums[csum_type].driver ?:
> +	return btrfs_csums[csum_type].driver[0] ?
> +		btrfs_csums[csum_type].driver :
>  		btrfs_csums[csum_type].name;
>  }
>  
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 2/4] btrfs: simplify tree block checksumming loop

[David Sterba]

Thw whole point of csum_tree_block is to iterate over all extent buffer
pages and pass it to checksumming functions. The bytes where checksum is
stored must be skipped, thus map_private_extent_buffer. This complicates
further offset calculations.

As the first page will be always present, checksum the relevant bytes
unconditionally and then do a simple iteration over the remaining pages.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/disk-io.c | 32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 3952e4a2f3d7..5f74eb69f2fe 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -259,38 +259,22 @@ struct extent_map *btree_get_extent(struct btrfs_inode *inode,
 static int csum_tree_block(struct extent_buffer *buf, u8 *result)
 {
 	struct btrfs_fs_info *fs_info = buf->fs_info;
+	const int num_pages = fs_info->nodesize >> PAGE_SHIFT;
 	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
-	unsigned long len;
-	unsigned long cur_len;
-	unsigned long offset = BTRFS_CSUM_SIZE;
 	char *kaddr;
-	unsigned long map_start;
-	unsigned long map_len;
-	int err;
+	int i;
 
 	shash->tfm = fs_info->csum_shash;
 	crypto_shash_init(shash);
+	kaddr = page_address(buf->pages[0]);
+	crypto_shash_update(shash, kaddr + BTRFS_CSUM_SIZE,
+			    PAGE_SIZE - BTRFS_CSUM_SIZE);
 
-	len = buf->len - offset;
-
-	while (len > 0) {
-		/*
-		 * Note: we don't need to check for the err == 1 case here, as
-		 * with the given combination of 'start = BTRFS_CSUM_SIZE (32)'
-		 * and 'min_len = 32' and the currently implemented mapping
-		 * algorithm we cannot cross a page boundary.
-		 */
-		err = map_private_extent_buffer(buf, offset, 32,
-					&kaddr, &map_start, &map_len);
-		if (WARN_ON(err))
-			return err;
-		cur_len = min(len, map_len - (offset - map_start));
-		crypto_shash_update(shash, kaddr + offset - map_start, cur_len);
-		len -= cur_len;
-		offset += cur_len;
+	for (i = 1; i < num_pages; i++) {
+		kaddr = page_address(buf->pages[i]);
+		crypto_shash_update(shash, kaddr, PAGE_SIZE);
 	}
 	memset(result, 0, BTRFS_CSUM_SIZE);
-
 	crypto_shash_final(shash, result);
 
 	return 0;
-- 
2.25.0

Re: [PATCH 2/4] btrfs: simplify tree block checksumming loop

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 2/4] btrfs: simplify tree block checksumming loop

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2543 bytes --]



On 2020/2/28 上午4:00, David Sterba wrote:
> Thw whole point of csum_tree_block is to iterate over all extent buffer
> pages and pass it to checksumming functions. The bytes where checksum is
> stored must be skipped, thus map_private_extent_buffer. This complicates
> further offset calculations.
> 
> As the first page will be always present, checksum the relevant bytes
> unconditionally and then do a simple iteration over the remaining pages.

The new operation looks much better.

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

Thanks,
Qu

> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/disk-io.c | 32 ++++++++------------------------
>  1 file changed, 8 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 3952e4a2f3d7..5f74eb69f2fe 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -259,38 +259,22 @@ struct extent_map *btree_get_extent(struct btrfs_inode *inode,
>  static int csum_tree_block(struct extent_buffer *buf, u8 *result)
>  {
>  	struct btrfs_fs_info *fs_info = buf->fs_info;
> +	const int num_pages = fs_info->nodesize >> PAGE_SHIFT;
>  	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
> -	unsigned long len;
> -	unsigned long cur_len;
> -	unsigned long offset = BTRFS_CSUM_SIZE;
>  	char *kaddr;
> -	unsigned long map_start;
> -	unsigned long map_len;
> -	int err;
> +	int i;
>  
>  	shash->tfm = fs_info->csum_shash;
>  	crypto_shash_init(shash);
> +	kaddr = page_address(buf->pages[0]);
> +	crypto_shash_update(shash, kaddr + BTRFS_CSUM_SIZE,
> +			    PAGE_SIZE - BTRFS_CSUM_SIZE);
>  
> -	len = buf->len - offset;
> -
> -	while (len > 0) {
> -		/*
> -		 * Note: we don't need to check for the err == 1 case here, as
> -		 * with the given combination of 'start = BTRFS_CSUM_SIZE (32)'
> -		 * and 'min_len = 32' and the currently implemented mapping
> -		 * algorithm we cannot cross a page boundary.
> -		 */
> -		err = map_private_extent_buffer(buf, offset, 32,
> -					&kaddr, &map_start, &map_len);
> -		if (WARN_ON(err))
> -			return err;
> -		cur_len = min(len, map_len - (offset - map_start));
> -		crypto_shash_update(shash, kaddr + offset - map_start, cur_len);
> -		len -= cur_len;
> -		offset += cur_len;
> +	for (i = 1; i < num_pages; i++) {
> +		kaddr = page_address(buf->pages[i]);
> +		crypto_shash_update(shash, kaddr, PAGE_SIZE);
>  	}
>  	memset(result, 0, BTRFS_CSUM_SIZE);
> -
>  	crypto_shash_final(shash, result);
>  
>  	return 0;
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 3/4] btrfs: return void from csum_tree_block

[David Sterba]

Now that csum_tree_block is not returning any errors, we can make
csum_tree_block return void and simplify callers.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/disk-io.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 5f74eb69f2fe..8401852cf9c0 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -253,10 +253,8 @@ struct extent_map *btree_get_extent(struct btrfs_inode *inode,
 
 /*
  * Compute the csum of a btree block and store the result to provided buffer.
- *
- * Returns error if the extent buffer cannot be mapped.
  */
-static int csum_tree_block(struct extent_buffer *buf, u8 *result)
+static void csum_tree_block(struct extent_buffer *buf, u8 *result)
 {
 	struct btrfs_fs_info *fs_info = buf->fs_info;
 	const int num_pages = fs_info->nodesize >> PAGE_SHIFT;
@@ -276,8 +274,6 @@ static int csum_tree_block(struct extent_buffer *buf, u8 *result)
 	}
 	memset(result, 0, BTRFS_CSUM_SIZE);
 	crypto_shash_final(shash, result);
-
-	return 0;
 }
 
 /*
@@ -528,8 +524,7 @@ static int csum_dirty_buffer(struct btrfs_fs_info *fs_info, struct page *page)
 				    offsetof(struct btrfs_header, fsid),
 				    BTRFS_FSID_SIZE) == 0);
 
-	if (csum_tree_block(eb, result))
-		return -EINVAL;
+	csum_tree_block(eb, result);
 
 	if (btrfs_header_level(eb))
 		ret = btrfs_check_node(eb);
@@ -640,9 +635,7 @@ 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(eb, result);
-	if (ret)
-		goto err;
+	csum_tree_block(eb, result);
 
 	if (memcmp_extent_buffer(eb, result, 0, csum_size)) {
 		u32 val;
-- 
2.25.0

Re: [PATCH 3/4] btrfs: return void from csum_tree_block

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 3/4] btrfs: return void from csum_tree_block

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2009 bytes --]



On 2020/2/28 上午4:00, David Sterba wrote:
> Now that csum_tree_block is not returning any errors, we can make
> csum_tree_block return void and simplify callers.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

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

Thanks,
Qu

> ---
>  fs/btrfs/disk-io.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 5f74eb69f2fe..8401852cf9c0 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -253,10 +253,8 @@ struct extent_map *btree_get_extent(struct btrfs_inode *inode,
>  
>  /*
>   * Compute the csum of a btree block and store the result to provided buffer.
> - *
> - * Returns error if the extent buffer cannot be mapped.
>   */
> -static int csum_tree_block(struct extent_buffer *buf, u8 *result)
> +static void csum_tree_block(struct extent_buffer *buf, u8 *result)
>  {
>  	struct btrfs_fs_info *fs_info = buf->fs_info;
>  	const int num_pages = fs_info->nodesize >> PAGE_SHIFT;
> @@ -276,8 +274,6 @@ static int csum_tree_block(struct extent_buffer *buf, u8 *result)
>  	}
>  	memset(result, 0, BTRFS_CSUM_SIZE);
>  	crypto_shash_final(shash, result);
> -
> -	return 0;
>  }
>  
>  /*
> @@ -528,8 +524,7 @@ static int csum_dirty_buffer(struct btrfs_fs_info *fs_info, struct page *page)
>  				    offsetof(struct btrfs_header, fsid),
>  				    BTRFS_FSID_SIZE) == 0);
>  
> -	if (csum_tree_block(eb, result))
> -		return -EINVAL;
> +	csum_tree_block(eb, result);
>  
>  	if (btrfs_header_level(eb))
>  		ret = btrfs_check_node(eb);
> @@ -640,9 +635,7 @@ 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(eb, result);
> -	if (ret)
> -		goto err;
> +	csum_tree_block(eb, result);
>  
>  	if (memcmp_extent_buffer(eb, result, 0, csum_size)) {
>  		u32 val;
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 4/4] btrfs: balance: factor out convert profile validation

[David Sterba]

The validation follows the same steps for all three block group types,
the existing helper validate_convert_profile can be enhanced and do more
of the common things.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/volumes.c | 45 +++++++++++++++++++++------------------------
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 3d35466f34b0..b5d7dc561b68 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3762,13 +3762,25 @@ static inline int balance_need_close(struct btrfs_fs_info *fs_info)
 		 atomic_read(&fs_info->balance_cancel_req) == 0);
 }
 
-/* Non-zero return value signifies invalidity */
-static inline int validate_convert_profile(struct btrfs_balance_args *bctl_arg,
-		u64 allowed)
+/*
+ * Validate target profile against allowed profiles and return true if it's OK.
+ * Otherwise print the error message and return false.
+ */
+static inline int validate_convert_profile(struct btrfs_fs_info *fs_info,
+		const struct btrfs_balance_args *bargs,
+		u64 allowed, const char *type)
 {
-	return ((bctl_arg->flags & BTRFS_BALANCE_ARGS_CONVERT) &&
-		(!alloc_profile_is_valid(bctl_arg->target, 1) ||
-		 (bctl_arg->target & ~allowed)));
+	if (!(bargs->flags & BTRFS_BALANCE_ARGS_CONVERT))
+		return true;
+
+	/* Profile is valid and does not have bits outside of the allowed set */
+	if (alloc_profile_is_valid(bargs->target, 1) &&
+	    (bargs->target & ~allowed) == 0)
+		return true;
+
+	btrfs_err(fs_info, "balance: invalid convert %s profile %s",
+			type, btrfs_bg_type_to_raid_name(bargs->target));
+	return false;
 }
 
 /*
@@ -3984,24 +3996,9 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 		if (num_devices >= btrfs_raid_array[i].devs_min)
 			allowed |= btrfs_raid_array[i].bg_flag;
 
-	if (validate_convert_profile(&bctl->data, allowed)) {
-		btrfs_err(fs_info,
-			  "balance: invalid convert data profile %s",
-			  btrfs_bg_type_to_raid_name(bctl->data.target));
-		ret = -EINVAL;
-		goto out;
-	}
-	if (validate_convert_profile(&bctl->meta, allowed)) {
-		btrfs_err(fs_info,
-			  "balance: invalid convert metadata profile %s",
-			  btrfs_bg_type_to_raid_name(bctl->meta.target));
-		ret = -EINVAL;
-		goto out;
-	}
-	if (validate_convert_profile(&bctl->sys, allowed)) {
-		btrfs_err(fs_info,
-			  "balance: invalid convert system profile %s",
-			  btrfs_bg_type_to_raid_name(bctl->sys.target));
+	if (!validate_convert_profile(fs_info, &bctl->data, allowed, "data") ||
+	    !validate_convert_profile(fs_info, &bctl->meta, allowed, "metadata") ||
+	    !validate_convert_profile(fs_info, &bctl->sys,  allowed, "system")) {
 		ret = -EINVAL;
 		goto out;
 	}
-- 
2.25.0

Re: [PATCH 4/4] btrfs: balance: factor out convert profile validation

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 4/4] btrfs: balance: factor out convert profile validation

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 3009 bytes --]



On 2020/2/28 上午4:00, David Sterba wrote:
> The validation follows the same steps for all three block group types,
> the existing helper validate_convert_profile can be enhanced and do more
> of the common things.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

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

Thanks,
Qu
> ---
>  fs/btrfs/volumes.c | 45 +++++++++++++++++++++------------------------
>  1 file changed, 21 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 3d35466f34b0..b5d7dc561b68 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3762,13 +3762,25 @@ static inline int balance_need_close(struct btrfs_fs_info *fs_info)
>  		 atomic_read(&fs_info->balance_cancel_req) == 0);
>  }
>  
> -/* Non-zero return value signifies invalidity */
> -static inline int validate_convert_profile(struct btrfs_balance_args *bctl_arg,
> -		u64 allowed)
> +/*
> + * Validate target profile against allowed profiles and return true if it's OK.
> + * Otherwise print the error message and return false.
> + */
> +static inline int validate_convert_profile(struct btrfs_fs_info *fs_info,
> +		const struct btrfs_balance_args *bargs,
> +		u64 allowed, const char *type)
>  {
> -	return ((bctl_arg->flags & BTRFS_BALANCE_ARGS_CONVERT) &&
> -		(!alloc_profile_is_valid(bctl_arg->target, 1) ||
> -		 (bctl_arg->target & ~allowed)));
> +	if (!(bargs->flags & BTRFS_BALANCE_ARGS_CONVERT))
> +		return true;
> +
> +	/* Profile is valid and does not have bits outside of the allowed set */
> +	if (alloc_profile_is_valid(bargs->target, 1) &&
> +	    (bargs->target & ~allowed) == 0)
> +		return true;
> +
> +	btrfs_err(fs_info, "balance: invalid convert %s profile %s",
> +			type, btrfs_bg_type_to_raid_name(bargs->target));
> +	return false;
>  }
>  
>  /*
> @@ -3984,24 +3996,9 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>  		if (num_devices >= btrfs_raid_array[i].devs_min)
>  			allowed |= btrfs_raid_array[i].bg_flag;
>  
> -	if (validate_convert_profile(&bctl->data, allowed)) {
> -		btrfs_err(fs_info,
> -			  "balance: invalid convert data profile %s",
> -			  btrfs_bg_type_to_raid_name(bctl->data.target));
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -	if (validate_convert_profile(&bctl->meta, allowed)) {
> -		btrfs_err(fs_info,
> -			  "balance: invalid convert metadata profile %s",
> -			  btrfs_bg_type_to_raid_name(bctl->meta.target));
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -	if (validate_convert_profile(&bctl->sys, allowed)) {
> -		btrfs_err(fs_info,
> -			  "balance: invalid convert system profile %s",
> -			  btrfs_bg_type_to_raid_name(bctl->sys.target));
> +	if (!validate_convert_profile(fs_info, &bctl->data, allowed, "data") ||
> +	    !validate_convert_profile(fs_info, &bctl->meta, allowed, "metadata") ||
> +	    !validate_convert_profile(fs_info, &bctl->sys,  allowed, "system")) {
>  		ret = -EINVAL;
>  		goto out;
>  	}
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]