| Message ID | 20230404084701.2791683-1-ryasuoka@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: Use for_each_perag() to iterate all available AGs | expand |
On 4/4/23 3:47 AM, Ryosuke Yasuoka wrote: > for_each_perag_wrap() doesn't expect 0 as 2nd arg. > To iterate all the available AGs, just use for_each_perag() instead. Can you explain what goes wrong if it is zero? Is there a test for this? If it's a general problem, what if the other 2 callers pass in the variable start_agno with a value of 0? (I may well be missing something obvious as those macros are a bit dense) Thanks, -Eric > Signed-off-by: Ryosuke Yasuoka <ryasuoka@redhat.com> > --- > fs/xfs/xfs_filestream.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c > index 22c13933c8f8..48f43c340c58 100644 > --- a/fs/xfs/xfs_filestream.c > +++ b/fs/xfs/xfs_filestream.c > @@ -151,7 +151,7 @@ xfs_filestream_pick_ag( > * grab. > */ > if (!max_pag) { > - for_each_perag_wrap(args->mp, 0, start_agno, args->pag) > + for_each_perag(args->mp, start_agno, args->pag) > break; > atomic_inc(&args->pag->pagf_fstrms); > *longest = 0;
On Tue, Apr 04, 2023 at 05:47:01PM +0900, Ryosuke Yasuoka wrote: > 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. > Signed-off-by: Ryosuke Yasuoka <ryasuoka@redhat.com> > --- > fs/xfs/xfs_filestream.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c > index 22c13933c8f8..48f43c340c58 100644 > --- a/fs/xfs/xfs_filestream.c > +++ b/fs/xfs/xfs_filestream.c > @@ -151,7 +151,7 @@ xfs_filestream_pick_ag( > * grab. > */ > if (!max_pag) { > - for_each_perag_wrap(args->mp, 0, start_agno, args->pag) > + for_each_perag(args->mp, start_agno, args->pag) > break; 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) Cheers, Dave.
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.
Eric, I failed to reply to you since I got some mistakes. Let me re-send my reply just in case. Thank you for reviewing my requests. > Can you explain what goes wrong if it is zero? Is there a test for this? > > If it's a general problem, what if the other 2 callers pass in the variable > start_agno with a value of 0? Sorry I couldn't prepare any tests to confirm what happens if it is zero because it is a kind of general problem. IIUC, passing zero to for_each_perag_wrap() is not problematic. As the comment describes, this macro iterates all AG from start_agno through wrap_agno, then wrap to restart_agno, and then iterates again toward to start_agno - 1. It looks like some issue occurs when start_agno is zero. However, for_each_perag_wrap() actually won't wrap if start_agno is zero. static inline struct xfs_perag * xfs_perag_next_wrap( struct xfs_perag *pag, xfs_agnumber_t *agno, xfs_agnumber_t stop_agno, xfs_agnumber_t restart_agno, xfs_agnumber_t wrap_agno) { struct xfs_mount *mp = pag->pag_mount; *agno = pag->pag_agno + 1; xfs_perag_rele(pag); while (*agno != stop_agno) { if (*agno >= wrap_agno) { if (restart_agno >= stop_agno) <<<--- HERE break; *agno = restart_agno; } pag = xfs_perag_grab(mp, *agno); if (pag) return pag; (*agno)++; } return NULL; } /* * Iterate all AGs from start_agno through wrap_agno, then restart_agno through * (start_agno - 1). */ #define for_each_perag_wrap_range(mp, start_agno, restart_agno, wrap_agno, agno, pag) \ for ((agno) = (start_agno), (pag) = xfs_perag_grab((mp), (agno)); \ (pag) != NULL; \ (pag) = xfs_perag_next_wrap((pag), &(agno), (start_agno), \ (restart_agno), (wrap_agno))) ... #define for_each_perag_wrap_at(mp, start_agno, wrap_agno, agno, pag) \ for_each_perag_wrap_range((mp), (start_agno), 0, (wrap_agno), (agno), (pag)) ... #define for_each_perag_wrap(mp, start_agno, agno, pag) \ for_each_perag_wrap_at((mp), (start_agno), (mp)->m_sb.sb_agcount, \ (agno), (pag)) OTOH, since we have already a for_each_perag() macro, which just iterates all AG from 0 and doesn't wrap, I think it is simpler to use for_earch_perag(). Regards, Ryosuke
Dave, > On Thu, Apr 6, 2023 at 8:04 AM 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() 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
On 4/6/23 11:03 AM, Ryosuke Yasuoka wrote: > Eric, > > I failed to reply to you since I got some mistakes. > Let me re-send my reply just in case. > > Thank you for reviewing my requests. > >> Can you explain what goes wrong if it is zero? Is there a test for this? >> >> If it's a general problem, what if the other 2 callers pass in the variable >> start_agno with a value of 0? > Sorry I couldn't prepare any tests to confirm what happens if it is zero > because it is a kind of general problem. > > IIUC, passing zero to for_each_perag_wrap() is not problematic. ... > OTOH, since we have already a for_each_perag() macro, which just iterates all AG > from 0 and doesn't wrap, I think it is simpler to use for_earch_perag(). > > Regards, > Ryosuke Ok - I couldn't tell from the original email if this was a bugfix or a cleanup, and wanted to be sure. Thanks! -Eric
diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c index 22c13933c8f8..48f43c340c58 100644 --- a/fs/xfs/xfs_filestream.c +++ b/fs/xfs/xfs_filestream.c @@ -151,7 +151,7 @@ xfs_filestream_pick_ag( * grab. */ if (!max_pag) { - for_each_perag_wrap(args->mp, 0, start_agno, args->pag) + for_each_perag(args->mp, start_agno, args->pag) break; atomic_inc(&args->pag->pagf_fstrms); *longest = 0;
for_each_perag_wrap() doesn't expect 0 as 2nd arg. To iterate all the available AGs, just use for_each_perag() instead. Signed-off-by: Ryosuke Yasuoka <ryasuoka@redhat.com> --- fs/xfs/xfs_filestream.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)