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

[Boris Burkov <boris@bur.io>]

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.

> 
> 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 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
> >   }
> >   /*