| Message ID | 20230718173906.12568-1-lhenriques@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: turn unpin_extent_cache() into a void function | expand |
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?
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,
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."
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);
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(-)