Re: [PATCH 00/14] More error handling and BUG_ON cleanups

[David Sterba <dsterba@suse.cz>]

On Thu, Feb 08, 2024 at 08:20:55PM +1030, Qu Wenruo wrote:
> >    btrfs: handle invalid extent item reference found in
> >      find_first_extent_item()
> 
> For bad extent item offsets, I'm totally fine with the current -EUCLEAN
> even it doesn't have any error message.
> As tree-checker has already verified the extent items, thus it's way
> much harder to get runtime corruption to create a -1 key.offset.

Yeah, this is the same as for the previous patchset, tree-checker
catches things early and the new error handling makes sure any
improbable error will not propagate further.

Regarding the messages, I'll add them in the futre, currently I'm
analyzing all cases we might need and prototyping the error/warning
message macros. I've been going over BUG_ONs only but we have too many
random WARN_ONs too, this should be better converted to warnings or
assertions with clear purpose and use case.

> >    btrfs: handle invalid root reference found in may_destroy_subvol()
> 
> But for this, I strongly recommend a new tree-checker entry for
> ROOT_REF, before returning -EUCLEAN without an error message.

Right, we don't seem to have a case for BTRFS_ROOT_REF_KEY in
tree-checker, at least the allowed value range could be there.

> >    btrfs: send: handle unexpected data in header buffer in begin_cmd()
> >    btrfs: send: handle unexpected inode in header process_recorded_refs()
> >    btrfs: send: handle path ref underflow in header iterate_inode_ref()
> >    btrfs: change BUG_ON to assertion in tree_move_down()
> >    btrfs: change BUG_ONs to assertions in btrfs_qgroup_trace_subtree()
> >    btrfs: delete pointless BUG_ON check on quota root in
> >      btrfs_qgroup_account_extent()
> >    btrfs: delete pointless BUG_ONs on extent item size
> 
> The above ones look good to me.
> 
> >    btrfs: handle unexpected parent block offset in
> >      btrfs_alloc_tree_block()
> 
> For this one, I'd prefer one error message or at least something noisy
> enough (aka, ASSERT()), just to make life a little easier when we
> screwed up in our development environment.

Well, there are too many BUG_ONs that somebody just sprinkled over the
code, it still documents the invariants but in a very random way.
Getting rid of them is much harder as it requires reasoning in a much
broader context. I looked at all callers, this looks like a proper API
usage check rather than a random error that would need a message.

Each call site with EUCLEAN can be revisited if we really want to get
noticed about that or not, but I'm not doing that right now unless I'm
convince we do from the context.