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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2024/2/8 19:29, David Sterba wrote:
> Continuation of https://lore.kernel.org/linux-btrfs/cover.1706130791.git.dsterba@suse.com/
> BUG_ON converted to error handling, removed or moved.
>
> David Sterba (14):
>    btrfs: push errors up from add_async_extent()
>    btrfs: update comment and drop assertion in extent item lookup in
>      find_parent_nodes()
>    btrfs: handle invalid extent item reference found in
>      extent_from_logical()

The above ones look good to me.

>    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.


>    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.

>    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.

Thanks,
Qu
>    btrfs: delete BUG_ON in btrfs_init_locked_inode()
>
>   fs/btrfs/backref.c     | 20 +++++++++++++++-----
>   fs/btrfs/extent-tree.c | 12 ++++++++++--
>   fs/btrfs/inode.c       | 23 ++++++++++++++++-------
>   fs/btrfs/qgroup.c      |  6 ++----
>   fs/btrfs/scrub.c       |  9 ++++++++-
>   fs/btrfs/send.c        | 27 +++++++++++++++++++++++----
>   6 files changed, 74 insertions(+), 23 deletions(-)
>