On 27.06.2018 18:45, Filipe Manana wrote:
> On Wed, Jun 27, 2018 at 4:44 PM, Nikolay Borisov <nborisov@xxxxxxxx> wrote:
>>
>>
>> On 27.06.2018 02:43, fdmanana@xxxxxxxxxx wrote:
>>> From: Filipe Manana <fdmanana@xxxxxxxx>
>>>
>>> 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@xxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
