Btrfs: fix mount failure when qgroup rescan is in progress
diff mbox

Message ID 20180626234315.15643-1-fdmanana@kernel.org
State New
Headers show

Commit Message

Filipe Manana June 26, 2018, 11:43 p.m. UTC
From: Filipe Manana <fdmanana@suse.com>

If a power failure happens while the qgroup rescan kthread is running,
the next mount operation will always fail. This is because of a recent
regression that makes qgroup_rescan_init() incorrectly return -EINVAL
when we are mounting the filesystem (through btrfs_read_qgroup_config()).
This causes the -EINVAL error to be returned regardless of any qgroup
flags being set instead of returning the error only when neither of
the flags BTRFS_QGROUP_STATUS_FLAG_RESCAN nor BTRFS_QGROUP_STATUS_FLAG_ON
are set.

A test case for fstests follows up soon.

Fixes: 9593bf49675e ("btrfs: qgroup: show more meaningful qgroup_rescan_init error message")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/qgroup.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Comments

Nikolay Borisov June 27, 2018, 3:44 p.m. UTC | #1
On 27.06.2018 02:43, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> If a power failure happens while the qgroup rescan kthread is running,
> the next mount operation will always fail. This is because of a recent
> regression that makes qgroup_rescan_init() incorrectly return -EINVAL
> when we are mounting the filesystem (through btrfs_read_qgroup_config()).
> This causes the -EINVAL error to be returned regardless of any qgroup
> flags being set instead of returning the error only when neither of
> the flags BTRFS_QGROUP_STATUS_FLAG_RESCAN nor BTRFS_QGROUP_STATUS_FLAG_ON
> are set.
> 
> A test case for fstests follows up soon.
> 
> Fixes: 9593bf49675e ("btrfs: qgroup: show more meaningful qgroup_rescan_init error message")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  fs/btrfs/qgroup.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 1874a6d2e6f5..d4171de93087 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -2784,13 +2784,20 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid,
>  
>  	if (!init_flags) {
>  		/* we're resuming qgroup rescan at mount time */
> -		if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN))
> +		if (!(fs_info->qgroup_flags &
> +		      BTRFS_QGROUP_STATUS_FLAG_RESCAN)) {
>  			btrfs_warn(fs_info,
>  			"qgroup rescan init failed, qgroup is not enabled");
> -		else if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_ON))
> +			ret = -EINVAL;
> +		} else if (!(fs_info->qgroup_flags &
> +			     BTRFS_QGROUP_STATUS_FLAG_ON)) {
>  			btrfs_warn(fs_info,
>  			"qgroup rescan init failed, qgroup rescan is not queued");
> -		return -EINVAL;
> +			ret = -EINVAL;
> +		}
> +
> +		if (ret)
> +			return ret;


How is this patch functionally different than the old code. In both
cases if either of those 2 is not set a warn is printed and -EINVAL is
returned?

>  	}
>  
>  	mutex_lock(&fs_info->qgroup_rescan_lock);
> 
--
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
Filipe Manana June 27, 2018, 3:45 p.m. UTC | #2
On Wed, Jun 27, 2018 at 4:44 PM, Nikolay Borisov <nborisov@suse.com> wrote:
>
>
> On 27.06.2018 02:43, fdmanana@kernel.org wrote:
>> From: Filipe Manana <fdmanana@suse.com>
>>
>> If a power failure happens while the qgroup rescan kthread is running,
>> the next mount operation will always fail. This is because of a recent
>> regression that makes qgroup_rescan_init() incorrectly return -EINVAL
>> when we are mounting the filesystem (through btrfs_read_qgroup_config()).
>> This causes the -EINVAL error to be returned regardless of any qgroup
>> flags being set instead of returning the error only when neither of
>> the flags BTRFS_QGROUP_STATUS_FLAG_RESCAN nor BTRFS_QGROUP_STATUS_FLAG_ON
>> are set.
>>
>> A test case for fstests follows up soon.
>>
>> Fixes: 9593bf49675e ("btrfs: qgroup: show more meaningful qgroup_rescan_init error message")
>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>> ---
>>  fs/btrfs/qgroup.c | 13 ++++++++++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index 1874a6d2e6f5..d4171de93087 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -2784,13 +2784,20 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid,
>>
>>       if (!init_flags) {
>>               /* we're resuming qgroup rescan at mount time */
>> -             if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN))
>> +             if (!(fs_info->qgroup_flags &
>> +                   BTRFS_QGROUP_STATUS_FLAG_RESCAN)) {
>>                       btrfs_warn(fs_info,
>>                       "qgroup rescan init failed, qgroup is not enabled");
>> -             else if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_ON))
>> +                     ret = -EINVAL;
>> +             } else if (!(fs_info->qgroup_flags &
>> +                          BTRFS_QGROUP_STATUS_FLAG_ON)) {
>>                       btrfs_warn(fs_info,
>>                       "qgroup rescan init failed, qgroup rescan is not queued");
>> -             return -EINVAL;
>> +                     ret = -EINVAL;
>> +             }
>> +
>> +             if (ret)
>> +                     return ret;
>
>
> How is this patch functionally different than the old code. In both
> cases if either of those 2 is not set a warn is printed and -EINVAL is
> returned?

