On Fri, Jun 11, 2021 at 04:23:29PM +0200, David Sterba wrote:

On Wed, May 19, 2021 at 08:09:45AM +0800, Qu Wenruo wrote:

quoted

On 2021/5/19 上午12:06, Sidong Yang wrote:

quoted

On Sun, May 16, 2021 at 07:55:25AM +0800, Qu Wenruo wrote:

quoted

On 2021/5/15 下午9:35, David Sterba wrote:

quoted

On Sat, May 15, 2021 at 10:42:15AM +0800, Qu Wenruo wrote:

quoted

On 2021/5/15 上午10:36, Sidong Yang wrote:

quoted

This patch adds the options --delete-qgroup and --no-delete-qgroup. When
the option is enabled, delete subvolume command destroies associated
qgroup together. This patch make it as default option. Even though quota
is disabled, it enables quota temporary and restore it after.

No, this is not a good idea at all.

First thing first, if quota is disabled, all qgroup info including the
level 0 qgroups will also be deleted, thus no need to enable in the
first place.

Secondly, there is already a patch in the past to delete level 0 qgroups
in kernel space, which should be a much better solution.

I've filed the issue to do it in the userspace because it gives user
more control whether to do the deletion or not and to also cover all
kernels that won't get the patch (ie. old stable kernels).

Changing the default behaviour is always risky and has a potential to
break user scripts. IMO adding the option to progs and changing the
default there is safer in this case.

Then shouldn't it still go through ioctl options?

Doing it completely in user space doesn't seem correct to me.

Yes, It still use ioctl calls for destroying qgroup.

What I mean is to add new ioctl flags for the existing destory subvolume.

Which is what I don't want to do. Why: it's doing more than operation so
the error code is unclear what it's related to. Either subvolume
deletion or qgroup deletion.

IMO, If new ioctl will be added for this problem, maybe it operates that
delete subvolume and it's qgroup. I think it makes some problem. If it
fails only deleting qgroup but subvolume, it needs to return the error
that describes it. It is also duplicated with old ioctl that destroy
subvolume only.

I think the point is how to support additional operation that usually
executed with. If the solution is adding a new ioctl includes old ioctl,
there will be too many ioctls and it's hard to manage it.

A similar suggestion was in for subvolume creation to force quota rescan,
https://lore.kernel.org/linux-btrfs/20210525162054.GE7604@twin.jikos.cz/333 (local)
same reason why not.