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

[Boris Burkov <boris@bur.io>]

On Tue, Apr 30, 2024 at 07:59:17AM +0930, Qu Wenruo wrote:
> 
> 
> 在 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.

That sounds like a good design to me. And as I mentioned, you might be
able to share the conditions for squota failing with EBUSY and normal
qgroup printing a debug message about being inconsistent

> 
> 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
> > > > > 
> > > >