Re: [PATCH] btrfs: handle errors properly in update_inline_extent_backref()

[David Sterba <dsterba@suse.cz>]

On Thu, Aug 10, 2023 at 09:13:43AM +0800, Qu Wenruo wrote:
> On 2023/8/10 02:59, David Sterba wrote:
> > On Wed, Aug 09, 2023 at 03:08:21PM +0800, Qu Wenruo wrote:
> >> [PROBLEM]
> >> Inside function update_inline_extent_backref(), we have several
> >> BUG_ON()s along with some ASSERT()s which can be triggered by corrupted
> >> filesystem.
> >>
> >> [ANAYLYSE]
> >> Most of those BUG_ON()s and ASSERT()s are just a way of handling
> >> unexpected on-disk data.
> >>
> >> Although we have tree-checker to rule out obviously incorrect extent
> >> tree blocks, it's not enough for those ones.
> >>
> >> Thus we need proper error handling for them.
> >>
> >> [FIX]
> >> Thankfully all the callers of update_inline_extent_backref() would
> >> eventually handle the errror by aborting the current transaction.
> >>
> >> So this patch would do the proper error handling by:
> >>
> >> - Make update_inline_extent_backref() to return int
> >>    The return value would be either 0 or -EUCLEAN.
> >>
> >> - Replace BUG_ON()s and ASSERT()s with proper error handling
> >>    This includes:
> >>    * Dump the bad extent tree leaf
> >>    * Output an error message for the cause
> >>      This would include the extent bytenr, num_bytes (if needed),
> >>      the bad values and expected good values.
> >>    * Return -EUCLEAN
> >>
> >>    Note here we remove all the WARN_ON()s, as eventually the transaction
> >>    would be aborted, thus a backtrace would be triggered anyway.
> >>
> >> - Better comments on why we expect refs == 1 and refs_to_mode == -1 for
> >>    tree blocks
> >>
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >> ---
> >>   fs/btrfs/extent-tree.c | 80 ++++++++++++++++++++++++++++++++++--------
> >>   1 file changed, 65 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> >> index 3cae798499e2..45e325523e81 100644
> >> --- a/fs/btrfs/extent-tree.c
> >> +++ b/fs/btrfs/extent-tree.c
> >> @@ -381,11 +381,11 @@ int btrfs_get_extent_inline_ref_type(const struct extent_buffer *eb,
> >>   		}
> >>   	}
> >>
> >> +	WARN_ON(1);
> >>   	btrfs_print_leaf(eb);
> >>   	btrfs_err(eb->fs_info,
> >>   		  "eb %llu iref 0x%lx invalid extent inline ref type %d",
> >>   		  eb->start, (unsigned long)iref, type);
> >> -	WARN_ON(1);
> >
> > Do we even want to print the warning here? There's the whole leaf, error
> > message, why would we need the stack trace?
> 
> Following the principle I mentioned in another thread, you're right, we
> don't need the warning as long as we move the transaction abort closer
> to the error site.
> 
> Otherwise we have two possible sites calling this function, thus harder
> to locate the real call chain.

So I've kept the warning for now, removing it deserves the reasoning and
review of the other call sites.