Darrick Thank you for reviewing. On Tue, Apr 11, 2023 at 1:30 AM Darrick J. Wong <djwong@xxxxxxxxxx> wrote: > > On Tue, Apr 11, 2023 at 01:07:27AM +0900, Ryosuke Yasuoka wrote: > > xfs_filestream_pick_ag() iterates all the available AGs when no > > unassociated AGs are available by using for_each_perag_wrap(). > > To iterate all the available AGs, just use for_each_perag_from() instead. > > > > > > This patch cleans up a code where xfs_filestream_pick_ag() iterates > > all the available AGs when no unassociated AGs are available. > > Current implementation is using a for_each_perag_wrap() macro which > > iterates all AGs from start_agno through wrap_agno, wraps to > > restart_agno, and then iterates again toward to (start_agno - 1). > > In this case, xfs_filestream_pick_ag() start to iterate from 0 and > > does't need to wrap. Although passing 0 as start_agno to > > for_each_perag_wrap() > > is not problematic, we have already a for_each_perag() macro family > > which just iterates all AGs from 0 and doesn't wrap. Hense, I propose > > to use for_each_perag() family simply. > > > > > > Changes since v1 [1]: > > Use for_each_perag_from() instead of for_each_perag() to clarify > > where we are iterating from. > > > > [1]: > > https://lore.kernel.org/linux-xfs/CAHpthZrvhqh8O1HO7U_jVnaq9R9Ur=Yq2eWzjWfNx3ryDbnGPA@xxxxxxxxxxxxxx/T/#m5704d0409bec1ce5273be0d3860e8ad60e9886fd > > > > Signed-off-by: Ryosuke Yasuoka <ryasuoka@xxxxxxxxxx> > > --- > > fs/xfs/xfs_filestream.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c > > index 22c13933c8f8..29acd9f7d422 100644 > > --- a/fs/xfs/xfs_filestream.c > > +++ b/fs/xfs/xfs_filestream.c > > @@ -151,7 +151,8 @@ xfs_filestream_pick_ag( > > * grab. > > */ > > if (!max_pag) { > > - for_each_perag_wrap(args->mp, 0, start_agno, args->pag) > > + start_agno = 0; > > + for_each_perag_from(args->mp, start_agno, args->pag) > > IDGI. for_each_perag initializes the loop variable and calls > for_each_perag_from, so this is open-coding an existing macro. > > If people are confused by the reuse of the function call parameter > variable for the second loop, then either declare a new variable and let > the compiler notice that we never use start_agno ever again and reuse > a cpu register: > > if (!max_pag) { > xfs_agnumber_t agno; > > for_each_perag(args->mp, agno, args->pag) > break; > ... > } > > Or reuse it explicitly and leave a comment: > > if (!max_pag) { > /* > * Use any AG that we can grab. start_agno is no longer > * pertinent here so we can reuse the variable. > */ > for_each_perag(args->mp, start_agno, args->pag) > break; > ... > } > > As a third alternative, I suppose you could encapsulate all of that into > a dorky helper since I bet this isn't the first or the last time we're > going to need something like this: > > static inline struct xfs_perag * > xfs_perag_get_first_avail( > struct xfs_mount *mp) > { > struct xfs_perag *pag; > xfs_agnumber_t agno; > > for_each_perag(mp, agno, pag) > return pag; > > ASSERT(0); > return NULL; > } > > if (!max_pag) { > args->pag = xfs_perag_get_first_avail(mp); > ... > } OK. I update my patch with your third alternative. I'll add xfs_perag_get_first_avail() in xfs_filestream.h. It is a great idea because it can detect with ASSERT(0) if for_each_perag() gets no pag structure. The following syzbot bug [2] is an indeed pattern of this. [2]: https://syzkaller.appspot.com/bug?id=f7682cf37b02ddf3c87d88b80f74024cf330017b On Tue, Apr 11, 2023 at 2:59 AM Darrick J. Wong <djwong@xxxxxxxxxx> wrote: > > Oh, also -- I ran the whole codebase through smatch this morning. > Could you please set @err to zero in its declaration above? If the > first for_each_perag_wrap never manages to get any perag structures > (currently impossible with the codebase) then err will be undefined and > probably nonzero. > > CHECK fs/xfs/xfs_filestream.c > fs/xfs/xfs_filestream.c:120 > xfs_filestream_pick_ag() error: uninitialized symbol 'err'. > > --D OK. I will also update it like this. Is my understanding correct? @@ -67,7 +67,7 @@ xfs_filestream_pick_ag( xfs_extlen_t free = 0, minfree, maxfree = 0; xfs_agnumber_t agno; bool first_pass = true; - int err; + int err = 0; Best regards, Ryosuke