It is explained in the changelog:

"This is because of a recent
regression that makes qgroup_rescan_init() incorrectly return -EINVAL
when we are mounting the filesystem (through btrfs_read_qgroup_config()).
This causes the -EINVAL error to be returned regardless of any qgroup
flags being set instead of returning the error only when neither of
the flags BTRFS_QGROUP_STATUS_FLAG_RESCAN nor BTRFS_QGROUP_STATUS_FLAG_ON
are set."

If you can't understand it, try the test case...


>
>>       }
>>
>>       mutex_lock(&fs_info->qgroup_rescan_lock);
>>
--
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
Nikolay Borisov June 27, 2018, 3:55 p.m. UTC | #3
On 27.06.2018 18:45, Filipe Manana wrote:
> On Wed, Jun 27, 2018 at 4:44 PM, Nikolay Borisov <nborisov@suse.com> wrote:
>>
>>
>> On 27.06.2018 02:43, fdmanana@kernel.org wrote:
>>> From: Filipe Manana <fdmanana@suse.com>
>>>
>>> If a power failure happens while the qgroup rescan kthread is running,
>>> the next mount operation will always fail. This is because of a recent
>>> regression that makes qgroup_rescan_init() incorrectly return -EINVAL
>>> when we are mounting the filesystem (through btrfs_read_qgroup_config()).
>>> This causes the -EINVAL error to be returned regardless of any qgroup
>>> flags being set instead of returning the error only when neither of
>>> the flags BTRFS_QGROUP_STATUS_FLAG_RESCAN nor BTRFS_QGROUP_STATUS_FLAG_ON
>>> are set.
>>>
>>> A test case for fstests follows up soon.
>>>
>>> Fixes: 9593bf49675e ("btrfs: qgroup: show more meaningful qgroup_rescan_init error message")
>>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>>> ---
>>>  fs/btrfs/qgroup.c | 13 ++++++++++---
>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>> index 1874a6d2e6f5..d4171de93087 100644
>>> --- a/fs/btrfs/qgroup.c
>>> +++ b/fs/btrfs/qgroup.c
>>> @@ -2784,13 +2784,20 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid,
>>>
>>>       if (!init_flags) {
>>>               /* we're resuming qgroup rescan at mount time */
>>> -             if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN))
>>> +             if (!(fs_info->qgroup_flags &
>>> +                   BTRFS_QGROUP_STATUS_FLAG_RESCAN)) {
>>>                       btrfs_warn(fs_info,
>>>                       "qgroup rescan init failed, qgroup is not enabled");
>>> -             else if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_ON))
>>> +                     ret = -EINVAL;
>>> +             } else if (!(fs_info->qgroup_flags &
>>> +                          BTRFS_QGROUP_STATUS_FLAG_ON)) {
>>>                       btrfs_warn(fs_info,
>>>                       "qgroup rescan init failed, qgroup rescan is not queued");
>>> -             return -EINVAL;
>>> +                     ret = -EINVAL;
>>> +             }
>>> +
>>> +             if (ret)
>>> +                     return ret;
>>
>>
>> How is this patch functionally different than the old code. In both
>> cases if either of those 2 is not set a warn is printed and -EINVAL is
>> returned?
> 
> It is explained in the changelog:

No need to be snide

