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

[Boris Burkov <boris@bur.io>]

On Fri, Apr 26, 2024 at 07:21:08AM +0930, Qu Wenruo wrote:
> 
> 
> 在 2024/4/25 22:04, David Sterba 写道:
> > On Thu, Apr 25, 2024 at 07:49:12AM +0930, Qu Wenruo wrote:
> > > 
> > > 
> > > 在 2024/4/24 22:11, David Sterba 写道:
> > > > On Fri, Apr 19, 2024 at 07:16:53PM +0930, Qu Wenruo wrote:
> > > > > Currently if we fully removed a subvolume (not only unlinked, but fully
> > > > > dropped its root item), its qgroup would not be removed.
> > > > > 
> > > > > Thus we have "btrfs qgroup clear-stale" to handle such 0 level qgroups.
> > > > 
> > > > There's also an option 'btrfs subvolume delete --delete-qgroup' that
> > > > does that and is going to be default in 6.9. With this kernel change it
> > > > would break the behaviour of the --no-delete-qgroup, which is there for
> > > > the case something depends on that.  For now I'd rather postpone
> > > > changing the kernel behaviour.
> > > > 
> > > 
> > > A quick glance of the --delete-qgroup shows it won't work as expected at
> > > all.
> > > 
> > > Firstly, the qgroup delete requires the qgroup numbers to be 0.
> > > Meanwhile qgroup numbers can only be 0 after 1) the full subvolume has
> > > been dropped 2) a transaction is committed to reflect the qgroup numbers.
> > 
> > The deletion option calls ioctl, so this means that 'btrfs qgroup remove'
> > will not delete it either?
> 
> Nope, at least if the subvolume is not cleaned up immediately.
> > 
> > > Both situation is only handled in my patchset, thus this means for a lot
> > > of cases it won't work at all.
> > > 
> > > Furthermore, there is the drop_subtree_threshold thing, which can mark
> > > qgroup inconsistent and skip accounting, making the target subvolume's
> > > qgroup numbers never fall back to 0 (until next rescan).
> > > 
> > > So I'm afraid the --delete-qgroup won't work until the 1/2 patch get
> > > merged (allowing deleting qgroups as long as the target subvolume is gone).
> > 
> > Ok, so for emulation of the complete removal in userspace it's
> > 
> > btrfs subvolume delete 123
> > btrfs subvolume sync 123
> > btrfs qgroup remove 0/123
> > 
> > but this needs to wait until the sync is finished and that is not
> > expected for the subvolume delete command.
> 
> That's the problem, and why doing it in user space has it limits.
> 
> Furthermore, with drop_subtree_threshold or other qgroup operations
> marking the qgroup inconsistent, you can not delete that qgroup at all,
> until the next rescan.
> 
> > It needs to be fixed but now
> > I'm not sure this can be default in 6.9 as planned.
> 
> I'd say, you should not implement this feature without really
> understanding the challenges in the first place.
> 
> And that's why I really prefer you send out non-trivial btrfs-progs for
> review, other than pushing them directly into github repo.
> 
> Thanks,
> Qu

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?

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

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.

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.

Thanks,
Boris