[PATCH 0/6] Minor cleanups

[PATCH 0/6] Minor cleanups

[David Sterba]

A few misc cleanups selected from my other branche, all should be safe.

David Sterba (6):
  btrfs: use common helpers for eb leak messages
  btrfs: use common helpers for extent IO state insertion messages
  btrfs: drop default value assignments in enums
  btrfs: use raid_attr to adjust minimal stripe size in
    btrfs_calc_avail_data_space
  btrfs: use raid_attr for minimum stripe count in
    btrfs_calc_avail_data_space
  btrfs: lift bio_set_dev from bio allocation helpers

 fs/btrfs/compression.c | 12 ++++++++----
 fs/btrfs/extent-tree.c | 14 +++++++-------
 fs/btrfs/extent_io.c   | 23 ++++++++++++++---------
 fs/btrfs/extent_io.h   |  2 +-
 fs/btrfs/file.c        |  6 +++---
 fs/btrfs/super.c       | 15 +++++++--------
 6 files changed, 40 insertions(+), 32 deletions(-)

-- 
2.21.0

[PATCH 1/6] btrfs: use common helpers for eb leak messages

[David Sterba]

The extent buffer leak detector is a debugging feature, let's use the
existing helper that prints the information about the filesystem,
unlike pr_err.

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

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 464e6b761a9c..8634eda07b7a 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -67,7 +67,8 @@ void btrfs_leak_debug_check(void)
 
 	while (!list_empty(&states)) {
 		state = list_entry(states.next, struct extent_state, leak_list);
-		pr_err("BTRFS: state leak: start %llu end %llu state %u in tree %d refs %d\n",
+		btrfs_debug_rl(eb->fs_info,
+		"state leak: start %llu end %llu state %u in tree %d refs %d",
 		       state->start, state->end, state->state,
 		       extent_state_in_tree(state),
 		       refcount_read(&state->refs));
@@ -77,7 +78,8 @@ void btrfs_leak_debug_check(void)
 
 	while (!list_empty(&buffers)) {
 		eb = list_entry(buffers.next, struct extent_buffer, leak_list);
-		pr_err("BTRFS: buffer leak start %llu len %lu refs %d bflags %lu\n",
+		btrfs_debug_rl(eb->fs_info,
+		       "buffer leak start %llu len %lu refs %d bflags %lu",
 		       eb->start, eb->len, atomic_read(&eb->refs), eb->bflags);
 		list_del(&eb->leak_list);
 		kmem_cache_free(extent_buffer_cache, eb);
-- 
2.21.0

[PATCH 2/6] btrfs: use common helpers for extent IO state insertion messages

[David Sterba]

Print the error messages using the helpers that also print the
filesystem identification.

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

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 8634eda07b7a..a6ad2f6f2bf7 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -524,9 +524,11 @@ static int insert_state(struct extent_io_tree *tree,
 {
 	struct rb_node *node;
 
-	if (end < start)
-		WARN(1, KERN_ERR "BTRFS: end < start %llu %llu\n",
-		       end, start);
+	if (end < start) {
+		btrfs_err(tree->fs_info,
+			"insert state: end < start %llu %llu", end, start);
+		WARN_ON(1);
+	}
 	state->start = start;
 	state->end = end;
 
@@ -536,7 +538,8 @@ static int insert_state(struct extent_io_tree *tree,
 	if (node) {
 		struct extent_state *found;
 		found = rb_entry(node, struct extent_state, rb_node);
-		pr_err("BTRFS: found node %llu %llu on insert of %llu %llu\n",
+		btrfs_err(tree->fs_info,
+		       "found node %llu %llu on insert of %llu %llu",
 		       found->start, found->end, start, end);
 		return -EEXIST;
 	}
-- 
2.21.0

Re: [PATCH 2/6] btrfs: use common helpers for extent IO state insertion messages

[Nikolay Borisov]

On 18.06.19 г. 21:00 ч., David Sterba wrote:
> Print the error messages using the helpers that also print the
> filesystem identification.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/extent_io.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 8634eda07b7a..a6ad2f6f2bf7 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -524,9 +524,11 @@ static int insert_state(struct extent_io_tree *tree,
>  {
>  	struct rb_node *node;
>  
> -	if (end < start)
> -		WARN(1, KERN_ERR "BTRFS: end < start %llu %llu\n",
> -		       end, start);
> +	if (end < start) {
> +		btrfs_err(tree->fs_info,
> +			"insert state: end < start %llu %llu", end, start);
> +		WARN_ON(1);
> +	}

nit: if (WARN_ON(end < start))
       btrfs_err(...)

>  	state->start = start;
>  	state->end = end;
>  
> @@ -536,7 +538,8 @@ static int insert_state(struct extent_io_tree *tree,
>  	if (node) {
>  		struct extent_state *found;
>  		found = rb_entry(node, struct extent_state, rb_node);
> -		pr_err("BTRFS: found node %llu %llu on insert of %llu %llu\n",
> +		btrfs_err(tree->fs_info,
> +		       "found node %llu %llu on insert of %llu %llu",
>  		       found->start, found->end, start, end);
>  		return -EEXIST;
>  	}
> 

Re: [PATCH 2/6] btrfs: use common helpers for extent IO state insertion messages

[David Sterba]

On Wed, Jun 19, 2019 at 09:54:16AM +0300, Nikolay Borisov wrote:
> 
> 
> On 18.06.19 г. 21:00 ч., David Sterba wrote:
> > Print the error messages using the helpers that also print the
> > filesystem identification.
> > 
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >  fs/btrfs/extent_io.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index 8634eda07b7a..a6ad2f6f2bf7 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -524,9 +524,11 @@ static int insert_state(struct extent_io_tree *tree,
> >  {
> >  	struct rb_node *node;
> >  
> > -	if (end < start)
> > -		WARN(1, KERN_ERR "BTRFS: end < start %llu %llu\n",
> > -		       end, start);
> > +	if (end < start) {
> > +		btrfs_err(tree->fs_info,
> > +			"insert state: end < start %llu %llu", end, start);
> > +		WARN_ON(1);
> > +	}
> 
> nit: if (WARN_ON(end < start))
>        btrfs_err(...)

That's not the same. The message is printed after the warning and with
panic-on-warn it's not printed at all. WARN prints the format string
first, so btrfs_err+WARN_ON preserves that.

[PATCH 3/6] btrfs: drop default value assignments in enums

[David Sterba]

A few more instances whre we don't need to specify the values as long as
they are the same that enum assigns automatically. All of the enums are
in-memory only and nothing relies on the exact values.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/extent-tree.c | 14 +++++++-------
 fs/btrfs/file.c        |  6 +++---
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 7d26ec1e8608..fa7ff6b81fe0 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -46,9 +46,9 @@
  *
  */
 enum {
-	CHUNK_ALLOC_NO_FORCE = 0,
-	CHUNK_ALLOC_LIMITED = 1,
-	CHUNK_ALLOC_FORCE = 2,
+	CHUNK_ALLOC_NO_FORCE,
+	CHUNK_ALLOC_LIMITED,
+	CHUNK_ALLOC_FORCE,
 };
 
 /*
@@ -7302,10 +7302,10 @@ wait_block_group_cache_done(struct btrfs_block_group_cache *cache)
 }
 
 enum btrfs_loop_type {
-	LOOP_CACHING_NOWAIT = 0,
-	LOOP_CACHING_WAIT = 1,
-	LOOP_ALLOC_CHUNK = 2,
-	LOOP_NO_EMPTY_SIZE = 3,
+	LOOP_CACHING_NOWAIT,
+	LOOP_CACHING_WAIT,
+	LOOP_ALLOC_CHUNK,
+	LOOP_NO_EMPTY_SIZE,
 };
 
 static inline void
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 5370152ea7e3..3c508d2a85bf 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -2791,9 +2791,9 @@ static int btrfs_fallocate_update_isize(struct inode *inode,
 }
 
 enum {
-	RANGE_BOUNDARY_WRITTEN_EXTENT = 0,
-	RANGE_BOUNDARY_PREALLOC_EXTENT = 1,
-	RANGE_BOUNDARY_HOLE = 2,
+	RANGE_BOUNDARY_WRITTEN_EXTENT,
+	RANGE_BOUNDARY_PREALLOC_EXTENT,
+	RANGE_BOUNDARY_HOLE,
 };
 
 static int btrfs_zero_range_check_range_boundary(struct inode *inode,
-- 
2.21.0

[PATCH 4/6] btrfs: use raid_attr to adjust minimal stripe size in btrfs_calc_avail_data_space

[David Sterba]

Special case for DUP can be replaced by lookup to the attribute table,
where the dev_stripes is the right coefficient.

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

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 6e196b8a0820..a813b582fa72 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1904,6 +1904,7 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
 	u64 min_stripe_size;
 	int min_stripes = 1, num_stripes = 1;
 	int i = 0, nr_devices;
+	const struct btrfs_raid_attr *rattr;
 
 	/*
 	 * We aren't under the device list lock, so this is racy-ish, but good
@@ -1927,6 +1928,8 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
 
 	/* calc min stripe number for data space allocation */
 	type = btrfs_data_alloc_profile(fs_info);
+	rattr = &btrfs_raid_array[btrfs_bg_flags_to_raid_index(type)];
+	ASSERT(rattr);
 	if (type & BTRFS_BLOCK_GROUP_RAID0) {
 		min_stripes = 2;
 		num_stripes = nr_devices;
@@ -1938,10 +1941,8 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
 		num_stripes = 4;
 	}
 
-	if (type & BTRFS_BLOCK_GROUP_DUP)
-		min_stripe_size = 2 * BTRFS_STRIPE_LEN;
-	else
-		min_stripe_size = BTRFS_STRIPE_LEN;
+	/* Adjust for more than 1 stripe per device */
+	min_stripe_size = rattr->dev_stripes * BTRFS_STRIPE_LEN;
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(device, &fs_devices->devices, dev_list) {
-- 
2.21.0

Re: [PATCH 4/6] btrfs: use raid_attr to adjust minimal stripe size in btrfs_calc_avail_data_space

[Nikolay Borisov]

On 18.06.19 г. 21:00 ч., David Sterba wrote:
> Special case for DUP can be replaced by lookup to the attribute table,
> where the dev_stripes is the right coefficient.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/super.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 6e196b8a0820..a813b582fa72 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1904,6 +1904,7 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
>  	u64 min_stripe_size;
>  	int min_stripes = 1, num_stripes = 1;
>  	int i = 0, nr_devices;
> +	const struct btrfs_raid_attr *rattr;
>  
>  	/*
>  	 * We aren't under the device list lock, so this is racy-ish, but good
> @@ -1927,6 +1928,8 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
>  
>  	/* calc min stripe number for data space allocation */
>  	type = btrfs_data_alloc_profile(fs_info);
> +	rattr = &btrfs_raid_array[btrfs_bg_flags_to_raid_index(type)];
> +	ASSERT(rattr);

That ASSERT is a noop since btrfs_bg_flags_to_raid_index always returns
a valid index.

>  	if (type & BTRFS_BLOCK_GROUP_RAID0) {
>  		min_stripes = 2;
>  		num_stripes = nr_devices;
> @@ -1938,10 +1941,8 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
>  		num_stripes = 4;
>  	}
>  
> -	if (type & BTRFS_BLOCK_GROUP_DUP)
> -		min_stripe_size = 2 * BTRFS_STRIPE_LEN;
> -	else
> -		min_stripe_size = BTRFS_STRIPE_LEN;
> +	/* Adjust for more than 1 stripe per device */
> +	min_stripe_size = rattr->dev_stripes * BTRFS_STRIPE_LEN;
>  
>  	rcu_read_lock();
>  	list_for_each_entry_rcu(device, &fs_devices->devices, dev_list) {
> 

[PATCH 5/6] btrfs: use raid_attr for minimum stripe count in btrfs_calc_avail_data_space

[David Sterba]

Minimum stripe count matches the minimum devices required for a given
profile. The open coded assignments match the raid_attr table.

What's changed here is the meaning for RAID5/6. Previously their
min_stripes would be 1, while newly it's devs_min. This however shold be
the same as before because it's not possible to create filesystem on
fewer devices than the raid_attr table allows.

There's no adjustment regarding the parity stripes (like
calc_data_stripes does), because we're interested in overall space that
would fit on the devices.

Missing devices make no difference for the whole calculation, we have
the size stored in the structures.

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

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index a813b582fa72..9286f9e49c0c 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1902,7 +1902,7 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
 	u64 type;
 	u64 avail_space;
 	u64 min_stripe_size;
-	int min_stripes = 1, num_stripes = 1;
+	int min_stripes, num_stripes = 1;
 	int i = 0, nr_devices;
 	const struct btrfs_raid_attr *rattr;
 
@@ -1930,14 +1930,12 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
 	type = btrfs_data_alloc_profile(fs_info);
 	rattr = &btrfs_raid_array[btrfs_bg_flags_to_raid_index(type)];
 	ASSERT(rattr);
+	min_stripes = rattr->devs_min;
 	if (type & BTRFS_BLOCK_GROUP_RAID0) {
-		min_stripes = 2;
 		num_stripes = nr_devices;
 	} else if (type & BTRFS_BLOCK_GROUP_RAID1) {
-		min_stripes = 2;
 		num_stripes = 2;
 	} else if (type & BTRFS_BLOCK_GROUP_RAID10) {
-		min_stripes = 4;
 		num_stripes = 4;
 	}
 
-- 
2.21.0

Re: [PATCH 5/6] btrfs: use raid_attr for minimum stripe count in btrfs_calc_avail_data_space

[Nikolay Borisov]

[-- Attachment #1: Type: text/plain, Size: 907 bytes --]



On 18.06.19 г. 21:00 ч., David Sterba wrote:
> Minimum stripe count matches the minimum devices required for a given
> profile. The open coded assignments match the raid_attr table.
> 
> What's changed here is the meaning for RAID5/6. Previously their
> min_stripes would be 1, while newly it's devs_min. This however shold be
> the same as before because it's not possible to create filesystem on
> fewer devices than the raid_attr table allows.
> 
> There's no adjustment regarding the parity stripes (like
> calc_data_stripes does), because we're interested in overall space that
> would fit on the devices.
> 
> Missing devices make no difference for the whole calculation, we have
> the size stored in the structures.

I think the clean up in this function should include more here's list of
things which I think will make it more readable. Something like the
attached diff on top of your patch:



[-- Attachment #2: calc-avail-space.diff --]
[-- Type: text/x-patch, Size: 1987 bytes --]

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 6e196b8a0820..230aef8314da 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1898,11 +1898,10 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
 	struct btrfs_device_info *devices_info;
 	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
 	struct btrfs_device *device;
-	u64 skip_space;
 	u64 type;
 	u64 avail_space;
 	u64 min_stripe_size;
-	int min_stripes = 1, num_stripes = 1;
+	int num_stripes = 1;
 	int i = 0, nr_devices;
 
 	/*
@@ -1957,28 +1956,21 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
 		avail_space = device->total_bytes - device->bytes_used;
 
 		/* align with stripe_len */
-		avail_space = div_u64(avail_space, BTRFS_STRIPE_LEN);
-		avail_space *= BTRFS_STRIPE_LEN;
+		avail_space = rounddown(avail_space, BTRFS_STRIPE_LEN);
 
 		/*
 		 * In order to avoid overwriting the superblock on the drive,
 		 * btrfs starts at an offset of at least 1MB when doing chunk
 		 * allocation.
+		 *
+		 * This ensures we have at least min_stripe_size free space
+		 * after excluding 1mb.
 		 */
-		skip_space = SZ_1M;
-
-		/*
-		 * we can use the free space in [0, skip_space - 1], subtract
-		 * it from the total.
-		 */
-		if (avail_space && avail_space >= skip_space)
-			avail_space -= skip_space;
-		else
-			avail_space = 0;
-
-		if (avail_space < min_stripe_size)
+		if (avail_space <= SZ_1M + min_stripe_size)
 			continue;
 
+		avail_space -= SZ_1M;
+
 		devices_info[i].dev = device;
 		devices_info[i].max_avail = avail_space;
 
@@ -1992,9 +1984,8 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
 
 	i = nr_devices - 1;
 	avail_space = 0;
-	while (nr_devices >= min_stripes) {
-		if (num_stripes > nr_devices)
-			num_stripes = nr_devices;
+	while (nr_devices >= rattr->devs_min) {
+		num_stripes = min(num_stripes, nr_devices);
 
 		if (devices_info[i].max_avail >= min_stripe_size) {
 			int j;

Re: [PATCH 5/6] btrfs: use raid_attr for minimum stripe count in btrfs_calc_avail_data_space

[David Sterba]

On Wed, Jun 19, 2019 at 10:51:14AM +0300, Nikolay Borisov wrote:
> 
> 
> On 18.06.19 г. 21:00 ч., David Sterba wrote:
> > Minimum stripe count matches the minimum devices required for a given
> > profile. The open coded assignments match the raid_attr table.
> > 
> > What's changed here is the meaning for RAID5/6. Previously their
> > min_stripes would be 1, while newly it's devs_min. This however shold be
> > the same as before because it's not possible to create filesystem on
> > fewer devices than the raid_attr table allows.
> > 
> > There's no adjustment regarding the parity stripes (like
> > calc_data_stripes does), because we're interested in overall space that
> > would fit on the devices.
> > 
> > Missing devices make no difference for the whole calculation, we have
> > the size stored in the structures.
> 
> I think the clean up in this function should include more here's list of
> things which I think will make it more readable.

I did not intend to clean up the whole function in this patch, only whre
the raid table could be used.

> Something like the
> attached diff on top of your patch:
> 
> 

> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 6e196b8a0820..230aef8314da 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1898,11 +1898,10 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
>  	struct btrfs_device_info *devices_info;
>  	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
>  	struct btrfs_device *device;
> -	u64 skip_space;
>  	u64 type;
>  	u64 avail_space;
>  	u64 min_stripe_size;
> -	int min_stripes = 1, num_stripes = 1;
> +	int num_stripes = 1;
>  	int i = 0, nr_devices;
>  
>  	/*
> @@ -1957,28 +1956,21 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
>  		avail_space = device->total_bytes - device->bytes_used;
>  
>  		/* align with stripe_len */
> -		avail_space = div_u64(avail_space, BTRFS_STRIPE_LEN);
> -		avail_space *= BTRFS_STRIPE_LEN;
> +		avail_space = rounddown(avail_space, BTRFS_STRIPE_LEN);

As long as the stripe length is a constant, this is fine. rounddown uses
% (modulo) so this can be come division that will not work for u64
types.

>  
>  		/*
>  		 * In order to avoid overwriting the superblock on the drive,
>  		 * btrfs starts at an offset of at least 1MB when doing chunk
>  		 * allocation.
> +		 *
> +		 * This ensures we have at least min_stripe_size free space
> +		 * after excluding 1mb.
>  		 */
> -		skip_space = SZ_1M;
> -
> -		/*
> -		 * we can use the free space in [0, skip_space - 1], subtract
> -		 * it from the total.
> -		 */
> -		if (avail_space && avail_space >= skip_space)
> -			avail_space -= skip_space;
> -		else
> -			avail_space = 0;
> -
> -		if (avail_space < min_stripe_size)
> +		if (avail_space <= SZ_1M + min_stripe_size)
>  			continue;
>  
> +		avail_space -= SZ_1M;
> +
>  		devices_info[i].dev = device;
>  		devices_info[i].max_avail = avail_space;
>  
> @@ -1992,9 +1984,8 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
>  
>  	i = nr_devices - 1;
>  	avail_space = 0;
> -	while (nr_devices >= min_stripes) {
> -		if (num_stripes > nr_devices)
> -			num_stripes = nr_devices;
> +	while (nr_devices >= rattr->devs_min) {
> +		num_stripes = min(num_stripes, nr_devices);
>  
>  		if (devices_info[i].max_avail >= min_stripe_size) {
>  			int j;

All of the above look good to me, feel free to send them as proper
patches.

[PATCH 6/6] btrfs: lift bio_set_dev from bio allocation helpers

[David Sterba]

The block device is passed around for the only purpose to set it in new
bios. Move the assignment one level up. This is a preparatory patch for
further bdev cleanups.

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

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index db41315f11eb..60c47b417a4b 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -340,7 +340,8 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start,
 
 	bdev = fs_info->fs_devices->latest_bdev;
 
-	bio = btrfs_bio_alloc(bdev, first_byte);
+	bio = btrfs_bio_alloc(first_byte);
+	bio_set_dev(bio, bdev);
 	bio->bi_opf = REQ_OP_WRITE | write_flags;
 	bio->bi_private = cb;
 	bio->bi_end_io = end_compressed_bio_write;
@@ -382,7 +383,8 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start,
 				bio_endio(bio);
 			}
 
-			bio = btrfs_bio_alloc(bdev, first_byte);
+			bio = btrfs_bio_alloc(first_byte);
+			bio_set_dev(bio, bdev);
 			bio->bi_opf = REQ_OP_WRITE | write_flags;
 			bio->bi_private = cb;
 			bio->bi_end_io = end_compressed_bio_write;
@@ -620,7 +622,8 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	/* include any pages we added in add_ra-bio_pages */
 	cb->len = bio->bi_iter.bi_size;
 
-	comp_bio = btrfs_bio_alloc(bdev, cur_disk_byte);
+	comp_bio = btrfs_bio_alloc(cur_disk_byte);
+	bio_set_dev(comp_bio, bdev);
 	comp_bio->bi_opf = REQ_OP_READ;
 	comp_bio->bi_private = cb;
 	comp_bio->bi_end_io = end_compressed_bio_read;
@@ -670,7 +673,8 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 				bio_endio(comp_bio);
 			}
 
-			comp_bio = btrfs_bio_alloc(bdev, cur_disk_byte);
+			comp_bio = btrfs_bio_alloc(cur_disk_byte);
+			bio_set_dev(comp_bio, bdev);
 			comp_bio->bi_opf = REQ_OP_READ;
 			comp_bio->bi_private = cb;
 			comp_bio->bi_end_io = end_compressed_bio_read;
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index a6ad2f6f2bf7..f21be5d3724f 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2863,12 +2863,11 @@ static inline void btrfs_io_bio_init(struct btrfs_io_bio *btrfs_bio)
  * never fail.  We're returning a bio right now but you can call btrfs_io_bio
  * for the appropriate container_of magic
  */
-struct bio *btrfs_bio_alloc(struct block_device *bdev, u64 first_byte)
+struct bio *btrfs_bio_alloc(u64 first_byte)
 {
 	struct bio *bio;
 
 	bio = bio_alloc_bioset(GFP_NOFS, BIO_MAX_PAGES, &btrfs_bioset);
-	bio_set_dev(bio, bdev);
 	bio->bi_iter.bi_sector = first_byte >> 9;
 	btrfs_io_bio_init(btrfs_io_bio(bio));
 	return bio;
@@ -2979,7 +2978,8 @@ static int submit_extent_page(unsigned int opf, struct extent_io_tree *tree,
 		}
 	}
 
-	bio = btrfs_bio_alloc(bdev, offset);
+	bio = btrfs_bio_alloc(offset);
+	bio_set_dev(bio, bdev);
 	bio_add_page(bio, page, page_size, pg_offset);
 	bio->bi_end_io = end_io_func;
 	bio->bi_private = tree;
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 844e595cde5b..6e13a62a2974 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -497,7 +497,7 @@ void extent_clear_unlock_delalloc(struct inode *inode, u64 start, u64 end,
 				 u64 delalloc_end, struct page *locked_page,
 				 unsigned bits_to_clear,
 				 unsigned long page_ops);
-struct bio *btrfs_bio_alloc(struct block_device *bdev, u64 first_byte);
+struct bio *btrfs_bio_alloc(u64 first_byte);
 struct bio *btrfs_io_bio_alloc(unsigned int nr_iovecs);
 struct bio *btrfs_bio_clone(struct bio *bio);
 struct bio *btrfs_bio_clone_partial(struct bio *orig, int offset, int size);
-- 
2.21.0

Re: [PATCH 6/6] btrfs: lift bio_set_dev from bio allocation helpers

[Nikolay Borisov]

On 18.06.19 г. 21:00 ч., David Sterba wrote:
> The block device is passed around for the only purpose to set it in new
> bios. Move the assignment one level up. This is a preparatory patch for
> further bdev cleanups.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Albeit I'd go as far as suggesting to make btrfs_bio_alloc a void
function similar to the generic bio alloc functions. It should be up to
the callers of bio alloc functions to initialize the bio in one place.
Even after your patch initialization is split across btrfs_bio_alloc and
its caller which seems a bit idiosyncratic.

> ---
>  fs/btrfs/compression.c | 12 ++++++++----
>  fs/btrfs/extent_io.c   |  6 +++---
>  fs/btrfs/extent_io.h   |  2 +-
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index db41315f11eb..60c47b417a4b 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -340,7 +340,8 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start,
>  
>  	bdev = fs_info->fs_devices->latest_bdev;
>  
> -	bio = btrfs_bio_alloc(bdev, first_byte);
> +	bio = btrfs_bio_alloc(first_byte);
> +	bio_set_dev(bio, bdev);
>  	bio->bi_opf = REQ_OP_WRITE | write_flags;
>  	bio->bi_private = cb;
>  	bio->bi_end_io = end_compressed_bio_write;
> @@ -382,7 +383,8 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start,
>  				bio_endio(bio);
>  			}
>  
> -			bio = btrfs_bio_alloc(bdev, first_byte);
> +			bio = btrfs_bio_alloc(first_byte);
> +			bio_set_dev(bio, bdev);
>  			bio->bi_opf = REQ_OP_WRITE | write_flags;
>  			bio->bi_private = cb;
>  			bio->bi_end_io = end_compressed_bio_write;
> @@ -620,7 +622,8 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  	/* include any pages we added in add_ra-bio_pages */
>  	cb->len = bio->bi_iter.bi_size;
>  
> -	comp_bio = btrfs_bio_alloc(bdev, cur_disk_byte);
> +	comp_bio = btrfs_bio_alloc(cur_disk_byte);
> +	bio_set_dev(comp_bio, bdev);
>  	comp_bio->bi_opf = REQ_OP_READ;
>  	comp_bio->bi_private = cb;
>  	comp_bio->bi_end_io = end_compressed_bio_read;
> @@ -670,7 +673,8 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  				bio_endio(comp_bio);
>  			}
>  
> -			comp_bio = btrfs_bio_alloc(bdev, cur_disk_byte);
> +			comp_bio = btrfs_bio_alloc(cur_disk_byte);
> +			bio_set_dev(comp_bio, bdev);
>  			comp_bio->bi_opf = REQ_OP_READ;
>  			comp_bio->bi_private = cb;
>  			comp_bio->bi_end_io = end_compressed_bio_read;
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index a6ad2f6f2bf7..f21be5d3724f 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2863,12 +2863,11 @@ static inline void btrfs_io_bio_init(struct btrfs_io_bio *btrfs_bio)
>   * never fail.  We're returning a bio right now but you can call btrfs_io_bio
>   * for the appropriate container_of magic
>   */
> -struct bio *btrfs_bio_alloc(struct block_device *bdev, u64 first_byte)
> +struct bio *btrfs_bio_alloc(u64 first_byte)
>  {
>  	struct bio *bio;
>  
>  	bio = bio_alloc_bioset(GFP_NOFS, BIO_MAX_PAGES, &btrfs_bioset);
> -	bio_set_dev(bio, bdev);
>  	bio->bi_iter.bi_sector = first_byte >> 9;
>  	btrfs_io_bio_init(btrfs_io_bio(bio));
>  	return bio;
> @@ -2979,7 +2978,8 @@ static int submit_extent_page(unsigned int opf, struct extent_io_tree *tree,
>  		}
>  	}
>  
> -	bio = btrfs_bio_alloc(bdev, offset);
> +	bio = btrfs_bio_alloc(offset);
> +	bio_set_dev(bio, bdev);
>  	bio_add_page(bio, page, page_size, pg_offset);
>  	bio->bi_end_io = end_io_func;
>  	bio->bi_private = tree;
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index 844e595cde5b..6e13a62a2974 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -497,7 +497,7 @@ void extent_clear_unlock_delalloc(struct inode *inode, u64 start, u64 end,
>  				 u64 delalloc_end, struct page *locked_page,
>  				 unsigned bits_to_clear,
>  				 unsigned long page_ops);
> -struct bio *btrfs_bio_alloc(struct block_device *bdev, u64 first_byte);
> +struct bio *btrfs_bio_alloc(u64 first_byte);
>  struct bio *btrfs_io_bio_alloc(unsigned int nr_iovecs);
>  struct bio *btrfs_bio_clone(struct bio *bio);
>  struct bio *btrfs_bio_clone_partial(struct bio *orig, int offset, int size);
> 

Re: [PATCH 6/6] btrfs: lift bio_set_dev from bio allocation helpers

[David Sterba]

On Wed, Jun 19, 2019 at 10:02:53AM +0300, Nikolay Borisov wrote:
> 
> 
> On 18.06.19 г. 21:00 ч., David Sterba wrote:
> > The block device is passed around for the only purpose to set it in new
> > bios. Move the assignment one level up. This is a preparatory patch for
> > further bdev cleanups.
> > 
> > Signed-off-by: David Sterba <dsterba@suse.com>
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> 
> Albeit I'd go as far as suggesting to make btrfs_bio_alloc a void
> function similar to the generic bio alloc functions. It should be up to
> the callers of bio alloc functions to initialize the bio in one place.
> Even after your patch initialization is split across btrfs_bio_alloc and
> its caller which seems a bit idiosyncratic.

The bio_set_dev will go away (device for a bio is set right before it's
submitted), so there will be only the allocation and the code as before.

Some kind of split is probably inevitable, btrfs_bio_alloc needs the
bioset that's private to extent_io.c and the callers know what to set to
bi_opf etc. Maybe, as I'm looking at it now, btrfs_bio_alloc can take
the value for opf, private and end_io as argument. All the callsites
fill these three. That would make it consistent.