[PATCH] btrfs: turn unpin_extent_cache() into a void function

[PATCH] btrfs: turn unpin_extent_cache() into a void function

[Luís Henriques]

The value of the 'ret' variable is never changed in function
unpin_extent_cache().  And since the only caller of this function doesn't
check the return value, it can simply be turned into a void function.

Signed-off-by: Luís Henriques <lhenriques@suse.de>
---
 fs/btrfs/extent_map.c | 7 ++-----
 fs/btrfs/extent_map.h | 2 +-
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index 0cdb3e86f29b..f99c458071a4 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -292,10 +292,9 @@ static void try_merge_map(struct extent_map_tree *tree, struct extent_map *em)
  * to the generation that actually added the file item to the inode so we know
  * we need to sync this extent when we call fsync().
  */
-int unpin_extent_cache(struct extent_map_tree *tree, u64 start, u64 len,
-		       u64 gen)
+void unpin_extent_cache(struct extent_map_tree *tree, u64 start, u64 len,
+			u64 gen)
 {
-	int ret = 0;
 	struct extent_map *em;
 	bool prealloc = false;
 
@@ -327,8 +326,6 @@ int unpin_extent_cache(struct extent_map_tree *tree, u64 start, u64 len,
 	free_extent_map(em);
 out:
 	write_unlock(&tree->lock);
-	return ret;
-
 }
 
 void clear_em_logging(struct extent_map_tree *tree, struct extent_map *em)
diff --git a/fs/btrfs/extent_map.h b/fs/btrfs/extent_map.h
index 35d27c756e08..486a8ea798c7 100644
--- a/fs/btrfs/extent_map.h
+++ b/fs/btrfs/extent_map.h
@@ -97,7 +97,7 @@ struct extent_map *alloc_extent_map(void);
 void free_extent_map(struct extent_map *em);
 int __init extent_map_init(void);
 void __cold extent_map_exit(void);
-int unpin_extent_cache(struct extent_map_tree *tree, u64 start, u64 len, u64 gen);
+void unpin_extent_cache(struct extent_map_tree *tree, u64 start, u64 len, u64 gen);
 void clear_em_logging(struct extent_map_tree *tree, struct extent_map *em);
 struct extent_map *search_extent_mapping(struct extent_map_tree *tree,
 					 u64 start, u64 len);

Re: [PATCH] btrfs: turn unpin_extent_cache() into a void function

[Johannes Thumshirn]

On 18.07.23 19:39, Luís Henriques wrote:
> The value of the 'ret' variable is never changed in function
> unpin_extent_cache().  And since the only caller of this function doesn't
> check the return value, it can simply be turned into a void function.
> 
> Signed-off-by: Luís Henriques <lhenriques@suse.de>

Hmm but inside unpin_extent_cache() there is this:


	/* [...] */
	em = lookup_extent_mapping(tree, start, len);

	WARN_ON(!em || em->start != start);

	if (!em)
		goto out;
	/* [...] */

out:
	write_unlock(&tree->lock);
	return ret;

}

Wouldn't it be better to either actually handle the error, OR
change the WARN_ON() into an ASSERT()?

Given the fact, that if the lookup fails, we've passed wrong 
parameters somehow, an ASSERT() would be a good way IMHO.

Thoughts?

Re: [PATCH] btrfs: turn unpin_extent_cache() into a void function

[Luís Henriques]

Johannes Thumshirn <Johannes.Thumshirn@wdc.com> writes:

> On 18.07.23 19:39, Luís Henriques wrote:
>> The value of the 'ret' variable is never changed in function
>> unpin_extent_cache().  And since the only caller of this function doesn't
>> check the return value, it can simply be turned into a void function.
>> 
>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>
> Hmm but inside unpin_extent_cache() there is this:
>
>
> 	/* [...] */
> 	em = lookup_extent_mapping(tree, start, len);
>
> 	WARN_ON(!em || em->start != start);
>
> 	if (!em)
> 		goto out;
> 	/* [...] */
>
> out:
> 	write_unlock(&tree->lock);
> 	return ret;
>
> }
>
> Wouldn't it be better to either actually handle the error, OR
> change the WARN_ON() into an ASSERT()?
>
> Given the fact, that if the lookup fails, we've passed wrong 
> parameters somehow, an ASSERT() would be a good way IMHO.
>
> Thoughts?

OK, I guess that using ASSERT() makes sense -- it's used in several other
places where lookup_extent_mapping() is called.

Returning an error to the caller can also be done but I wonder if the only
place where it is called actually cares about it.  That's in
btrfs_finish_one_ordered(), and it basically does:


	if (test_bit(BTRFS_ORDERED_PREALLOC))
		ret = btrfs_mark_extent_written();
	else
		ret = insert_ordered_extent_file_extent();

	unpin_extent_cache();

	if (ret < 0) {
		btrfs_abort_transaction();
		goto out;
	}

Even if unpin_extent_cache() would return an error, I'd say that it is
better to try to proceed anyway rather than abort if unpinning an extent
from cache fails.  But my opinion isn't very solid ;-)

Cheers,
-- 
Luís

Re: [PATCH] btrfs: turn unpin_extent_cache() into a void function

[David Sterba]

On Tue, Jul 18, 2023 at 06:39:06PM +0100, Luís Henriques wrote:
> The value of the 'ret' variable is never changed in function
> unpin_extent_cache().  And since the only caller of this function doesn't
> check the return value, it can simply be turned into a void function.
> 
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>  fs/btrfs/extent_map.c | 7 ++-----
>  fs/btrfs/extent_map.h | 2 +-
>  2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
> index 0cdb3e86f29b..f99c458071a4 100644
> --- a/fs/btrfs/extent_map.c
> +++ b/fs/btrfs/extent_map.c
> @@ -292,10 +292,9 @@ static void try_merge_map(struct extent_map_tree *tree, struct extent_map *em)
>   * to the generation that actually added the file item to the inode so we know
>   * we need to sync this extent when we call fsync().
>   */
> -int unpin_extent_cache(struct extent_map_tree *tree, u64 start, u64 len,
> -		       u64 gen)
> +void unpin_extent_cache(struct extent_map_tree *tree, u64 start, u64 len,
> +			u64 gen)
>  {
> -	int ret = 0;
>  	struct extent_map *em;
>  	bool prealloc = false;
>  
> @@ -327,8 +326,6 @@ int unpin_extent_cache(struct extent_map_tree *tree, u64 start, u64 len,
>  	free_extent_map(em);
>  out:
>  	write_unlock(&tree->lock);
> -	return ret;
> -
>  }

This function has unfortunatelly attracting attention to do a simple fix
to just return void, several have been aleary sent but none of them
fixes it properly. To the point I don't want to reply anymore.

https://lore.kernel.org/linux-btrfs/20180815124425.GM24025@twin.jikos.cz/
https://lore.kernel.org/linux-btrfs/20230530150359.GS575@twin.jikos.cz/
https://patchwork.kernel.org/project/linux-btrfs/patch/20190416055739.25771-1-wqu@suse.com/#22596309

"When switching a fuction to return void, please check the whole
callgraph for functions that do not properly handler errors and do
BUG_ON. You won't see errors passed from them so this gives the
impression no error handling is needed in the caller."