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.
