Re: [PATCH 1/7] btrfs: correctly model root qgroup rsv in convert

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

在 2024/3/28 03:50, Boris Burkov 写道:
> On Wed, Mar 27, 2024 at 08:30:47AM +1030, Qu Wenruo wrote:
>>
>>
>> 在 2024/3/27 08:09, Boris Burkov 写道:
>>> We use add_root_meta_rsv and sub_root_meta_rsv to track prealloc and
>>> pertrans reservations for subvols when quotas are enabled. The convert
>>> function does not properly increment pertrans after decrementing
>>> prealloc, so the count is not accurate.
>>>
>>> Note: we check that the fs is not read-only to mirror the logic in
>>> qgroup_convert_meta, which checks that before adding to the pertrans rsv.
>>>
>>> Fixes: 8287475a2055 ("btrfs: qgroup: Use root::qgroup_meta_rsv_* to record qgroup meta reserved space")
>>> Signed-off-by: Boris Burkov <boris@bur.io>
>>> ---
>>>    fs/btrfs/qgroup.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>> index a8197e25192c..2cba6451d164 100644
>>> --- a/fs/btrfs/qgroup.c
>>> +++ b/fs/btrfs/qgroup.c
>>> @@ -4492,6 +4492,8 @@ void btrfs_qgroup_convert_reserved_meta(struct btrfs_root *root, int num_bytes)
>>>    				      BTRFS_QGROUP_RSV_META_PREALLOC);
>>>    	trace_qgroup_meta_convert(root, num_bytes);
>>>    	qgroup_convert_meta(fs_info, root->root_key.objectid, num_bytes);
>>> +	if (!sb_rdonly(fs_info->sb))
>>> +		add_root_meta_rsv(root, num_bytes, BTRFS_QGROUP_RSV_META_PERTRANS);
>>
>> Don't we already call qgroup_rsv_add() inside qgroup_convert_meta()?
>> This sounds like a double add here.
>
> qgroup_rsv_add doesn't call add_root_meta_rsv. AFAICT, this is the
> separate accounting in root->qgroup_meta_rsv_pertrans to fixup underflow
> errors as we free.

My bad, it's true that we have extra per-root accounting that's not the
same as qgroup rsv.


>
>>
>> And if you have any example to show the problem in a more detailed way, it
>> would be great help.
>
> I don't have a reproducer for this, it was just something I noticed. I'm
> fine to drop this patch if you don't think it's worth the churn (and
> certainly if I'm just a dummy and didn't see where we already call it)

In that case, I think you're correct and the patch looks good to me.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

>
> In fact, this counter only exists to avoid underflow, but PERTRANS is
> cleared by exact amount, and not via btrfs_qgroup_free_meta_pertrans, so
> it might just be moot to track it at all in this way.
>
>>
>> Thanks,
>> Qu
>>>    }
>>>    /*
>