[PATCH 00/11] Minor cleanups

[PATCH 00/11] Minor cleanups

[David Sterba]

Minor and safe cleanups.

David Sterba (11):
  btrfs: use struct_size to calculate size of raid hash table
  btrfs: move mapping of block for discard to its caller
  btrfs: open code trivial helper btrfs_header_fsid
  btrfs: open code trivial helper btrfs_header_chunk_tree_uuid
  btrfs: simplify parameters of btrfs_set_disk_extent_flags
  btrfs: adjust message level for unrecognized mount option
  btrfs: raid56: simplify sort_parity_stripes
  btrfs: replace u_long type cast with unsigned long
  btrfs: adjust delayed refs message level
  btrfs: merge unlocking to common exit block in
    btrfs_commit_transaction
  btrfs: reduce pointer intdirections in btree_readpage_end_io_hook

 fs/btrfs/ctree.c       |  4 +--
 fs/btrfs/ctree.h       | 12 +--------
 fs/btrfs/disk-io.c     | 18 +++++++------
 fs/btrfs/extent-tree.c |  7 +++---
 fs/btrfs/raid56.c      |  4 +--
 fs/btrfs/super.c       |  2 +-
 fs/btrfs/transaction.c | 57 +++++++++++++++---------------------------
 fs/btrfs/volumes.c     | 33 ++++++++----------------
 8 files changed, 49 insertions(+), 88 deletions(-)

-- 
2.25.0

[PATCH 01/11] btrfs: use struct_size to calculate size of raid hash table

[David Sterba]

The struct_size macro does the same calculation and is safe regarding
overflows. Though we're not expecting them to happen, use the helper for
clarity.

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

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 406b1efd3ba5..c870ef70f817 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -206,7 +206,6 @@ int btrfs_alloc_stripe_hash_table(struct btrfs_fs_info *info)
 	struct btrfs_stripe_hash *h;
 	int num_entries = 1 << BTRFS_STRIPE_HASH_TABLE_BITS;
 	int i;
-	int table_size;
 
 	if (info->stripe_hash_table)
 		return 0;
@@ -218,8 +217,7 @@ int btrfs_alloc_stripe_hash_table(struct btrfs_fs_info *info)
 	 * Try harder to allocate and fallback to vmalloc to lower the chance
 	 * of a failing mount.
 	 */
-	table_size = sizeof(*table) + sizeof(*h) * num_entries;
-	table = kvzalloc(table_size, GFP_KERNEL);
+	table = kvzalloc(struct_size(table, table, num_entries), GFP_KERNEL);
 	if (!table)
 		return -ENOMEM;
 
-- 
2.25.0

Re: [PATCH 01/11] btrfs: use struct_size to calculate size of raid hash table

[Johannes Thumshirn]

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

[PATCH 02/11] btrfs: move mapping of block for discard to its caller

[David Sterba]

There's a simple forwarded call based on the operation that would better
fit the caller btrfs_map_block that's until now a trivial wrapper.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index d8a88866aaa3..997f2c70cb6c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -5953,10 +5953,7 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
 	struct btrfs_io_geometry geom;
 
 	ASSERT(bbio_ret);
-
-	if (op == BTRFS_MAP_DISCARD)
-		return __btrfs_map_block_for_discard(fs_info, logical,
-						     length, bbio_ret);
+	ASSERT(op != BTRFS_MAP_DISCARD);
 
 	ret = btrfs_get_io_geometry(fs_info, op, logical, *length, &geom);
 	if (ret < 0)
@@ -6186,6 +6183,10 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 		      u64 logical, u64 *length,
 		      struct btrfs_bio **bbio_ret, int mirror_num)
 {
+	if (op == BTRFS_MAP_DISCARD)
+		return __btrfs_map_block_for_discard(fs_info, logical,
+						     length, bbio_ret);
+
 	return __btrfs_map_block(fs_info, op, logical, length, bbio_ret,
 				 mirror_num, 0);
 }
-- 
2.25.0

Re: [PATCH 02/11] btrfs: move mapping of block for discard to its caller

[Johannes Thumshirn]

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

Re: [PATCH 02/11] btrfs: move mapping of block for discard to its caller

[Anand Jain]

