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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2024/4/29 22:43, Boris Burkov 写道:
> On Fri, Apr 26, 2024 at 07:21:08AM +0930, Qu Wenruo wrote:
[...]
>> 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?

Yep, that's the problem.
Although I can also do the extra dropping for regular qgroups, but since
it's already inconsistent, modifying the numbers won't make much
difference anyway.

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

This may be a little more complex, we need to do the extra check at
qgroup accounting time.
And IIRC there may be some corner case that numbers of a qgroup dropped
to 0 temporarily, but later extents accounting would increase it back.

Another concern is related to rescan.
Rescan would mark all qgroup to have 0 numbers first, then let rescan to
refill them all.

This means for a fully dropped subvolume in INCONSISTENT mode, there is
no other timing other than rescan setting its numbers to 0.
So dropping the qgroup when its number goes back to 0 won't happen for it.

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

I do not think any one would intentionally prefer a stale one.

But considering if squota requires a stale qgroup to keep the accounting
correct, the difference between regular qgroup and squota may grow
larger and larger in the future.

I'll need to re-consider the qgroup drop condition to make it only
possible to delete really stale qgroups (all zero and no subvolume for
both regular qgroup/squota, but for inconsistent regular qgroup,
allowing dropping non-zero qgroups).

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

Like a compat bit to represent if we do the auto-reap.
This would definitely make older script happy.

But I think it's a little overkilled.

Thanks,
Qu
>
> Thanks,
> Boris
>