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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHpthZrcegPXhti5tDdb=_nwafWnU-FXmtc6aRU7juowMpOnUQ@mail.gmail.com>
Date:   Fri, 7 Apr 2023 01:03:26 +0900
From:   Ryosuke Yasuoka <ryasuoka@...hat.com>
To:     Eric Sandeen <sandeen@...deen.net>
Cc:     djwong@...nel.org, linux-xfs@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] xfs: Use for_each_perag() to iterate all available AGs

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