On 2/22/20 12:31 AM, David Sterba wrote:
> There's a simple forwarded call based on the operation that would better
> fit the caller btrfs_map_block that's until now a trivial wrapper.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

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

[PATCH 03/11] btrfs: open code trivial helper btrfs_header_fsid

[David Sterba]

The helper btrfs_header_fsid follows naming convention of other struct
accessors but does something compeletly different. As the offsetof
calculation is clear in the context of extent buffer operations we can
remove it.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.h   | 5 -----
 fs/btrfs/disk-io.c | 6 ++++--
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index bb237d577725..6f4272006029 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1976,11 +1976,6 @@ static inline void btrfs_set_header_backref_rev(struct extent_buffer *eb,
 	btrfs_set_header_flags(eb, flags);
 }
 
-static inline unsigned long btrfs_header_fsid(void)
-{
-	return offsetof(struct btrfs_header, fsid);
-}
-
 static inline unsigned long btrfs_header_chunk_tree_uuid(const struct extent_buffer *eb)
 {
 	return offsetof(struct btrfs_header, chunk_tree_uuid);
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 756bf2ab64cd..63d009816264 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -541,7 +541,8 @@ static int csum_dirty_buffer(struct btrfs_fs_info *fs_info, struct page *page)
 		return -EUCLEAN;
 
 	ASSERT(memcmp_extent_buffer(eb, fs_info->fs_devices->metadata_uuid,
-			btrfs_header_fsid(), BTRFS_FSID_SIZE) == 0);
+				    offsetof(struct btrfs_header, fsid),
+				    BTRFS_FSID_SIZE) == 0);
 
 	if (csum_tree_block(eb, result))
 		return -EINVAL;
@@ -571,7 +572,8 @@ static int check_tree_block_fsid(struct extent_buffer *eb)
 	u8 fsid[BTRFS_FSID_SIZE];
 	int ret = 1;
 
