[PATCH 0/2] btrfs: zoned: fix zone activation logic

[PATCH 0/2] btrfs: zoned: fix zone activation logic

[Naohiro Aota]

btrfs_zone_activate() "continue"s on seeing a devie without max_active_zone
restriction, and it never set the block group as active if the block group
does not contain a device with the restriction.

While it still return true and make the allocation works, it is confusing
for other code and it is waste of time to iterate the loop every time
btrfs_zone_activate() is called.

Also, there is a non-changing condition check in the loop.

Fix them with this series.

Naohiro Aota (2):
  btrfs: zoned: move non-changing condition check out of the loop
  btrfs: zoned: activate block group properly on unlimited active zone
    device

 fs/btrfs/zoned.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

-- 
2.35.1

[PATCH 1/2] btrfs: zoned: move non-changing condition check out of the loop

[Naohiro Aota]

btrfs_zone_activate() checks if block_group->alloc_offset ==
block_group->zone_capacity every time it iterates the loop. But, it is not
depending on the index. Move out the check and do it only once.

Fixes: f9a912a3c45f ("btrfs: zoned: make zone activation multi stripe capable")
Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 fs/btrfs/zoned.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 997a96d7a3d5..80a86f26ab7b 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -1835,6 +1835,12 @@ bool btrfs_zone_activate(struct btrfs_block_group *block_group)
 		goto out_unlock;
 	}
 
+	/* No space left */
+	if (block_group->alloc_offset == block_group->zone_capacity) {
+		ret = false;
+		goto out_unlock;
+	}
+
 	for (i = 0; i < map->num_stripes; i++) {
 		device = map->stripes[i].dev;
 		physical = map->stripes[i].physical;
@@ -1842,12 +1848,6 @@ bool btrfs_zone_activate(struct btrfs_block_group *block_group)
 		if (device->zone_info->max_active_zones == 0)
 			continue;
 
-		/* No space left */
-		if (block_group->alloc_offset == block_group->zone_capacity) {
-			ret = false;
-			goto out_unlock;
-		}
-
 		if (!btrfs_dev_set_active_zone(device, physical)) {
 			/* Cannot activate the zone */
 			ret = false;
-- 
2.35.1

[PATCH 2/2] btrfs: zoned: activate block group properly on unlimited active zone device

[Naohiro Aota]

btrfs_zone_activate() checks if it activated all the underlying zones in
the loop. However, that check never hit on an unlimited activate zone
device (max_active_zones == 0).

Fortunately, it still works without ENOSPC because btrfs_zone_activate()
returns true in the end, even if block_group->zone_is_active == 0. But, it
is confusing to have non zone_is_active block group still usable for
allocation. Also, we are wasting CPU time to iterate the loop every time
btrfs_zone_activate() is called for the BGs.

Since error case in the loop is handled by out_unlock, we can just set
zone_is_active and do the list stuff after the loop.

Fixes: f9a912a3c45f ("btrfs: zoned: make zone activation multi stripe capable")
Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 fs/btrfs/zoned.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 80a86f26ab7b..6e91022ae9f6 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -1853,24 +1853,18 @@ bool btrfs_zone_activate(struct btrfs_block_group *block_group)
 			ret = false;
 			goto out_unlock;
 		}
-
-		/* Successfully activated all the zones */
-		if (i == map->num_stripes - 1)
-			block_group->zone_is_active = 1;
-
-
 	}
+
+	/* Successfully activated all the zones */
+	block_group->zone_is_active = 1;
 	spin_unlock(&block_group->lock);
 
-	if (block_group->zone_is_active) {
-		/* For the active block group list */
-		btrfs_get_block_group(block_group);
+	/* For the active block group list */
+	btrfs_get_block_group(block_group);
 
-		spin_lock(&fs_info->zone_active_bgs_lock);
-		list_add_tail(&block_group->active_bg_list,
-			      &fs_info->zone_active_bgs);
-		spin_unlock(&fs_info->zone_active_bgs_lock);
-	}
+	spin_lock(&fs_info->zone_active_bgs_lock);
+	list_add_tail(&block_group->active_bg_list, &fs_info->zone_active_bgs);
+	spin_unlock(&fs_info->zone_active_bgs_lock);
 
 	return true;
 
-- 
2.35.1

Re: [PATCH 0/2] btrfs: zoned: fix zone activation logic

[Johannes Thumshirn]

Oops I'm sorry.

For the series,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 0/2] btrfs: zoned: fix zone activation logic

[David Sterba]

On Tue, May 03, 2022 at 02:10:03PM -0700, Naohiro Aota wrote:
> btrfs_zone_activate() "continue"s on seeing a devie without max_active_zone
> restriction, and it never set the block group as active if the block group
> does not contain a device with the restriction.
> 
> While it still return true and make the allocation works, it is confusing
> for other code and it is waste of time to iterate the loop every time
> btrfs_zone_activate() is called.
> 
> Also, there is a non-changing condition check in the loop.
> 
> Fix them with this series.
> 
> Naohiro Aota (2):
>   btrfs: zoned: move non-changing condition check out of the loop
>   btrfs: zoned: activate block group properly on unlimited active zone
>     device

There's a minor conflict with the other series fixing the zone finish
but this 2 patch series is a clear regression so I'll take it first for
rc, not sure about the other yet.

Re: [PATCH 0/2] btrfs: zoned: fix zone activation logic

[Naohiro Aota]

On Wed, May 04, 2022 at 06:55:59PM +0200, David Sterba wrote:
> On Tue, May 03, 2022 at 02:10:03PM -0700, Naohiro Aota wrote:
> > btrfs_zone_activate() "continue"s on seeing a devie without max_active_zone
> > restriction, and it never set the block group as active if the block group
> > does not contain a device with the restriction.
> > 
> > While it still return true and make the allocation works, it is confusing
> > for other code and it is waste of time to iterate the loop every time
> > btrfs_zone_activate() is called.
> > 
> > Also, there is a non-changing condition check in the loop.
> > 
> > Fix them with this series.
> > 
> > Naohiro Aota (2):
> >   btrfs: zoned: move non-changing condition check out of the loop
> >   btrfs: zoned: activate block group properly on unlimited active zone
> >     device
> 
> There's a minor conflict with the other series fixing the zone finish
> but this 2 patch series is a clear regression so I'll take it first for
> rc, not sure about the other yet.

Oh, sorry for confusion. As noted in "btrfs: zoned: fixes for zone
finishing" series, that series depends on this series as they both touch
the btrfs_zone_activate() function.