[PATCH] btrfs: zoned: skip ZONE FINISH of conventional zones

[PATCH] btrfs: zoned: skip ZONE FINISH of conventional zones

[Johannes Thumshirn]

From: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Don't call ZONE FINISH for conventional zones as this will result in I/O
errors. Instead check if the zone that needs finishing is a conventional
zone and if yes skip it.

Also factor out the actual handling of finishing a single zone into a
helper function, as do_zone_finish() is growing ever bigger and the
indentations levels are getting higher.

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

---
This is a preparation patch for Naohiro's patch titled:
"btrfs: zoned: limit active zones to max_open_zones"
which can be found at:
https://lore.kernel.org/linux-btrfs/47f7423f53492e0ee1cd40f204db8354efb8d6b1.1752652539.git.naohiro.aota@wdc.com
---
 fs/btrfs/zoned.c | 55 ++++++++++++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 20 deletions(-)

diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index eeb049994cfe..ddacdb75d45c 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -2257,6 +2257,40 @@ static void wait_eb_writebacks(struct btrfs_block_group *block_group)
 	rcu_read_unlock();
 }
 
+static int call_zone_finish(struct btrfs_block_group *block_group,
+			    struct btrfs_io_stripe *stripe)
+{
+	struct btrfs_device *device = stripe->dev;
+	const u64 physical = stripe->physical;
+	struct btrfs_zoned_device_info *zinfo = device->zone_info;
+	int ret;
+
+	if (!device->bdev)
+		return 0;
+
+	if (zinfo->max_active_zones == 0)
+		return 0;
+
+	if (btrfs_dev_is_sequential(device, physical)) {
+		unsigned int nofs_flags;
+
+		nofs_flags = memalloc_nofs_save();
+		ret = blkdev_zone_mgmt(device->bdev, REQ_OP_ZONE_FINISH,
+				       physical >> SECTOR_SHIFT,
+				       zinfo->zone_size >> SECTOR_SHIFT);
+		memalloc_nofs_restore(nofs_flags);
+
+		if (ret)
+			return ret;
+	}
+
+	if (!(block_group->flags & BTRFS_BLOCK_GROUP_DATA))
+		zinfo->reserved_active_zones++;
+	btrfs_dev_clear_active_zone(device, physical);
+
+	return 0;
+}
+
 static int do_zone_finish(struct btrfs_block_group *block_group, bool fully_written)
 {
 	struct btrfs_fs_info *fs_info = block_group->fs_info;
@@ -2341,31 +2375,12 @@ static int do_zone_finish(struct btrfs_block_group *block_group, bool fully_writ
 	down_read(&dev_replace->rwsem);
 	map = block_group->physical_map;
 	for (i = 0; i < map->num_stripes; i++) {
-		struct btrfs_device *device = map->stripes[i].dev;
-		const u64 physical = map->stripes[i].physical;
-		struct btrfs_zoned_device_info *zinfo = device->zone_info;
-		unsigned int nofs_flags;
-
-		if (!device->bdev)
-			continue;
-
-		if (zinfo->max_active_zones == 0)
-			continue;
-
-		nofs_flags = memalloc_nofs_save();
-		ret = blkdev_zone_mgmt(device->bdev, REQ_OP_ZONE_FINISH,
-				       physical >> SECTOR_SHIFT,
-				       zinfo->zone_size >> SECTOR_SHIFT);
-		memalloc_nofs_restore(nofs_flags);
 
+		ret = call_zone_finish(block_group, &map->stripes[i]);
 		if (ret) {
 			up_read(&dev_replace->rwsem);
 			return ret;
 		}
-
-		if (!(block_group->flags & BTRFS_BLOCK_GROUP_DATA))
-			zinfo->reserved_active_zones++;
-		btrfs_dev_clear_active_zone(device, physical);
 	}
 	up_read(&dev_replace->rwsem);
 
-- 
2.50.1

Re: [PATCH] btrfs: zoned: skip ZONE FINISH of conventional zones

[Damien Le Moal]

On 7/23/25 22:38, Johannes Thumshirn wrote:
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> Don't call ZONE FINISH for conventional zones as this will result in I/O
> errors. Instead check if the zone that needs finishing is a conventional
> zone and if yes skip it.
> 
> Also factor out the actual handling of finishing a single zone into a
> helper function, as do_zone_finish() is growing ever bigger and the
> indentations levels are getting higher.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> ---
> This is a preparation patch for Naohiro's patch titled:
> "btrfs: zoned: limit active zones to max_open_zones"
> which can be found at:
> https://lore.kernel.org/linux-btrfs/47f7423f53492e0ee1cd40f204db8354efb8d6b1.1752652539.git.naohiro.aota@wdc.com
> ---
>  fs/btrfs/zoned.c | 55 ++++++++++++++++++++++++++++++------------------
>  1 file changed, 35 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index eeb049994cfe..ddacdb75d45c 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -2257,6 +2257,40 @@ static void wait_eb_writebacks(struct btrfs_block_group *block_group)
>  	rcu_read_unlock();
>  }
>  
> +static int call_zone_finish(struct btrfs_block_group *block_group,
> +			    struct btrfs_io_stripe *stripe)
> +{
> +	struct btrfs_device *device = stripe->dev;
> +	const u64 physical = stripe->physical;
> +	struct btrfs_zoned_device_info *zinfo = device->zone_info;
> +	int ret;
> +
> +	if (!device->bdev)
> +		return 0;
> +
> +	if (zinfo->max_active_zones == 0)
> +		return 0;

Should these 2 returns be replaced with a "goto out;"...

> +
> +	if (btrfs_dev_is_sequential(device, physical)) {
> +		unsigned int nofs_flags;
> +
> +		nofs_flags = memalloc_nofs_save();
> +		ret = blkdev_zone_mgmt(device->bdev, REQ_OP_ZONE_FINISH,
> +				       physical >> SECTOR_SHIFT,
> +				       zinfo->zone_size >> SECTOR_SHIFT);
> +		memalloc_nofs_restore(nofs_flags);
> +
> +		if (ret)
> +			return ret;
> +	}
> +

With "out:" label here ?

That was not done before, but I wonder if that is needed.

> +	if (!(block_group->flags & BTRFS_BLOCK_GROUP_DATA))
> +		zinfo->reserved_active_zones++;
> +	btrfs_dev_clear_active_zone(device, physical);
> +
> +	return 0;
> +}
> +
>  static int do_zone_finish(struct btrfs_block_group *block_group, bool fully_written)
>  {
>  	struct btrfs_fs_info *fs_info = block_group->fs_info;
> @@ -2341,31 +2375,12 @@ static int do_zone_finish(struct btrfs_block_group *block_group, bool fully_writ
>  	down_read(&dev_replace->rwsem);
>  	map = block_group->physical_map;
>  	for (i = 0; i < map->num_stripes; i++) {
> -		struct btrfs_device *device = map->stripes[i].dev;
> -		const u64 physical = map->stripes[i].physical;
> -		struct btrfs_zoned_device_info *zinfo = device->zone_info;
> -		unsigned int nofs_flags;
> -
> -		if (!device->bdev)
> -			continue;
> -
> -		if (zinfo->max_active_zones == 0)
> -			continue;
> -
> -		nofs_flags = memalloc_nofs_save();
> -		ret = blkdev_zone_mgmt(device->bdev, REQ_OP_ZONE_FINISH,
> -				       physical >> SECTOR_SHIFT,
> -				       zinfo->zone_size >> SECTOR_SHIFT);
> -		memalloc_nofs_restore(nofs_flags);
>  
> +		ret = call_zone_finish(block_group, &map->stripes[i]);
>  		if (ret) {
>  			up_read(&dev_replace->rwsem);
>  			return ret;
>  		}
> -
> -		if (!(block_group->flags & BTRFS_BLOCK_GROUP_DATA))
> -			zinfo->reserved_active_zones++;
> -		btrfs_dev_clear_active_zone(device, physical);
>  	}
>  	up_read(&dev_replace->rwsem);
>  


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] btrfs: zoned: skip ZONE FINISH of conventional zones

[Johannes Thumshirn]

On Thu, Jul 24, 2025 at 08:56:43AM +0900, Damien Le Moal wrote:
> > +	if (!device->bdev)
> > +		return 0;
> > +
> > +	if (zinfo->max_active_zones == 0)
> > +		return 0;
> 
> Should these 2 returns be replaced with a "goto out;"...
> 
> > +
> > +	if (btrfs_dev_is_sequential(device, physical)) {
> > +		unsigned int nofs_flags;
> > +
> > +		nofs_flags = memalloc_nofs_save();
> > +		ret = blkdev_zone_mgmt(device->bdev, REQ_OP_ZONE_FINISH,
> > +				       physical >> SECTOR_SHIFT,
> > +				       zinfo->zone_size >> SECTOR_SHIFT);
> > +		memalloc_nofs_restore(nofs_flags);
> > +
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> 
> With "out:" label here ?
> 
> That was not done before, but I wonder if that is needed.
> 
> > +	if (!(block_group->flags & BTRFS_BLOCK_GROUP_DATA))
> > +		zinfo->reserved_active_zones++;
> > +	btrfs_dev_clear_active_zone(device, physical);

I don't think so. If device->bdev == NULL it means the device is missing and
we can't do anything with it. If zone_info->max_active_zones == 0, it means we
don't do active zone tracking on that device.

Re: [PATCH] btrfs: zoned: skip ZONE FINISH of conventional zones

[Anand Jain]

On 23/7/25 21:38, Johannes Thumshirn wrote:
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> Don't call ZONE FINISH for conventional zones as this will result in I/O
> errors. Instead check if the zone that needs finishing is a conventional
> zone and if yes skip it.
> 
> Also factor out the actual handling of finishing a single zone into a
> helper function, as do_zone_finish() is growing ever bigger and the
> indentations levels are getting higher.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 

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


> ---
> This is a preparation patch for Naohiro's patch titled:
> "btrfs: zoned: limit active zones to max_open_zones"
> which can be found at:
> https://lore.kernel.org/linux-btrfs/47f7423f53492e0ee1cd40f204db8354efb8d6b1.1752652539.git.naohiro.aota@wdc.com
> ---
>   fs/btrfs/zoned.c | 55 ++++++++++++++++++++++++++++++------------------
>   1 file changed, 35 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index eeb049994cfe..ddacdb75d45c 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -2257,6 +2257,40 @@ static void wait_eb_writebacks(struct btrfs_block_group *block_group)
>   	rcu_read_unlock();
>   }
>   
> +static int call_zone_finish(struct btrfs_block_group *block_group,
> +			    struct btrfs_io_stripe *stripe)
> +{
> +	struct btrfs_device *device = stripe->dev;
> +	const u64 physical = stripe->physical;
> +	struct btrfs_zoned_device_info *zinfo = device->zone_info;
> +	int ret;
> +
> +	if (!device->bdev)
> +		return 0;
> +
> +	if (zinfo->max_active_zones == 0)
> +		return 0;
> +
> +	if (btrfs_dev_is_sequential(device, physical)) {
> +		unsigned int nofs_flags;
> +
> +		nofs_flags = memalloc_nofs_save();
> +		ret = blkdev_zone_mgmt(device->bdev, REQ_OP_ZONE_FINISH,
> +				       physical >> SECTOR_SHIFT,
> +				       zinfo->zone_size >> SECTOR_SHIFT);
> +		memalloc_nofs_restore(nofs_flags);
> +
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!(block_group->flags & BTRFS_BLOCK_GROUP_DATA))
> +		zinfo->reserved_active_zones++;
> +	btrfs_dev_clear_active_zone(device, physical);
> +
> +	return 0;
> +}
> +
>   static int do_zone_finish(struct btrfs_block_group *block_group, bool fully_written)
>   {
>   	struct btrfs_fs_info *fs_info = block_group->fs_info;
> @@ -2341,31 +2375,12 @@ static int do_zone_finish(struct btrfs_block_group *block_group, bool fully_writ
>   	down_read(&dev_replace->rwsem);
>   	map = block_group->physical_map;
>   	for (i = 0; i < map->num_stripes; i++) {
> -		struct btrfs_device *device = map->stripes[i].dev;
> -		const u64 physical = map->stripes[i].physical;
> -		struct btrfs_zoned_device_info *zinfo = device->zone_info;
> -		unsigned int nofs_flags;
> -
> -		if (!device->bdev)
> -			continue;
> -
> -		if (zinfo->max_active_zones == 0)
> -			continue;
> -
> -		nofs_flags = memalloc_nofs_save();
> -		ret = blkdev_zone_mgmt(device->bdev, REQ_OP_ZONE_FINISH,
> -				       physical >> SECTOR_SHIFT,
> -				       zinfo->zone_size >> SECTOR_SHIFT);
> -		memalloc_nofs_restore(nofs_flags);
>   
> +		ret = call_zone_finish(block_group, &map->stripes[i]);
>   		if (ret) {
>   			up_read(&dev_replace->rwsem);
>   			return ret;
>   		}
> -
> -		if (!(block_group->flags & BTRFS_BLOCK_GROUP_DATA))
> -			zinfo->reserved_active_zones++;
> -		btrfs_dev_clear_active_zone(device, physical);
>   	}
>   	up_read(&dev_replace->rwsem);
>   

Re: [PATCH] btrfs: zoned: skip ZONE FINISH of conventional zones

[Naohiro Aota]

On Wed Jul 23, 2025 at 10:38 PM JST, Johannes Thumshirn wrote:
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>
> Don't call ZONE FINISH for conventional zones as this will result in I/O
> errors. Instead check if the zone that needs finishing is a conventional
> zone and if yes skip it.
>
> Also factor out the actual handling of finishing a single zone into a
> helper function, as do_zone_finish() is growing ever bigger and the
> indentations levels are getting higher.
>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Looks good to me.

Reviewed-by: Naohiro Aota <naohiro.aota@wdc.com>