On 2017年10月25日 00:22, Edmund Nadolski wrote:
>
>
> 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.
What about qgroup_rsv_add/remove()?
These functions are just designed to increase/decrease the value(s) of
btrfs_qgroup_rsv structure, and should not be accessed outside of
qgroup_reserve() and some special call sides.
Maybe __qgroup_rsv_add/remove() will be better considering it's only
used internally.
Thanks,
Qu
>
> 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
>
signature.asc
Description: OpenPGP digital signature
