On 2018/07/27 15:09, Qu Wenruo wrote:
>
>
> On 2018年07月27日 09:43, Misono Tomohiro wrote:
>> On 2018/07/27 10:19, Qu Wenruo wrote:
>>>
>>>
>>> On 2018年07月27日 09:10, Misono Tomohiro wrote:
>>>> On 2018/07/26 18:15, Qu Wenruo wrote:
>>>>> Between btrfs_quota_enable() finished and rescan kicked in, there is a
>>>>> small window that quota status has (ON | INCONSISTENT) bits set but
>>>>> without RESCAN bits set.
>>>>>
>>>>> And transaction is committed inside the window and then power loss
>>>>> happens, we will have a quota tree with all qgroup numbers set to 0, and
>>>>> not RESCAN bit set.
>>>>>
>>>>> At next mount time, qgroup rescan will not kick in due to the missing of
>>>>> RESCAN bit, user needs to kick in rescan manually.
>>>>>
>>>>> This patch will fix it by setting RESCAN bit at btrfs_quota_enable(),
>>>>> so even after power loss we will still kick in rescan automatically.
>>>>>
>>>>> Suggested-by: Misono Tomohiro <misono.tomoh...@jp.fujitsu.com>
>>>>> Signed-off-by: Qu Wenruo <w...@suse.com>
>>>>> ---
>>>>> fs/btrfs/qgroup.c | 5 +++--
>>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>>>> index c25dc47210a3..13c1c7dd278d 100644
>>>>> --- a/fs/btrfs/qgroup.c
>>>>> +++ b/fs/btrfs/qgroup.c
>>>>> @@ -930,7 +930,8 @@ int btrfs_quota_enable(struct btrfs_trans_handle
>>>>> *trans,
>>>>> btrfs_set_qgroup_status_generation(leaf, ptr, trans->transid);
>>>>> btrfs_set_qgroup_status_version(leaf, ptr, BTRFS_QGROUP_STATUS_VERSION);
>>>>> fs_info->qgroup_flags = BTRFS_QGROUP_STATUS_FLAG_ON |
>>>>> - BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT;
>>>>> + BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT |
>>>>> + BTRFS_QGROUP_STATUS_FLAG_RESCAN;
>>>>> btrfs_set_qgroup_status_flags(leaf, ptr, fs_info->qgroup_flags);
>>>>> btrfs_set_qgroup_status_rescan(leaf, ptr, 0);
>>>>>
>>>>> @@ -987,7 +988,7 @@ int btrfs_quota_enable(struct btrfs_trans_handle
>>>>> *trans,
>>>>> fs_info->quota_root = quota_root;
>>>>> set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
>>>>> spin_unlock(&fs_info->qgroup_lock);
>>>>> - ret = qgroup_rescan_init(fs_info, 0, 1);
>>>>> + ret = qgroup_rescan_init(fs_info, 0, 0);
>>>>> if (!ret) {
>>>>> qgroup_rescan_zero_tracking(fs_info);
>>>>> btrfs_queue_work(fs_info->qgroup_rescan_workers,
>>>>>
>>>>
>>>> This is what I think at first, but is it ok not holding
>>>> fs_info->qgroup_ioctl_lock
>>>> in brfs_qgroup_rescan() as you concerned in previous thread?
>>>
>>> I think it's OK, since we have larger mutex (subvol_sem) for
>>> quota_enable/disable() so there will be no concurrency modifying flags.
>>> And we're holding trans handler from btrfs_ioctl_quota_ctl(),
>>> transaction won't be committed in btrfs_quota_enable().
>>
>> Ok, but nikolay's patch in misc-next moves transaction commit in
>> btrfs_quota_enable():
>> https://patchwork.kernel.org/patch/10508819/
>> ("btrfs: qgroups: Move transaction management inside
>> btrfs_quota_enable/disable")
>
> Since qgroup_rescan_init() has nothing do to with transaction, it looks
> OK even with Nikolay's patch.
>
Understood. Thanks for your explanation.
Misono
>>
>> This is related to https://marc.info/?l=linux-btrfs&m=152999289017582.
>> However, it seems that other people does not see the problem,
>> so I'm not sure how the above patch ends up...
>
> IIRC I also failed to reproduce it, thus can't provide much help for
> that thread.
>
> Thanks,
> Qu
>
>
>
>>
>> Thanks,
>> Tomohiro Misono
>>
>>>
>>> So I think it's OK.
>>>
>>> Thanks,
>>> Qu
>>>
>>>
>>
>
--
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
