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

[Ryosuke Yasuoka]

Dave,

> On Thu, Apr 6, 2023 at 8:04 AM Dave Chinner <david@xxxxxxxxxxxxx> 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() instead.
> > > >
> > > > 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 bug 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 quite
> > > > right. If we want to iterate all AGs, then we should be starting the
> > > > 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 (agno),
> > > 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 = 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