[PATCH] btrfs: collapse report_eb_range() into its single caller

[PATCH] btrfs: collapse report_eb_range() into its single caller

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

There's no point in having this static function since it's too short and
it has a single caller (check_eb_range()), so move its code into the
caller and remove it.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/extent_io.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 6a6d98eec787..2b0aec031a9a 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3976,17 +3976,6 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int mirror_num,
 	return 0;
 }
 
-static bool report_eb_range(const struct extent_buffer *eb, unsigned long start,
-			    unsigned long len)
-{
-	btrfs_warn(eb->fs_info,
-		"access to eb bytenr %llu len %u out of range start %lu len %lu",
-		eb->start, eb->len, start, len);
-	DEBUG_WARN();
-
-	return true;
-}
-
 /*
  * Check if the [start, start + len) range is valid before reading/writing
  * the eb.
@@ -4000,8 +3989,13 @@ static inline int check_eb_range(const struct extent_buffer *eb,
 	unsigned long offset;
 
 	/* start, start + len should not go beyond eb->len nor overflow */
-	if (unlikely(check_add_overflow(start, len, &offset) || offset > eb->len))
-		return report_eb_range(eb, start, len);
+	if (unlikely(check_add_overflow(start, len, &offset) || offset > eb->len)) {
+		btrfs_warn(eb->fs_info,
+		   "access to eb bytenr %llu len %u out of range start %lu len %lu",
+			   eb->start, eb->len, start, len);
+		DEBUG_WARN();
+		return true;
+	}
 
 	return false;
 }
-- 
2.47.2

Re: [PATCH] btrfs: collapse report_eb_range() into its single caller

[Johannes Thumshirn]

Looks good,

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

Re: [PATCH] btrfs: collapse report_eb_range() into its single caller

[Qu Wenruo]

在 2026/4/23 21:08, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> There's no point in having this static function since it's too short and
> it has a single caller (check_eb_range()), so move its code into the
> caller and remove it.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>   fs/btrfs/extent_io.c | 20 +++++++-------------
>   1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 6a6d98eec787..2b0aec031a9a 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3976,17 +3976,6 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int mirror_num,
>   	return 0;
>   }
>   
> -static bool report_eb_range(const struct extent_buffer *eb, unsigned long start,
> -			    unsigned long len)
> -{
> -	btrfs_warn(eb->fs_info,
> -		"access to eb bytenr %llu len %u out of range start %lu len %lu",
> -		eb->start, eb->len, start, len);
> -	DEBUG_WARN();
> -
> -	return true;
> -}
> -
>   /*
>    * Check if the [start, start + len) range is valid before reading/writing
>    * the eb.
> @@ -4000,8 +3989,13 @@ static inline int check_eb_range(const struct extent_buffer *eb,
>   	unsigned long offset;
>   
>   	/* start, start + len should not go beyond eb->len nor overflow */
> -	if (unlikely(check_add_overflow(start, len, &offset) || offset > eb->len))
> -		return report_eb_range(eb, start, len);
> +	if (unlikely(check_add_overflow(start, len, &offset) || offset > eb->len)) {
> +		btrfs_warn(eb->fs_info,
> +		   "access to eb bytenr %llu len %u out of range start %lu len %lu",
> +			   eb->start, eb->len, start, len);
> +		DEBUG_WARN();
> +		return true;
> +	}
>   
>   	return false;
>   }

Re: [PATCH] btrfs: collapse report_eb_range() into its single caller

[David Sterba]

On Thu, Apr 23, 2026 at 12:38:13PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> There's no point in having this static function since it's too short and
> it has a single caller (check_eb_range()), so move its code into the
> caller and remove it.

The point is to separate the check and report functionality, so the
check part is inline and a simple if (a few instructions), while the
report is not always inlined and is considered cold code.

Re: [PATCH] btrfs: collapse report_eb_range() into its single caller

[David Sterba]

On Fri, Apr 24, 2026 at 04:38:35AM +0200, David Sterba wrote:
> On Thu, Apr 23, 2026 at 12:38:13PM +0100, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> > 
> > There's no point in having this static function since it's too short and
> > it has a single caller (check_eb_range()), so move its code into the
> > caller and remove it.
> 
> The point is to separate the check and report functionality, so the
> check part is inline and a simple if (a few instructions), while the
> report is not always inlined and is considered cold code.

But what compiler does is not what was intended, there should be either
__cold or noinline for the report function.

The effects of __cold on unpatched:

     text	   data	    bss	    dec	    hex	filename
  1742535	 150224	  15560	1908319	 1d1e5f	pre/btrfs.ko
  1742159	 150224	  15560	1907943	 1d1ce7	post/btrfs.ko

  DELTA: -376

Stack effects:

   copy_extent_buffer                           -8 (80 -> 72)
   extent_buffer_get_byte                      -24 (32 -> 8)
   memcmp_extent_buffer                        -16 (80 -> 64)
   memcpy_extent_buffer                         -8 (72 -> 64)
   memmove_extent_buffer                       -16 (64 -> 48)
   memzero_extent_buffer                       -32 (40 -> 8)
   read_extent_buffer                          -24 (72 -> 48)
   read_extent_buffer_to_user_nofault          -24 (72 -> 48)
   __write_extent_buffer                        -8 (88 -> 80)

   LOST (0):

   NEW (40):
	   report_eb_range                      40
   LOST/NEW DELTA:      +40
   PRE/POST DELTA:     -120

