[PATCH 0/9] Misc fixups and cleanups

[PATCH 0/9] Misc fixups and cleanups

[David Sterba]

David Sterba (9):
  btrfs: sysfs: fix format string for some discard stats
  btrfs: clear defrag status of a root if starting transaction fails
  btrfs: clear log tree recovering status if starting transaction fails
  btrfs: scrub: factor out common scrub_stripe constraints
  btrfs: document byte swap optimization of root_item::flags accessors
  btrfs: reduce compressed_bio members' types
  btrfs: remove extra sb::s_id from message in
    btrfs_validate_metadata_buffer
  btrfs: simplify eb checksum verification in
    btrfs_validate_metadata_buffer
  btrfs: clean up header members offsets in write helpers

 fs/btrfs/compression.c |  2 +-
 fs/btrfs/compression.h | 20 ++++++++++----------
 fs/btrfs/ctree.h       |  2 ++
 fs/btrfs/disk-io.c     | 14 +++++++-------
 fs/btrfs/extent_io.c   | 13 +++++++------
 fs/btrfs/scrub.c       |  9 ++-------
 fs/btrfs/sysfs.c       |  4 ++--
 fs/btrfs/transaction.c |  6 ++++--
 fs/btrfs/tree-log.c    |  1 +
 9 files changed, 36 insertions(+), 35 deletions(-)

-- 
2.29.2

[PATCH 1/9] btrfs: sysfs: fix format string for some discard stats

[David Sterba]

The type of discard_bitmap_bytes and discard_extent_bytes is u64 so the
format should be %llu, though the actual values would hardly ever
overflow to negative values.

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

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index c45d9b6dfdb5..4b508938e728 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -429,7 +429,7 @@ static ssize_t btrfs_discard_bitmap_bytes_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
 
-	return scnprintf(buf, PAGE_SIZE, "%lld\n",
+	return scnprintf(buf, PAGE_SIZE, "%llu\n",
 			fs_info->discard_ctl.discard_bitmap_bytes);
 }
 BTRFS_ATTR(discard, discard_bitmap_bytes, btrfs_discard_bitmap_bytes_show);
@@ -451,7 +451,7 @@ static ssize_t btrfs_discard_extent_bytes_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
 
-	return scnprintf(buf, PAGE_SIZE, "%lld\n",
+	return scnprintf(buf, PAGE_SIZE, "%llu\n",
 			fs_info->discard_ctl.discard_extent_bytes);
 }
 BTRFS_ATTR(discard, discard_extent_bytes, btrfs_discard_extent_bytes_show);
-- 
2.29.2

Re: [PATCH 1/9] btrfs: sysfs: fix format string for some discard stats

[Qu Wenruo]

On 2021/5/26 上午1:08, David Sterba wrote:
> The type of discard_bitmap_bytes and discard_extent_bytes is u64 so the
> format should be %llu, though the actual values would hardly ever
> overflow to negative values.
>
> Signed-off-by: David Sterba <dsterba@suse.com>

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

