Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230410160727.3748239-1-ryasuoka@redhat.com> <20230410163029.GC360889@frogsfrogsfrogs>
 <20230410175946.GD360889@frogsfrogsfrogs>
In-Reply-To: <20230410175946.GD360889@frogsfrogsfrogs>
From:   Ryosuke Yasuoka <ryasuoka@redhat.com>
Date:   Wed, 12 Apr 2023 14:20:48 +0900
Message-ID: <CAHpthZp+h-4nEGhNhBvSXxv6LGFUX=H646U6tHEF5_kwM3ij6Q@mail.gmail.com>
Subject: Re: [PATCH v2] xfs: Use for_each_perag_from() to iterate all
 available AGs
To:     "Darrick J. Wong" <djwong@kernel.org>
Cc:     linux-xfs@vger.kernel.org, linux-kernel@vger.kernel.org,
        sandeen@sandeen.net, david@fromorbit.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Darrick

Thank you for reviewing.

On Tue, Apr 11, 2023 at 1:30=E2=80=AFAM Darrick J. Wong <djwong@kernel.org>=
 wrote:
>
> On Tue, Apr 11, 2023 at 01:07:27AM +0900, Ryosuke Yasuoka wrote:
> > xfs_filestream_pick_ag() iterates all the available AGs when no
> > unassociated AGs are available by using for_each_perag_wrap().
> > To iterate all the available AGs, just use for_each_perag_from() instea=
d.
> >
> >
> > This patch cleans up a code where xfs_filestream_pick_ag() iterates
> > all the available AGs when no unassociated AGs are available.
> > Current implementation is using a for_each_perag_wrap() macro which
> > iterates all AGs from start_agno through wrap_agno, wraps to
> > restart_agno, and then iterates again toward to (start_agno - 1).
> > In this case, xfs_filestream_pick_ag() start to iterate from 0 and
> > does't need to wrap. Although passing 0 as start_agno to
> > for_each_perag_wrap()
> > is not problematic, we have already a for_each_perag() macro family
> > which just iterates all AGs from 0 and doesn't wrap. Hense, I propose
> > to use for_each_perag() family simply.
> >
> >
> > Changes since v1 [1]:
> > Use for_each_perag_from() instead of for_each_perag() to clarify
> > where we are iterating from.
> >
> > [1]:
> > https://lore.kernel.org/linux-xfs/CAHpthZrvhqh8O1HO7U_jVnaq9R9Ur=3DYq2e=
WzjWfNx3ryDbnGPA@mail.gmail.com/T/#m5704d0409bec1ce5273be0d3860e8ad60e9886f=
d
> >
> > Signed-off-by: Ryosuke Yasuoka <ryasuoka@redhat.com>
> > ---
> >  fs/xfs/xfs_filestream.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c
> > index 22c13933c8f8..29acd9f7d422 100644
> > --- a/fs/xfs/xfs_filestream.c
> > +++ b/fs/xfs/xfs_filestream.c
> > @@ -151,7 +151,8 @@ xfs_filestream_pick_ag(
> >                * grab.
> >                */
> >               if (!max_pag) {
> > -                     for_each_perag_wrap(args->mp, 0, start_agno, args=
->pag)
> > +                     start_agno =3D 0;
> > +                     for_each_perag_from(args->mp, start_agno, args->p=
ag)
>
> IDGI.  for_each_perag initializes the loop variable and calls
> for_each_perag_from, so this is open-coding an existing macro.
>
> If people are confused by the reuse of the function call parameter
> variable for the second loop, then either declare a new variable and let
> the compiler notice that we never use start_agno ever again and reuse
> a cpu register:
>
>         if (!max_pag) {
>                 xfs_agnumber_t  agno;
>
>                 for_each_perag(args->mp, agno, args->pag)
>                         break;
>                 ...
>         }
>
> Or reuse it explicitly and leave a comment:
>
>         if (!max_pag) {
>                 /*
>                  * Use any AG that we can grab.  start_agno is no longer
>                  * pertinent here so we can reuse the variable.
>                  */
>                 for_each_perag(args->mp, start_agno, args->pag)
>                         break;
>                 ...
>         }
>
> As a third alternative, I suppose you could encapsulate all of that into
> a dorky helper since I bet this isn't the first or the last time we're
> going to need something like this:
>
> static inline struct xfs_perag *
> xfs_perag_get_first_avail(
>         struct xfs_mount        *mp)
> {
>         struct xfs_perag        *pag;
>         xfs_agnumber_t          agno;
>
>         for_each_perag(mp, agno, pag)
>                 return pag;
>
>         ASSERT(0);
>         return NULL;
> }
>
>         if (!max_pag) {
>                 args->pag =3D xfs_perag_get_first_avail(mp);
>                 ...
>         }

OK. I update my patch with your third alternative. I'll add
xfs_perag_get_first_avail() in xfs_filestream.h.

It is a great idea because it can detect with ASSERT(0) if
for_each_perag() gets no pag structure.

The following syzbot bug [2] is an indeed pattern of this.

[2]: https://syzkaller.appspot.com/bug?id=3Df7682cf37b02ddf3c87d88b80f74024=
cf330017b

On Tue, Apr 11, 2023 at 2:59=E2=80=AFAM Darrick J. Wong <djwong@kernel.org>=
 wrote:
>
> Oh, also -- I ran the whole codebase through smatch this morning.
> Could you please set @err to zero in its declaration above?  If the
> first for_each_perag_wrap never manages to get any perag structures
> (currently impossible with the codebase) then err will be undefined and
> probably nonzero.
>
>   CHECK  fs/xfs/xfs_filestream.c
> fs/xfs/xfs_filestream.c:120
> xfs_filestream_pick_ag() error: uninitialized symbol 'err'.
>
> --D

OK. I will also update it like this. Is my understanding correct?

@@ -67,7 +67,7 @@ xfs_filestream_pick_ag(
xfs_extlen_t free =3D 0, minfree, maxfree =3D 0;
xfs_agnumber_t agno;
bool first_pass =3D true;
- int err;
+ int err =3D 0;

Best regards,
Ryosuke