| Message ID | cover.1706130791.git.dsterba@suse.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Error handling fixes | expand |
On Wed, Jan 24, 2024 at 10:17:35PM +0100, 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. > Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef
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(-) >
On Thu, Jan 25, 2024 at 02:51:34PM +1030, Qu Wenruo wrote: > > > 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: I've replied to the patches and readin this mail I don't see any unanswered questions or points. The main problem with the error handling is that it's not done consistently everywhere, I'm going to spend more time on that and the discussion we had helped me to clear some points so I'll drop it to the documentation for further reference. Also it's not set in stone so we may refine it as needed. Thanks.
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(-) >
On Fri, Jan 26, 2024 at 08:28:12PM +0800, Anand Jain wrote: > 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. I'll try to sum up suggestions from the whole thread, the proposed end result of the error handling should follow this: - critical errors should be followed by a message at the place where they happen, typically next to an EUCLEAN - an exception could be something that is called frequently and the messages would flood the logs - transaction abort should be at the place where an error is detected, assuming that it's clear from the context that it's unrecoverable and no rollback is possible (ie. free structures, undo changes and only return an error) - this also needs inspection for special cases and exceptions - tree-checker verifies data at the time of reading from disk, we can make some assumptions about structure consistency - error handling should cover all possible returned values, as an example search slot can return < 0, 0 or 1, although some of them might not be possible on a consistent filesystem, all should be handled and return EUCLEAN + message for the "impossible" ones There are already examples that can be followed and copied elsewhere, however we still lack some helpers for the EUCLEAN and messages pattern.
On 1/30/24 00:13, David Sterba wrote: > On Fri, Jan 26, 2024 at 08:28:12PM +0800, Anand Jain wrote: >> 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. > > > I'll try to sum up suggestions from the whole thread, the proposed end > result of the error handling should follow this: > > - critical errors should be followed by a message at the place where > they happen, typically next to an EUCLEAN > - an exception could be something that is called frequently and the > messages would flood the logs > > - transaction abort should be at the place where an error is detected, > assuming that it's clear from the context that it's unrecoverable and > no rollback is possible (ie. free structures, undo changes and only > return an error) > - this also needs inspection for special cases and exceptions > > - tree-checker verifies data at the time of reading from disk, we can > make some assumptions about structure consistency > > - error handling should cover all possible returned values, as an > example search slot can return < 0, 0 or 1, although some of them > might not be possible on a consistent filesystem, all should be > handled and return EUCLEAN + message for the "impossible" ones > > There are already examples that can be followed and copied elsewhere, > however we still lack some helpers for the EUCLEAN and messages > pattern. Some of error log might need a rate-limited messages. Thanks for the holistic error handling in the next series. For this series: Reviewed-by: Anand Jain <anand.jain@oracle.com>