Re: [PATCH v2 1/2] btrfs: slightly loose the requirement for qgroup removal

[Qu Wenruo <wqu@suse.com>]

在 2024/4/30 07:49, Boris Burkov 写道:
> On Tue, Apr 30, 2024 at 07:30:58AM +0930, Qu Wenruo wrote:
[...]
>>
>> I'm totally fine to do it conditional, although I'd also like to get
>> feedback on dropping the numbers from parent qgroup (so we can do it for
>> both qgroup and squota).
>>
>> Would the auto drop for parent numbers be a solution?
> 
> It's a good question. It never occurred to me until this discussion
> today.
> 
> Thinking out loud, squotas as a feature is already "comfortable" with
> unaccounted space. If you enable it on a live FS, all the extents that
> already exist are unaccounted, so there is no objection on that front.
> 
> I think from a container isolation perspective, the current behavior
> makes more sense than auto dropping from the parent on subvol delete to
> allow cleaning up the qgroup.
> 
> Suppose we create a container wrapping parent qgroup with ID 1/100. To
> enforce a limit on the container customer, we set some limit on 1/100.
> Then we create a container subvol and put 0/svid0 into 1/100. The
> customer gets to do stuff in the subvol. They are a fancy customer and
> create a subvol svid1, snapshot it svid2, and delete svid1.
> 
> svid1 and svid2 are created in the subvol of id svid0, so auto-inheritance
> ensured that 1/100 was the parent of 0/svid1 and 0/svid2, but now
> deleting svid1 frees all that customer usage from 1/100 and allows the
> customer to escape the limit. This is obviously quite undesirable, and
> wouldn't require a particularly malicious customer to hit.

OK, got your point. Thanks for the detailed explanation, and I can not 
come up with any alternative so far.

So I'll make the qgroup drop to have an extra condition for squota, so 
that a squota qgroup can only be dropped when:

1) The subvolume is fully dropped
    The same one for both regular qgroup and squota

2) The number are all zero
    This would be squota specific one.

So that the numbers would still be correct while regular qgroup can 
still handle auto-deletion for inconsistent case.

Thanks,
Qu

> 
> Boris
> 
>>
>> Thanks,
>> Qu
>>>
>>> Thanks,
>>> Boris
>>>
>>>> +	key.objectid = qgroup->qgroupid;
>>>> +	key.type = BTRFS_ROOT_ITEM_KEY;
>>>> +	key.offset = -1ULL;
>>>> +	path = btrfs_alloc_path();
>>>> +	if (!path)
>>>> +		return false;
>>>> +
>>>> +	ret = btrfs_find_root(fs_info->tree_root, &key, path, NULL, NULL);
>>>> +	btrfs_free_path(path);
>>>> +	if (ret > 0)
>>>> +		return true;
>>>> +	return false;
>>>>    }
>>>>
>>>>    int btrfs_remove_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
>>>> @@ -1764,7 +1789,7 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
>>>>    		goto out;
>>>>    	}
>>>>
>>>> -	if (is_fstree(qgroupid) && qgroup_has_usage(qgroup)) {
>>>> +	if (!can_delete_qgroup(fs_info, qgroup)) {
>>>>    		ret = -EBUSY;
>>>>    		goto out;
>>>>    	}
>>>> @@ -1789,6 +1814,36 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
>>>>    	}
>>>>
>>>>    	spin_lock(&fs_info->qgroup_lock);
>>>> +	/*
>>>> +	 * Warn on reserved space. The subvolume should has no child nor
>>>> +	 * corresponding subvolume.
>>>> +	 * Thus its reserved space should all be zero, no matter if qgroup
>>>> +	 * is consistent or the mode.
>>>> +	 */
>>>> +	WARN_ON(qgroup->rsv.values[BTRFS_QGROUP_RSV_DATA] ||
>>>> +		qgroup->rsv.values[BTRFS_QGROUP_RSV_META_PREALLOC] ||
>>>> +		qgroup->rsv.values[BTRFS_QGROUP_RSV_META_PERTRANS]);
>>>> +	/*
>>>> +	 * The same for rfer/excl numbers, but that's only if our qgroup is
>>>> +	 * consistent and if it's in regular qgroup mode.
>>>> +	 * For simple mode it's not as accurate thus we can hit non-zero values
>>>> +	 * very frequently.
>>>> +	 */
>>>> +	if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_FULL &&
>>>> +	    !(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT)) {
>>>> +		if (WARN_ON(qgroup->rfer || qgroup->excl ||
>>>> +			    qgroup->rfer_cmpr || qgroup->excl_cmpr)) {
>>>> +			btrfs_warn_rl(fs_info,
>>>> +"to be deleted qgroup %u/%llu has non-zero numbers, rfer %llu rfer_cmpr %llu excl %llu excl_cmpr %llu",
>>>> +				      btrfs_qgroup_level(qgroup->qgroupid),
>>>> +				      btrfs_qgroup_subvolid(qgroup->qgroupid),
>>>> +				      qgroup->rfer,
>>>> +				      qgroup->rfer_cmpr,
>>>> +				      qgroup->excl,
>>>> +				      qgroup->excl_cmpr);
>>>> +			qgroup_mark_inconsistent(fs_info);
>>>> +		}
>>>> +	}
>>>
>>> If you go ahead with making it conditional, I would fold this warning
>>> into the check logic. Either way is fine, of course.
>>>
>>>>    	del_qgroup_rb(fs_info, qgroupid);
>>>>    	spin_unlock(&fs_info->qgroup_lock);
>>>>
>>>> --
>>>> 2.44.0
>>>>
>>>