Re: [PATCH 1/3] btrfs: Introduce extent_changeset_revert() for qgroup

[Josef Bacik <josef@toxicpanda.com>]

On 7/1/20 8:14 PM, 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 mostly due to the fact that we don't have a good way to revert
> changeset modification accurately.
> 
> Currently the changeset infrastructure is implemented using ulist, which
> can only store two u64 values, used as start and length for each changed
> extent range.
> 
> So we can't record which changed extent is modified in last
> modification, thus unable to revert to previous status.
> 
> [FIX]
> This patch will re-implement using pure rbtree, adding a new member,
> changed_extent::seq, so we can remove changed extents which is
> modified in previous modification.
> 
> This allows us to implement qgroup_revert(), which allow btrfs to revert
> its modification to the io_tree.
> 

I'm having a hard time groking what's going on here.  These changesets are 
limited to a [start, end] range correct?  Why can we have multiple changesets 
for the same range?  Are we reserving doubly for modifications in the same 
range?  Because it seems here if you find your changeset at all we'll clear the 
io_tree, but if you can have multiple changesets for the same range then....why 
even bother?  Thanks,

Josef