* [PATCH] Btrfs: fix mount failure when qgroup rescan is in progress
@ 2018-06-26 23:43 fdmanana
2018-06-27 15:44 ` Nikolay Borisov
2018-06-27 23:12 ` Qu Wenruo
0 siblings, 2 replies; 6+ messages in thread
From: fdmanana @ 2018-06-26 23:43 UTC (permalink / raw)
To: linux-btrfs
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;
}
mutex_lock(&fs_info->qgroup_rescan_lock);
--
2.11.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Btrfs: fix mount failure when qgroup rescan is in progress
2018-06-26 23:43 [PATCH] Btrfs: fix mount failure when qgroup rescan is in progress fdmanana
@ 2018-06-27 15:44 ` Nikolay Borisov
2018-06-27 15:45 ` Filipe Manana
2018-06-27 23:12 ` Qu Wenruo
1 sibling, 1 reply; 6+ messages in thread
From: Nikolay Borisov @ 2018-06-27 15:44 UTC (permalink / raw)
To: fdmanana, linux-btrfs
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);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Btrfs: fix mount failure when qgroup rescan is in progress
2018-06-27 15:44 ` Nikolay Borisov
@ 2018-06-27 15:45 ` Filipe Manana
2018-06-27 15:55 ` Nikolay Borisov
0 siblings, 1 reply; 6+ messages in thread
From: Filipe Manana @ 2018-06-27 15:45 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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);
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Btrfs: fix mount failure when qgroup rescan is in progress
2018-06-27 15:45 ` Filipe Manana
@ 2018-06-27 15:55 ` Nikolay Borisov
2018-06-27 16:05 ` Filipe Manana
0 siblings, 1 reply; 6+ messages in thread
From: Nikolay Borisov @ 2018-06-27 15:55 UTC (permalink / raw)
To: Filipe Manana; +Cc: linux-btrfs
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);
>>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Btrfs: fix mount failure when qgroup rescan is in progress
2018-06-27 15:55 ` Nikolay Borisov
@ 2018-06-27 16:05 ` Filipe Manana
0 siblings, 0 replies; 6+ messages in thread
From: Filipe Manana @ 2018-06-27 16:05 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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);
>>>>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Btrfs: fix mount failure when qgroup rescan is in progress
2018-06-26 23:43 [PATCH] Btrfs: fix mount failure when qgroup rescan is in progress fdmanana
2018-06-27 15:44 ` Nikolay Borisov
@ 2018-06-27 23:12 ` Qu Wenruo
1 sibling, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2018-06-27 23:12 UTC (permalink / raw)
To: fdmanana, linux-btrfs
[-- Attachment #1.1: Type: text/plain, Size: 2021 bytes --]
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);
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-06-27 23:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-26 23:43 [PATCH] Btrfs: fix mount failure when qgroup rescan is in progress fdmanana
2018-06-27 15:44 ` Nikolay Borisov
2018-06-27 15:45 ` Filipe Manana
2018-06-27 15:55 ` Nikolay Borisov
2018-06-27 16:05 ` Filipe Manana
2018-06-27 23:12 ` Qu Wenruo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.