[PATCH 0/3] Remove some BUG_ONs

[PATCH 0/3] Remove some BUG_ONs

[David Sterba]

Minor cleanup in the BUG_ONs, where could be either removed or replaced by
proper error handling where the call chain is ready for that. For 4.11.

David Sterba (3):
      btrfs: remove BUG_ON from __tree_mod_log_insert
      btrfs: handle allocation error in update_dev_stat_item
      btrfs: do proper error handling in btrfs_insert_xattr_item

 fs/btrfs/ctree.c    | 2 --
 fs/btrfs/dir-item.c | 3 ++-
 fs/btrfs/volumes.c  | 3 ++-
 3 files changed, 4 insertions(+), 4 deletions(-)

[PATCH 1/3] btrfs: remove BUG_ON from __tree_mod_log_insert

[David Sterba]

All callers dereference the 'tm' parameter before it gets to this
function, the NULL check does not make much sense here.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 1192bc7d2ee7..2c3c943bfcdc 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -453,8 +453,6 @@ __tree_mod_log_insert(struct btrfs_fs_info *fs_info, struct tree_mod_elem *tm)
 	struct rb_node *parent = NULL;
 	struct tree_mod_elem *cur;
 
-	BUG_ON(!tm);
-
 	tm->seq = btrfs_inc_tree_mod_seq(fs_info);
 
 	tm_root = &fs_info->tree_mod_log;
-- 
2.10.1

[PATCH 2/3] btrfs: handle allocation error in update_dev_stat_item

[David Sterba]

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/volumes.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 1fac98728814..64d6665f6eda 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6954,7 +6954,8 @@ static int update_dev_stat_item(struct btrfs_trans_handle *trans,
 	key.offset = device->devid;
 
 	path = btrfs_alloc_path();
-	BUG_ON(!path);
+	if (!path)
+		return -ENOMEM;
 	ret = btrfs_search_slot(trans, dev_root, &key, path, -1, 1);
 	if (ret < 0) {
 		btrfs_warn_in_rcu(fs_info,
-- 
2.10.1

[PATCH 3/3] btrfs: do proper error handling in btrfs_insert_xattr_item

[David Sterba]

The space check in btrfs_insert_xattr_item is duplicated in it's caller
(do_setxattr) so we won't hit the BUG_ON. Continuing without any check
could be disasterous so turn it to a proper error handling.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/dir-item.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
index 724504a2d7ac..640801082533 100644
--- a/fs/btrfs/dir-item.c
+++ b/fs/btrfs/dir-item.c
@@ -80,7 +80,8 @@ int btrfs_insert_xattr_item(struct btrfs_trans_handle *trans,
 	struct extent_buffer *leaf;
 	u32 data_size;
 
-	BUG_ON(name_len + data_len > BTRFS_MAX_XATTR_SIZE(root->fs_info));
+	if (name_len + data_len > BTRFS_MAX_XATTR_SIZE(root->fs_info))
+		return -ENOSPC;
 
 	key.objectid = objectid;
 	key.type = BTRFS_XATTR_ITEM_KEY;
-- 
2.10.1

Re: [PATCH 1/3] btrfs: remove BUG_ON from __tree_mod_log_insert

[Liu Bo]

On Mon, Feb 20, 2017 at 07:25:01PM +0100, David Sterba wrote:
> All callers dereference the 'tm' parameter before it gets to this
> function, the NULL check does not make much sense here.

Reviewed-by: Liu Bo <bo.li.liu@oracle.com>

Thanks,

-liubo
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/ctree.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 1192bc7d2ee7..2c3c943bfcdc 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -453,8 +453,6 @@ __tree_mod_log_insert(struct btrfs_fs_info *fs_info, struct tree_mod_elem *tm)
>  	struct rb_node *parent = NULL;
>  	struct tree_mod_elem *cur;
>  
> -	BUG_ON(!tm);
> -
>  	tm->seq = btrfs_inc_tree_mod_seq(fs_info);
>  
>  	tm_root = &fs_info->tree_mod_log;
> -- 
> 2.10.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] btrfs: handle allocation error in update_dev_stat_item

[Liu Bo]

On Mon, Feb 20, 2017 at 07:25:04PM +0100, David Sterba wrote:
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/volumes.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 1fac98728814..64d6665f6eda 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6954,7 +6954,8 @@ static int update_dev_stat_item(struct btrfs_trans_handle *trans,
>  	key.offset = device->devid;
>  
>  	path = btrfs_alloc_path();
> -	BUG_ON(!path);
> +	if (!path)
> +		return -ENOMEM;
>  	ret = btrfs_search_slot(trans, dev_root, &key, path, -1, 1);
>  	if (ret < 0) {
>  		btrfs_warn_in_rcu(fs_info,

Reviewed-by: Liu Bo <bo.li.liu@oracle.com>

Thanks,

-liubo
> -- 
> 2.10.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] btrfs: do proper error handling in btrfs_insert_xattr_item

[Liu Bo]

On Mon, Feb 20, 2017 at 07:25:06PM +0100, David Sterba wrote:
> The space check in btrfs_insert_xattr_item is duplicated in it's caller
> (do_setxattr) so we won't hit the BUG_ON. Continuing without any check
> could be disasterous so turn it to a proper error handling.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/dir-item.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
> index 724504a2d7ac..640801082533 100644
> --- a/fs/btrfs/dir-item.c
> +++ b/fs/btrfs/dir-item.c
> @@ -80,7 +80,8 @@ int btrfs_insert_xattr_item(struct btrfs_trans_handle *trans,
>  	struct extent_buffer *leaf;
>  	u32 data_size;
>  
> -	BUG_ON(name_len + data_len > BTRFS_MAX_XATTR_SIZE(root->fs_info));
> +	if (name_len + data_len > BTRFS_MAX_XATTR_SIZE(root->fs_info))
> +		return -ENOSPC;
>

Besides making it silent, how about adding a ASSERT to cry out?
(Although currently we'd never come into this case.)

Reviewed-by: Liu Bo <bo.li.liu@oracle.com>

Thanks,

-liubo
>  	key.objectid = objectid;
>  	key.type = BTRFS_XATTR_ITEM_KEY;
> -- 
> 2.10.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] btrfs: do proper error handling in btrfs_insert_xattr_item

[David Sterba]

On Tue, Feb 21, 2017 at 09:39:05PM -0800, Liu Bo wrote:
> On Mon, Feb 20, 2017 at 07:25:06PM +0100, David Sterba wrote:
> > The space check in btrfs_insert_xattr_item is duplicated in it's caller
> > (do_setxattr) so we won't hit the BUG_ON. Continuing without any check
> > could be disasterous so turn it to a proper error handling.
> > 
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >  fs/btrfs/dir-item.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
> > index 724504a2d7ac..640801082533 100644
> > --- a/fs/btrfs/dir-item.c
> > +++ b/fs/btrfs/dir-item.c
> > @@ -80,7 +80,8 @@ int btrfs_insert_xattr_item(struct btrfs_trans_handle *trans,
> >  	struct extent_buffer *leaf;
> >  	u32 data_size;
> >  
> > -	BUG_ON(name_len + data_len > BTRFS_MAX_XATTR_SIZE(root->fs_info));
> > +	if (name_len + data_len > BTRFS_MAX_XATTR_SIZE(root->fs_info))
> > +		return -ENOSPC;
> >
> 
> Besides making it silent, how about adding a ASSERT to cry out?
> (Although currently we'd never come into this case.)

I don't think we need the assert, the caller is supposed to handle the
error. In this case it's validation of input parameters, that could
possibly happen as the function is not static.