| Message ID | 20241022121355.261836-3-hch@lst.de (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | [1/2] xfs: streamline xfs_filestream_pick_ag | expand |
On Tue, Oct 22, 2024 at 02:13:38PM +0200, Christoph Hellwig wrote: > When the main loop in xfs_filestream_pick_ag fails to find a suitable > AG it tries to just pick the online AG. But the loop for that uses > args->pag as loop iterator while the later code expects pag to be > set. Fix this by reusing the max_pag case for this last resort, and > also add a check for impossible case of no AG just to make sure that > the uninitialized pag doesn't even escape in theory. > > Reported-by: syzbot+4125a3c514e3436a02e6@syzkaller.appspotmail.com > Signed-off-by: Christoph Hellwig <hch@lst.de> > Tested-by: syzbot+4125a3c514e3436a02e6@syzkaller.appspotmail.com Well, that bug was pretty subtle. :( Reviewed-by: Darrick J. Wong <djwong@kernel.org> Nit: should trace_xfs_filestream_pick() lose its third argument? --D > --- > fs/xfs/xfs_filestream.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c > index f523027cc32586..ecf8a0f6c1362e 100644 > --- a/fs/xfs/xfs_filestream.c > +++ b/fs/xfs/xfs_filestream.c > @@ -64,7 +64,7 @@ xfs_filestream_pick_ag( > struct xfs_perag *pag; > struct xfs_perag *max_pag = NULL; > xfs_extlen_t minlen = *longest; > - xfs_extlen_t free = 0, minfree, maxfree = 0; > + xfs_extlen_t minfree, maxfree = 0; > xfs_agnumber_t agno; > bool first_pass = true; > > @@ -113,7 +113,6 @@ xfs_filestream_pick_ag( > !(flags & XFS_PICK_USERDATA) || > (flags & XFS_PICK_LOWSPACE))) { > /* Break out, retaining the reference on the AG. */ > - free = pag->pagf_freeblks; > if (max_pag) > xfs_perag_rele(max_pag); > goto done; > @@ -149,18 +148,19 @@ xfs_filestream_pick_ag( > * filestream. It none suit, just use whatever AG we can grab. > */ > if (!max_pag) { > - for_each_perag_wrap(args->mp, 0, start_agno, args->pag) > + for_each_perag_wrap(args->mp, 0, start_agno, pag) { > + max_pag = pag; > break; > - atomic_inc(&args->pag->pagf_fstrms); > - *longest = 0; > - } else { > - pag = max_pag; > - free = maxfree; > - atomic_inc(&pag->pagf_fstrms); > + } > + /* Bail if there are no AGs at all to select from. */ > + if (!max_pag) > + return -ENOSPC; > } > > + pag = max_pag; > + atomic_inc(&pag->pagf_fstrms); > done: > - trace_xfs_filestream_pick(pag, pino, free); > + trace_xfs_filestream_pick(pag, pino, pag->pagf_freeblks); > args->pag = pag; > return 0; > > -- > 2.45.2 > >
On Tue, Oct 22, 2024 at 10:59:28AM -0700, Darrick J. Wong wrote: > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > > Nit: should trace_xfs_filestream_pick() lose its third argument? Yes.
diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c index f523027cc32586..ecf8a0f6c1362e 100644 --- a/fs/xfs/xfs_filestream.c +++ b/fs/xfs/xfs_filestream.c @@ -64,7 +64,7 @@ xfs_filestream_pick_ag( struct xfs_perag *pag; struct xfs_perag *max_pag = NULL; xfs_extlen_t minlen = *longest; - xfs_extlen_t free = 0, minfree, maxfree = 0; + xfs_extlen_t minfree, maxfree = 0; xfs_agnumber_t agno; bool first_pass = true; @@ -113,7 +113,6 @@ xfs_filestream_pick_ag( !(flags & XFS_PICK_USERDATA) || (flags & XFS_PICK_LOWSPACE))) { /* Break out, retaining the reference on the AG. */ - free = pag->pagf_freeblks; if (max_pag) xfs_perag_rele(max_pag); goto done; @@ -149,18 +148,19 @@ xfs_filestream_pick_ag( * filestream. It none suit, just use whatever AG we can grab. */ if (!max_pag) { - for_each_perag_wrap(args->mp, 0, start_agno, args->pag) + for_each_perag_wrap(args->mp, 0, start_agno, pag) { + max_pag = pag; break; - atomic_inc(&args->pag->pagf_fstrms); - *longest = 0; - } else { - pag = max_pag; - free = maxfree; - atomic_inc(&pag->pagf_fstrms); + } + /* Bail if there are no AGs at all to select from. */ + if (!max_pag) + return -ENOSPC; } + pag = max_pag; + atomic_inc(&pag->pagf_fstrms); done: - trace_xfs_filestream_pick(pag, pino, free); + trace_xfs_filestream_pick(pag, pino, pag->pagf_freeblks); args->pag = pag; return 0;