| Message ID | 6cd106844e522bbdd21f15572d81d4c9186725cc.1706130791.git.dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Error handling fixes | expand |
On 2024/1/25 07:47, David Sterba wrote: > Turn a BUG_ON to a properly handled error and update the error message > in the caller. It is expected that @em_in and @start passed to > btrfs_add_extent_mapping() overlap. Besides tests, the only caller > btrfs_get_extent() makes sure this is true. > > Signed-off-by: David Sterba <dsterba@suse.com> > --- > fs/btrfs/extent_map.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c > index f170e7122e74..ac5e366d57b2 100644 > --- a/fs/btrfs/extent_map.c > +++ b/fs/btrfs/extent_map.c > @@ -539,7 +539,8 @@ static noinline int merge_extent_mapping(struct extent_map_tree *em_tree, > u64 end; > u64 start_diff; > > - BUG_ON(map_start < em->start || map_start >= extent_map_end(em)); > + if (map_start < em->start || map_start >= extent_map_end(em)) > + return -EINVAL; Shouldn't we go -EUCLEAN? This is not something we really expect to hit, as it normally means something wrong with the extent map. Thanks, Qu > > if (existing->start > map_start) { > next = existing; > @@ -634,9 +635,9 @@ int btrfs_add_extent_mapping(struct btrfs_fs_info *fs_info, > free_extent_map(em); > *em_in = NULL; > WARN_ONCE(ret, > -"unexpected error %d: merge existing(start %llu len %llu) with em(start %llu len %llu)\n", > - ret, existing->start, existing->len, > - orig_start, orig_len); > +"extent map merge error existing [%llu, %llu) with em [%llu, %llu) start %llu\n", > + existing->start, existing->len, > + orig_start, orig_len, start); > } > free_extent_map(existing); > }
On Thu, Jan 25, 2024 at 02:23:03PM +1030, Qu Wenruo wrote: > > > On 2024/1/25 07:47, David Sterba wrote: > > Turn a BUG_ON to a properly handled error and update the error message > > in the caller. It is expected that @em_in and @start passed to > > btrfs_add_extent_mapping() overlap. Besides tests, the only caller > > btrfs_get_extent() makes sure this is true. > > > > Signed-off-by: David Sterba <dsterba@suse.com> > > --- > > fs/btrfs/extent_map.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c > > index f170e7122e74..ac5e366d57b2 100644 > > --- a/fs/btrfs/extent_map.c > > +++ b/fs/btrfs/extent_map.c > > @@ -539,7 +539,8 @@ static noinline int merge_extent_mapping(struct extent_map_tree *em_tree, > > u64 end; > > u64 start_diff; > > > > - BUG_ON(map_start < em->start || map_start >= extent_map_end(em)); > > + if (map_start < em->start || map_start >= extent_map_end(em)) > > + return -EINVAL; > > Shouldn't we go -EUCLEAN? > > This is not something we really expect to hit, as it normally means > something wrong with the extent map. The logic I used here is that it's a simple helper and it verifies the arguments. If they're invalid then it's EINVAL. EUCLEAN needs some interpretation like in the other patches unexpectedly finding an item, it's up to the caller.
diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c index f170e7122e74..ac5e366d57b2 100644 --- a/fs/btrfs/extent_map.c +++ b/fs/btrfs/extent_map.c @@ -539,7 +539,8 @@ static noinline int merge_extent_mapping(struct extent_map_tree *em_tree, u64 end; u64 start_diff; - BUG_ON(map_start < em->start || map_start >= extent_map_end(em)); + if (map_start < em->start || map_start >= extent_map_end(em)) + return -EINVAL; if (existing->start > map_start) { next = existing; @@ -634,9 +635,9 @@ int btrfs_add_extent_mapping(struct btrfs_fs_info *fs_info, free_extent_map(em); *em_in = NULL; WARN_ONCE(ret, -"unexpected error %d: merge existing(start %llu len %llu) with em(start %llu len %llu)\n", - ret, existing->start, existing->len, - orig_start, orig_len); +"extent map merge error existing [%llu, %llu) with em [%llu, %llu) start %llu\n", + existing->start, existing->len, + orig_start, orig_len, start); } free_extent_map(existing); }
Turn a BUG_ON to a properly handled error and update the error message in the caller. It is expected that @em_in and @start passed to btrfs_add_extent_mapping() overlap. Besides tests, the only caller btrfs_get_extent() makes sure this is true. Signed-off-by: David Sterba <dsterba@suse.com> --- fs/btrfs/extent_map.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)