On 2020/7/10 上午12:02, David Sterba wrote:
> On Wed, Jul 08, 2020 at 02:24:45PM +0800, 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)
>
> I've renamed it to qgroup_unreserve_range, as it's not clear what is
> being reverted.
>
>> +{
>> + 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;
>
> Please don't use single letter variables in new code unless it's 'i' for
> integer indexing. 'node' is fine.
>
>> + else
>> + break;
>> + }
>> + /* Empty changeset */
>> + if (!entry)
>> + goto out;
>
> Switched to return as suggested.
>
>> +
>> + 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) {
>> + struct rb_node *tmp = rb_next(n);
>
> Renamed to 'next'
>
> All non-functional changes, no need to resend.
>
I haven't see it in misc-next yet, and consider the remaining two
patches will need some modification anyway, would you mind to update the
patchset?
Thanks,
Qu
Attachment:
signature.asc
Description: OpenPGP digital signature
