On 10/24/2017 06:05 AM, Qu Wenruo wrote:
>
>
> On 2017年10月24日 19:07, Nikolay Borisov wrote:
>>
>>
>> On 24.10.2017 11:39, Qu Wenruo wrote:
>>> Introduce helpers to:
>>>
>>> 1) Get total reserved space
>>> For limit calculation
>>>
>>> 2) Increase reserved space for given type
>>> 2) Decrease reserved space for given type
>>>
>>> Signed-off-by: Qu Wenruo <w...@suse.com>
>>> ---
>>> fs/btrfs/qgroup.c | 66
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 66 insertions(+)
>>>
>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>> index fe3adb996883..04fd145516ad 100644
>>> --- a/fs/btrfs/qgroup.c
>>> +++ b/fs/btrfs/qgroup.c
>>> @@ -47,6 +47,72 @@
>>> * - check all ioctl parameters
>>> */
>>>
>>> +/*
>>> + * Helpers to access qgroup reservation
>>> + *
>>> + * Callers should ensure the lock context and type are valid
>>> + */
>>> +
>>> +static u64 qgroup_rsv_total(const struct btrfs_qgroup *qgroup)
>>> +{
>>> + u64 ret = 0;
>>> + int i;
>>> +
>>> + for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
>>
>> So if you actually take up on my idea of having an explicit value which
>> denotes the end, the loop here would be just < *_END rather than the <=
>> which instantly looks suspicious of an off-by-one error.
>
> Yeah, I really like to do it, as mentioned in previous mail.
>
> But to follow the schema used elsewhere, I had no choice.
>
>>
>>> + ret += qgroup->rsv.values[i];
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static const char *qgroup_rsv_type_str(enum btrfs_qgroup_rsv_type type)
>>> +{
>>> + if (type == BTRFS_QGROUP_RSV_DATA)
>>> + return "data";
>>> + if (type == BTRFS_QGROUP_RSV_META)
>>> + return "meta";
>>> + return NULL;
>>> +}
>>> +
>>> +static void qgroup_rsv_increase(struct btrfs_qgroup *qgroup, u64 num_bytes,
>>> + enum btrfs_qgroup_rsv_type type)
>>> +{
>>> + qgroup->rsv.values[type] += num_bytes;
>>> +}
>>> +
>>> +static void qgroup_rsv_decrease(struct btrfs_qgroup *qgroup, u64 num_bytes,
>>> + enum btrfs_qgroup_rsv_type type)
>>> +{
>>> + if (qgroup->rsv.values[type] >= num_bytes) {
>>> + qgroup->rsv.values[type] -= num_bytes;
>>> + return;
>>> + }
>>> +#ifdef CONFIG_BTRFS_DEBUG
>>> + WARN_RATELIMIT(1,
>>> + "qgroup %llu %s reserved space underflow, have %llu to free
>>> %llu",
>>> + qgroup->qgroupid, qgroup_rsv_type_str(type),
>>> + qgroup->rsv.values[type], num_bytes);
>>> +#endif
>>> + qgroup->rsv.values[type] = 0;
>>> +}
>>
>>
>> Perhaps these could be modelled after
>> block_rsv_use_bytes/block_rsv_add_bytes ? I'm not entirely sure of what
>> increasing the value actually does - reserving bytes or using already
>> reserved bytes, I guess it should be reserving. In this case what about
>> qgroup_bytes_reserve/qgroup_bytes_(free|unreserve) ?
>
> I'm really bad at naming functions.
> My original idea is qgroup_rsv_add() and qgroup_rsv_rename(), but
> "rename" is 3 characters longer than "add", which makes me quite
> uncomfortable.
> (Maybe 'add' and 'sub' better?)
>
> For the "reserve|free", it can be easily confused with
> btrfs_qgroup_reserve|release_data().
>
> But in fact, this qgroup_rsv_* functions are only used to maintain
> qgroup->rsv, so it's less meaningful compared to other functions, like
> btrfs_qgroup_reserve|release_data().
>
> The value itself represents how many bytes it has already reserved for
> later operation.
> So qgroup_rsv_increase() normally means qgroup is reserving more space,
> while qgroup_rsv_decrease() means qgroup reserved space is decreasing,
> either released or freed.
>
> Hopes above explanation could inspire you about better naming ideas.
> (Because I really have no idea how to name it better while keeping the
> name the same length)
I'm not sure I understand the concern about the length. These names do
not seem excessive to me, so a few extra characters for the sake of
clarity would be well worth it.
Ed
>>> +
>>> +static void qgroup_rsv_increase_by_qgroup(struct btrfs_qgroup *dest,
>>> + struct btrfs_qgroup *src)
>>> +{
>>> + int i;
>>> +
>>> + for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
>>> + qgroup_rsv_increase(dest, src->rsv.values[i], i);
>>> +}
>>> +
>>> +static void qgroup_rsv_decrease_by_qgroup(struct btrfs_qgroup *dest,
>>> + struct btrfs_qgroup *src)
>>> +{
>>> + int i;
>>> +
>>> + for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
>>> + qgroup_rsv_decrease(dest, src->rsv.values[i], i);
>>> +}
>>
>> Why don't you model these similar to what we already have for the block
>> rsv. In this case I believe those would correspond to the
>> btrfs_block_rsv_migrate. Admittedly we don't have a counterpart to
>> rsv_decrease_by_qgroup.
>
> Not sure about 'migrate', I think it's more like 'inherit', since @src
> is not modified at all.
>
> 'increase' and 'decrease' are preferred mainly because they are the same
> length, and represents the value change obviously enough.
>
> I'm completely open to better naming schema.
> (But it's better to have same length, I'm kind of paranoid about this)
>
> Thanks,
> Qu
>
>>
>>
>>> +
>>> static void btrfs_qgroup_update_old_refcnt(struct btrfs_qgroup *qg, u64
>>> seq,
>>> int mod)
>>> {
>>>
>> --
>> 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
>>
>
--
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
