All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] Misc cleanups
@ 2020-02-27 20:00 David Sterba
  2020-02-27 20:00 ` [PATCH 1/4] btrfs: inline checksum name and driver definitions David Sterba
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: David Sterba @ 2020-02-27 20:00 UTC (permalink / raw)
  To: linux-btrfs; +Cc: 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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/4] btrfs: inline checksum name and driver definitions
  2020-02-27 20:00 [PATCH 0/4] Misc cleanups David Sterba
@ 2020-02-27 20:00 ` David Sterba
  2020-02-27 22:31   ` Johannes Thumshirn
  2020-02-28  8:38   ` Qu Wenruo
  2020-02-27 20:00 ` [PATCH 2/4] btrfs: simplify tree block checksumming loop David Sterba
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 15+ messages in thread
From: David Sterba @ 2020-02-27 20:00 UTC (permalink / raw)
  To: linux-btrfs; +Cc: 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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/4] btrfs: simplify tree block checksumming loop
  2020-02-27 20:00 [PATCH 0/4] Misc cleanups David Sterba
  2020-02-27 20:00 ` [PATCH 1/4] btrfs: inline checksum name and driver definitions David Sterba
@ 2020-02-27 20:00 ` David Sterba
  2020-02-27 22:52   ` Johannes Thumshirn
  2020-02-28  8:41   ` Qu Wenruo
  2020-02-27 20:00 ` [PATCH 3/4] btrfs: return void from csum_tree_block David Sterba
  2020-02-27 20:00 ` [PATCH 4/4] btrfs: balance: factor out convert profile validation David Sterba
  3 siblings, 2 replies; 15+ messages in thread
From: David Sterba @ 2020-02-27 20:00 UTC (permalink / raw)
  To: linux-btrfs; +Cc: 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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/4] btrfs: return void from csum_tree_block
  2020-02-27 20:00 [PATCH 0/4] Misc cleanups David Sterba
  2020-02-27 20:00 ` [PATCH 1/4] btrfs: inline checksum name and driver definitions David Sterba
  2020-02-27 20:00 ` [PATCH 2/4] btrfs: simplify tree block checksumming loop David Sterba
@ 2020-02-27 20:00 ` David Sterba
  2020-02-27 22:32   ` Johannes Thumshirn
  2020-02-28  8:41   ` Qu Wenruo
  2020-02-27 20:00 ` [PATCH 4/4] btrfs: balance: factor out convert profile validation David Sterba
  3 siblings, 2 replies; 15+ messages in thread
From: David Sterba @ 2020-02-27 20:00 UTC (permalink / raw)
  To: linux-btrfs; +Cc: 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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 4/4] btrfs: balance: factor out convert profile validation
  2020-02-27 20:00 [PATCH 0/4] Misc cleanups David Sterba
                   ` (2 preceding siblings ...)
  2020-02-27 20:00 ` [PATCH 3/4] btrfs: return void from csum_tree_block David Sterba
@ 2020-02-27 20:00 ` David Sterba
  2020-02-27 22:33   ` Johannes Thumshirn
  2020-02-28  8:43   ` Qu Wenruo
  3 siblings, 2 replies; 15+ messages in thread
From: David Sterba @ 2020-02-27 20:00 UTC (permalink / raw)
  To: linux-btrfs; +Cc: 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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/4] btrfs: inline checksum name and driver definitions
  2020-02-27 20:00 ` [PATCH 1/4] btrfs: inline checksum name and driver definitions David Sterba
@ 2020-02-27 22:31   ` Johannes Thumshirn
  2020-02-28  8:38   ` Qu Wenruo
  1 sibling, 0 replies; 15+ messages in thread
From: Johannes Thumshirn @ 2020-02-27 22:31 UTC (permalink / raw)
  To: David Sterba, linux-btrfs

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/4] btrfs: return void from csum_tree_block
  2020-02-27 20:00 ` [PATCH 3/4] btrfs: return void from csum_tree_block David Sterba
@ 2020-02-27 22:32   ` Johannes Thumshirn
  2020-02-28  8:41   ` Qu Wenruo
  1 sibling, 0 replies; 15+ messages in thread
From: Johannes Thumshirn @ 2020-02-27 22:32 UTC (permalink / raw)
  To: David Sterba, linux-btrfs

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/4] btrfs: balance: factor out convert profile validation
  2020-02-27 20:00 ` [PATCH 4/4] btrfs: balance: factor out convert profile validation David Sterba
@ 2020-02-27 22:33   ` Johannes Thumshirn
  2020-02-28  8:43   ` Qu Wenruo
  1 sibling, 0 replies; 15+ messages in thread
From: Johannes Thumshirn @ 2020-02-27 22:33 UTC (permalink / raw)
  To: David Sterba, linux-btrfs

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/4] btrfs: simplify tree block checksumming loop
  2020-02-27 20:00 ` [PATCH 2/4] btrfs: simplify tree block checksumming loop David Sterba
@ 2020-02-27 22:52   ` Johannes Thumshirn
  2020-02-28  8:41   ` Qu Wenruo
  1 sibling, 0 replies; 15+ messages in thread
From: Johannes Thumshirn @ 2020-02-27 22:52 UTC (permalink / raw)
  To: David Sterba, linux-btrfs

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/4] btrfs: inline checksum name and driver definitions
  2020-02-27 20:00 ` [PATCH 1/4] btrfs: inline checksum name and driver definitions David Sterba
  2020-02-27 22:31   ` Johannes Thumshirn
