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@mail.gmail.com/T/#m5704d0409bec1ce5273be0d3860e8ad60e9886fd
Signed-off-by: Ryosuke Yasuoka <[email protected]>
---
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)
break;
atomic_inc(&args->pag->pagf_fstrms);
*longest = 0;
--
2.39.2
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@mail.gmail.com/T/#m5704d0409bec1ce5273be0d3860e8ad60e9886fd
>
> Signed-off-by: Ryosuke Yasuoka <[email protected]>
> ---
> 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);
...
}
--D
> break;
> atomic_inc(&args->pag->pagf_fstrms);
> *longest = 0;
> --
> 2.39.2
>
On Mon, Apr 10, 2023 at 09:30:29AM -0700, Darrick J. Wong 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@mail.gmail.com/T/#m5704d0409bec1ce5273be0d3860e8ad60e9886fd
> >
> > Signed-off-by: Ryosuke Yasuoka <[email protected]>
> > ---
> > 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(
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
> > * 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);
> ...
> }
>
> --D
>
> > break;
> > atomic_inc(&args->pag->pagf_fstrms);
> > *longest = 0;
> > --
> > 2.39.2
> >
Darrick
Thank you for reviewing.
On Tue, Apr 11, 2023 at 1:30 AM Darrick J. Wong <[email protected]> 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@mail.gmail.com/T/#m5704d0409bec1ce5273be0d3860e8ad60e9886fd
> >
> > Signed-off-by: Ryosuke Yasuoka <[email protected]>
> > ---
> > 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 <[email protected]> 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