On 2020/5/6 下午11:04, David Sterba wrote:
> On Wed, Apr 08, 2020 at 08:36:27PM +0800, Qu Wenruo wrote:
>> Forgot to mention, although this doesn't cause any data corruption, it
>> breaks snapper, which has some kind of "space aware cleaner algorithm",
>> which put all newly created snapshots into 1/0, but not the current root
>> subvolume.
>>
>> And since snapper uses snapshot ioctl to assign qgroup relationship
>> directly, without using qgrou assign ioctl, it has no way to detect such
>> problem.
>>
>> Hopes we can get this patch into current release cycle.
>
> It's still time to get it to 5.8, with CC: stable it could get
> propagated to other versions. For 5.7 it's not clear at this point as
> the bug does not seem to be urgent and as far as I understand it,
> there's a workaround.
>
> Also with an application (snapper) using some semantics of the ioctls,
> we need to actually test it with patched and unpatched kernel, or maybe
> snapper needs some fixup first.
>
>>> --- a/fs/btrfs/qgroup.c
>>> +++ b/fs/btrfs/qgroup.c
>>> @@ -2622,6 +2622,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>>> struct btrfs_root *quota_root;
>>> struct btrfs_qgroup *srcgroup;
>>> struct btrfs_qgroup *dstgroup;
>>> + bool need_rescan = false;
>>> u32 level_size = 0;
>>> u64 nums;
>>>
>>> @@ -2765,6 +2766,13 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>>> goto unlock;
>>> }
>>> ++i_qgroups;
>>> +
>>> + /*
>>> + * If we're doing a snapshot, and adding the snapshot to a new
>>> + * qgroup, the numbers are guaranteed to be incorrect.
>>> + */
>>> + if (srcid)
>>> + need_rescan = true;
>>> }
>>>
>>> for (i = 0; i < inherit->num_ref_copies; ++i, i_qgroups += 2) {
>>> @@ -2784,6 +2792,9 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>>>
>>> dst->rfer = src->rfer - level_size;
>>> dst->rfer_cmpr = src->rfer_cmpr - level_size;
>>> +
>>> + /* Manually tweaking numbers? No way to keep qgroup sane */
>>> + need_rescan = true;
>>> }
>>> for (i = 0; i < inherit->num_excl_copies; ++i, i_qgroups += 2) {
>>> struct btrfs_qgroup *src;
>>> @@ -2802,6 +2813,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>>>
>>> dst->excl = src->excl + level_size;
>>> dst->excl_cmpr = src->excl_cmpr + level_size;
>>> + need_rescan = true;
>>> }
>>>
>>> unlock:
>>> @@ -2809,6 +2821,8 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>>> out:
>>> if (!committing)
>>> mutex_unlock(&fs_info->qgroup_ioctl_lock);
>>> + if (need_rescan)
>>> + fs_info->qgroup_flags |= BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT;
>
> This got me curious, a non-atomic change to qgroup_flags and without
> any protection. The function is not running in a safe context (like
> quota enable or disable) so lack of synchronization seems suspicious. I
> grepped for other changes to the qgroup_flags and it's very
> inconsistent. Sometimes it's the fs_info::qgroup_lock, no lokcing at all
> or no obvious lock but likely fs_info::qgroup_ioctl_lock or
> qgroup_rescan_lock.
>
> I was considering using atomic bit updates but that would be another
> patch.
>
Isn't the context safe because we're at the commit transaction critical
section?
Thanks,
Qu
Attachment:
signature.asc
Description: OpenPGP digital signature