@ 2020-02-28  8:38   ` Qu Wenruo
  1 sibling, 0 replies; 15+ messages in thread
From: Qu Wenruo @ 2020-02-28  8:38 UTC (permalink / raw)
  To: David Sterba, linux-btrfs


[-- 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 --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/4] btrfs: simplify tree block checksumming loop
  2020-02-27 20:00 ` [PATCH 2/4] btrfs: simplify tree block checksumming loop David Sterba
  2020-02-27 22:52   ` Johannes Thumshirn
@ 2020-02-28  8:41   ` Qu Wenruo
  1 sibling, 0 replies; 15+ messages in thread
From: Qu Wenruo @ 2020-02-28  8:41 UTC (permalink / raw)
  To: David Sterba, linux-btrfs


[-- 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 --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/4] btrfs: return void from csum_tree_block
  2020-02-27 20:00 ` [PATCH 3/4] btrfs: return void from csum_tree_block David Sterba
  2020-02-27 22:32   ` Johannes Thumshirn
@ 2020-02-28  8:41   ` Qu Wenruo
  1 sibling, 0 replies; 15+ messages in thread
From: Qu Wenruo @ 2020-02-28  8:41 UTC (permalink / raw)
  To: David Sterba, linux-btrfs


[-- 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 --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/4] btrfs: balance: factor out convert profile validation
  2020-02-27 20:00 ` [PATCH 4/4] btrfs: balance: factor out convert profile validation David Sterba
  2020-02-27 22:33   ` Johannes Thumshirn
@ 2020-02-28  8:43   ` Qu Wenruo
  1 sibling, 0 replies; 15+ messages in thread
From: Qu Wenruo @ 2020-02-28  8:43 UTC (permalink / raw)
  To: David Sterba, linux-btrfs


[-- 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 --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 0/4] Misc cleanups
@ 2018-06-27 13:38 Nikolay Borisov
  0 siblings, 0 replies; 15+ messages in thread
From: Nikolay Borisov @ 2018-06-27 13:38 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Nikolay Borisov

Here are a couples of cleanups of things I observed while looking at the 
extent_buffer management code. 

Patch 1 rewrites a do {} while into a simple for() construct. This survived 
xfstest + selftests 

Patch 2 substitutes and outdated comment for a lockdep_assert_held call

Patch 3 rename the idiotic EXTENT_BUFFER_DUMMY to something more meaningful

Patch 4 removes some cargo-cult copied code when performing qgroup leaf scan 

Nikolay Borisov (4):
  btrfs: Refactor loop in btrfs_release_extent_buffer_page
  btrfs: Document locking require via lockdep_assert_held
  btrfs: Rename EXTENT_BUFFER_DUMMY to EXTENT_BUFFER_PRIVATE
  btrfs: Remove unnecessary locking code in qgroup_rescan_leaf

 fs/btrfs/disk-io.c   |  2 +-
 fs/btrfs/extent_io.c | 26 +++++++++++---------------
 fs/btrfs/extent_io.h |  2 +-
 fs/btrfs/qgroup.c    |  7 +------
 4 files changed, 14 insertions(+), 23 deletions(-)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 0/4] Misc cleanups
@ 2018-04-11  8:21 Nikolay Borisov
  0 siblings, 0 replies; 15+ messages in thread
From: Nikolay Borisov @ 2018-04-11  8:21 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Nikolay Borisov

Here's a collection of various cleanups I've done while working on other things. 
All of these should be pretty low-risk, have soaked for a while on my test 
branch and survived multiple xfstest runs. 

They strive to mostly untangle the code and make it more readable. 

Nikolay Borisov (4):
  btrfs: Use while loop instead of labels in __endio_write_update_ordered
  btrfs: Fix lock release order
  btrfs: Consolidate error checking for btrfs_alloc_chunk
  btrfs: Rewrite retry logic in do_chunk_alloc

 fs/btrfs/extent-tree.c | 79 +++++++++++++++++++++++---------------------------
 fs/btrfs/inode.c       | 52 ++++++++++++++++-----------------
 2 files changed, 61 insertions(+), 70 deletions(-)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2020-02-28  8:43 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-27 20:00 [PATCH 0/4] Misc cleanups David Sterba
2020-02-27 20:00 ` [PATCH 1/4] btrfs: inline checksum name and driver definitions David Sterba
2020-02-27 22:31   ` Johannes Thumshirn
2020-02-28  8:38   ` Qu Wenruo
2020-02-27 20:00 ` [PATCH 2/4] btrfs: simplify tree block checksumming loop David Sterba
2020-02-27 22:52   ` Johannes Thumshirn
2020-02-28  8:41   ` Qu Wenruo
2020-02-27 20:00 ` [PATCH 3/4] btrfs: return void from csum_tree_block David Sterba
2020-02-27 22:32   ` Johannes Thumshirn
2020-02-28  8:41   ` Qu Wenruo
2020-02-27 20:00 ` [PATCH 4/4] btrfs: balance: factor out convert profile validation David Sterba
2020-02-27 22:33   ` Johannes Thumshirn
2020-02-28  8:43   ` Qu Wenruo
  -- strict thread matches above, loose matches on Subject: below --
2018-06-27 13:38 [PATCH 0/4] Misc cleanups Nikolay Borisov
2018-04-11  8:21 Nikolay Borisov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.