> 
> "This is because of a recent
> regression that makes qgroup_rescan_init() incorrectly return -EINVAL
> when we are mounting the filesystem (through btrfs_read_qgroup_config()).
> This causes the -EINVAL error to be returned regardless of any qgroup
> flags being set instead of returning the error only when neither of
> the flags BTRFS_QGROUP_STATUS_FLAG_RESCAN nor BTRFS_QGROUP_STATUS_FLAG_ON
> are set."
> 
> If you can't understand it, try the test case...

Ok I see it now, however your description contradicts the code.
Currently -EINVAL will be returned when either of the 2 flags is unset i.e

!BTRFS_QGROUP_STATUS_FLAG_RESCAN && BTRFS_QGROUP_STATUS_FLAG_ON
!BTRFS_QGROUP_STATUS_FLAG_ON && BTRFS_QGROUP_STATUS_FLAG_RESCAN

and in your description you refer to neither that is the 2 flags being
unset. Perhaps those combinations are invalid due to some other reasons
which are not visible in the code but in this case the changelog should
be expanded to cover why those 2 combinations are impossible (because if
they are -EINVAL is still likely ) ?

> 
> 
>>
>>>       }
>>>
>>>       mutex_lock(&fs_info->qgroup_rescan_lock);
>>>
> 
--
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
Filipe Manana June 27, 2018, 4:05 p.m. UTC | #4
On Wed, Jun 27, 2018 at 4:55 PM, Nikolay Borisov <nborisov@suse.com> wrote:
>
>
> On 27.06.2018 18:45, Filipe Manana wrote:
>> On Wed, Jun 27, 2018 at 4:44 PM, Nikolay Borisov <nborisov@suse.com> wrote:
>>>
>>>
>>> On 27.06.2018 02:43, fdmanana@kernel.org wrote:
>>>> From: Filipe Manana <fdmanana@suse.com>
>>>>
>>>> If a power failure happens while the qgroup rescan kthread is running,
>>>> the next mount operation will always fail. This is because of a recent
>>>> regression that makes qgroup_rescan_init() incorrectly return -EINVAL
>>>> when we are mounting the filesystem (through btrfs_read_qgroup_config()).
>>>> This causes the -EINVAL error to be returned regardless of any qgroup
>>>> flags being set instead of returning the error only when neither of
>>>> the flags BTRFS_QGROUP_STATUS_FLAG_RESCAN nor BTRFS_QGROUP_STATUS_FLAG_ON
>>>> are set.
>>>>
>>>> A test case for fstests follows up soon.
>>>>
>>>> Fixes: 9593bf49675e ("btrfs: qgroup: show more meaningful qgroup_rescan_init error message")
>>>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>>>> ---
>>>>  fs/btrfs/qgroup.c | 13 ++++++++++---
>>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>>> index 1874a6d2e6f5..d4171de93087 100644
>>>> --- a/fs/btrfs/qgroup.c
>>>> +++ b/fs/btrfs/qgroup.c
>>>> @@ -2784,13 +2784,20 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid,
>>>>
>>>>       if (!init_flags) {
>>>>               /* we're resuming qgroup rescan at mount time */
>>>> -             if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN))
>>>> +             if (!(fs_info->qgroup_flags &
>>>> +                   BTRFS_QGROUP_STATUS_FLAG_RESCAN)) {
>>>>                       btrfs_warn(fs_info,
>>>>                       "qgroup rescan init failed, qgroup is not enabled");
>>>> -             else if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_ON))
>>>> +                     ret = -EINVAL;
>>>> +             } else if (!(fs_info->qgroup_flags &
>>>> +                          BTRFS_QGROUP_STATUS_FLAG_ON)) {
>>>>                       btrfs_warn(fs_info,
>>>>                       "qgroup rescan init failed, qgroup rescan is not queued");
>>>> -             return -EINVAL;
>>>> +                     ret = -EINVAL;
>>>> +             }
>>>> +
>>>> +             if (ret)
>>>> +                     return ret;
>>>
>>>
>>> How is this patch functionally different than the old code. In both
>>> cases if either of those 2 is not set a warn is printed and -EINVAL is
>>> returned?
>>
>> It is explained in the changelog:
>
> No need to be snide

No one's being snide. Simply, I can't see how the changelog doesn't
explain (what is already quite easy to notice from the code).

