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

[Christoph Hellwig <hch@infradead.org>]

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.

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

> +void

Stick to the unique XFS style here and in other places, please.

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

> +{
> +	ASSERT(rtg);
> +
> +	int			error = 0;

No need for the assert, and code goes after declarations.

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

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

> +	xfs_rgnumber_t          rgno = rtg_rgno(rtg);
> +
> +	struct xfs_rtalloc_args args = {

Weird empty line between the declarations.

> +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?


Highlevel note:  this seems to only cover classic bitmap based RTGs,
and not zoned ones.  You might want to look into the latter because
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.