On 2017年10月26日 22:00, Nikolay Borisov wrote: > > > On 25.10.2017 05:54, Qu Wenruo wrote: >> When modifying qgroup relationship, for qgroup which only owns exclusive >> extents, we will go through quick update path. >> >> In quick update path, we will adding/substracting exclusive and reference >> number for parent qgroup, since the source (child) qgroup only have >> exclusive extents, destination (parent) qgroup will also own or lose these > ^ what > does own or lose mean how is it decided whether it's losing them or > owning them ??
Adding a child qgroup to parent, then the parent qgroup *owns* all these
extents of the child groups.
And vice verse (lose).
Of course, only for "exclusive-only" child qgroup, whose extents are all
exclusive.
>> extents exclusively.
>>
>> So should be the same for reservation, since later reservation
> What does the 'same' refer to here - the "will also own or lose these
> extents exclusively" ? It's a bit ambiguous given the surrounding context.
>> adding/releasing will also affect parent qgroup, without the servation
>
> nit: s/servation/reservation
>
>> carried from child, parent will underflow reservation or have dead
>> reservation which will never be freed.
>>
>> However original code doesn't do the same thing for reservation.
>
> what does "the same" refer to here?
"The same" here means, the old code doesn't carry the reservation from
child, unlike it carries all rfer/excl from child.
>
>> It handles qgroup reservation quite differently:
>>
>> It removes qgroup reservation, as it's allocating space from the
>> reserved qgroup for relationship adding.
>> But does nothing for qgroup reservation if we're removing a qgroup
>> relationship.
>>
>> According to the original code, it looks just like because we're adding
>> qgroup->rfer, the code assumes we're writing new data, so it's follows
>
> nit:s/it's/it/
>
> It might not be worth it doing a resend of the series with those fixes,
> I guess David could fix them up while committing them but just answering
> my question might help in fleshing out e clearer commit message.
No problem,
And answering your question can also help me to refine not only the
commit message the term used and the logic.
Thanks,
Qu
>
>> the normal write routine, by reducing qgroup->reserved and adding
>> qgroup->rfer/excl.
>>
>> This old behavior is wrong, and should be fixed to follow the same
>> excl/rfer behavior.
>>
>> Just fix it by using the correct behavior described above.
>>
>> Fixes: 31193213f1f9 ("Btrfs: qgroup: Introduce a may_use to account
>> space_info->bytes_may_use.")
>> Signed-off-by: Qu Wenruo <w...@suse.com>
>> ---
>> fs/btrfs/qgroup.c | 44 +++++++++++++++++++++++---------------------
>> 1 file changed, 23 insertions(+), 21 deletions(-)
>>
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index 2c690aa19d84..cbc31cfabf44 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -1071,21 +1071,30 @@ static void report_reserved_underflow(struct
>> btrfs_fs_info *fs_info,
>> #endif
>> qgroup->reserved = 0;
>> }
>> +
>> /*
>> - * The easy accounting, if we are adding/removing the only ref for an extent
>> - * then this qgroup and all of the parent qgroups get their reference and
>> - * exclusive counts adjusted.
>> + * The easy accounting, we're updating qgroup relationship whose child
>> qgroup
>> + * only has exclusive extents.
>> + *
>> + * In this case, all exclsuive extents will also be exlusive for parent, so
>> + * excl/rfer just get added/removed.
>> + *
>> + * So is qgroup reservation space, which should also be added/removed to
>> + * parent.
>> + * Or when child tries to release reservation space, parent will underflow
>> its
>> + * reservation (for relationship adding case).
>> *
>> * Caller should hold fs_info->qgroup_lock.
>> */
>> static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>> struct ulist *tmp, u64 ref_root,
>> - u64 num_bytes, int sign)
>> + struct btrfs_qgroup *src, int sign)
>> {
>> struct btrfs_qgroup *qgroup;
>> struct btrfs_qgroup_list *glist;
>> struct ulist_node *unode;
>> struct ulist_iterator uiter;
>> + u64 num_bytes = src->excl;
>> int ret = 0;
>>
>> qgroup = find_qgroup_rb(fs_info, ref_root);
>> @@ -1098,13 +1107,11 @@ static int __qgroup_excl_accounting(struct
>> btrfs_fs_info *fs_info,
>> WARN_ON(sign < 0 && qgroup->excl < num_bytes);
>> qgroup->excl += sign * num_bytes;
>> qgroup->excl_cmpr += sign * num_bytes;
>> - if (sign > 0) {
>> - trace_qgroup_update_reserve(fs_info, qgroup, -(s64)num_bytes);
>> - if (qgroup->reserved < num_bytes)
>> - report_reserved_underflow(fs_info, qgroup, num_bytes);
>> - else
>> - qgroup->reserved -= num_bytes;
>> - }
>> +
>> + if (sign > 0)
>> + qgroup_rsv_add_by_qgroup(qgroup, src);
>> + else
>> + qgroup_rsv_release_by_qgroup(qgroup, src);
>>
>> qgroup_dirty(fs_info, qgroup);
>>
>> @@ -1124,15 +1131,10 @@ static int __qgroup_excl_accounting(struct
>> btrfs_fs_info *fs_info,
>> qgroup->rfer_cmpr += sign * num_bytes;
>> WARN_ON(sign < 0 && qgroup->excl < num_bytes);
>> qgroup->excl += sign * num_bytes;
>> - if (sign > 0) {
>> - trace_qgroup_update_reserve(fs_info, qgroup,
>> - -(s64)num_bytes);
>> - if (qgroup->reserved < num_bytes)
>> - report_reserved_underflow(fs_info, qgroup,
>> - num_bytes);
>> - else
>> - qgroup->reserved -= num_bytes;
>> - }
>> + if (sign > 0)
>> + qgroup_rsv_add_by_qgroup(qgroup, src);
>> + else
>> + qgroup_rsv_release_by_qgroup(qgroup, src);
>> qgroup->excl_cmpr += sign * num_bytes;
>> qgroup_dirty(fs_info, qgroup);
>>
>> @@ -1175,7 +1177,7 @@ static int quick_update_accounting(struct
>> btrfs_fs_info *fs_info,
>> if (qgroup->excl == qgroup->rfer) {
>> ret = 0;
>> err = __qgroup_excl_accounting(fs_info, tmp, dst,
>> - qgroup->excl, sign);
>> + qgroup, sign);
>> if (err < 0) {
>> ret = err;
>> goto out;
>>
> --
> 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
>
signature.asc
Description: OpenPGP digital signature
