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

[Dave Chinner <david@fromorbit.com>]

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.

> Although I haven't checked other callers deeply, we should modify
> the macro as follows:
> 
>  #define for_each_perag(mp, agno, pag) \
> -   (agno) = 0; \
>   for_each_perag_from((mp), (agno), (pag))

That is not correct, either. agno needs to be a variable - it is
the loop agno counter that tracks the iteration.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com