diff mbox series

[v2] btrfs: don't BUG_ON() in btrfs_drop_extents()

Message ID be18a9fcfa768add6a23e3dee5dfcce55b0814f5.1732812639.git.jth@kernel.org (mailing list archive)
State New
Headers show
Series [v2] btrfs: don't BUG_ON() in btrfs_drop_extents() | expand

Commit Message

Johannes Thumshirn Nov. 29, 2024, 1:11 p.m. UTC 
From: Johannes Thumshirn <johannes.thumshirn@wdc.com>

btrfs_drop_extents() calls BUG_ON() in case the counter of to be deleted
extents is greater than 0. But all of these code paths can handle errors,
so there's no need to crash the kernel. Instead WARN() that the condition
has been met and gracefully bail out.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
Changes to v1:
- Fix spelling error in commit message
- Change ASSERT() to WARN_ON()
- Take care of the other BUG_ON() cases as well

Link to v1:
- https://lore.kernel.org/linux-btrfs/20241128093428.21485-1-jth@kernel.org
---
 fs/btrfs/file.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

Comments

Filipe Manana Nov. 29, 2024, 1:18 p.m. UTC | #1 
On Fri, Nov 29, 2024 at 1:11 PM Johannes Thumshirn <jth@kernel.org> wrote:
>
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>
> btrfs_drop_extents() calls BUG_ON() in case the counter of to be deleted
> extents is greater than 0. But all of these code paths can handle errors,
> so there's no need to crash the kernel. Instead WARN() that the condition
> has been met and gracefully bail out.
>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
> Changes to v1:
> - Fix spelling error in commit message
> - Change ASSERT() to WARN_ON()
> - Take care of the other BUG_ON() cases as well
>
> Link to v1:
> - https://lore.kernel.org/linux-btrfs/20241128093428.21485-1-jth@kernel.org
> ---
>  fs/btrfs/file.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index fbb753300071..f70ce6c65d12 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -245,7 +245,10 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
>  next_slot:
>                 leaf = path->nodes[0];
>                 if (path->slots[0] >= btrfs_header_nritems(leaf)) {
> -                       BUG_ON(del_nr > 0);
> +                       if (WARN_ON(del_nr > 0)) {
> +                               ret = -EINVAL;
> +                               break;
> +                       }
>                         ret = btrfs_next_leaf(root, path);
>                         if (ret < 0)
>                                 break;
> @@ -321,7 +324,10 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
>                  *  | -------- extent -------- |
>                  */
>                 if (args->start > key.offset && args->end < extent_end) {
> -                       BUG_ON(del_nr > 0);
> +                       if (WARN_ON(del_nr > 0)) {
> +                               ret = -EINVAL;
> +                               break;
> +                       }
>                         if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
>                                 ret = -EOPNOTSUPP;
>                                 break;
> @@ -409,7 +415,10 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
>                  *  | -------- extent -------- |
>                  */
>                 if (args->start > key.offset && args->end >= extent_end) {
> -                       BUG_ON(del_nr > 0);
> +                       if (WARN_ON(del_nr > 0)) {
> +                               ret = -EINVAL;
> +                               break;
> +                       }
>                         if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
>                                 ret = -EOPNOTSUPP;
>                                 break;
> @@ -437,7 +446,10 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
>                                 del_slot = path->slots[0];
>                                 del_nr = 1;
>                         } else {
> -                               BUG_ON(del_slot + del_nr != path->slots[0]);
> +                               if (WARN_ON(del_slot + del_nr != path->slots[0])) {
> +                                       ret = -EINVAL;
> +                                       break;
> +                               }
>                                 del_nr++;
>                         }
>
> --
> 2.43.0
>
>
Qu Wenruo Nov. 29, 2024, 8:37 p.m. UTC | #2 
在 2024/11/29 23:41, Johannes Thumshirn 写道:
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>
> btrfs_drop_extents() calls BUG_ON() in case the counter of to be deleted
> extents is greater than 0. But all of these code paths can handle errors,
> so there's no need to crash the kernel. Instead WARN() that the condition
> has been met and gracefully bail out.
>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
> Changes to v1:
> - Fix spelling error in commit message
> - Change ASSERT() to WARN_ON()
> - Take care of the other BUG_ON() cases as well
>
> Link to v1:
> - https://lore.kernel.org/linux-btrfs/20241128093428.21485-1-jth@kernel.org
> ---
>   fs/btrfs/file.c | 20 ++++++++++++++++----
>   1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index fbb753300071..f70ce6c65d12 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -245,7 +245,10 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
>   next_slot:
>   		leaf = path->nodes[0];
>   		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
> -			BUG_ON(del_nr > 0);
> +			if (WARN_ON(del_nr > 0)) {
> +				ret = -EINVAL;
> +				break;
> +			}
>   			ret = btrfs_next_leaf(root, path);
>   			if (ret < 0)
>   				break;
> @@ -321,7 +324,10 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
>   		 *  | -------- extent -------- |
>   		 */
>   		if (args->start > key.offset && args->end < extent_end) {
> -			BUG_ON(del_nr > 0);
> +			if (WARN_ON(del_nr > 0)) {
> +				ret = -EINVAL;

Do we also want to dump the leaf and output a more readable error message?

Just like what we did inside
lookup_inlin_extent_backref()/update_inline_extent_backref()/insert_inline_extent_backref()/__btrfs_free_extent()/...?

Thanks,
Qu

> +				break;
> +			}
>   			if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
>   				ret = -EOPNOTSUPP;
>   				break;
> @@ -409,7 +415,10 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
>   		 *  | -------- extent -------- |
>   		 */
>   		if (args->start > key.offset && args->end >= extent_end) {
> -			BUG_ON(del_nr > 0);
> +			if (WARN_ON(del_nr > 0)) {
> +				ret = -EINVAL;
> +				break;
> +			}
>   			if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
>   				ret = -EOPNOTSUPP;
>   				break;
> @@ -437,7 +446,10 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
>   				del_slot = path->slots[0];
>   				del_nr = 1;
>   			} else {
> -				BUG_ON(del_slot + del_nr != path->slots[0]);
> +				if (WARN_ON(del_slot + del_nr != path->slots[0])) {
> +					ret = -EINVAL;
> +					break;
> +				}
>   				del_nr++;
>   			}
>
diff mbox series

Patch

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index fbb753300071..f70ce6c65d12 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -245,7 +245,10 @@  int btrfs_drop_extents(struct btrfs_trans_handle *trans,
 next_slot:
 		leaf = path->nodes[0];
 		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
-			BUG_ON(del_nr > 0);
+			if (WARN_ON(del_nr > 0)) {
+				ret = -EINVAL;
+				break;
+			}
 			ret = btrfs_next_leaf(root, path);
 			if (ret < 0)
 				break;
@@ -321,7 +324,10 @@  int btrfs_drop_extents(struct btrfs_trans_handle *trans,
 		 *  | -------- extent -------- |
 		 */
 		if (args->start > key.offset && args->end < extent_end) {
-			BUG_ON(del_nr > 0);
+			if (WARN_ON(del_nr > 0)) {
+				ret = -EINVAL;
+				break;
+			}
 			if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
 				ret = -EOPNOTSUPP;
 				break;
@@ -409,7 +415,10 @@  int btrfs_drop_extents(struct btrfs_trans_handle *trans,
 		 *  | -------- extent -------- |
 		 */
 		if (args->start > key.offset && args->end >= extent_end) {
-			BUG_ON(del_nr > 0);
+			if (WARN_ON(del_nr > 0)) {
+				ret = -EINVAL;
+				break;
+			}
 			if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
 				ret = -EOPNOTSUPP;
 				break;
@@ -437,7 +446,10 @@  int btrfs_drop_extents(struct btrfs_trans_handle *trans,
 				del_slot = path->slots[0];
 				del_nr = 1;
 			} else {
-				BUG_ON(del_slot + del_nr != path->slots[0]);
+				if (WARN_ON(del_slot + del_nr != path->slots[0])) {
+					ret = -EINVAL;
+					break;
+				}
 				del_nr++;
 			}