Re: [PATCH] btrfs: remove unneeded variable: "ret"

[David Sterba <dsterba@suse.cz>]

On Mon, Jun 28, 2021 at 08:19:28PM +0800, Qu Wenruo wrote:
> On 2021/6/28 下午8:04, David Sterba wrote:
> > On Mon, Jun 28, 2021 at 09:48:40AM +0000, Damien Le Moal wrote:
> >> This one is actually a good fix :)
> > 
> > It's not a good fix and has been posted several times already. What
> > needs to be done in this function is to propagate error codes from the
> > whole call tree starting in that function. Removing code triggering a
> > warning is perhaps the simplest thing to make the warning go away but
> > the right fix needs some understanding of the function and context.
> > 
> > The patch also comes without any explanation so that does not help to
> > back the intentions to remove it. Reveiew of such path is "Please
> > explain".
> > 
> > Replying to patches attempting to fix the warning (and not the code)
> > does not seem to help, it's just pointing to the previous iterations.
> 
> But in this particular patch, it's really doing proper cleanup.
> 
> The truth is, the whole function, btrfs_destroy_delayed_refs(), only get 
> called when a transaction is aborted, and to cleanup all pending delayed 
> refs.
> 
> Thus in this particular function, there is nothing to return an error, 
> since we're already erroring out.
> 
> And in fact we should use void for btrfs_destroy_delayed_refs().
> 
> Thus opposite to my initial thought, it's in fact OK for the cleanup.
> Just one step left to change the function to return void.
> 
> And obviously, with all these comment above added to commit message.

The logic about making a function void and removing the return value has
some assumptions:

- no BUG/BUG_ON inside the function itself
- all called functions are return-value clean and either handle
  everything transparently or return a value
- the above applies recursively to the whole call chain

btrfs_destroy_delayed_refs itself contains one BUG, so that needs to be
fixed. The whole problem of making it void is in the pinned rage, see
eg. btrfs_error_unpin_extent_range. Nikolay did some cleanups there but
there are still some BUGs left in place of error handling, namely in
unpin_extent_range and I did not check completely.

I understand that it's tempting to just turn the function to void,
because there's nothing obviously wrong, but that's only on first sight.
A proper review must follow the code and once there's unhandled error
deeper in the callchain, it has to be solved first.

IIRC we've cleaned most of if not all the easy cases so it may not be
that easy to identify something we haven't seen yet. I'll put something
to the wiki in case people are interested. There's
https://btrfs.wiki.kernel.org/index.php/Project_ideas#Cleanup_projects
it hasn't been up to date but it is the place where to look for such
things.