Re: [PATCH 00/20] Error handling fixes

Qu Wenruo
2024-01-25T04:21:40Z

On 2024/1/25 07:47, 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.

Sorry, I bombarded the mailing list again...

So there is the summary of my comments here:

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

For those error handling patches, I believe for all the BUG_ON() cases,
we should return -EUCLEAN, with an error message.

If possible we should also abort the transaction early (which can be
noisy enough to be caught by test cases, and give better call trace)

> 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()

For those key.offset == -1 search case, tree-checker may be a better
solution, as it provides more detailed info for us to debug.

But you may only want such offset == -1 check for certain key types.
As I'm not sure if something key types (e.g. DIR_ITEM, whose key offset
is a hash).

Then those call sites can convert to ASSERT() for key hit case.

> 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()

I'm totally fine with BUG_ON() to ASSERT() changes.

> 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()

And earlier transaction abort is also pretty good to me.

Thanks,
Qu

>
> 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(-)
>