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: <20230404084701.2791683-1-ryasuoka@redhat.com> <20230405010403.GO3223426@dread.disaster.area>
 <CAHpthZoWRWS2bXFDQrB+iOz7AA_ZLGJKmytHjN582VaWQ_TRwg@mail.gmail.com> <20230405230415.GT3223426@dread.disaster.area>
In-Reply-To: <20230405230415.GT3223426@dread.disaster.area>
From:   Ryosuke Yasuoka <ryasuoka@redhat.com>
Date:   Fri, 7 Apr 2023 18:06:57 +0900
Message-ID: <CAHpthZrvhqh8O1HO7U_jVnaq9R9Ur=Yq2eWzjWfNx3ryDbnGPA@mail.gmail.com>
Subject: Re: [PATCH] xfs: Use for_each_perag() to iterate all available AGs
To:     Dave Chinner <david@fromorbit.com>
Cc:     djwong@kernel.org, linux-xfs@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Dave,

> On Thu, Apr 6, 2023 at 8:04=E2=80=AFAM Dave Chinner <david@fromorbit.com>=
 wrote:
> >
> > On Wed, Apr 05, 2023 at 05:04:14PM +0900, Ryosuke Yasuoka wrote:
> > > Dave,
> > >
> > > Thank you for reviewing my requests.
> > >
> > > > > for_each_perag_wrap() doesn't expect 0 as 2nd arg.
> > > > > To iterate all the available AGs, just use for_each_perag() inste=
ad.
> > > >
> > > > Thanks, Ryosuke-san. IIUC, this is a fix for the recent sysbot
> > > > reported filestreams oops regression?
> > > >
> > > > Can you include the context of the failure it reported (i.e. the
> > > > trace from the oops), and the 'reported-by' tag for the syzbot
> > > > report?
> > > >
> > > > It should probably also include a 'Fixes: bd4f5d09cc93 ("xfs:
> > > > refactor the filestreams allocator pick functions")' tag as well.
> > >
> > > No. my request is in the same code area where syzbot bug was reported=
,
> > > but it might not be relevant. A kernel applying my patch got the same=
 Oops.
> > >
> > > I'm indeed checking the syzbot's bug and I realized that this small b=
ug fix
> > > is not related to it based on my tests. Thus I sent the patch
> > > as a separate one.
> > >
> > > > While this will definitely avoid the oops, I don't think it is quit=
e
> > > > right. If we want to iterate all AGs, then we should be starting th=
e
> > > > iteration at AG 0, not start_agno. i.e.
> > > >
> > > > +                       for_each_perag(args->mp, 0, args->pag)
> > >
> > > I agree with your proposal because it is more direct.
> > > However, as the current for_each_perag() macro always assigns 0 to (a=
gno),
> > > it will cause compilation errors.
> >
> > Yup, I didn't compile test my suggestion - i just quickly wrote it
> > down to demonstrate what I was thinking. I expect that you have
> > understood that using for_each_perag() was what I was suggesting is
> > used, not that the sample code I wrote is exactly correct. IOWs,
> >
> >                 for_each_perag(args->mp, start_agno, args->pag)
> >
> > would have worked, even though the code does not do what it looks
> > like it should from the context of start_agno. Which means this
> > would be better:
> >
> >                 start_agno =3D 0;
> >                 for_each_perag_from(args->mp, start_agno, args->pag)
> >
> > because it directly documents the value we are iterating from.

OK. I'll update my patch, run a compile test, and then send again as a
v2 another thread


Thank you for reviewing.
Ryosuke