[3/3] btrfs: qgroup, don't try to insert status item after ENOMEM in rescan worker
diff mbox

Message ID 20180426192351.473-3-jeffm@suse.com
State New
Headers show

Commit Message

Jeff Mahoney April 26, 2018, 7:23 p.m. UTC
From: Jeff Mahoney <jeffm@suse.com>

If we fail to allocate memory for a path, don't bother trying to
insert the qgroup status item.  We haven't done anything yet and it'll
fail also.  Just print an error and be done with it.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 fs/btrfs/qgroup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Nikolay Borisov April 26, 2018, 8:39 p.m. UTC | #1
On 26.04.2018 22:23, jeffm@suse.com wrote:
> From: Jeff Mahoney <jeffm@suse.com>
> 
> If we fail to allocate memory for a path, don't bother trying to
> insert the qgroup status item.  We haven't done anything yet and it'll
> fail also.  Just print an error and be done with it.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>

nit: So the code is correct however, having the out label there is
really ugly. What about on path alloc failure just have the print in the
if branch do goto done?


Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  fs/btrfs/qgroup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 8de423a0c7e3..4c0978bce5b9 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -2648,7 +2648,6 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)
>  			btrfs_end_transaction(trans);
>  	}
>  
> -out:
>  	btrfs_free_path(path);
>  
>  	mutex_lock(&fs_info->qgroup_rescan_lock);
> @@ -2688,6 +2687,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)
>  		btrfs_info(fs_info, "qgroup scan completed%s",
>  			err > 0 ? " (inconsistency flag cleared)" : "");
>  	} else {
> +out:
>  		btrfs_err(fs_info, "qgroup scan failed with %d", err);
>  	}
>  
> 
--
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
David Sterba April 27, 2018, 3:44 p.m. UTC | #2
On Thu, Apr 26, 2018 at 11:39:50PM +0300, Nikolay Borisov wrote:
> On 26.04.2018 22:23, jeffm@suse.com wrote:
> > From: Jeff Mahoney <jeffm@suse.com>
> > 
> > If we fail to allocate memory for a path, don't bother trying to
> > insert the qgroup status item.  We haven't done anything yet and it'll
> > fail also.  Just print an error and be done with it.
> > 
> > Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> 
> nit: So the code is correct however, having the out label there is
> really ugly. What about on path alloc failure just have the print in the
> if branch do goto done?

Yeah, I don't like jumping to the inner blocks either. I saw this in the
qgroup code so we should clean it up and not add new instances.

In this case, only the path allocation failure jumps to the out label,
so printing the message and then jump to done makes sense to me.
However, the message would have to be duplicated in the end, and I don't
see a better way without further restructuring the code.
--
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
Jeff Mahoney April 27, 2018, 4:08 p.m. UTC | #3
On 4/27/18 11:44 AM, David Sterba wrote:
> On Thu, Apr 26, 2018 at 11:39:50PM +0300, Nikolay Borisov wrote:
>> On 26.04.2018 22:23, jeffm@suse.com wrote:
>>> From: Jeff Mahoney <jeffm@suse.com>
>>>
>>> If we fail to allocate memory for a path, don't bother trying to
>>> insert the qgroup status item.  We haven't done anything yet and it'll
>>> fail also.  Just print an error and be done with it.
>>>
>>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>>
>> nit: So the code is correct however, having the out label there is
>> really ugly. What about on path alloc failure just have the print in the
>> if branch do goto done?
> 
> Yeah, I don't like jumping to the inner blocks either. I saw this in the
> qgroup code so we should clean it up and not add new instances.
> 
> In this case, only the path allocation failure jumps to the out label,
> so printing the message and then jump to done makes sense to me.
> However, the message would have to be duplicated in the end, and I don't
> see a better way without further restructuring the code.
> 

It doesn't require major surgery.  The else can be disconnected.

-Jeff

Patch
diff mbox

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 8de423a0c7e3..4c0978bce5b9 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2648,7 +2648,6 @@  static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)
 			btrfs_end_transaction(trans);
 	}
 
-out:
 	btrfs_free_path(path);
 
 	mutex_lock(&fs_info->qgroup_rescan_lock);
@@ -2688,6 +2687,7 @@  static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)
 		btrfs_info(fs_info, "qgroup scan completed%s",
 			err > 0 ? " (inconsistency flag cleared)" : "");
 	} else {
+out:
 		btrfs_err(fs_info, "qgroup scan failed with %d", err);
 	}