-	read_extent_buffer(eb, fsid, btrfs_header_fsid(), BTRFS_FSID_SIZE);
+	read_extent_buffer(eb, fsid, offsetof(struct btrfs_header, fsid),
+			   BTRFS_FSID_SIZE);
 	while (fs_devices) {
 		u8 *metadata_uuid;
 
-- 
2.25.0

Re: [PATCH 03/11] btrfs: open code trivial helper btrfs_header_fsid

[Johannes Thumshirn]

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

[PATCH 04/11] btrfs: open code trivial helper btrfs_header_chunk_tree_uuid

[David Sterba]

The helper btrfs_header_chunk_tree_uuid follows naming convention of
other struct accessors but does something compeletly different. As the
offsetof calculation is clear in the context of extent buffer operations
we can remove it.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.h   | 5 -----
 fs/btrfs/disk-io.c | 3 ++-
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 6f4272006029..d7e54cbc8dce 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1976,11 +1976,6 @@ static inline void btrfs_set_header_backref_rev(struct extent_buffer *eb,
 	btrfs_set_header_flags(eb, flags);
 }
 
-static inline unsigned long btrfs_header_chunk_tree_uuid(const struct extent_buffer *eb)
-{
-	return offsetof(struct btrfs_header, chunk_tree_uuid);
-}
-
 static inline int btrfs_is_leaf(const struct extent_buffer *eb)
 {
 	return btrfs_header_level(eb) == 0;
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 63d009816264..77e1b66ebcfb 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3093,7 +3093,8 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 	chunk_root->commit_root = btrfs_root_node(chunk_root);
 
 	read_extent_buffer(chunk_root->node, fs_info->chunk_tree_uuid,
-	   btrfs_header_chunk_tree_uuid(chunk_root->node), BTRFS_UUID_SIZE);
+			   offsetof(struct btrfs_header, chunk_tree_uuid),
+			   BTRFS_UUID_SIZE);
 
 	ret = btrfs_read_chunk_tree(fs_info);
 	if (ret) {
-- 
2.25.0

Re: [PATCH 04/11] btrfs: open code trivial helper btrfs_header_chunk_tree_uuid

[Johannes Thumshirn]

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

[PATCH 05/11] btrfs: simplify parameters of btrfs_set_disk_extent_flags

[David Sterba]

All callers pass extent buffer start and length so the extent buffer
itself should work fine.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.c       | 4 +---
 fs/btrfs/ctree.h       | 2 +-
 fs/btrfs/extent-tree.c | 7 +++----
 3 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index b62721ac5ee8..f948435e87df 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -925,9 +925,7 @@ static noinline int update_ref_for_cow(struct btrfs_trans_handle *trans,
 		if (new_flags != 0) {
 			int level = btrfs_header_level(buf);
 
-			ret = btrfs_set_disk_extent_flags(trans,
-							  buf->start,
-							  buf->len,
+			ret = btrfs_set_disk_extent_flags(trans, buf,
 							  new_flags, level, 0);
 			if (ret)
 				return ret;
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index d7e54cbc8dce..2910ac257a5d 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2485,7 +2485,7 @@ int btrfs_inc_ref(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 int btrfs_dec_ref(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 		  struct extent_buffer *buf, int full_backref);
 int btrfs_set_disk_extent_flags(struct btrfs_trans_handle *trans,
-				u64 bytenr, u64 num_bytes, u64 flags,
+				struct extent_buffer *eb, u64 flags,
 				int level, int is_data);
 int btrfs_free_extent(struct btrfs_trans_handle *trans, struct btrfs_ref *ref);
 
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 7eef91d6c2b6..7affa6342688 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2231,7 +2231,7 @@ int btrfs_run_delayed_refs(struct btrfs_trans_handle *trans,
 }
 
 int btrfs_set_disk_extent_flags(struct btrfs_trans_handle *trans,
-				u64 bytenr, u64 num_bytes, u64 flags,
+				struct extent_buffer *eb, u64 flags,
 				int level, int is_data)
 {
 	struct btrfs_delayed_extent_op *extent_op;
@@ -2247,7 +2247,7 @@ int btrfs_set_disk_extent_flags(struct btrfs_trans_handle *trans,
 	extent_op->is_data = is_data ? true : false;
 	extent_op->level = level;
 
-	ret = btrfs_add_delayed_extent_op(trans, bytenr, num_bytes, extent_op);
+	ret = btrfs_add_delayed_extent_op(trans, eb->start, eb->len, extent_op);
 	if (ret)
 		btrfs_free_delayed_extent_op(extent_op);
 	return ret;
@@ -4741,8 +4741,7 @@ static noinline int walk_down_proc(struct btrfs_trans_handle *trans,
 		BUG_ON(ret); /* -ENOMEM */
 		ret = btrfs_dec_ref(trans, root, eb, 0);
 		BUG_ON(ret); /* -ENOMEM */
-		ret = btrfs_set_disk_extent_flags(trans, eb->start,
-						  eb->len, flag,
+		ret = btrfs_set_disk_extent_flags(trans, eb, flag,
 						  btrfs_header_level(eb), 0);
 		BUG_ON(ret); /* -ENOMEM */
 		wc->flags[level] |= flag;
-- 
2.25.0

Re: [PATCH 05/11] btrfs: simplify parameters of btrfs_set_disk_extent_flags

[Johannes Thumshirn]

A similar change should work for btrfs_add_delayed_extent_op() as well, 
shouldn't it?

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

Re: [PATCH 05/11] btrfs: simplify parameters of btrfs_set_disk_extent_flags

[David Sterba]

On Sat, Feb 22, 2020 at 08:45:32AM +0000, Johannes Thumshirn wrote:
> A similar change should work for btrfs_add_delayed_extent_op() as well, 
> shouldn't it?

Yes. Possibly pushing that further down the call chain to the delayed
refs, if all num_bytes is extent_buffer->len then this can be simplified
to fs_info->nodesize.

Re: [PATCH 05/11] btrfs: simplify parameters of btrfs_set_disk_extent_flags

[Anand Jain]

On 2/22/20 12:31 AM, David Sterba wrote:
> All callers pass extent buffer start and length so the extent buffer
> itself should work fine.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
Reviewed-by: Anand Jain <anand.jain@oracle.com>

[PATCH 06/11] btrfs: adjust message level for unrecognized mount option

[David Sterba]

An unrecognized option is a failure that should get user/administrator
attention, the info level is often below what gets logged, so make it
error.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 5c16b4bcde9b..e6784cd3f179 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -873,7 +873,7 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 			break;
 #endif
 		case Opt_err:
-			btrfs_info(info, "unrecognized mount option '%s'", p);
+			btrfs_err(info, "unrecognized mount option '%s'", p);
 			ret = -EINVAL;
 			goto out;
 		default:
-- 
2.25.0

Re: [PATCH 06/11] btrfs: adjust message level for unrecognized mount option

[Johannes Thumshirn]

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

Re: [PATCH 06/11] btrfs: adjust message level for unrecognized mount option

[Anand Jain]

On 2/22/20 12:31 AM, David Sterba wrote:
> An unrecognized option is a failure that should get user/administrator
> attention, the info level is often below what gets logged, so make it
> error.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

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

[PATCH 07/11] btrfs: raid56: simplify sort_parity_stripes

[David Sterba]

Remove trivial comprator and open coded swap of two values.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 997f2c70cb6c..945b89e2104f 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -5428,31 +5428,19 @@ static int find_live_mirror(struct btrfs_fs_info *fs_info,
 	return preferred_mirror;
 }
 
-static inline int parity_smaller(u64 a, u64 b)
-{
-	return a > b;
-}
-
 /* Bubble-sort the stripe set to put the parity/syndrome stripes last */
 static void sort_parity_stripes(struct btrfs_bio *bbio, int num_stripes)
 {
-	struct btrfs_bio_stripe s;
 	int i;
-	u64 l;
 	int again = 1;
 
 	while (again) {
 		again = 0;
 		for (i = 0; i < num_stripes - 1; i++) {
-			if (parity_smaller(bbio->raid_map[i],
-					   bbio->raid_map[i+1])) {
-				s = bbio->stripes[i];
-				l = bbio->raid_map[i];
-				bbio->stripes[i] = bbio->stripes[i+1];
-				bbio->raid_map[i] = bbio->raid_map[i+1];
-				bbio->stripes[i+1] = s;
-				bbio->raid_map[i+1] = l;
-
+			/* Swap if parity is on a smaller index */
+			if (bbio->raid_map[i] > bbio->raid_map[i + 1]) {
+				swap(bbio->stripes[i], bbio->stripes[i + 1]);
+				swap(bbio->raid_map[i], bbio->raid_map[i + 1]);
 				again = 1;
 			}
 		}
-- 
2.25.0

Re: [PATCH 07/11] btrfs: raid56: simplify sort_parity_stripes

[Johannes Thumshirn]

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

[PATCH 08/11] btrfs: replace u_long type cast with unsigned long

[David Sterba]

We don't use the u_XX types anywhere, though they're defined.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 945b89e2104f..fe1f609c736b 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6269,8 +6269,8 @@ static void submit_stripe_bio(struct btrfs_bio *bbio, struct bio *bio,
 	btrfs_debug_in_rcu(fs_info,
 	"btrfs_map_bio: rw %d 0x%x, sector=%llu, dev=%lu (%s id %llu), size=%u",
 		bio_op(bio), bio->bi_opf, (u64)bio->bi_iter.bi_sector,
-		(u_long)dev->bdev->bd_dev, rcu_str_deref(dev->name), dev->devid,
-		bio->bi_iter.bi_size);
+		(unsigned long)dev->bdev->bd_dev, rcu_str_deref(dev->name),
+		dev->devid, bio->bi_iter.bi_size);
 	bio_set_dev(bio, dev->bdev);
 
 	btrfs_bio_counter_inc_noblocked(fs_info);
-- 
2.25.0

Re: [PATCH 08/11] btrfs: replace u_long type cast with unsigned long

[Johannes Thumshirn]

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

Re: [PATCH 08/11] btrfs: replace u_long type cast with unsigned long

[Christoph Hellwig]

On Fri, Feb 21, 2020 at 05:31:17PM +0100, David Sterba wrote:
> We don't use the u_XX types anywhere, though they're defined.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/volumes.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 945b89e2104f..fe1f609c736b 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6269,8 +6269,8 @@ static void submit_stripe_bio(struct btrfs_bio *bbio, struct bio *bio,
>  	btrfs_debug_in_rcu(fs_info,
>  	"btrfs_map_bio: rw %d 0x%x, sector=%llu, dev=%lu (%s id %llu), size=%u",
>  		bio_op(bio), bio->bi_opf, (u64)bio->bi_iter.bi_sector,
> -		(u_long)dev->bdev->bd_dev, rcu_str_deref(dev->name), dev->devid,
> -		bio->bi_iter.bi_size);
> +		(unsigned long)dev->bdev->bd_dev, rcu_str_deref(dev->name),
> +		dev->devid, bio->bi_iter.bi_size);

Mostly we just print major and minor separately, which would remove the
cast entirely.

[PATCH 09/11] btrfs: adjust delayed refs message level

[David Sterba]

The message seems to be for debugging and has little value for users.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 77e1b66ebcfb..ab8d975f071c 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4280,7 +4280,7 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
 	spin_lock(&delayed_refs->lock);
 	if (atomic_read(&delayed_refs->num_entries) == 0) {
 		spin_unlock(&delayed_refs->lock);
-		btrfs_info(fs_info, "delayed_refs has NO entry");
+		btrfs_debug(fs_info, "delayed_refs has NO entry");
 		return ret;
 	}
 
-- 
2.25.0

Re: [PATCH 09/11] btrfs: adjust delayed refs message level

[Johannes Thumshirn]

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

Re: [PATCH 09/11] btrfs: adjust delayed refs message level

[Anand Jain]

On 2/22/20 12:31 AM, David Sterba wrote:
> The message seems to be for debugging and has little value for users.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---

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

[PATCH 10/11] btrfs: merge unlocking to common exit block in btrfs_commit_transaction

[David Sterba]

The tree_log_mutex and reloc_mutex locks are properly nested so we can
simplify error handling and add labels for them. This reduces line count
of the function.

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

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index fdfdfc426539..3610b6fec627 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -2194,10 +2194,8 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	 * core function of the snapshot creation.
 	 */
 	ret = create_pending_snapshots(trans);
-	if (ret) {
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret)
+		goto unlock_reloc;
 
 	/*
 	 * We insert the dir indexes of the snapshots and update the inode
@@ -2210,16 +2208,12 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	 * the nodes and leaves.
 	 */
 	ret = btrfs_run_delayed_items(trans);
-	if (ret) {
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret)
+		goto unlock_reloc;
 
 	ret = btrfs_run_delayed_refs(trans, (unsigned long)-1);
-	if (ret) {
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret)
+		goto unlock_reloc;
 
 	/*
 	 * make sure none of the code above managed to slip in a
@@ -2245,11 +2239,8 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	mutex_lock(&fs_info->tree_log_mutex);
 
 	ret = commit_fs_roots(trans);
-	if (ret) {
-		mutex_unlock(&fs_info->tree_log_mutex);
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret)
+		goto unlock_reloc;
 
 	/*
 	 * Since the transaction is done, we can apply the pending changes
@@ -2267,29 +2258,20 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	 * new delayed refs. Must handle them or qgroup can be wrong.
 	 */
 	ret = btrfs_run_delayed_refs(trans, (unsigned long)-1);
-	if (ret) {
-		mutex_unlock(&fs_info->tree_log_mutex);
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret)
+		goto unlock_tree_log;
 
 	/*
 	 * Since fs roots are all committed, we can get a quite accurate
 	 * new_roots. So let's do quota accounting.
 	 */
 	ret = btrfs_qgroup_account_extents(trans);
-	if (ret < 0) {
-		mutex_unlock(&fs_info->tree_log_mutex);
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret < 0)
+		goto unlock_tree_log;
 
 	ret = commit_cowonly_roots(trans);
-	if (ret) {
-		mutex_unlock(&fs_info->tree_log_mutex);
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret)
+		goto unlock_tree_log;
 
 	/*
 	 * The tasks which save the space cache and inode cache may also
@@ -2297,9 +2279,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	 */
 	if (TRANS_ABORTED(cur_trans)) {
 		ret = cur_trans->aborted;
-		mutex_unlock(&fs_info->tree_log_mutex);
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
+		goto unlock_tree_log;
 	}
 
 	btrfs_prepare_extent_commit(fs_info);
@@ -2346,8 +2326,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	if (ret) {
 		btrfs_handle_fs_error(fs_info, ret,
 				      "Error while writing out transaction");
-		mutex_unlock(&fs_info->tree_log_mutex);
-		goto scrub_continue;
+		goto unlock_tree_log;
 	}
 
 	ret = write_all_supers(fs_info, 0);
@@ -2394,6 +2373,10 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 
 	return ret;
 
+unlock_tree_log:
+	mutex_unlock(&fs_info->tree_log_mutex);
+unlock_reloc:
+	mutex_unlock(&fs_info->reloc_mutex);
 scrub_continue:
 	btrfs_scrub_continue(fs_info);
 cleanup_transaction:
-- 
2.25.0

Re: [PATCH 10/11] btrfs: merge unlocking to common exit block in btrfs_commit_transaction

[Johannes Thumshirn]

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

Re: [PATCH 10/11] btrfs: merge unlocking to common exit block in btrfs_commit_transaction

[Anand Jain]

On 2/22/20 12:31 AM, David Sterba wrote:
> The tree_log_mutex and reloc_mutex locks are properly nested so we can
> simplify error handling and add labels for them. This reduces line count
> of the function.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/transaction.c | 57 +++++++++++++++---------------------------
>   1 file changed, 20 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index fdfdfc426539..3610b6fec627 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -2194,10 +2194,8 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>   	 * core function of the snapshot creation.
>   	 */
>   	ret = create_pending_snapshots(trans);
> -	if (ret) {
> -		mutex_unlock(&fs_info->reloc_mutex);
> -		goto scrub_continue;
> -	}
> +	if (ret)
> +		goto unlock_reloc;
>   
>   	/*
>   	 * We insert the dir indexes of the snapshots and update the inode
> @@ -2210,16 +2208,12 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>   	 * the nodes and leaves.
>   	 */
>   	ret = btrfs_run_delayed_items(trans);
> -	if (ret) {
> -		mutex_unlock(&fs_info->reloc_mutex);
> -		goto scrub_continue;
> -	}
> +	if (ret)
> +		goto unlock_reloc;
>   
>   	ret = btrfs_run_delayed_refs(trans, (unsigned long)-1);
> -	if (ret) {
> -		mutex_unlock(&fs_info->reloc_mutex);
> -		goto scrub_continue;
> -	}
> +	if (ret)
> +		goto unlock_reloc;
>   
>   	/*
>   	 * make sure none of the code above managed to slip in a
> @@ -2245,11 +2239,8 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>   	mutex_lock(&fs_info->tree_log_mutex);
>   
>   	ret = commit_fs_roots(trans);
> -	if (ret) {
> -		mutex_unlock(&fs_info->tree_log_mutex);
> -		mutex_unlock(&fs_info->reloc_mutex);
> -		goto scrub_continue;
> -	}
> +	if (ret)
> +		goto unlock_reloc;

                goto unlock_tree_log;


Otherwise looks good. When fixed this you may add.

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

Thanks, Anand

>   	/*
>   	 * Since the transaction is done, we can apply the pending changes
> @@ -2267,29 +2258,20 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>   	 * new delayed refs. Must handle them or qgroup can be wrong.
>   	 */
>   	ret = btrfs_run_delayed_refs(trans, (unsigned long)-1);
> -	if (ret) {
> -		mutex_unlock(&fs_info->tree_log_mutex);
> -		mutex_unlock(&fs_info->reloc_mutex);
> -		goto scrub_continue;
> -	}
> +	if (ret)
> +		goto unlock_tree_log;
>   
>   	/*
>   	 * Since fs roots are all committed, we can get a quite accurate
>   	 * new_roots. So let's do quota accounting.
>   	 */
>   	ret = btrfs_qgroup_account_extents(trans);
> -	if (ret < 0) {
> -		mutex_unlock(&fs_info->tree_log_mutex);
> -		mutex_unlock(&fs_info->reloc_mutex);
> -		goto scrub_continue;
> -	}
> +	if (ret < 0)
> +		goto unlock_tree_log;
>   
>   	ret = commit_cowonly_roots(trans);
> -	if (ret) {
> -		mutex_unlock(&fs_info->tree_log_mutex);
> -		mutex_unlock(&fs_info->reloc_mutex);
> -		goto scrub_continue;
> -	}
> +	if (ret)
> +		goto unlock_tree_log;
>   
>   	/*
>   	 * The tasks which save the space cache and inode cache may also
> @@ -2297,9 +2279,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>   	 */
>   	if (TRANS_ABORTED(cur_trans)) {
>   		ret = cur_trans->aborted;
> -		mutex_unlock(&fs_info->tree_log_mutex);
> -		mutex_unlock(&fs_info->reloc_mutex);
> -		goto scrub_continue;
> +		goto unlock_tree_log;
>   	}
>   
>   	btrfs_prepare_extent_commit(fs_info);
> @@ -2346,8 +2326,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>   	if (ret) {
>   		btrfs_handle_fs_error(fs_info, ret,
>   				      "Error while writing out transaction");
> -		mutex_unlock(&fs_info->tree_log_mutex);
> -		goto scrub_continue;
> +		goto unlock_tree_log;
>   	}
>   
>   	ret = write_all_supers(fs_info, 0);
> @@ -2394,6 +2373,10 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>   
>   	return ret;
>   
> +unlock_tree_log:
> +	mutex_unlock(&fs_info->tree_log_mutex);
> +unlock_reloc:
> +	mutex_unlock(&fs_info->reloc_mutex);
>   scrub_continue:
>   	btrfs_scrub_continue(fs_info);
>   cleanup_transaction:
> 

Re: [PATCH 10/11] btrfs: merge unlocking to common exit block in btrfs_commit_transaction

[David Sterba]

On Fri, Feb 21, 2020 at 05:31:22PM +0100, David Sterba wrote:
>  	btrfs_prepare_extent_commit(fs_info);
> @@ -2346,8 +2326,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>  	if (ret) {
>  		btrfs_handle_fs_error(fs_info, ret,
>  				      "Error while writing out transaction");
> -		mutex_unlock(&fs_info->tree_log_mutex);
> -		goto scrub_continue;
> +		goto unlock_tree_log;

Hm and this one is also wrong, in other cases that jump to
unlock_tree_log the unlocking order is tree_log_mutex/reloc_mutex, while
a few lines before there is unlock of reloc_mutex (while tree_log_mutex
is still held). This means the unlocking order is reversed compared to
the other cases and we can't jump to the label as this would lead to
double unlock of reloc_mutex.

So the above hunk must stay as is, with a comment.

>  	}
>  
>  	ret = write_all_supers(fs_info, 0);
> @@ -2394,6 +2373,10 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>  
>  	return ret;
>  
> +unlock_tree_log:
> +	mutex_unlock(&fs_info->tree_log_mutex);
> +unlock_reloc:
> +	mutex_unlock(&fs_info->reloc_mutex);
>  scrub_continue:
>  	btrfs_scrub_continue(fs_info);
>  cleanup_transaction:
> -- 
> 2.25.0

[PATCH v2] btrfs: merge unlocking to common exit block in btrfs_commit_transaction

[David Sterba]

The tree_log_mutex and reloc_mutex locks are properly nested so we can
simplify error handling and add labels for them. This reduces line count
of the function.

Signed-off-by: David Sterba <dsterba@suse.com>
---

v2:
- fixed label after commit_fs_roots, to point to unlock_tree_log
- added comment after btrfs_handle_fs_error and keep the locks as is

 fs/btrfs/transaction.c | 58 +++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 35 deletions(-)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index fdfdfc426539..702e0f2b8307 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -2194,10 +2194,8 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	 * core function of the snapshot creation.
 	 */
 	ret = create_pending_snapshots(trans);
-	if (ret) {
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret)
+		goto unlock_reloc;
 
 	/*
 	 * We insert the dir indexes of the snapshots and update the inode
@@ -2210,16 +2208,12 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	 * the nodes and leaves.
 	 */
 	ret = btrfs_run_delayed_items(trans);
-	if (ret) {
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret)
+		goto unlock_reloc;
 
 	ret = btrfs_run_delayed_refs(trans, (unsigned long)-1);
-	if (ret) {
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret)
+		goto unlock_reloc;
 
 	/*
 	 * make sure none of the code above managed to slip in a
@@ -2245,11 +2239,8 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	mutex_lock(&fs_info->tree_log_mutex);
 
 	ret = commit_fs_roots(trans);
-	if (ret) {
-		mutex_unlock(&fs_info->tree_log_mutex);
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret)
+		goto unlock_tree_log;
 
 	/*
 	 * Since the transaction is done, we can apply the pending changes
@@ -2267,29 +2258,20 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	 * new delayed refs. Must handle them or qgroup can be wrong.
 	 */
 	ret = btrfs_run_delayed_refs(trans, (unsigned long)-1);
-	if (ret) {
-		mutex_unlock(&fs_info->tree_log_mutex);
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret)
+		goto unlock_tree_log;
 
 	/*
 	 * Since fs roots are all committed, we can get a quite accurate
 	 * new_roots. So let's do quota accounting.
 	 */
 	ret = btrfs_qgroup_account_extents(trans);
-	if (ret < 0) {
-		mutex_unlock(&fs_info->tree_log_mutex);
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret < 0)
+		goto unlock_tree_log;
 
 	ret = commit_cowonly_roots(trans);
-	if (ret) {
-		mutex_unlock(&fs_info->tree_log_mutex);
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret)
+		goto unlock_tree_log;
 
 	/*
 	 * The tasks which save the space cache and inode cache may also
@@ -2297,9 +2279,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	 */
 	if (TRANS_ABORTED(cur_trans)) {
 		ret = cur_trans->aborted;
-		mutex_unlock(&fs_info->tree_log_mutex);
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
+		goto unlock_tree_log;
 	}
 
 	btrfs_prepare_extent_commit(fs_info);
@@ -2346,6 +2326,10 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	if (ret) {
 		btrfs_handle_fs_error(fs_info, ret,
 				      "Error while writing out transaction");
+		/*
+		 * reloc_mutex has been unlocked, tree_log_mutex is still held
+		 * but we can't jump to unlock_tree_log causing double unlock
+		 */
 		mutex_unlock(&fs_info->tree_log_mutex);
 		goto scrub_continue;
 	}
@@ -2394,6 +2378,10 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 
 	return ret;
 
+unlock_tree_log:
+	mutex_unlock(&fs_info->tree_log_mutex);
+unlock_reloc:
+	mutex_unlock(&fs_info->reloc_mutex);
 scrub_continue:
 	btrfs_scrub_continue(fs_info);
 cleanup_transaction:
-- 
2.25.0

Re: [PATCH v2] btrfs: merge unlocking to common exit block in btrfs_commit_transaction

[Anand Jain]

On 2/24/20 11:13 PM, David Sterba wrote:
> The tree_log_mutex and reloc_mutex locks are properly nested so we can
> simplify error handling and add labels for them. This reduces line count
> of the function.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> 
> v2:
> - fixed label after commit_fs_roots, to point to unlock_tree_log
> - added comment after btrfs_handle_fs_error and keep the locks as is

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

[PATCH 11/11] btrfs: reduce pointer intdirections in btree_readpage_end_io_hook

[David Sterba]

All we need to read is checksum size from fs_info superblock, and
fs_info is provided by extent buffer so we can get rid of the wild
pointer indirections from page/inode/root.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index ab8d975f071c..bccdb55ececf 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -604,9 +604,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
 	u64 found_start;
 	int found_level;
 	struct extent_buffer *eb;
-	struct btrfs_root *root = BTRFS_I(page->mapping->host)->root;
-	struct btrfs_fs_info *fs_info = root->fs_info;
-	u16 csum_size = btrfs_super_csum_size(fs_info->super_copy);
+	struct btrfs_fs_info *fs_info;
+	u16 csum_size;
 	int ret = 0;
 	u8 result[BTRFS_CSUM_SIZE];
 	int reads_done;
@@ -615,6 +614,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
 		goto out;
 
 	eb = (struct extent_buffer *)page->private;
+	fs_info = eb->fs_info;
+	csum_size = btrfs_super_csum_size(fs_info->super_copy);
 
 	/* the pending IO might have been the only thing that kept this buffer
 	 * in memory.  Make sure we have a ref for all this other checks
-- 
2.25.0

Re: [PATCH 11/11] btrfs: reduce pointer intdirections in btree_readpage_end_io_hook

[Johannes Thumshirn]

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