>
>>
>> "This is because of a recent
>> regression that makes qgroup_rescan_init() incorrectly return -EINVAL
>> when we are mounting the filesystem (through btrfs_read_qgroup_config()).
>> This causes the -EINVAL error to be returned regardless of any qgroup
>> flags being set instead of returning the error only when neither of
>> the flags BTRFS_QGROUP_STATUS_FLAG_RESCAN nor BTRFS_QGROUP_STATUS_FLAG_ON
>> are set."
>>
>> If you can't understand it, try the test case...
>
> Ok I see it now, however your description contradicts the code.
> Currently -EINVAL will be returned when either of the 2 flags is unset i.e
>
> !BTRFS_QGROUP_STATUS_FLAG_RESCAN && BTRFS_QGROUP_STATUS_FLAG_ON
> !BTRFS_QGROUP_STATUS_FLAG_ON && BTRFS_QGROUP_STATUS_FLAG_RESCAN
>
> and in your description you refer to neither that is the 2 flags being
> unset. Perhaps those combinations are invalid due to some other reasons
> which are not visible in the code but in this case the changelog should
> be expanded to cover why those 2 combinations are impossible (because if
> they are -EINVAL is still likely ) ?

I don't think the changelog is contradictory.
It says that -EINVAL is always returned, independently of which qgroup
flags are set/missing.
Further it says that the error should be returned only when one of
those 2 qgroup flags is not set (or both are not set).

>
>>
>>
>>>
>>>>       }
>>>>
>>>>       mutex_lock(&fs_info->qgroup_rescan_lock);
>>>>
>>
--
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
Qu Wenruo June 27, 2018, 11:12 p.m. UTC | #5
On 2018年06月27日 07:43, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> If a power failure happens while the qgroup rescan kthread is running,
> the next mount operation will always fail. This is because of a recent
> regression that makes qgroup_rescan_init() incorrectly return -EINVAL
> when we are mounting the filesystem (through btrfs_read_qgroup_config()).
> This causes the -EINVAL error to be returned regardless of any qgroup
> flags being set instead of returning the error only when neither of
> the flags BTRFS_QGROUP_STATUS_FLAG_RESCAN nor BTRFS_QGROUP_STATUS_FLAG_ON
> are set.

My fault.

> 
> A test case for fstests follows up soon.
> 
> Fixes: 9593bf49675e ("btrfs: qgroup: show more meaningful qgroup_rescan_init error message")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>  fs/btrfs/qgroup.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 1874a6d2e6f5..d4171de93087 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -2784,13 +2784,20 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid,
>  
>  	if (!init_flags) {
>  		/* we're resuming qgroup rescan at mount time */
> -		if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN))
> +		if (!(fs_info->qgroup_flags &
> +		      BTRFS_QGROUP_STATUS_FLAG_RESCAN)) {
>  			btrfs_warn(fs_info,
>  			"qgroup rescan init failed, qgroup is not enabled");
> -		else if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_ON))
> +			ret = -EINVAL;
> +		} else if (!(fs_info->qgroup_flags &
> +			     BTRFS_QGROUP_STATUS_FLAG_ON)) {
>  			btrfs_warn(fs_info,
>  			"qgroup rescan init failed, qgroup rescan is not queued");
> -		return -EINVAL;
> +			ret = -EINVAL;
> +		}
> +
> +		if (ret)
> +			return ret;
>  	}
>  
>  	mutex_lock(&fs_info->qgroup_rescan_lock);
>

Patch
diff mbox

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 1874a6d2e6f5..d4171de93087 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2784,13 +2784,20 @@  qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid,
 
 	if (!init_flags) {
 		/* we're resuming qgroup rescan at mount time */
-		if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN))
+		if (!(fs_info->qgroup_flags &
+		      BTRFS_QGROUP_STATUS_FLAG_RESCAN)) {
 			btrfs_warn(fs_info,
 			"qgroup rescan init failed, qgroup is not enabled");
-		else if (!(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_ON))
+			ret = -EINVAL;
+		} else if (!(fs_info->qgroup_flags &
+			     BTRFS_QGROUP_STATUS_FLAG_ON)) {
 			btrfs_warn(fs_info,
 			"qgroup rescan init failed, qgroup rescan is not queued");
-		return -EINVAL;
+			ret = -EINVAL;
+		}
+
+		if (ret)
+			return ret;
 	}
 
 	mutex_lock(&fs_info->qgroup_rescan_lock);