Re: [PATCH] xfs: Use for_each_perag() to iterate all available AGs

[Ryosuke Yasuoka]

Eric,

I failed to reply to you since I got some mistakes.
Let me re-send my reply just in case.

Thank you for reviewing my requests.

> Can you explain what goes wrong if it is zero? Is there a test for this?
>
> If it's a general problem, what if the other 2 callers pass in the variable
> start_agno with a value of 0?

Sorry I couldn't prepare any tests to confirm what happens if it is zero
because it is a kind of general problem.

IIUC, passing zero to for_each_perag_wrap() is not problematic.

As the comment describes, this macro iterates all AG from start_agno through
wrap_agno, then wrap to restart_agno, and then iterates again toward to
start_agno - 1. It looks like some issue occurs when start_agno is zero.
However, for_each_perag_wrap() actually won't wrap if start_agno is zero.

static inline struct xfs_perag *
xfs_perag_next_wrap(
struct xfs_perag *pag,
xfs_agnumber_t *agno,
xfs_agnumber_t stop_agno,
xfs_agnumber_t restart_agno,
xfs_agnumber_t wrap_agno)
{
struct xfs_mount *mp = pag->pag_mount;

*agno = pag->pag_agno + 1;
xfs_perag_rele(pag);
while (*agno != stop_agno) {
if (*agno >= wrap_agno) {
if (restart_agno >= stop_agno) <<<--- HERE
break;
*agno = restart_agno;
}

pag = xfs_perag_grab(mp, *agno);
if (pag)
return pag;
(*agno)++;
}
return NULL;
}

/*
* Iterate all AGs from start_agno through wrap_agno, then restart_agno through
* (start_agno - 1).
*/
#define for_each_perag_wrap_range(mp, start_agno, restart_agno,
wrap_agno, agno, pag) \
for ((agno) = (start_agno), (pag) = xfs_perag_grab((mp), (agno)); \
(pag) != NULL; \
(pag) = xfs_perag_next_wrap((pag), &(agno), (start_agno), \
(restart_agno), (wrap_agno)))
...
#define for_each_perag_wrap_at(mp, start_agno, wrap_agno, agno, pag) \
for_each_perag_wrap_range((mp), (start_agno), 0, (wrap_agno), (agno), (pag))
...
#define for_each_perag_wrap(mp, start_agno, agno, pag) \
for_each_perag_wrap_at((mp), (start_agno), (mp)->m_sb.sb_agcount, \
(agno), (pag))

OTOH, since we have already a for_each_perag() macro, which just iterates all AG
from 0 and doesn't wrap, I think it is simpler to use for_earch_perag().

Regards,
Ryosuke