| Message ID | 71f49d2923b8bff3a06006abfcb298b10e7a0683.1711488980.git.boris@bur.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: various qg meta rsv leak fixes | expand |
在 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. And if you have any example to show the problem in a more detailed way, it would be great help. Thanks, Qu > } > > /*
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 > > } > > /*
在 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 >>> } >>> /* >
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); } /*
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(+)