btrfs: avoid assigning twice to block_start at btrfs_do_readpage()

Commit Message

Filipe Manana Feb. 5, 2025, 11:36 a.m. UTC

From: Filipe Manana <[e-mail]>

At btrfs_do_readpage() if we get an extent map for a prealloc extent we
end up assigning twice to the 'block_start' variable, first the value
returned by extent_map_block_start() and then EXTENT_MAP_HOLE. This is
pointless so make it more clear by using an if-else statement and doing
only one assignment.

Signed-off-by: Filipe Manana <[e-mail]>
---
 fs/btrfs/extent_io.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Johannes Thumshirn Feb. 5, 2025, 11:48 a.m. UTC | #1

On 05.02.25 12:36, [e-mail] wrote:
> From: Filipe Manana <[e-mail]>
> 
> At btrfs_do_readpage() if we get an extent map for a prealloc extent we
> end up assigning twice to the 'block_start' variable, first the value
> returned by extent_map_block_start() and then EXTENT_MAP_HOLE. This is
> pointless so make it more clear by using an if-else statement and doing
> only one assignment.

I think you could also move the declaration of block_start into the 
while() loop, as it's not used outside of it, while you're at it.
But that's not a big issue.

Anyways,
Reviewed-by: Johannes Thumshirn <[e-mail]>

Filipe Manana Feb. 5, 2025, noon UTC | #2

On Wed, Feb 5, 2025 at 11:48 AM Johannes Thumshirn
<[e-mail]> wrote:
>
> On 05.02.25 12:36, [e-mail] wrote:
> > From: Filipe Manana <[e-mail]>
> >
> > At btrfs_do_readpage() if we get an extent map for a prealloc extent we
> > end up assigning twice to the 'block_start' variable, first the value
> > returned by extent_map_block_start() and then EXTENT_MAP_HOLE. This is
> > pointless so make it more clear by using an if-else statement and doing
> > only one assignment.
>
> I think you could also move the declaration of block_start into the
> while() loop, as it's not used outside of it, while you're at it.
> But that's not a big issue.

Sure, I can do that at commit time.
Thanks.

>
> Anyways,
> Reviewed-by: Johannes Thumshirn <[e-mail]>

Qu Wenruo Feb. 5, 2025, 9:06 p.m. UTC | #3

在 2025/2/5 22:06, [e-mail] 写道:
> From: Filipe Manana <[e-mail]>
>
> At btrfs_do_readpage() if we get an extent map for a prealloc extent we
> end up assigning twice to the 'block_start' variable, first the value
> returned by extent_map_block_start() and then EXTENT_MAP_HOLE. This is
> pointless so make it more clear by using an if-else statement and doing
> only one assignment.
>
> Signed-off-by: Filipe Manana <[e-mail]>

Reviewed-by: Qu Wenruo <[e-mail]>

Thanks,
Qu

> ---
>   fs/btrfs/extent_io.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index bdb9816bf125..9e70d43e19cb 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -991,9 +991,11 @@ static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached,
>   			disk_bytenr = em->disk_bytenr;
>   		else
>   			disk_bytenr = extent_map_block_start(em) + extent_offset;
> -		block_start = extent_map_block_start(em);
> +
>   		if (em->flags & EXTENT_FLAG_PREALLOC)
>   			block_start = EXTENT_MAP_HOLE;
> +		else
> +			block_start = extent_map_block_start(em);
>
>   		/*
>   		 * If we have a file range that points to a compressed extent

Patch

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index bdb9816bf125..9e70d43e19cb 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -991,9 +991,11 @@  static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached,
 			disk_bytenr = em->disk_bytenr;
 		else
 			disk_bytenr = extent_map_block_start(em) + extent_offset;
-		block_start = extent_map_block_start(em);
+
 		if (em->flags & EXTENT_FLAG_PREALLOC)
 			block_start = EXTENT_MAP_HOLE;
+		else
+			block_start = extent_map_block_start(em);
 
 		/*
 		 * If we have a file range that points to a compressed extent