Re: [RFC v1 4/4] xfs: Add support to shrink multiple empty realtime groups

Nirjhar Roy (IBM)
2026-02-25T05:17:47Z

On Wed, 2026-02-18 at 22:23 -0800, Christoph Hellwig wrote:
> Just q quick glance, no real review:
>
> > +int
> > +xfs_group_get_active_refcount(struct xfs_group *xg)
> > +{
> > + ASSERT(xg);
> > + return atomic_read(&xg->xg_active_ref);
> > +}
> > +
> > +int
> > +xfs_group_get_passive_refcount(struct xfs_group *xg)
> > +{
> > + ASSERT(xg);
> > + return atomic_read(&xg->xg_ref);
>
> Using "get" to read a refcount value is very confusing. Looking
> at the users I'm tempted to say just open code these and the
> ag/rtg wrappers.

Noted.
>
> > - rtg = xfs_rtgroup_grab(mp, prev_rgcount - 1);
> > + if (prev_rgcount >= mp->m_sb.sb_rgcount)
> > + rgno = mp->m_sb.sb_rgcount - 1;
> > + else
> > + rgno = prev_rgcount - 1;
> > + rtg = xfs_rtgroup_grab(mp, rgno);
>
> Throw in a comment that this is about grow/shrink?

Noted.
>
> > +void
>
> Stick to the unique XFS style here and in other places, please.

Sure.
>
> > +xfs_rtgroup_activate(struct xfs_rtgroup *rtg)
> > +{
> > + ASSERT(!xfs_rtgroup_is_active(rtg));
> > + init_waitqueue_head(&rtg_group(rtg)->xg_active_wq);
> > + atomic_set(&rtg_group(rtg)->xg_active_ref, 1);
> > + xfs_add_frextents(rtg_mount(rtg),
> > + xfs_rtgroup_extents(rtg_mount(rtg), rtg_rgno(rtg)));
> > +}
> > +
> > +int
> > +xfs_rtgroup_deactivate(struct xfs_rtgroup *rtg)
>
> It might also make sense to explain what activate/deactive means here.

Okay, will update in the upcoming revisions.
>
> > +{
> > + ASSERT(rtg);
> > +
> > + int error = 0;
>
> No need for the assert, and code goes after declarations.

Noted.
>
> > + xfs_rgnumber_t rgno = rtg_rgno(rtg);
> > + struct xfs_mount *mp = rtg_mount(rtg);
> > + xfs_rtxnum_t rtextents =
> > + xfs_rtgroup_extents(mp, rgno);
>
> This assignment fits onto a single line easily.

Okay - will fix this in the next revision.
>
> > + ASSERT(xfs_rtgroup_is_active(rtg));
> > + ASSERT(rtg_rgno(rtg) < mp->m_sb.sb_rgcount);
> > +
> > + if (!xfs_rtgroup_is_empty(rtg))
> > + return -ENOTEMPTY;
> > + /*
> > + * Manually reduce/reserve 1 realtime group worth of
> > + * free realtime extents from the global counters. This is necessary
> > + * in order to prevent a race where, some rtgs have been temporarily
> > + * offlined but the delayed allocator has already promised some bytes
> > + * and later the real extent/block allocation is failing due to
> > + * the rtgs(s) being offline.
> > + * If the overall shrink fails, we will restore the values.
>
> Formatting: use up all 80 characters.

Okay.
>
> > + xfs_rgnumber_t rgno = rtg_rgno(rtg);
> > +
> > + struct xfs_rtalloc_args args = {
>
> Weird empty line between the declarations.

Will fix this.
>
> > +bool xfs_rtgroup_is_empty(struct xfs_rtgroup *rtg);
> > +
> > +#define for_each_rgno_range_reverse(agno, old_rgcount, new_rgcount) \
> > + for ((agno) = ((old_rgcount) - 1); (typeof(old_rgcount))(agno) >= \
> > + ((typeof(old_rgcount))(new_rgcount) - 1); (agno)--)
>
> I don't think this is helpful vs just open coding the loop. The mix
> of ag and rg naming is also a bit odd.
>
> > + for_each_rgno_range_reverse(rgno, old_rgcount, new_rgcount + 1) {
> > + rtg = xfs_rtgroup_get(mp, rgno);
>
> Given that we have this pattern a bit, maybe add a reverse version
> of xfs_group_next_range to encapsulate it?

Okay - I will try to come up with something. Thank you for the suggestion.
>
>
> Highlevel note: this seems to only cover classic bitmap based RTGs,
> and not zoned ones. You might want to look into the latter because

Okay. So are you suggesting to add the shrink support only for the zoned devices or extend this
patch set to cover shrink for the zoned devices too?
> they're actually much simpler, and with the zoned GC code we actually
> have a super nice way to move data out of the RTG. I'd be happy to
> supple you a code sniplet for the latter.

Sure, that would be super useful. Also, since I don't have much experience in zoned devices, can you
please point to some relevant resources and recent zoned device related patchsets that can help me
get started with?
--NR