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;
Date:   Thu, 6 Apr 2023 09:04:15 +1000
From:   Dave Chinner <david@fromorbit.com>
To:     Ryosuke Yasuoka <ryasuoka@redhat.com>
Cc:     djwong@kernel.org, linux-xfs@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] xfs: Use for_each_perag() to iterate all available AGs
Message-ID: <20230405230415.GT3223426@dread.disaster.area>
References: <20230404084701.2791683-1-ryasuoka@redhat.com>
 <20230405010403.GO3223426@dread.disaster.area>
 <CAHpthZoWRWS2bXFDQrB+iOz7AA_ZLGJKmytHjN582VaWQ_TRwg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHpthZoWRWS2bXFDQrB+iOz7AA_ZLGJKmytHjN582VaWQ_TRwg@mail.gmail.com>
Precedence: bulk

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