[PATCH 0/2] btrfs: zoned: two small style improvements for zone finishing

[PATCH 0/2] btrfs: zoned: two small style improvements for zone finishing

[Johannes Thumshirn]

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

Two small improvements for zone finish calls. The frist one changes
btrfs_zone_finish_endio_workfn() to directly call do_zone_finish(), as most of
the work done in btrfs_zone_finish_endio() is not needed in this context.

The second one adds error propagation to btrfs_zone_finish_endio() so it's
caller btrfs_finish_one_ordered() can do error handling (in case the chunk map
block group lookup failes for some reason).

Johannes Thumshirn (2):
  btrfs: directly call do_zone_finish() from
    btrfs_zone_finish_endio_workfn()
  btrfs: zoned: return error from btrfs_zone_finish_endio()

 fs/btrfs/inode.c |  8 +++++---
 fs/btrfs/zoned.c | 13 +++++++++----
 fs/btrfs/zoned.h |  9 ++++++---
 3 files changed, 20 insertions(+), 10 deletions(-)

-- 
2.50.0

[PATCH 1/2] btrfs: directly call do_zone_finish() from btrfs_zone_finish_endio_workfn()

[Johannes Thumshirn]

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

When btrfs_zone_finish_endio_workfn() is calling btrfs_zone_finish_endio()
it already has a pointer to the block group. Furthermore
btrfs_zone_finish_endio() does additional checks if the block group can be
finished or not.

But in the context of btrfs_zone_finish_endio_workfn() only the actual
call to do_zone_finish() is of interest, as the skipping condition when
there is still room to allocate from the block group cannot be checked.

Directly call do_zone_finish() on the block group.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/zoned.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 245e813ecd78..4444a667c71e 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -2466,7 +2466,8 @@ static void btrfs_zone_finish_endio_workfn(struct work_struct *work)
 
 	wait_on_extent_buffer_writeback(bg->last_eb);
 	free_extent_buffer(bg->last_eb);
-	btrfs_zone_finish_endio(bg->fs_info, bg->start, bg->length);
+	bg->last_eb = NULL;
+	do_zone_finish(bg, true);
 	btrfs_put_block_group(bg);
 }
 
-- 
2.50.0

[PATCH 2/2] btrfs: zoned: return error from btrfs_zone_finish_endio()

[Johannes Thumshirn]

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

Now that btrfs_zone_finish_endio_workfn() is directly calling
do_zone_finish() the only caller of btrfs_zone_finish_endio() is
btrfs_finish_one_ordered().

btrfs_finish_one_ordered() already has error handling in-place so
btrfs_zone_finish_endio() can return an error if the block group lookup
fails.

Also as btrfs_zone_finish_endio() already checks for zoned filesystems and
returns early, there's no need to do this in the caller. For developer
builds leave the ASSERT() in place to check for a block-group lookup
failure.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/inode.c |  8 +++++---
 fs/btrfs/zoned.c | 10 +++++++---
 fs/btrfs/zoned.h |  9 ++++++---
 3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 7ed340cac33f..bfddbbd46366 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3102,9 +3102,11 @@ int btrfs_finish_one_ordered(struct btrfs_ordered_extent *ordered_extent)
 		goto out;
 	}
 
-	if (btrfs_is_zoned(fs_info))
-		btrfs_zone_finish_endio(fs_info, ordered_extent->disk_bytenr,
-					ordered_extent->disk_num_bytes);
+	ret = btrfs_zone_finish_endio(fs_info, ordered_extent->disk_bytenr,
+				      ordered_extent->disk_num_bytes);
+	if (ret) {
+		goto out;
+	}
 
 	if (test_bit(BTRFS_ORDERED_TRUNCATED, &ordered_extent->flags)) {
 		truncated = true;
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 4444a667c71e..b1320b12e0e4 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -2431,16 +2431,19 @@ bool btrfs_can_activate_zone(struct btrfs_fs_devices *fs_devices, u64 flags)
 	return ret;
 }
 
