[PATCH] Btrfs: fix endless loop in balancing block groups

[PATCH] Btrfs: fix endless loop in balancing block groups

[Liu Bo]

Qgroup function may overwrite the saved error 'err' with 0
in case quota is not enabled, and this ends up with a
endless loop in balance because we keep going back to balance
the same block group.

It really should use 'ret' instead.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/relocation.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 8a2c2a0..c0c13dc 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -4200,9 +4200,11 @@ restart:
 		err = PTR_ERR(trans);
 		goto out_free;
 	}
-	err = qgroup_fix_relocated_data_extents(trans, rc);
-	if (err < 0) {
-		btrfs_abort_transaction(trans, err);
+	ret = qgroup_fix_relocated_data_extents(trans, rc);
+	if (ret < 0) {
+		btrfs_abort_transaction(trans, ret);
+		if (!err)
+			err = ret;
 		goto out_free;
 	}
 	btrfs_commit_transaction(trans, rc->extent_root);
-- 
2.5.5

Re: [PATCH] Btrfs: fix endless loop in balancing block groups

[Qu Wenruo]

At 09/01/2016 07:43 AM, Liu Bo wrote:
> Qgroup function may overwrite the saved error 'err' with 0
> in case quota is not enabled, and this ends up with a
> endless loop in balance because we keep going back to balance
> the same block group.

In which case?

If join_trans() fails, we won't go through qgroup fix.
And before join_trans(), they all go to out_free/out tag.
Nothing to do with qgroup fix.

And if we hit qgroup_fix_relocated_data_extents(), then 'err' is alreayd 
0, nothing we need to save.

And even for quota disabled case, qgroup_fix_relocated_data_extents() 
will just return 0. Nothing wrong at all.

Or did I miss something?

Thanks,
Qu
>
> It really should use 'ret' instead.
>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  fs/btrfs/relocation.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 8a2c2a0..c0c13dc 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -4200,9 +4200,11 @@ restart:
>  		err = PTR_ERR(trans);
>  		goto out_free;
>  	}
> -	err = qgroup_fix_relocated_data_extents(trans, rc);
> -	if (err < 0) {
> -		btrfs_abort_transaction(trans, err);
> +	ret = qgroup_fix_relocated_data_extents(trans, rc);
> +	if (ret < 0) {
> +		btrfs_abort_transaction(trans, ret);
> +		if (!err)
> +			err = ret;
>  		goto out_free;
>  	}
>  	btrfs_commit_transaction(trans, rc->extent_root);
>

Re: [PATCH] Btrfs: fix endless loop in balancing block groups

[Liu Bo]

On Thu, Sep 01, 2016 at 08:32:00AM +0800, Qu Wenruo wrote:
> 
> 
> At 09/01/2016 07:43 AM, Liu Bo wrote:
> > Qgroup function may overwrite the saved error 'err' with 0
> > in case quota is not enabled, and this ends up with a
> > endless loop in balance because we keep going back to balance
> > the same block group.
> 
> In which case?
> 
> If join_trans() fails, we won't go through qgroup fix.
> And before join_trans(), they all go to out_free/out tag.
> Nothing to do with qgroup fix.

I don't think so.

It doesn't always go to out_free, in the while() loop, it'd break the
loop on any error and record it in @err, then go through
prepare_to_merge() + merge_reloc_roots() and other stuff.

Here's an example for keeping err after the above while loop(),
...
err = prepare_to_merge(rc, err);
...

Thanks,

-liubo

> 
> And if we hit qgroup_fix_relocated_data_extents(), then 'err' is alreayd 0,
> nothing we need to save.
> 
> And even for quota disabled case, qgroup_fix_relocated_data_extents() will
> just return 0. Nothing wrong at all.
> 
> Or did I miss something?
> 
> Thanks,
> Qu
> > 
> > It really should use 'ret' instead.
> > 
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> >  fs/btrfs/relocation.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> > index 8a2c2a0..c0c13dc 100644
> > --- a/fs/btrfs/relocation.c
> > +++ b/fs/btrfs/relocation.c
> > @@ -4200,9 +4200,11 @@ restart:
> >  		err = PTR_ERR(trans);
> >  		goto out_free;
> >  	}
> > -	err = qgroup_fix_relocated_data_extents(trans, rc);
> > -	if (err < 0) {
> > -		btrfs_abort_transaction(trans, err);
> > +	ret = qgroup_fix_relocated_data_extents(trans, rc);
> > +	if (ret < 0) {
> > +		btrfs_abort_transaction(trans, ret);
> > +		if (!err)
> > +			err = ret;
> >  		goto out_free;
> >  	}
> >  	btrfs_commit_transaction(trans, rc->extent_root);
> > 
> 
> 

Re: [PATCH] Btrfs: fix endless loop in balancing block groups

[Qu Wenruo]

At 09/01/2016 08:54 AM, Liu Bo wrote:
> On Thu, Sep 01, 2016 at 08:32:00AM +0800, Qu Wenruo wrote:
>>
>>
>> At 09/01/2016 07:43 AM, Liu Bo wrote:
>>> Qgroup function may overwrite the saved error 'err' with 0
>>> in case quota is not enabled, and this ends up with a
>>> endless loop in balance because we keep going back to balance
>>> the same block group.
>>
>> In which case?
>>
>> If join_trans() fails, we won't go through qgroup fix.
>> And before join_trans(), they all go to out_free/out tag.
>> Nothing to do with qgroup fix.
>
> I don't think so.
>
> It doesn't always go to out_free, in the while() loop, it'd break the
> loop on any error and record it in @err, then go through
> prepare_to_merge() + merge_reloc_roots() and other stuff.
>
> Here's an example for keeping err after the above while loop(),
> ...
> err = prepare_to_merge(rc, err);
> ...

OK, that's right.
Reviewed-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

While IMHO it's a larger problem that we didn't handle the error 
immediately after prepare_to_merge() or inside the while(1) loop, but 
postpone it to later parts.

Thanks,
Qu


>
> Thanks,
>
> -liubo
>
>>
>> And if we hit qgroup_fix_relocated_data_extents(), then 'err' is alreayd 0,
>> nothing we need to save.
>>
>> And even for quota disabled case, qgroup_fix_relocated_data_extents() will
>> just return 0. Nothing wrong at all.
>>
>> Or did I miss something?
>>
>> Thanks,
>> Qu
>>>
>>> It really should use 'ret' instead.
>>>
>>> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
>>> ---
>>>  fs/btrfs/relocation.c | 8 +++++---
>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>>> index 8a2c2a0..c0c13dc 100644
>>> --- a/fs/btrfs/relocation.c
>>> +++ b/fs/btrfs/relocation.c
>>> @@ -4200,9 +4200,11 @@ restart:
>>>  		err = PTR_ERR(trans);
>>>  		goto out_free;
>>>  	}
>>> -	err = qgroup_fix_relocated_data_extents(trans, rc);
>>> -	if (err < 0) {
>>> -		btrfs_abort_transaction(trans, err);
>>> +	ret = qgroup_fix_relocated_data_extents(trans, rc);
>>> +	if (ret < 0) {
>>> +		btrfs_abort_transaction(trans, ret);
>>> +		if (!err)
>>> +			err = ret;
>>>  		goto out_free;
>>>  	}
>>>  	btrfs_commit_transaction(trans, rc->extent_root);
>>>
>>
>>
>
>