Thanks,
Qu
> ---
>   fs/btrfs/sysfs.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index c45d9b6dfdb5..4b508938e728 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -429,7 +429,7 @@ static ssize_t btrfs_discard_bitmap_bytes_show(struct kobject *kobj,
>   {
>   	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
>
> -	return scnprintf(buf, PAGE_SIZE, "%lld\n",
> +	return scnprintf(buf, PAGE_SIZE, "%llu\n",
>   			fs_info->discard_ctl.discard_bitmap_bytes);
>   }
>   BTRFS_ATTR(discard, discard_bitmap_bytes, btrfs_discard_bitmap_bytes_show);
> @@ -451,7 +451,7 @@ static ssize_t btrfs_discard_extent_bytes_show(struct kobject *kobj,
>   {
>   	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
>
> -	return scnprintf(buf, PAGE_SIZE, "%lld\n",
> +	return scnprintf(buf, PAGE_SIZE, "%llu\n",
>   			fs_info->discard_ctl.discard_extent_bytes);
>   }
>   BTRFS_ATTR(discard, discard_extent_bytes, btrfs_discard_extent_bytes_show);
>

Re: [PATCH 1/9] btrfs: sysfs: fix format string for some discard stats

[Anand Jain]

On 26/05/2021 01:08, David Sterba wrote:
> The type of discard_bitmap_bytes and discard_extent_bytes is u64 so the
> format should be %llu, though the actual values would hardly ever
> overflow to negative values.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

Reviewed-by: Anand Jain <anand.jain@oracle.com>


> ---
>   fs/btrfs/sysfs.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index c45d9b6dfdb5..4b508938e728 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -429,7 +429,7 @@ static ssize_t btrfs_discard_bitmap_bytes_show(struct kobject *kobj,
>   {
>   	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
>   
> -	return scnprintf(buf, PAGE_SIZE, "%lld\n",
> +	return scnprintf(buf, PAGE_SIZE, "%llu\n",
>   			fs_info->discard_ctl.discard_bitmap_bytes);
>   }
>   BTRFS_ATTR(discard, discard_bitmap_bytes, btrfs_discard_bitmap_bytes_show);
> @@ -451,7 +451,7 @@ static ssize_t btrfs_discard_extent_bytes_show(struct kobject *kobj,
>   {
>   	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
>   
> -	return scnprintf(buf, PAGE_SIZE, "%lld\n",
> +	return scnprintf(buf, PAGE_SIZE, "%llu\n",
>   			fs_info->discard_ctl.discard_extent_bytes);
>   }
>   BTRFS_ATTR(discard, discard_extent_bytes, btrfs_discard_extent_bytes_show);
> 

[PATCH 2/9] btrfs: clear defrag status of a root if starting transaction fails

[David Sterba]

The defrag loop processes leaves in batches and starting transaction for
each. The whole defragmentation on a given root is protected by a bit
but in case the transaction fails, the bit is not cleared

In case the transaction fails the bit would prevent starting
defragmentation again, so make sure it's cleared.

CC: stable@vger.kernel.org # 4.4+
Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/transaction.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index e0a82aa7da89..22951621363f 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1406,8 +1406,10 @@ int btrfs_defrag_root(struct btrfs_root *root)
 
 	while (1) {
 		trans = btrfs_start_transaction(root, 0);
-		if (IS_ERR(trans))
-			return PTR_ERR(trans);
+		if (IS_ERR(trans)) {
+			ret = PTR_ERR(trans);
+			break;
+		}
 
 		ret = btrfs_defrag_leaves(trans, root);
 
-- 
2.29.2

Re: [PATCH 2/9] btrfs: clear defrag status of a root if starting transaction fails

[Qu Wenruo]

On 2021/5/26 上午1:08, David Sterba wrote:
> The defrag loop processes leaves in batches and starting transaction for
> each. The whole defragmentation on a given root is protected by a bit
> but in case the transaction fails, the bit is not cleared
>
> In case the transaction fails the bit would prevent starting
> defragmentation again, so make sure it's cleared.
>
> CC: stable@vger.kernel.org # 4.4+
> Signed-off-by: David Sterba <dsterba@suse.com>

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

Thanks,
Qu
> ---
>   fs/btrfs/transaction.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index e0a82aa7da89..22951621363f 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1406,8 +1406,10 @@ int btrfs_defrag_root(struct btrfs_root *root)
>
>   	while (1) {
>   		trans = btrfs_start_transaction(root, 0);
> -		if (IS_ERR(trans))
> -			return PTR_ERR(trans);
> +		if (IS_ERR(trans)) {
> +			ret = PTR_ERR(trans);
> +			break;
> +		}
>
>   		ret = btrfs_defrag_leaves(trans, root);
>
>

Re: [PATCH 2/9] btrfs: clear defrag status of a root if starting transaction fails

[Anand Jain]

On 26/05/2021 01:08, David Sterba wrote:
> The defrag loop processes leaves in batches and starting transaction for
> each. The whole defragmentation on a given root is protected by a bit
> but in case the transaction fails, the bit is not cleared
> 
> In case the transaction fails the bit would prevent starting
> defragmentation again, so make sure it's cleared.
> 
> CC: stable@vger.kernel.org # 4.4+
> Signed-off-by: David Sterba <dsterba@suse.com>

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks.

> ---
>   fs/btrfs/transaction.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index e0a82aa7da89..22951621363f 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1406,8 +1406,10 @@ int btrfs_defrag_root(struct btrfs_root *root)
>   
>   	while (1) {
>   		trans = btrfs_start_transaction(root, 0);
> -		if (IS_ERR(trans))
> -			return PTR_ERR(trans);
> +		if (IS_ERR(trans)) {
> +			ret = PTR_ERR(trans);
> +			break;
> +		}
>   
>   		ret = btrfs_defrag_leaves(trans, root);
>   
> 

[PATCH 3/9] btrfs: clear log tree recovering status if starting transaction fails

[David Sterba]

When a log recovery is in progress, lots of operations have to take that
into account, so we keep this status per tree during the operation. Long
time ago error handling revamp patch 79787eaab461 ("btrfs: replace many
BUG_ONs with proper error handling") removed clearing of the status in
an error branch. Add it back as was intended in e02119d5a7b4 ("Btrfs:
Add a write ahead tree log to optimize synchronous operations").

There are probably no visible effects, log replay is done only during
mount and if it fails all structures are cleared so the stale status
won't be kept.

Fixes: 79787eaab461 ("btrfs: replace many BUG_ONs with proper error handling")
Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/tree-log.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index c6d4aeede159..5c1d58706fa9 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -6372,6 +6372,7 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
 error:
 	if (wc.trans)
 		btrfs_end_transaction(wc.trans);
+	clear_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags);
 	btrfs_free_path(path);
 	return ret;
 }
-- 
2.29.2

Re: [PATCH 3/9] btrfs: clear log tree recovering status if starting transaction fails

[Qu Wenruo]

On 2021/5/26 上午1:08, David Sterba wrote:
> When a log recovery is in progress, lots of operations have to take that
> into account, so we keep this status per tree during the operation. Long
> time ago error handling revamp patch 79787eaab461 ("btrfs: replace many
> BUG_ONs with proper error handling") removed clearing of the status in
> an error branch. Add it back as was intended in e02119d5a7b4 ("Btrfs:
> Add a write ahead tree log to optimize synchronous operations").
>
> There are probably no visible effects, log replay is done only during
> mount and if it fails all structures are cleared so the stale status
> won't be kept.
>
> Fixes: 79787eaab461 ("btrfs: replace many BUG_ONs with proper error handling")
> Signed-off-by: David Sterba <dsterba@suse.com>

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

Thanks,
Qu
> ---
>   fs/btrfs/tree-log.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index c6d4aeede159..5c1d58706fa9 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -6372,6 +6372,7 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
>   error:
>   	if (wc.trans)
>   		btrfs_end_transaction(wc.trans);
> +	clear_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags);
>   	btrfs_free_path(path);
>   	return ret;
>   }
>

Re: [PATCH 3/9] btrfs: clear log tree recovering status if starting transaction fails

[Anand Jain]

On 26/05/2021 01:08, David Sterba wrote:
> When a log recovery is in progress, lots of operations have to take that
> into account, so we keep this status per tree during the operation. Long
> time ago error handling revamp patch 79787eaab461 ("btrfs: replace many
> BUG_ONs with proper error handling") removed clearing of the status in
> an error branch. Add it back as was intended in e02119d5a7b4 ("Btrfs:
> Add a write ahead tree log to optimize synchronous operations").
> 
> There are probably no visible effects, log replay is done only during
> mount and if it fails all structures are cleared so the stale status
> won't be kept.
> 
> Fixes: 79787eaab461 ("btrfs: replace many BUG_ONs with proper error handling")
> Signed-off-by: David Sterba <dsterba@suse.com>

  Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks.

> ---
>   fs/btrfs/tree-log.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index c6d4aeede159..5c1d58706fa9 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -6372,6 +6372,7 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
>   error:
>   	if (wc.trans)
>   		btrfs_end_transaction(wc.trans);
> +	clear_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags);
>   	btrfs_free_path(path);
>   	return ret;
>   }
> 

[PATCH 4/9] btrfs: scrub: factor out common scrub_stripe constraints

[David Sterba]

There are common values set for the stripe constraints, some of them
are already factored out. Do that for increment and mirror_num as well.

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

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 518415d0c122..5839ad1e25a2 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -3204,28 +3204,23 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
 	physical = map->stripes[num].physical;
 	offset = 0;
 	nstripes = div64_u64(length, map->stripe_len);
+	mirror_num = 1;
+	increment = map->stripe_len;
 	if (map->type & BTRFS_BLOCK_GROUP_RAID0) {
 		offset = map->stripe_len * num;
 		increment = map->stripe_len * map->num_stripes;
-		mirror_num = 1;
 	} else if (map->type & BTRFS_BLOCK_GROUP_RAID10) {
 		int factor = map->num_stripes / map->sub_stripes;
 		offset = map->stripe_len * (num / map->sub_stripes);
 		increment = map->stripe_len * factor;
 		mirror_num = num % map->sub_stripes + 1;
 	} else if (map->type & BTRFS_BLOCK_GROUP_RAID1_MASK) {
-		increment = map->stripe_len;
 		mirror_num = num % map->num_stripes + 1;
 	} else if (map->type & BTRFS_BLOCK_GROUP_DUP) {
-		increment = map->stripe_len;
 		mirror_num = num % map->num_stripes + 1;
 	} else if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) {
 		get_raid56_logic_offset(physical, num, map, &offset, NULL);
 		increment = map->stripe_len * nr_data_stripes(map);
-		mirror_num = 1;
-	} else {
-		increment = map->stripe_len;
-		mirror_num = 1;
 	}
 
 	path = btrfs_alloc_path();
-- 
2.29.2

Re: [PATCH 4/9] btrfs: scrub: factor out common scrub_stripe constraints

[Qu Wenruo]

On 2021/5/26 上午1:08, David Sterba wrote:
> There are common values set for the stripe constraints, some of them
> are already factored out. Do that for increment and mirror_num as well.
>
> Signed-off-by: David Sterba <dsterba@suse.com>

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

Thanks,
Qu
> ---
>   fs/btrfs/scrub.c | 9 ++-------
>   1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index 518415d0c122..5839ad1e25a2 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -3204,28 +3204,23 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
>   	physical = map->stripes[num].physical;
>   	offset = 0;
>   	nstripes = div64_u64(length, map->stripe_len);
> +	mirror_num = 1;
> +	increment = map->stripe_len;
>   	if (map->type & BTRFS_BLOCK_GROUP_RAID0) {
>   		offset = map->stripe_len * num;
>   		increment = map->stripe_len * map->num_stripes;
> -		mirror_num = 1;
>   	} else if (map->type & BTRFS_BLOCK_GROUP_RAID10) {
>   		int factor = map->num_stripes / map->sub_stripes;
>   		offset = map->stripe_len * (num / map->sub_stripes);
>   		increment = map->stripe_len * factor;
>   		mirror_num = num % map->sub_stripes + 1;
>   	} else if (map->type & BTRFS_BLOCK_GROUP_RAID1_MASK) {
> -		increment = map->stripe_len;
>   		mirror_num = num % map->num_stripes + 1;
>   	} else if (map->type & BTRFS_BLOCK_GROUP_DUP) {
> -		increment = map->stripe_len;
>   		mirror_num = num % map->num_stripes + 1;
>   	} else if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) {
>   		get_raid56_logic_offset(physical, num, map, &offset, NULL);
>   		increment = map->stripe_len * nr_data_stripes(map);
> -		mirror_num = 1;
> -	} else {
> -		increment = map->stripe_len;
> -		mirror_num = 1;
>   	}
>
>   	path = btrfs_alloc_path();
>

Re: [PATCH 4/9] btrfs: scrub: factor out common scrub_stripe constraints

[Anand Jain]

On 26/05/2021 01:08, David Sterba wrote:
> There are common values set for the stripe constraints, some of them
> are already factored out. Do that for increment and mirror_num as well.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

  Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks.

> ---
>   fs/btrfs/scrub.c | 9 ++-------
>   1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index 518415d0c122..5839ad1e25a2 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -3204,28 +3204,23 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
>   	physical = map->stripes[num].physical;
>   	offset = 0;
>   	nstripes = div64_u64(length, map->stripe_len);
> +	mirror_num = 1;
> +	increment = map->stripe_len;
>   	if (map->type & BTRFS_BLOCK_GROUP_RAID0) {
>   		offset = map->stripe_len * num;
>   		increment = map->stripe_len * map->num_stripes;
> -		mirror_num = 1;
>   	} else if (map->type & BTRFS_BLOCK_GROUP_RAID10) {
>   		int factor = map->num_stripes / map->sub_stripes;
>   		offset = map->stripe_len * (num / map->sub_stripes);
>   		increment = map->stripe_len * factor;
>   		mirror_num = num % map->sub_stripes + 1;
>   	} else if (map->type & BTRFS_BLOCK_GROUP_RAID1_MASK) {
> -		increment = map->stripe_len;
>   		mirror_num = num % map->num_stripes + 1;
>   	} else if (map->type & BTRFS_BLOCK_GROUP_DUP) {
> -		increment = map->stripe_len;
>   		mirror_num = num % map->num_stripes + 1;
>   	} else if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) {
>   		get_raid56_logic_offset(physical, num, map, &offset, NULL);
>   		increment = map->stripe_len * nr_data_stripes(map);
> -		mirror_num = 1;
> -	} else {
> -		increment = map->stripe_len;
> -		mirror_num = 1;
>   	}
>   
>   	path = btrfs_alloc_path();
> 

[PATCH 5/9] btrfs: document byte swap optimization of root_item::flags accessors

[David Sterba]

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index d78cb2d89d7e..a3b628ea4d64 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2216,11 +2216,13 @@ BTRFS_SETGET_STACK_FUNCS(root_rtransid, struct btrfs_root_item,
 
 static inline bool btrfs_root_readonly(const struct btrfs_root *root)
 {
+	/* Byte-swap the constant at compile time, root_item::flags is LE */
 	return (root->root_item.flags & cpu_to_le64(BTRFS_ROOT_SUBVOL_RDONLY)) != 0;
 }
 
 static inline bool btrfs_root_dead(const struct btrfs_root *root)
 {
+	/* Byte-swap the constant at compile time, root_item::flags is LE */
 	return (root->root_item.flags & cpu_to_le64(BTRFS_ROOT_SUBVOL_DEAD)) != 0;
 }
 
-- 
2.29.2

Re: [PATCH 5/9] btrfs: document byte swap optimization of root_item::flags accessors

[Anand Jain]

On 26/05/2021 01:08, David Sterba wrote:
> Signed-off-by: David Sterba <dsterba@suse.com>

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks.

> ---
>   fs/btrfs/ctree.h | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index d78cb2d89d7e..a3b628ea4d64 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -2216,11 +2216,13 @@ BTRFS_SETGET_STACK_FUNCS(root_rtransid, struct btrfs_root_item,
>   
>   static inline bool btrfs_root_readonly(const struct btrfs_root *root)
>   {
> +	/* Byte-swap the constant at compile time, root_item::flags is LE */
>   	return (root->root_item.flags & cpu_to_le64(BTRFS_ROOT_SUBVOL_RDONLY)) != 0;
>   }
>   
>   static inline bool btrfs_root_dead(const struct btrfs_root *root)
>   {
> +	/* Byte-swap the constant at compile time, root_item::flags is LE */
>   	return (root->root_item.flags & cpu_to_le64(BTRFS_ROOT_SUBVOL_DEAD)) != 0;
>   }
>   
> 

[PATCH 6/9] btrfs: reduce compressed_bio members' types

[David Sterba]

Several members of compressed_bio are of type that's unnecessarily big
for the values that they'd hold:

- the size of the uncompressed and compressed data is 128K now, we can
  keep is as int
- same for number of pages
- the compress type fits to a byte
- the errors is 0/1

The size of the unpatched structure is 80 bytes with several holes.
Reordering nr_pages next to the pages the hole after pending_bios is
filled and the resulting size is 56 bytes. This keeps the csums array
aligned to 8 bytes, which is nice. Further size optimizations may be
possible but right now it looks good to me:

struct compressed_bio {
        refcount_t                 pending_bios;         /*     0     4 */
        unsigned int               nr_pages;             /*     4     4 */
        struct page * *            compressed_pages;     /*     8     8 */
        struct inode *             inode;                /*    16     8 */
        u64                        start;                /*    24     8 */
        unsigned int               len;                  /*    32     4 */
        unsigned int               compressed_len;       /*    36     4 */
        u8                         compress_type;        /*    40     1 */
        u8                         errors;               /*    41     1 */

        /* XXX 2 bytes hole, try to pack */

        int                        mirror_num;           /*    44     4 */
        struct bio *               orig_bio;             /*    48     8 */
        u8                         sums[];               /*    56     0 */

        /* size: 56, cachelines: 1, members: 12 */
        /* sum members: 54, holes: 1, sum holes: 2 */
        /* last cacheline: 56 bytes */
};

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/compression.c |  2 +-
 fs/btrfs/compression.h | 20 ++++++++++----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 9a0c26e4e389..c006f5d81c2a 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -507,7 +507,7 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 		}
 		if (bytes_left < PAGE_SIZE) {
 			btrfs_info(fs_info,
-					"bytes left %lu compress len %lu nr %lu",
+					"bytes left %lu compress len %u nr %u",
 			       bytes_left, cb->compressed_len, cb->nr_pages);
 		}
 		bytes_left -= PAGE_SIZE;
diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
index 8001b700ea3a..00d8439048c9 100644
--- a/fs/btrfs/compression.h
+++ b/fs/btrfs/compression.h
@@ -31,6 +31,9 @@ struct compressed_bio {
 	/* number of bios pending for this compressed extent */
 	refcount_t pending_bios;
 
+	/* Number of compressed pages in the array */
+	unsigned int nr_pages;
+
 	/* the pages with the compressed data on them */
 	struct page **compressed_pages;
 
@@ -40,20 +43,17 @@ struct compressed_bio {
 	/* starting offset in the inode for our pages */
 	u64 start;
 
-	/* number of bytes in the inode we're working on */
-	unsigned long len;
-
-	/* number of bytes on disk */
-	unsigned long compressed_len;
+	/* Number of bytes in the inode we're working on */
+	unsigned int len;
 
-	/* the compression algorithm for this bio */
-	int compress_type;
+	/* Number of bytes on disk */
+	unsigned int compressed_len;
 
-	/* number of compressed pages in the array */
-	unsigned long nr_pages;
+	/* The compression algorithm for this bio */
+	u8 compress_type;
 
 	/* IO errors */
-	int errors;
+	u8 errors;
 	int mirror_num;
 
 	/* for reads, this is the bio we are copying the data into */
-- 
2.29.2

[PATCH] btrfs: optimize users of members of the struct compressed_bio

[Anand Jain]

Commit
 btrfs: reduce compressed_bio member's types
reduced some of the members size.

It has its cascading effects which are done here.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/compression.c | 18 +++++++++---------
 fs/btrfs/compression.h |  6 +++---
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index c006f5d81c2a..36731b598770 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -149,7 +149,7 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio,
 	const u32 csum_size = fs_info->csum_size;
 	const u32 sectorsize = fs_info->sectorsize;
 	struct page *page;
-	unsigned long i;
+	unsigned int i;
 	char *kaddr;
 	u8 csum[BTRFS_CSUM_SIZE];
 	struct compressed_bio *cb = bio->bi_private;
@@ -208,7 +208,7 @@ static void end_compressed_bio_read(struct bio *bio)
 	struct compressed_bio *cb = bio->bi_private;
 	struct inode *inode;
 	struct page *page;
-	unsigned long index;
+	unsigned int index;
 	unsigned int mirror = btrfs_io_bio(bio)->mirror_num;
 	int ret = 0;
 
@@ -334,7 +334,7 @@ static void end_compressed_bio_write(struct bio *bio)
 	struct compressed_bio *cb = bio->bi_private;
 	struct inode *inode;
 	struct page *page;
-	unsigned long index;
+	unsigned int index;
 
 	if (bio->bi_status)
 		cb->errors = 1;
@@ -387,10 +387,10 @@ static void end_compressed_bio_write(struct bio *bio)
  * the end io hooks.
  */
 blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
-				 unsigned long len, u64 disk_start,
-				 unsigned long compressed_len,
+				 unsigned int len, u64 disk_start,
+				 unsigned int compressed_len,
 				 struct page **compressed_pages,
-				 unsigned long nr_pages,
+				 unsigned int nr_pages,
 				 unsigned int write_flags,
 				 struct cgroup_subsys_state *blkcg_css)
 {
@@ -669,9 +669,9 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct extent_map_tree *em_tree;
 	struct compressed_bio *cb;
-	unsigned long compressed_len;
-	unsigned long nr_pages;
-	unsigned long pg_index;
+	unsigned int compressed_len;
+	unsigned int nr_pages;
+	unsigned int pg_index;
 	struct page *page;
 	struct bio *comp_bio;
 	u64 cur_disk_byte = bio->bi_iter.bi_sector << 9;
diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
index 00d8439048c9..c359f20920d0 100644
--- a/fs/btrfs/compression.h
+++ b/fs/btrfs/compression.h
@@ -91,10 +91,10 @@ int btrfs_decompress_buf2page(const char *buf, unsigned long buf_start,
 			      struct bio *bio);
 
 blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
-				  unsigned long len, u64 disk_start,
-				  unsigned long compressed_len,
+				  unsigned int len, u64 disk_start,
+				  unsigned int compressed_len,
 				  struct page **compressed_pages,
-				  unsigned long nr_pages,
+				  unsigned int nr_pages,
 				  unsigned int write_flags,
 				  struct cgroup_subsys_state *blkcg_css);
 blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
-- 
2.30.2

Re: [PATCH] btrfs: optimize users of members of the struct compressed_bio

[David Sterba]

On Wed, May 26, 2021 at 04:40:54PM +0800, Anand Jain wrote:
> Commit
>  btrfs: reduce compressed_bio member's types
> reduced some of the members size.
> 
> It has its cascading effects which are done here.

Agreed, feel free to send it as proper patch(es) (also updating the
async_extent). The compression code uses a lot of types that are wider
than necessary, so optimizing that could improve the stack footprint.

Re: [PATCH 6/9] btrfs: reduce compressed_bio members' types

[Anand Jain]

On 26/05/2021 01:08, David Sterba wrote:
> Several members of compressed_bio are of type that's unnecessarily big
> for the values that they'd hold:
> 
> - the size of the uncompressed and compressed data is 128K now, we can
>    keep is as int
> - same for number of pages
> - the compress type fits to a byte
> - the errors is 0/1
> 
> The size of the unpatched structure is 80 bytes with several holes.
> Reordering nr_pages next to the pages the hole after pending_bios is
> filled and the resulting size is 56 bytes. This keeps the csums array
> aligned to 8 bytes, which is nice. Further size optimizations may be
> possible but right now it looks good to me:
> 
> struct compressed_bio {
>          refcount_t                 pending_bios;         /*     0     4 */
>          unsigned int               nr_pages;             /*     4     4 */
>          struct page * *            compressed_pages;     /*     8     8 */
>          struct inode *             inode;                /*    16     8 */
>          u64                        start;                /*    24     8 */
>          unsigned int               len;                  /*    32     4 */
>          unsigned int               compressed_len;       /*    36     4 */
>          u8                         compress_type;        /*    40     1 */
>          u8                         errors;               /*    41     1 */
> 
>          /* XXX 2 bytes hole, try to pack */
> 
>          int                        mirror_num;           /*    44     4 */
>          struct bio *               orig_bio;             /*    48     8 */
>          u8                         sums[];               /*    56     0 */
> 
>          /* size: 56, cachelines: 1, members: 12 */
>          /* sum members: 54, holes: 1, sum holes: 2 */
>          /* last cacheline: 56 bytes */
> };
> 

The following member types of struct compressed_bio are changed here:

-       unsigned long len;
+       unsigned int len;

-       int compress_type;
+       u8 compress_type;

-       unsigned long nr_pages;
+       unsigned int nr_pages;

-       unsigned long compressed_len;
+       unsigned int compressed_len;

-       int errors;
+       u8 errors;

There are no warnings.

But in btrfs_submit_compressed_write()

Essentially struct compressed_bio is updated from struct async_extent.

struct async_extent {
         u64 start;
         u64 ram_size;
         u64 compressed_size;
         struct page **pages;
         unsigned long nr_pages;
         int compress_type;
         struct list_head list;
};

which can be looked into later.

For now, with the patch
  btrfs: optimize users of members of the struct compressed_bio
which is sent to ML.

Reviewed-by: Anand Jain <anand.jain@oracle.com>


> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/compression.c |  2 +-
>   fs/btrfs/compression.h | 20 ++++++++++----------
>   2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 9a0c26e4e389..c006f5d81c2a 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -507,7 +507,7 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
>   		}
>   		if (bytes_left < PAGE_SIZE) {
>   			btrfs_info(fs_info,
> -					"bytes left %lu compress len %lu nr %lu",
> +					"bytes left %lu compress len %u nr %u",
>   			       bytes_left, cb->compressed_len, cb->nr_pages);
>   		}
>   		bytes_left -= PAGE_SIZE;
> diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
> index 8001b700ea3a..00d8439048c9 100644
> --- a/fs/btrfs/compression.h
> +++ b/fs/btrfs/compression.h
> @@ -31,6 +31,9 @@ struct compressed_bio {
>   	/* number of bios pending for this compressed extent */
>   	refcount_t pending_bios;
>   
> +	/* Number of compressed pages in the array */
> +	unsigned int nr_pages;
> +
>   	/* the pages with the compressed data on them */
>   	struct page **compressed_pages;
>   
> @@ -40,20 +43,17 @@ struct compressed_bio {
>   	/* starting offset in the inode for our pages */
>   	u64 start;
>   
> -	/* number of bytes in the inode we're working on */
> -	unsigned long len;
> -
> -	/* number of bytes on disk */
> -	unsigned long compressed_len;
> +	/* Number of bytes in the inode we're working on */
> +	unsigned int len;
>   
> -	/* the compression algorithm for this bio */
> -	int compress_type;
> +	/* Number of bytes on disk */
> +	unsigned int compressed_len;
>   
> -	/* number of compressed pages in the array */
> -	unsigned long nr_pages;
> +	/* The compression algorithm for this bio */
> +	u8 compress_type;
>   
>   	/* IO errors */
> -	int errors;
> +	u8 errors;
>   	int mirror_num;
>   
>   	/* for reads, this is the bio we are copying the data into */
> 

[PATCH 7/9] btrfs: remove extra sb::s_id from message in btrfs_validate_metadata_buffer

[David Sterba]

The s_id is already printed by message helpers.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 8c3db9076988..6dc137684899 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -614,8 +614,8 @@ static int validate_extent_buffer(struct extent_buffer *eb)
 
 		read_extent_buffer(eb, &val, 0, csum_size);
 		btrfs_warn_rl(fs_info,
-	"%s checksum verify failed on %llu wanted " CSUM_FMT " found " CSUM_FMT " level %d",
-			      fs_info->sb->s_id, eb->start,
+	"checksum verify failed on %llu wanted " CSUM_FMT " found " CSUM_FMT " level %d",
+			      eb->start,
 			      CSUM_FMT_VALUE(csum_size, val),
 			      CSUM_FMT_VALUE(csum_size, result),
 			      btrfs_header_level(eb));
-- 
2.29.2

Re: [PATCH 7/9] btrfs: remove extra sb::s_id from message in btrfs_validate_metadata_buffer

[Qu Wenruo]

On 2021/5/26 上午1:08, David Sterba wrote:
> The s_id is already printed by message helpers.
>
> Signed-off-by: David Sterba <dsterba@suse.com>

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

Thanks,
Qu
> ---
>   fs/btrfs/disk-io.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 8c3db9076988..6dc137684899 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -614,8 +614,8 @@ static int validate_extent_buffer(struct extent_buffer *eb)
>
>   		read_extent_buffer(eb, &val, 0, csum_size);
>   		btrfs_warn_rl(fs_info,
> -	"%s checksum verify failed on %llu wanted " CSUM_FMT " found " CSUM_FMT " level %d",
> -			      fs_info->sb->s_id, eb->start,
> +	"checksum verify failed on %llu wanted " CSUM_FMT " found " CSUM_FMT " level %d",
> +			      eb->start,
>   			      CSUM_FMT_VALUE(csum_size, val),
>   			      CSUM_FMT_VALUE(csum_size, result),
>   			      btrfs_header_level(eb));
>

Re: [PATCH 7/9] btrfs: remove extra sb::s_id from message in btrfs_validate_metadata_buffer

[Anand Jain]

On 26/05/2021 01:08, David Sterba wrote:
> The s_id is already printed by message helpers.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

Reviewed-by: Anand Jain <anand.jain@oracle.com>
Thanks.

> ---
>   fs/btrfs/disk-io.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 8c3db9076988..6dc137684899 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -614,8 +614,8 @@ static int validate_extent_buffer(struct extent_buffer *eb)
>   
>   		read_extent_buffer(eb, &val, 0, csum_size);
>   		btrfs_warn_rl(fs_info,
> -	"%s checksum verify failed on %llu wanted " CSUM_FMT " found " CSUM_FMT " level %d",
> -			      fs_info->sb->s_id, eb->start,
> +	"checksum verify failed on %llu wanted " CSUM_FMT " found " CSUM_FMT " level %d",
> +			      eb->start,
>   			      CSUM_FMT_VALUE(csum_size, val),
>   			      CSUM_FMT_VALUE(csum_size, result),
>   			      btrfs_header_level(eb));
> 

[PATCH 8/9] btrfs: simplify eb checksum verification in btrfs_validate_metadata_buffer

[David Sterba]

The verification copies the calculated checksum bytes to a temporary
buffer but this is not necessary. We can map the eb header on the first
page and use the checksum bytes directly.

This saves at least one function call and boundary checks so it could
lead to a minor performance improvement.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 6dc137684899..868e358f6923 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -584,6 +584,7 @@ static int validate_extent_buffer(struct extent_buffer *eb)
 	const u32 csum_size = fs_info->csum_size;
 	u8 found_level;
 	u8 result[BTRFS_CSUM_SIZE];
+	const struct btrfs_header *header;
 	int ret = 0;
 
 	found_start = btrfs_header_bytenr(eb);
@@ -608,15 +609,14 @@ static int validate_extent_buffer(struct extent_buffer *eb)
 	}
 
 	csum_tree_block(eb, result);
+	header = page_address(eb->pages[0]) +
+		 get_eb_offset_in_page(eb, offsetof(struct btrfs_leaf, header));
 
-	if (memcmp_extent_buffer(eb, result, 0, csum_size)) {
-		u8 val[BTRFS_CSUM_SIZE] = { 0 };
-
-		read_extent_buffer(eb, &val, 0, csum_size);
+	if (memcmp(result, header->csum, csum_size) != 0) {
 		btrfs_warn_rl(fs_info,
 	"checksum verify failed on %llu wanted " CSUM_FMT " found " CSUM_FMT " level %d",
 			      eb->start,
-			      CSUM_FMT_VALUE(csum_size, val),
+			      CSUM_FMT_VALUE(csum_size, header->csum),
 			      CSUM_FMT_VALUE(csum_size, result),
 			      btrfs_header_level(eb));
 		ret = -EUCLEAN;
-- 
2.29.2

Re: [PATCH 8/9] btrfs: simplify eb checksum verification in btrfs_validate_metadata_buffer

[Qu Wenruo]

On 2021/5/26 上午1:08, David Sterba wrote:
> The verification copies the calculated checksum bytes to a temporary
> buffer but this is not necessary. We can map the eb header on the first
> page and use the checksum bytes directly.
>
> This saves at least one function call and boundary checks so it could
> lead to a minor performance improvement.
>
> Signed-off-by: David Sterba <dsterba@suse.com>

The idea is good, and should be pretty simple to test.

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

But still a nitpick inlined below.
> ---
>   fs/btrfs/disk-io.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 6dc137684899..868e358f6923 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -584,6 +584,7 @@ static int validate_extent_buffer(struct extent_buffer *eb)
>   	const u32 csum_size = fs_info->csum_size;
>   	u8 found_level;
>   	u8 result[BTRFS_CSUM_SIZE];
> +	const struct btrfs_header *header;
>   	int ret = 0;
>
>   	found_start = btrfs_header_bytenr(eb);
> @@ -608,15 +609,14 @@ static int validate_extent_buffer(struct extent_buffer *eb)
>   	}
>
>   	csum_tree_block(eb, result);
> +	header = page_address(eb->pages[0]) +
> +		 get_eb_offset_in_page(eb, offsetof(struct btrfs_leaf, header));

It takes me near a minute to figure why it's not just
"get_eb_offset_in_page(eb, 0)".

I'm not sure if we really need that explicit way to just get 0,
especially most of us (and even some advanced users) know that csum
comes at the very beginning of a tree block.

And the mention of btrfs_leave can sometimes be confusing, especially
when we could have tree nodes passed in.

Thanks,
Qu
>
> -	if (memcmp_extent_buffer(eb, result, 0, csum_size)) {
> -		u8 val[BTRFS_CSUM_SIZE] = { 0 };
> -
> -		read_extent_buffer(eb, &val, 0, csum_size);
> +	if (memcmp(result, header->csum, csum_size) != 0) {
>   		btrfs_warn_rl(fs_info,
>   	"checksum verify failed on %llu wanted " CSUM_FMT " found " CSUM_FMT " level %d",
>   			      eb->start,
> -			      CSUM_FMT_VALUE(csum_size, val),
> +			      CSUM_FMT_VALUE(csum_size, header->csum),
>   			      CSUM_FMT_VALUE(csum_size, result),
>   			      btrfs_header_level(eb));
>   		ret = -EUCLEAN;
>

Re: [PATCH 8/9] btrfs: simplify eb checksum verification in btrfs_validate_metadata_buffer

[David Sterba]

On Wed, May 26, 2021 at 08:05:51AM +0800, Qu Wenruo wrote:
> On 2021/5/26 上午1:08, David Sterba wrote:
> > The verification copies the calculated checksum bytes to a temporary
> > buffer but this is not necessary. We can map the eb header on the first
> > page and use the checksum bytes directly.
> >
> > This saves at least one function call and boundary checks so it could
> > lead to a minor performance improvement.
> >
> > Signed-off-by: David Sterba <dsterba@suse.com>
> 
> The idea is good, and should be pretty simple to test.
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
> But still a nitpick inlined below.
> > ---
> >   fs/btrfs/disk-io.c | 10 +++++-----
> >   1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index 6dc137684899..868e358f6923 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -584,6 +584,7 @@ static int validate_extent_buffer(struct extent_buffer *eb)
> >   	const u32 csum_size = fs_info->csum_size;
> >   	u8 found_level;
> >   	u8 result[BTRFS_CSUM_SIZE];
> > +	const struct btrfs_header *header;
> >   	int ret = 0;
> >
> >   	found_start = btrfs_header_bytenr(eb);
> > @@ -608,15 +609,14 @@ static int validate_extent_buffer(struct extent_buffer *eb)
> >   	}
> >
> >   	csum_tree_block(eb, result);
> > +	header = page_address(eb->pages[0]) +
> > +		 get_eb_offset_in_page(eb, offsetof(struct btrfs_leaf, header));
> 
> It takes me near a minute to figure why it's not just
> "get_eb_offset_in_page(eb, 0)".
> 
> I'm not sure if we really need that explicit way to just get 0,
> especially most of us (and even some advanced users) know that csum
> comes at the very beginning of a tree block.
> 
> And the mention of btrfs_leave can sometimes be confusing, especially
> when we could have tree nodes passed in.

Ah right, I wanted to use the offsetof for clarity but that it could be
used with nodes makes it confusing again. What if it's replaced by

	get_eb_offset_in_page(eb, offsetof(struct btrfs_header, csum));

This makes it clear that it's the checksum and from the experience we
know it's at offset 0. I'd rather avoid magic constants and offsets but
you're right that everybody knows that the checksum is at the beginning
of btree block.

Re: [PATCH 8/9] btrfs: simplify eb checksum verification in btrfs_validate_metadata_buffer

[David Sterba]

On Wed, May 26, 2021 at 06:31:39PM +0200, David Sterba wrote:
> > > +	header = page_address(eb->pages[0]) +
> > > +		 get_eb_offset_in_page(eb, offsetof(struct btrfs_leaf, header));
> > 
> > It takes me near a minute to figure why it's not just
> > "get_eb_offset_in_page(eb, 0)".
> > 
> > I'm not sure if we really need that explicit way to just get 0,
> > especially most of us (and even some advanced users) know that csum
> > comes at the very beginning of a tree block.
> > 
> > And the mention of btrfs_leave can sometimes be confusing, especially
> > when we could have tree nodes passed in.
> 
> Ah right, I wanted to use the offsetof for clarity but that it could be
> used with nodes makes it confusing again. What if it's replaced by
> 
> 	get_eb_offset_in_page(eb, offsetof(struct btrfs_header, csum));
> 
> This makes it clear that it's the checksum and from the experience we
> know it's at offset 0. I'd rather avoid magic constants and offsets but
> you're right that everybody knows that the checksum is at the beginning
> of btree block.

--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -584,7 +584,7 @@ static int validate_extent_buffer(struct extent_buffer *eb)
        const u32 csum_size = fs_info->csum_size;
        u8 found_level;
        u8 result[BTRFS_CSUM_SIZE];
-       const struct btrfs_header *header;
+       const u8 *header_csum;
        int ret = 0;
 
        found_start = btrfs_header_bytenr(eb);
@@ -609,14 +609,14 @@ static int validate_extent_buffer(struct extent_buffer *eb)
        }
 
        csum_tree_block(eb, result);
-       header = page_address(eb->pages[0]) +
-                get_eb_offset_in_page(eb, offsetof(struct btrfs_leaf, header));
+       header_csum = page_address(eb->pages[0]) +
+               get_eb_offset_in_page(eb, offsetof(struct btrfs_header, csum));
 
-       if (memcmp(result, header->csum, csum_size) != 0) {
+       if (memcmp(result, header_csum, csum_size) != 0) {
                btrfs_warn_rl(fs_info,
        "checksum verify failed on %llu wanted " CSUM_FMT " found " CSUM_FMT " level %d",
                              eb->start,
-                             CSUM_FMT_VALUE(csum_size, header->csum),
+                             CSUM_FMT_VALUE(csum_size, header_csum),
                              CSUM_FMT_VALUE(csum_size, result),
                              btrfs_header_level(eb));
                ret = -EUCLEAN;

Re: [PATCH 8/9] btrfs: simplify eb checksum verification in btrfs_validate_metadata_buffer

[Qu Wenruo]

On 2021/5/27 上午12:58, David Sterba wrote:
> On Wed, May 26, 2021 at 06:31:39PM +0200, David Sterba wrote:
>>>> +	header = page_address(eb->pages[0]) +
>>>> +		 get_eb_offset_in_page(eb, offsetof(struct btrfs_leaf, header));
>>>
>>> It takes me near a minute to figure why it's not just
>>> "get_eb_offset_in_page(eb, 0)".
>>>
>>> I'm not sure if we really need that explicit way to just get 0,
>>> especially most of us (and even some advanced users) know that csum
>>> comes at the very beginning of a tree block.
>>>
>>> And the mention of btrfs_leave can sometimes be confusing, especially
>>> when we could have tree nodes passed in.
>>
>> Ah right, I wanted to use the offsetof for clarity but that it could be
>> used with nodes makes it confusing again. What if it's replaced by
>>
>> 	get_eb_offset_in_page(eb, offsetof(struct btrfs_header, csum));
>>
>> This makes it clear that it's the checksum and from the experience we
>> know it's at offset 0. I'd rather avoid magic constants and offsets but
>> you're right that everybody knows that the checksum is at the beginning
>> of btree block.
>
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -584,7 +584,7 @@ static int validate_extent_buffer(struct extent_buffer *eb)
>          const u32 csum_size = fs_info->csum_size;
>          u8 found_level;
>          u8 result[BTRFS_CSUM_SIZE];
> -       const struct btrfs_header *header;
> +       const u8 *header_csum;
>          int ret = 0;
>
>          found_start = btrfs_header_bytenr(eb);
> @@ -609,14 +609,14 @@ static int validate_extent_buffer(struct extent_buffer *eb)
>          }
>
>          csum_tree_block(eb, result);
> -       header = page_address(eb->pages[0]) +
> -                get_eb_offset_in_page(eb, offsetof(struct btrfs_leaf, header));
> +       header_csum = page_address(eb->pages[0]) +
> +               get_eb_offset_in_page(eb, offsetof(struct btrfs_header, csum));

This version looks better to me.

THanks,
Qu
>
> -       if (memcmp(result, header->csum, csum_size) != 0) {
> +       if (memcmp(result, header_csum, csum_size) != 0) {
>                  btrfs_warn_rl(fs_info,
>          "checksum verify failed on %llu wanted " CSUM_FMT " found " CSUM_FMT " level %d",
>                                eb->start,
> -                             CSUM_FMT_VALUE(csum_size, header->csum),
> +                             CSUM_FMT_VALUE(csum_size, header_csum),
>                                CSUM_FMT_VALUE(csum_size, result),
>                                btrfs_header_level(eb));
>                  ret = -EUCLEAN;
>

Re: [PATCH 8/9] btrfs: simplify eb checksum verification in btrfs_validate_metadata_buffer

[David Sterba]

On Thu, May 27, 2021 at 07:13:24AM +0800, Qu Wenruo wrote:
> 
> 
> On 2021/5/27 上午12:58, David Sterba wrote:
> > On Wed, May 26, 2021 at 06:31:39PM +0200, David Sterba wrote:
> >>>> +	header = page_address(eb->pages[0]) +
> >>>> +		 get_eb_offset_in_page(eb, offsetof(struct btrfs_leaf, header));
> >>>
> >>> It takes me near a minute to figure why it's not just
> >>> "get_eb_offset_in_page(eb, 0)".
> >>>
> >>> I'm not sure if we really need that explicit way to just get 0,
> >>> especially most of us (and even some advanced users) know that csum
> >>> comes at the very beginning of a tree block.
> >>>
> >>> And the mention of btrfs_leave can sometimes be confusing, especially
> >>> when we could have tree nodes passed in.
> >>
> >> Ah right, I wanted to use the offsetof for clarity but that it could be
> >> used with nodes makes it confusing again. What if it's replaced by
> >>
> >> 	get_eb_offset_in_page(eb, offsetof(struct btrfs_header, csum));
> >>
> >> This makes it clear that it's the checksum and from the experience we
> >> know it's at offset 0. I'd rather avoid magic constants and offsets but
> >> you're right that everybody knows that the checksum is at the beginning
> >> of btree block.
> >
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -584,7 +584,7 @@ static int validate_extent_buffer(struct extent_buffer *eb)
> >          const u32 csum_size = fs_info->csum_size;
> >          u8 found_level;
> >          u8 result[BTRFS_CSUM_SIZE];
> > -       const struct btrfs_header *header;
> > +       const u8 *header_csum;
> >          int ret = 0;
> >
> >          found_start = btrfs_header_bytenr(eb);
> > @@ -609,14 +609,14 @@ static int validate_extent_buffer(struct extent_buffer *eb)
> >          }
> >
> >          csum_tree_block(eb, result);
> > -       header = page_address(eb->pages[0]) +
> > -                get_eb_offset_in_page(eb, offsetof(struct btrfs_leaf, header));
> > +       header_csum = page_address(eb->pages[0]) +
> > +               get_eb_offset_in_page(eb, offsetof(struct btrfs_header, csum));
> 
> This version looks better to me.

Thanks, I've preemptively squashed it to the commit, now in misc-next.

[PATCH 9/9] btrfs: clean up header members offsets in write helpers

[David Sterba]

Move header offsetof() to the expression that calculates the address so
it's part of get_eb_offset_in_page where the 2nd parameter is the member
offset.

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

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 2b250c610562..2e924f60ea6f 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -6519,9 +6519,10 @@ void write_extent_buffer_chunk_tree_uuid(const struct extent_buffer *eb,
 	char *kaddr;
 
 	assert_eb_page_uptodate(eb, eb->pages[0]);
-	kaddr = page_address(eb->pages[0]) + get_eb_offset_in_page(eb, 0);
-	memcpy(kaddr + offsetof(struct btrfs_header, chunk_tree_uuid), srcv,
-			BTRFS_FSID_SIZE);
+	kaddr = page_address(eb->pages[0]) +
+		get_eb_offset_in_page(eb, offsetof(struct btrfs_header,
+						   chunk_tree_uuid));
+	memcpy(kaddr, srcv, BTRFS_FSID_SIZE);
 }
 
 void write_extent_buffer_fsid(const struct extent_buffer *eb, const void *srcv)
@@ -6529,9 +6530,9 @@ void write_extent_buffer_fsid(const struct extent_buffer *eb, const void *srcv)
 	char *kaddr;
 
 	assert_eb_page_uptodate(eb, eb->pages[0]);
-	kaddr = page_address(eb->pages[0]) + get_eb_offset_in_page(eb, 0);
-	memcpy(kaddr + offsetof(struct btrfs_header, fsid), srcv,
-			BTRFS_FSID_SIZE);
+	kaddr = page_address(eb->pages[0]) +
+		get_eb_offset_in_page(eb, offsetof(struct btrfs_header, fsid));
+	memcpy(kaddr, srcv, BTRFS_FSID_SIZE);
 }
 
 void write_extent_buffer(const struct extent_buffer *eb, const void *srcv,
-- 
2.29.2

Re: [PATCH 9/9] btrfs: clean up header members offsets in write helpers

[Qu Wenruo]

On 2021/5/26 上午1:08, David Sterba wrote:
> Move header offsetof() to the expression that calculates the address so
> it's part of get_eb_offset_in_page where the 2nd parameter is the member
> offset.
>
> Signed-off-by: David Sterba <dsterba@suse.com>

Indeed better than the original expression.

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

Thanks,
Qu
> ---
>   fs/btrfs/extent_io.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 2b250c610562..2e924f60ea6f 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -6519,9 +6519,10 @@ void write_extent_buffer_chunk_tree_uuid(const struct extent_buffer *eb,
>   	char *kaddr;
>
>   	assert_eb_page_uptodate(eb, eb->pages[0]);
> -	kaddr = page_address(eb->pages[0]) + get_eb_offset_in_page(eb, 0);
> -	memcpy(kaddr + offsetof(struct btrfs_header, chunk_tree_uuid), srcv,
> -			BTRFS_FSID_SIZE);
> +	kaddr = page_address(eb->pages[0]) +
> +		get_eb_offset_in_page(eb, offsetof(struct btrfs_header,
> +						   chunk_tree_uuid));
> +	memcpy(kaddr, srcv, BTRFS_FSID_SIZE);
>   }
>
>   void write_extent_buffer_fsid(const struct extent_buffer *eb, const void *srcv)
> @@ -6529,9 +6530,9 @@ void write_extent_buffer_fsid(const struct extent_buffer *eb, const void *srcv)
>   	char *kaddr;
>
>   	assert_eb_page_uptodate(eb, eb->pages[0]);
> -	kaddr = page_address(eb->pages[0]) + get_eb_offset_in_page(eb, 0);
> -	memcpy(kaddr + offsetof(struct btrfs_header, fsid), srcv,
> -			BTRFS_FSID_SIZE);
> +	kaddr = page_address(eb->pages[0]) +
> +		get_eb_offset_in_page(eb, offsetof(struct btrfs_header, fsid));
> +	memcpy(kaddr, srcv, BTRFS_FSID_SIZE);
>   }
>
>   void write_extent_buffer(const struct extent_buffer *eb, const void *srcv,
>