-void btrfs_zone_finish_endio(struct btrfs_fs_info *fs_info, u64 logical, u64 length)
+int btrfs_zone_finish_endio(struct btrfs_fs_info *fs_info, u64 logical, u64 length)
 {
 	struct btrfs_block_group *block_group;
 	u64 min_alloc_bytes;
 
 	if (!btrfs_is_zoned(fs_info))
-		return;
+		return 0;
 
 	block_group = btrfs_lookup_block_group(fs_info, logical);
-	ASSERT(block_group);
+	if (!block_group) {
+		ASSERT(block_group);
+		return -EIO;
+	}
 
 	/* No MIXED_BG on zoned btrfs. */
 	if (block_group->flags & BTRFS_BLOCK_GROUP_DATA)
@@ -2457,6 +2460,7 @@ void btrfs_zone_finish_endio(struct btrfs_fs_info *fs_info, u64 logical, u64 len
 
 out:
 	btrfs_put_block_group(block_group);
+	return 0;
 }
 
 static void btrfs_zone_finish_endio_workfn(struct work_struct *work)
diff --git a/fs/btrfs/zoned.h b/fs/btrfs/zoned.h
index 6e11533b8e14..17c5656580dd 100644
--- a/fs/btrfs/zoned.h
+++ b/fs/btrfs/zoned.h
@@ -83,7 +83,7 @@ int btrfs_sync_zone_write_pointer(struct btrfs_device *tgt_dev, u64 logical,
 bool btrfs_zone_activate(struct btrfs_block_group *block_group);
 int btrfs_zone_finish(struct btrfs_block_group *block_group);
 bool btrfs_can_activate_zone(struct btrfs_fs_devices *fs_devices, u64 flags);
-void btrfs_zone_finish_endio(struct btrfs_fs_info *fs_info, u64 logical,
+int btrfs_zone_finish_endio(struct btrfs_fs_info *fs_info, u64 logical,
 			     u64 length);
 void btrfs_schedule_zone_finish_bg(struct btrfs_block_group *bg,
 				   struct extent_buffer *eb);
@@ -234,8 +234,11 @@ static inline bool btrfs_can_activate_zone(struct btrfs_fs_devices *fs_devices,
 	return true;
 }
 
-static inline void btrfs_zone_finish_endio(struct btrfs_fs_info *fs_info,
-					   u64 logical, u64 length) { }
+static inline int btrfs_zone_finish_endio(struct btrfs_fs_info *fs_info,
+					   u64 logical, u64 length)
+{
+	return 0;
+}
 
 static inline void btrfs_schedule_zone_finish_bg(struct btrfs_block_group *bg,
 						 struct extent_buffer *eb) { }
-- 
2.50.0

Re: [PATCH 1/2] btrfs: directly call do_zone_finish() from btrfs_zone_finish_endio_workfn()

[Filipe Manana]

On Mon, Jul 21, 2025 at 8:02 AM Johannes Thumshirn <jth@kernel.org> wrote:
>
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>
> When btrfs_zone_finish_endio_workfn() is calling btrfs_zone_finish_endio()
> it already has a pointer to the block group. Furthermore
> btrfs_zone_finish_endio() does additional checks if the block group can be
> finished or not.
>
> But in the context of btrfs_zone_finish_endio_workfn() only the actual
> call to do_zone_finish() is of interest, as the skipping condition when
> there is still room to allocate from the block group cannot be checked.
>
> Directly call do_zone_finish() on the block group.
>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  fs/btrfs/zoned.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index 245e813ecd78..4444a667c71e 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -2466,7 +2466,8 @@ static void btrfs_zone_finish_endio_workfn(struct work_struct *work)
>
>         wait_on_extent_buffer_writeback(bg->last_eb);
>         free_extent_buffer(bg->last_eb);
> -       btrfs_zone_finish_endio(bg->fs_info, bg->start, bg->length);
> +       bg->last_eb = NULL;

Setting bg->last_eb to NULL is something new, and it's not done in
btrfs_zone_finish_endio() as well.
Isn't this a separate change that should have its own changelog? Do we
have the potential for some use-after-free without this change?

> +       do_zone_finish(bg, true);

This part looks good.
This can return errors but we are ignoring them here, as well as in
btrfs_zone_finish_endio(). Can we at least assert that it's returning
0?

Thanks.

>         btrfs_put_block_group(bg);
>  }
>
> --
> 2.50.0
>
>

Re: [PATCH 2/2] btrfs: zoned: return error from btrfs_zone_finish_endio()

[Filipe Manana]

On Mon, Jul 21, 2025 at 8:02 AM Johannes Thumshirn <jth@kernel.org> wrote:
>
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>
> Now that btrfs_zone_finish_endio_workfn() is directly calling
> do_zone_finish() the only caller of btrfs_zone_finish_endio() is
> btrfs_finish_one_ordered().
>
> btrfs_finish_one_ordered() already has error handling in-place so
> btrfs_zone_finish_endio() can return an error if the block group lookup
> fails.
>
> Also as btrfs_zone_finish_endio() already checks for zoned filesystems and
> returns early, there's no need to do this in the caller. For developer
> builds leave the ASSERT() in place to check for a block-group lookup
> failure.
>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  fs/btrfs/inode.c |  8 +++++---
>  fs/btrfs/zoned.c | 10 +++++++---
>  fs/btrfs/zoned.h |  9 ++++++---
>  3 files changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 7ed340cac33f..bfddbbd46366 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -3102,9 +3102,11 @@ int btrfs_finish_one_ordered(struct btrfs_ordered_extent *ordered_extent)
>                 goto out;
>         }
>
> -       if (btrfs_is_zoned(fs_info))
> -               btrfs_zone_finish_endio(fs_info, ordered_extent->disk_bytenr,
> -                                       ordered_extent->disk_num_bytes);
> +       ret = btrfs_zone_finish_endio(fs_info, ordered_extent->disk_bytenr,
> +                                     ordered_extent->disk_num_bytes);
> +       if (ret) {
> +               goto out;
> +       }

There's a single statement inside the if block, so no { } please.

>
>         if (test_bit(BTRFS_ORDERED_TRUNCATED, &ordered_extent->flags)) {
>                 truncated = true;
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index 4444a667c71e..b1320b12e0e4 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -2431,16 +2431,19 @@ bool btrfs_can_activate_zone(struct btrfs_fs_devices *fs_devices, u64 flags)
>         return ret;
>  }
>
> -void btrfs_zone_finish_endio(struct btrfs_fs_info *fs_info, u64 logical, u64 length)
> +int btrfs_zone_finish_endio(struct btrfs_fs_info *fs_info, u64 logical, u64 length)
>  {
>         struct btrfs_block_group *block_group;
>         u64 min_alloc_bytes;
>
>         if (!btrfs_is_zoned(fs_info))
> -               return;
> +               return 0;
>
>         block_group = btrfs_lookup_block_group(fs_info, logical);
> -       ASSERT(block_group);
> +       if (!block_group) {
> +               ASSERT(block_group);

This is an odd style and was pointed out before in other patches.
The ASSERT before the if check was perfectly fine and less odd.

But if we can handle the error, why an assert at all?
We could just do:

if (WARN_ON_ONCE(!block_group))
     return -ENOENT;

As it's one of those "impossible" errors, the stack trace from the
WARN_ON will let us know where it happened in case CONFIG_BTRFS_ASSERT
is not enabled plus allows the compiler to generate better code due to
'unlikely' inside the macro.

> +               return -EIO;

-EIO is an odd choice as this is not an IO error.
-ENOENT is more meaningful here.

Thanks.

> +       }
>
>         /* No MIXED_BG on zoned btrfs. */
>         if (block_group->flags & BTRFS_BLOCK_GROUP_DATA)
> @@ -2457,6 +2460,7 @@ void btrfs_zone_finish_endio(struct btrfs_fs_info *fs_info, u64 logical, u64 len
>
>  out:
>         btrfs_put_block_group(block_group);
> +       return 0;
>  }
>
>  static void btrfs_zone_finish_endio_workfn(struct work_struct *work)
> diff --git a/fs/btrfs/zoned.h b/fs/btrfs/zoned.h
> index 6e11533b8e14..17c5656580dd 100644
> --- a/fs/btrfs/zoned.h
> +++ b/fs/btrfs/zoned.h
> @@ -83,7 +83,7 @@ int btrfs_sync_zone_write_pointer(struct btrfs_device *tgt_dev, u64 logical,
>  bool btrfs_zone_activate(struct btrfs_block_group *block_group);
>  int btrfs_zone_finish(struct btrfs_block_group *block_group);
>  bool btrfs_can_activate_zone(struct btrfs_fs_devices *fs_devices, u64 flags);
> -void btrfs_zone_finish_endio(struct btrfs_fs_info *fs_info, u64 logical,
> +int btrfs_zone_finish_endio(struct btrfs_fs_info *fs_info, u64 logical,
>                              u64 length);
>  void btrfs_schedule_zone_finish_bg(struct btrfs_block_group *bg,
>                                    struct extent_buffer *eb);
> @@ -234,8 +234,11 @@ static inline bool btrfs_can_activate_zone(struct btrfs_fs_devices *fs_devices,
>         return true;
>  }
>
> -static inline void btrfs_zone_finish_endio(struct btrfs_fs_info *fs_info,
> -                                          u64 logical, u64 length) { }
> +static inline int btrfs_zone_finish_endio(struct btrfs_fs_info *fs_info,
> +                                          u64 logical, u64 length)
> +{
> +       return 0;
> +}
>
>  static inline void btrfs_schedule_zone_finish_bg(struct btrfs_block_group *bg,
>                                                  struct extent_buffer *eb) { }
> --
> 2.50.0
>
>