On 2020/7/8 下午10:09, Josef Bacik wrote:
> On 7/8/20 2:24 AM, Qu Wenruo wrote:
>> [PROBLEM]
>> Before this patch, when btrfs_qgroup_reserve_data() fails, we free all
>> reserved space of the changeset.
>>
>> For example:
>> ret = btrfs_qgroup_reserve_data(inode, changeset, 0, SZ_1M);
>> ret = btrfs_qgroup_reserve_data(inode, changeset, SZ_1M, SZ_1M);
>> ret = btrfs_qgroup_reserve_data(inode, changeset, SZ_2M, SZ_1M);
>>
>> If the last btrfs_qgroup_reserve_data() failed, it will release all [0,
>> 3M) range.
>>
>> This behavior is kinda OK for now, as when we hit -EDQUOT, we normally
>> go error handling and need to release all reserved ranges anyway.
>>
>> But this also means the following call is not possible:
>> ret = btrfs_qgroup_reserve_data();
>> if (ret == -EDQUOT) {
>> /* Do something to free some qgroup space */
>> ret = btrfs_qgroup_reserve_data();
>> }
>>
>> As if the first btrfs_qgroup_reserve_data() fails, it will free all
>> reserved qgroup space.
>>
>> [CAUSE]
>> This is because we release all reserved ranges when
>> btrfs_qgroup_reserve_data() fails.
>>
>> [FIX]
>> This patch will implement a new function, qgroup_revert(), to iterate
>> through the ulist nodes, to find any nodes in the failure range, and
>> remove the EXTENT_QGROUP_RESERVED bits from the io_tree, and decrease
>> the extent_changeset::bytes_changed, so that we can revert to previous
>> status.
>>
>> This allows later patches to retry btrfs_qgroup_reserve_data() if EDQUOT
>> happens.
>>
>> Suggested-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
>> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
>> ---
>> fs/btrfs/qgroup.c | 90 +++++++++++++++++++++++++++++++++++++++--------
>> 1 file changed, 75 insertions(+), 15 deletions(-)
>>
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index ee0ad33b659c..84a452dea3f9 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -3447,6 +3447,71 @@ btrfs_qgroup_rescan_resume(struct btrfs_fs_info
>> *fs_info)
>> }
>> }
>> +static int qgroup_revert(struct btrfs_inode *inode,
>> + struct extent_changeset *reserved, u64 start,
>> + u64 len)
>> +{
>> + struct rb_node *n = reserved->range_changed.root.rb_node;
>> + struct ulist_node *entry = NULL;
>> + int ret = 0;
>> +
>> + while (n) {
>> + entry = rb_entry(n, struct ulist_node, rb_node);
>> + if (entry->val < start)
>> + n = n->rb_right;
>> + else if (entry)
>> + n = n->rb_left;
>> + else
>> + break;
>> + }
>> + /* Empty changeset */
>> + if (!entry)
>> + goto out;
>
> Don't need the goto out here, just return ret;
>
>> +
>> + if (entry->val > start && rb_prev(&entry->rb_node))
>> + entry = rb_entry(rb_prev(&entry->rb_node), struct ulist_node,
>> + rb_node);
>> +
>> + n = &entry->rb_node;
>> + while (n) {
>
> for (n = &entry->rb_node; n; n = rb_next(n)) {
You get into the trap!
Since @n can be deleted from the tree, it needs the @tmp to record
what's the real next node.
But I can still use for loop with @tmp involved though.
Thanks,
Qu
>
> Thanks,
>
> Josef
