On Wed, Jun 27, 2018 at 4:55 PM, Nikolay Borisov <nbori...@suse.com> wrote:
>
>
> On 27.06.2018 18:45, Filipe Manana wrote:
>> On Wed, Jun 27, 2018 at 4:44 PM, Nikolay Borisov <nbori...@suse.com> wrote:
>>>
>>>
>>> On 27.06.2018 02:43, fdman...@kernel.org wrote:
>>>> From: Filipe Manana <fdman...@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 <fdman...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
