Re: [PATCH v2 1/3] btrfs: qgroup: Allow btrfs_qgroup_reserve_data() to revert EXTENT_QGROUP_RESERVED bits when it fails

[Josef Bacik <josef@toxicpanda.com>]

On 7/3/20 2:19 AM, Qu Wenruo wrote:
> [PROBLEM]
> Before this patch, when btrfs_qgroup_reserve_data() fails, we free all
> reserved space of the changeset.
> 
> This 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, so the next btrfs_qgroup_reserve_data() will
> always success, and can go beyond qgroup limit.
> 
> [CAUSE]
> This is caused by the fact that we didn't expect to call
> btrfs_qgroup_reserve_data() again after error.
> 
> Thus btrfs_qgroup_reserve_data() frees all its reserved space.
> 
> [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@toxicpanda.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>

I spent like 10 minutes reading all this trying to figure out why we need this 
new thing.  What you are saying is in the case of say fallocate, where you have 
something like

while (cur_len < total_len) {
	ret = btrfs_qgroup_reserve_data(&changeset);
	if (ret == -EDQUOT) {
		/* Flush */
		ret = btrfs_qgroup_reserve_data(&changeset);
	}
}

then doing the free that was in btrfs_qgroup_reserve_data() could free up space 
that we had already actually used.  So the problem isn't the flushing + 
subsequent retry, it's that we could have previous reservations on that 
changeset that _were_ successful, and previously we would free all of it which 
would allow us to go over the limit.  That's not clear at all from the commit 
message.  That being said the code is fine.  Thanks,

Josef