Re: [PATCH 00/20] Error handling fixes

[Anand Jain <anand.jain@oracle.com>]

On 1/25/24 05:17, David Sterba wrote:
> Get rid of some easy BUG_ONs, there are a few groups fixing the same
> pattern. At the end there are three patches that move transaction abort
> to the right place. I have tested it on my setups only, not the CI, but
> given the type of fix we'd never hit any of them without special
> instrumentation.
> 

What is our guideline for the error message location in the function
stack, leaf function or at the root function? IMO, it should be
in the leaf functions(), so that in the event of a serious error,
we have substantial logs to pinpoint the issue. Here, despite -EUCLEAN
we have no errors logged, in some of the patches. Why not also add
error message where it wont' endup with abort(). Otherwise, debugging
becomes too difficult when looking at the errors due to corruption.

Thanks, Anand


> David Sterba (20):
>    btrfs: handle directory and dentry mismatch in btrfs_may_delete()
>    btrfs: handle invalid range and start in merge_extent_mapping()
>    btrfs: handle block group lookup error when it's being removed
>    btrfs: handle root deletion lookup error in btrfs_del_root()
>    btrfs: handle invalid root reference found in btrfs_find_root()
>    btrfs: handle invalid root reference found in btrfs_init_root_free_objectid()
>    btrfs: handle chunk tree lookup error in btrfs_relocate_sys_chunks()
>    btrfs: handle invalid extent item reference found in check_committed_ref()
>    btrfs: export: handle invalid inode or root reference in btrfs_get_parent()
>    btrfs: delayed-inode: drop pointless BUG_ON in __btrfs_remove_delayed_item()
>    btrfs: change BUG_ON to assertion when checking for delayed_node root
>    btrfs: defrag: change BUG_ON to assertion in btrfs_defrag_leaves()
>    btrfs: change BUG_ON to assertion in btrfs_read_roots()
>    btrfs: change BUG_ON to assertion when verifying lockdep class setup
>    btrfs: change BUG_ON to assertion when verifying root in btrfs_alloc_reserved_file_extent()
>    btrfs: change BUG_ON to assertion in reset_balance_state()
>    btrfs: unify handling of return values of btrfs_insert_empty_items()
>    btrfs: move transaction abort to the error site in btrfs_delete_free_space_tree()
>    btrfs: move transaction abort to the error site in btrfs_create_free_space_tree()
>    btrfs: move transaction abort to the error site btrfs_rebuild_free_space_tree()
> 
>   fs/btrfs/block-group.c     |  4 ++-
>   fs/btrfs/ctree.c           |  4 +++
>   fs/btrfs/defrag.c          |  2 +-
>   fs/btrfs/delayed-inode.c   |  4 +--
>   fs/btrfs/disk-io.c         | 11 ++++++--
>   fs/btrfs/export.c          |  9 +++++-
>   fs/btrfs/extent-tree.c     | 11 ++++++--
>   fs/btrfs/extent_map.c      |  9 +++---
>   fs/btrfs/file-item.c       |  3 --
>   fs/btrfs/free-space-tree.c | 56 ++++++++++++++++++++++----------------
>   fs/btrfs/ioctl.c           |  4 ++-
>   fs/btrfs/locking.c         |  2 +-
>   fs/btrfs/root-tree.c       | 16 +++++++++--
>   fs/btrfs/uuid-tree.c       |  2 +-
>   fs/btrfs/volumes.c         | 14 ++++++++--
>   15 files changed, 102 insertions(+), 49 deletions(-)
>