On 2017年10月24日 20:29, Jeff Mahoney wrote:
> On 10/24/17 7:51 AM, Qu Wenruo wrote:
>>
>>
>> On 2017年10月24日 19:00, Nikolay Borisov wrote:
>>>
>>>
>>> On 24.10.2017 11:39, Qu Wenruo wrote:
>>>> Instead of single qgroup->reserved, use a new structure btrfs_qgroup_rsv
>>>> to restore different types of reservation.
>>>>
>>>> This patch only updates the header and needed modification to pass
>>>> compile.
>>>>
>>>> Signed-off-by: Qu Wenruo <w...@suse.com>
>>>> ---
>>>> fs/btrfs/qgroup.c | 16 ++++++++++------
>>>> fs/btrfs/qgroup.h | 27 +++++++++++++++++++++++++--
>>>> 2 files changed, 35 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>>> index e172d4843eae..fe3adb996883 100644
>>>> --- a/fs/btrfs/qgroup.c
>>>> +++ b/fs/btrfs/qgroup.c
>>>> @@ -2444,7 +2444,8 @@ static int qgroup_reserve(struct btrfs_root *root,
>>>> u64 num_bytes, bool enforce)
>>>> }
>>>>
>>>> void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
>>>> - u64 ref_root, u64 num_bytes)
>>>> + u64 ref_root, u64 num_bytes,
>>>> + enum btrfs_qgroup_rsv_type type)
>>>> {
>>>> struct btrfs_root *quota_root;
>>>> struct btrfs_qgroup *qgroup;
>>>> @@ -2936,7 +2937,8 @@ static int qgroup_free_reserved_data(struct inode
>>>> *inode,
>>>> goto out;
>>>> freed += changeset.bytes_changed;
>>>> }
>>>> - btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed);
>>>> + btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed,
>>>> + BTRFS_QGROUP_RSV_DATA);
>>>> ret = freed;
>>>> out:
>>>> extent_changeset_release(&changeset);
>>>> @@ -2968,7 +2970,7 @@ static int __btrfs_qgroup_release_data(struct inode
>>>> *inode,
>>>> if (free)
>>>> btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
>>>> BTRFS_I(inode)->root->objectid,
>>>> - changeset.bytes_changed);
>>>> + changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>>>> ret = changeset.bytes_changed;
>>>> out:
>>>> extent_changeset_release(&changeset);
>>>> @@ -3045,7 +3047,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root
>>>> *root)
>>>> if (reserved == 0)
>>>> return;
>>>> trace_qgroup_meta_reserve(root, -(s64)reserved);
>>>> - btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved);
>>>> + btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved,
>>>> + BTRFS_QGROUP_RSV_META);
>>>> }
>>>>
>>>> void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
>>>> @@ -3060,7 +3063,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root,
>>>> int num_bytes)
>>>> WARN_ON(atomic64_read(&root->qgroup_meta_rsv) < num_bytes);
>>>> atomic64_sub(num_bytes, &root->qgroup_meta_rsv);
>>>> trace_qgroup_meta_reserve(root, -(s64)num_bytes);
>>>> - btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes);
>>>> + btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes,
>>>> + BTRFS_QGROUP_RSV_META);
>>>> }
>>>>
>>>> /*
>>>> @@ -3088,7 +3092,7 @@ void btrfs_qgroup_check_reserved_leak(struct inode
>>>> *inode)
>>>> }
>>>> btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
>>>> BTRFS_I(inode)->root->objectid,
>>>> - changeset.bytes_changed);
>>>> + changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>>>>
>>>> }
>>>> extent_changeset_release(&changeset);
>>>> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
>>>> index d9984e87cddf..0b04cbc5b5ce 100644
>>>> --- a/fs/btrfs/qgroup.h
>>>> +++ b/fs/btrfs/qgroup.h
>>>> @@ -61,6 +61,26 @@ struct btrfs_qgroup_extent_record {
>>>> struct ulist *old_roots;
>>>> };
>>>>
>>>> +enum btrfs_qgroup_rsv_type {
>>>> + BTRFS_QGROUP_RSV_DATA = 0,
>>>> + BTRFS_QGROUP_RSV_META = 1,
>>>> + BTRFS_QGROUP_RSV_TYPES = 1,
>>>
>>> nit: Why not BTRFS_QGROUP_RSV_TYPES_MAX = 2;
>>
>> My original plan is just as the same as yours.
>>
>> However I still remember I did it before and David fixed it by using
>> TYPES, so I follow his naming schema here.
>>
>> Kernel is also using this naming schema else where:
>> d91876496bcf ("btrfs: compress: put variables defined per compress type
>> in struct to make cache friendly")
>
> The COMPRESS_TYPES pattern isn't the right pattern to follow here.
> That's a special case since there's a _NONE that doesn't have anything
> associated with it, so we don't need to take a slot in the array.
>
> We also don't care about any of the specific values, just that they
> start at 0. The BTRFS_COMPRESS_TYPES example also has a
> BTRFS_COMPRESS_LAST item in the enum, which serves the same purpose as
> MAX.
At least in latest mainline kernel, there is no COMPRESS_LAST now.
---
enum btrfs_compression_type {
BTRFS_COMPRESS_NONE = 0,
BTRFS_COMPRESS_ZLIB = 1,
BTRFS_COMPRESS_LZO = 2,
BTRFS_COMPRESS_ZSTD = 3,
BTRFS_COMPRESS_TYPES = 3,
};
---
> I don't have a strong opinion on the naming, just that we don't
> play games with +1 when handling arrays since, as you say, that's just
> waiting for subtle bugs later.
Not waiting, already experienced it during coding.
>
> enum btrfs_qgroup_rsv_type {
> BTRFS_QGROUP_RSV_DATA = 0,
> BTRFS_QGROUP_RSV_META,
> BTRFS_QGROUP_RSV_LAST,
> };
>
>>>> +};
>>>> +
>>>> +/*
>>>> + * Represents how many bytes we reserved for this qgroup.
>>>> + *
>>>> + * Each type should have different reservation behavior.
>>>> + * E.g, data follows its io_tree flag modification, while
>>>> + * *currently* meta is just reserve-and-clear during transcation.
>>>> + *
>>>> + * TODO: Add new type for delalloc, which can exist across several
>>>> + * transaction.
>>>> + */
>
> Minor nit: It's not just delalloc. Delayed items and inodes can as
> well. The general rule is that qgroup reservations aren't essentially
> different from block reservations and follow the same usage patterns
> when operating on leaf nodes.
Yep, any reserveration who can survive transaction commit should go here
with a new reservation type.
I'll change the TODO description here.
>
>>>> +struct btrfs_qgroup_rsv {
>>>> + u64 values[BTRFS_QGROUP_RSV_TYPES + 1];
>>>
>>> nit: And here just BTRFS_QGROUP_RSV_TYPES_MAX rather than the +1 here,
>>> seems more idiomatic to me.
>>
>> To follow same naming schema from David.
>> (IIRC it was about tree-checker patchset, checking file extent type part)
>>
>> In fact, I crashed kernel several times due to the tiny +1, without even
>> a clue for hours just testing blindly, until latest gcc gives warning
>> about it.
>
> BTRFS_QGROUP_RSV_LAST would do the job here.
I'll go with this method.
Thanks,
Qu
>
> -Jeff
>
>>>
>>>> +};
>>>> +
>>>> /*
>>>> * one struct for each qgroup, organized in fs_info->qgroup_tree.
>>>> */
>>>> @@ -88,6 +108,7 @@ struct btrfs_qgroup {
>>>> * reservation tracking
>>>> */
>>>> u64 reserved;
>>>> + struct btrfs_qgroup_rsv rsv;
>>>>
>>>> /*
>>>> * lists
>>>> @@ -228,12 +249,14 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle
>>>> *trans,
>>>> struct btrfs_fs_info *fs_info, u64 srcid, u64 objectid,
>>>> struct btrfs_qgroup_inherit *inherit);
>>>> void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
>>>> - u64 ref_root, u64 num_bytes);
>>>> + u64 ref_root, u64 num_bytes,
>>>> + enum btrfs_qgroup_rsv_type type);
>>>> static inline void btrfs_qgroup_free_delayed_ref(struct btrfs_fs_info
>>>> *fs_info,
>>>> u64 ref_root, u64 num_bytes)
>>>> {
>>>> trace_btrfs_qgroup_free_delayed_ref(fs_info, ref_root, num_bytes);
>>>> - btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
>>>> + btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes,
>>>> + BTRFS_QGROUP_RSV_DATA);
>>>> }
>>>>
>>>> #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>>>>
>>> --
>>> 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
