Re: [PATCH v2 2/2] btrfs: automatically remove the subvolume qgroup

[David Sterba <dsterba@suse.cz>]

On Mon, Apr 29, 2024 at 06:13:33AM -0700, Boris Burkov wrote:
> I support the auto deletion in the kernel as you propose, I think it
> just makes sense. Who wants stale, empty qgroups around that aren't
> attached to any subvol? I suppose that with the drop_thresh thing, it is
> possible some parent qgroup still reflects the usage until the next full
> scan?

The stale qgroups have been out for a long time so removing them after
subvolume deletion is changing default behaviour, this always breaks
somebody's scripts or tools.

> Thinking out loud -- for regular qgroups, we could avoid this all if we
> do the reaping when usage goes to 0 and there is no subvol. So remove
> the qgroup as a consequence of the rescan, not the subvol delete. I
> imagine this is quite a bit messier, though :(
> 
> We could also just not auto-reap if that condition occurs (inconsistent
> qg with a parent), but I think that may be surprising for the same
> reasons that got you working on this in the first place...
> 
> Do we know of an explicit need to support --no-delete-qgroup? It feels
> like it is perfectly normal for us to improve the default behavior of
> the kernel or userspace tools without supporting the old behavior as a
> flag forever (without a user).

$ id=$(btrfs inspect rootid subvol)
$ btrfs subvolume delete subvol
$ btrfs qgroup remove 0/$id 1/1 .                   <---- fails
$ btrfs qgroup destroy 0/$id .                      <---- fails

> Put another way, I think it would be perfectly reasonable to term the
> stale qgroups a leaked memory resource and this patch a bug fix, if we
> are willing to get overly philosophical about it. We don't carry around
> eternal flags for bug fixes, unless it's some rather exceptional case.

The command line option does not do what I expected, if somebody would
have to update the scripts to add it then we can do the kernel
auto-remove and the document it. Eventually we can translate the -ENOENT
error code to be ignored.

> If we are on the hook for supporing that flag because we already added
> it to progs and don't want to deprecate it, then maybe we can think of
> something compatible with default auto-reap.