As can be seen it's all in the extent buffer manipulation helpers so
the hot path functions where the micro optimizations have impact.

The effects of 'noinline:

     text	   data	    bss	    dec	    hex	filename
  1742535	 150224	  15560	1908319	 1d1e5f	pre/btrfs.ko
  1742033	 150224	  15560	1907817	 1d1c69	post/btrfs.ko

  DELTA: -502

Stack:

  copy_extent_buffer                            -8 (80 -> 72)
  extent_buffer_get_byte                       -24 (32 -> 8)
  memcmp_extent_buffer                         -16 (80 -> 64)
  memcpy_extent_buffer                          -8 (72 -> 64)
  memmove_extent_buffer                        -16 (64 -> 48)
  memzero_extent_buffer                        -32 (40 -> 8)
  read_extent_buffer                           -24 (72 -> 48)
  read_extent_buffer_to_user_nofault           -24 (72 -> 48)
  __write_extent_buffer                         -8 (88 -> 80)

  LOST (0):

  NEW (40):
	  report_eb_range                       40
  LOST/NEW DELTA:      +40
  PRE/POST DELTA:     -120

So the same effects on stack (this is consumed by setting up the printk
and other things to report it), and difference how exactly is the code
inlined.

Either way, merging the report to check is not right.

Re: [PATCH] btrfs: collapse report_eb_range() into its single caller

[Filipe Manana]

On Fri, Apr 24, 2026 at 4:12 AM David Sterba <dsterba@suse.cz> wrote:
>
> On Fri, Apr 24, 2026 at 04:38:35AM +0200, David Sterba wrote:
> > On Thu, Apr 23, 2026 at 12:38:13PM +0100, fdmanana@kernel.org wrote:
> > > From: Filipe Manana <fdmanana@suse.com>
> > >
> > > There's no point in having this static function since it's too short and
> > > it has a single caller (check_eb_range()), so move its code into the
> > > caller and remove it.
> >
> > The point is to separate the check and report functionality, so the
> > check part is inline and a simple if (a few instructions), while the
> > report is not always inlined and is considered cold code.
>
> But what compiler does is not what was intended, there should be either
> __cold or noinline for the report function.
>
> The effects of __cold on unpatched:
>
>      text          data     bss     dec     hex filename
>   1742535        150224   15560 1908319  1d1e5f pre/btrfs.ko
>   1742159        150224   15560 1907943  1d1ce7 post/btrfs.ko
>
>   DELTA: -376
>
> Stack effects:
>
>    copy_extent_buffer                           -8 (80 -> 72)
>    extent_buffer_get_byte                      -24 (32 -> 8)
>    memcmp_extent_buffer                        -16 (80 -> 64)
>    memcpy_extent_buffer                         -8 (72 -> 64)
>    memmove_extent_buffer                       -16 (64 -> 48)
>    memzero_extent_buffer                       -32 (40 -> 8)
>    read_extent_buffer                          -24 (72 -> 48)
>    read_extent_buffer_to_user_nofault          -24 (72 -> 48)
>    __write_extent_buffer                        -8 (88 -> 80)
>
>    LOST (0):
>
>    NEW (40):
>            report_eb_range                      40
>    LOST/NEW DELTA:      +40
>    PRE/POST DELTA:     -120
>
> As can be seen it's all in the extent buffer manipulation helpers so
> the hot path functions where the micro optimizations have impact.
>
> The effects of 'noinline:
>
>      text          data     bss     dec     hex filename
>   1742535        150224   15560 1908319  1d1e5f pre/btrfs.ko
>   1742033        150224   15560 1907817  1d1c69 post/btrfs.ko
>
>   DELTA: -502
>
> Stack:
>
>   copy_extent_buffer                            -8 (80 -> 72)
>   extent_buffer_get_byte                       -24 (32 -> 8)
>   memcmp_extent_buffer                         -16 (80 -> 64)
>   memcpy_extent_buffer                          -8 (72 -> 64)
>   memmove_extent_buffer                        -16 (64 -> 48)
>   memzero_extent_buffer                        -32 (40 -> 8)
>   read_extent_buffer                           -24 (72 -> 48)
>   read_extent_buffer_to_user_nofault           -24 (72 -> 48)
>   __write_extent_buffer                         -8 (88 -> 80)
>
>   LOST (0):
>
>   NEW (40):
>           report_eb_range                       40
>   LOST/NEW DELTA:      +40
>   PRE/POST DELTA:     -120
>
> So the same effects on stack (this is consumed by setting up the printk
> and other things to report it), and difference how exactly is the code
> inlined.
>
> Either way, merging the report to check is not right.

Yes, oversight on the fact that check_eb_range() is inline and it has
quite a significant number of callers.
Updated in another patch:
https://lore.kernel.org/linux-btrfs/5082990dc32613cf128c27ef01086178edcf4e24.1777043747.git.fdmanana@suse.com/

Thanks.