| Message ID | 157924228165.3029431.1835481566077971155.stgit@magnolia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: make buffer functions return error codes | expand |
On Thu, Jan 16, 2020 at 10:24:41PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Refactor xfs_read_agf and xfs_alloc_read_agf to return EAGAIN if the > caller passed TRYLOCK and we weren't able to get the lock; and change > the callers to recognize this. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/libxfs/xfs_alloc.c | 31 +++++++++++++++---------------- > fs/xfs/libxfs/xfs_bmap.c | 9 +++++---- > fs/xfs/xfs_filestream.c | 11 ++++++----- > 3 files changed, 26 insertions(+), 25 deletions(-) > > > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c > index 83273975df77..26f3e4db84e0 100644 > --- a/fs/xfs/libxfs/xfs_alloc.c > +++ b/fs/xfs/libxfs/xfs_alloc.c > @@ -2502,13 +2502,15 @@ xfs_alloc_fix_freelist( > > if (!pag->pagf_init) { > error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp); > - if (error) > - goto out_no_agbp; > - if (!pag->pagf_init) { > + if (error == -EAGAIN) { > + /* Couldn't lock the AGF so skip this AG. */ > ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK); > ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING)); > - goto out_agbp_relse; > + error = 0; > + goto out_no_agbp; > } > + if (error) > + goto out_no_agbp; I wonder if something like: if (error) { if (error == -EAGAIN) { /* Couldn't lock the AGF so skip this AG. */ ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK); ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING)); error = 0; } goto out_no_agbp; } would be a little nicer here? > @@ -2533,13 +2535,15 @@ xfs_alloc_fix_freelist( > */ > if (!agbp) { > error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp); > - if (error) > - goto out_no_agbp; > - if (!agbp) { > + if (error == -EAGAIN) { > + /* Couldn't lock the AGF so skip this AG. */ > ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK); > ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING)); > + error = 0; > goto out_no_agbp; > } > + if (error) > + goto out_no_agbp; > } Same here. Also shouldn't those asserts just move into xfs_alloc_read_agf or go away now that we have a proper return value and not the magic NULL buffer? > + error = xfs_alloc_read_agf(mp, tp, agno, flags, &bp); > + if (error) > return error; > + xfs_trans_brelse(tp, bp); > return 0; Maybe simplify this further to: error = xfs_alloc_read_agf(mp, tp, agno, flags, &bp); if (!error) xfs_trans_brelse(tp, bp); return error; > @@ -2958,12 +2962,9 @@ xfs_read_agf( > trace_xfs_read_agf(mp, agno); > > ASSERT(agno != NULLAGNUMBER); > - error = xfs_trans_read_buf( > - mp, tp, mp->m_ddev_targp, > + error = xfs_trans_read_buf(mp, tp, mp->m_ddev_targp, This hunk should probably go into the patch that changed the xfs_trans_read_buf return value instead. > + if (error == -EAGAIN) { > + /* Couldn't lock the AGF, so skip this AG. */ > *notinit = 1; > + error = 0; > goto out; > } > + if (error) > + goto out; Should probably be: if (error) { if (error == -EAGAIN) { /* Couldn't lock the AGF, so skip this AG. */ *notinit = 1; error = 0; } goto out;
On Thu, Jan 16, 2020 at 10:59:57PM -0800, Christoph Hellwig wrote: > On Thu, Jan 16, 2020 at 10:24:41PM -0800, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Refactor xfs_read_agf and xfs_alloc_read_agf to return EAGAIN if the > > caller passed TRYLOCK and we weren't able to get the lock; and change > > the callers to recognize this. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > fs/xfs/libxfs/xfs_alloc.c | 31 +++++++++++++++---------------- > > fs/xfs/libxfs/xfs_bmap.c | 9 +++++---- > > fs/xfs/xfs_filestream.c | 11 ++++++----- > > 3 files changed, 26 insertions(+), 25 deletions(-) > > > > > > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c > > index 83273975df77..26f3e4db84e0 100644 > > --- a/fs/xfs/libxfs/xfs_alloc.c > > +++ b/fs/xfs/libxfs/xfs_alloc.c > > @@ -2502,13 +2502,15 @@ xfs_alloc_fix_freelist( > > > > if (!pag->pagf_init) { > > error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp); > > - if (error) > > - goto out_no_agbp; > > - if (!pag->pagf_init) { > > + if (error == -EAGAIN) { > > + /* Couldn't lock the AGF so skip this AG. */ > > ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK); > > ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING)); > > - goto out_agbp_relse; > > + error = 0; > > + goto out_no_agbp; > > } > > + if (error) > > + goto out_no_agbp; > > I wonder if something like: > > if (error) { > if (error == -EAGAIN) { > /* Couldn't lock the AGF so skip this AG. */ > ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK); > ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING)); > error = 0; > } > goto out_no_agbp; > } > > would be a little nicer here? > > @@ -2533,13 +2535,15 @@ xfs_alloc_fix_freelist( > > */ > > if (!agbp) { > > error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp); > > - if (error) > > - goto out_no_agbp; > > - if (!agbp) { > > + if (error == -EAGAIN) { > > + /* Couldn't lock the AGF so skip this AG. */ > > ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK); > > ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING)); > > + error = 0; > > goto out_no_agbp; > > } > > + if (error) > > + goto out_no_agbp; > > } > > Same here. Also shouldn't those asserts just move into > xfs_alloc_read_agf or go away now that we have a proper return value > and not the magic NULL buffer? I'll move the assert into xfs_alloc_read_agf, so these all turn into: if (error) { if (error == -EAGAIN) error = 0; goto next ag; } > > > + error = xfs_alloc_read_agf(mp, tp, agno, flags, &bp); > > + if (error) > > return error; > > + xfs_trans_brelse(tp, bp); > > return 0; > > Maybe simplify this further to: > > error = xfs_alloc_read_agf(mp, tp, agno, flags, &bp); > if (!error) > xfs_trans_brelse(tp, bp); > return error; Ok. > > @@ -2958,12 +2962,9 @@ xfs_read_agf( > > trace_xfs_read_agf(mp, agno); > > > > ASSERT(agno != NULLAGNUMBER); > > - error = xfs_trans_read_buf( > > - mp, tp, mp->m_ddev_targp, > > + error = xfs_trans_read_buf(mp, tp, mp->m_ddev_targp, > > This hunk should probably go into the patch that changed the > xfs_trans_read_buf return value instead. Ok. > > + if (error == -EAGAIN) { > > + /* Couldn't lock the AGF, so skip this AG. */ > > *notinit = 1; > > + error = 0; > > goto out; > > } > > + if (error) > > + goto out; > > Should probably be: > > if (error) { > if (error == -EAGAIN) { > /* Couldn't lock the AGF, so skip this AG. */ > *notinit = 1; > error = 0; > } > goto out; Fixed. --D
diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c index 83273975df77..26f3e4db84e0 100644 --- a/fs/xfs/libxfs/xfs_alloc.c +++ b/fs/xfs/libxfs/xfs_alloc.c @@ -2502,13 +2502,15 @@ xfs_alloc_fix_freelist( if (!pag->pagf_init) { error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp); - if (error) - goto out_no_agbp; - if (!pag->pagf_init) { + if (error == -EAGAIN) { + /* Couldn't lock the AGF so skip this AG. */ ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK); ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING)); - goto out_agbp_relse; + error = 0; + goto out_no_agbp; } + if (error) + goto out_no_agbp; } /* @@ -2533,13 +2535,15 @@ xfs_alloc_fix_freelist( */ if (!agbp) { error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp); - if (error) - goto out_no_agbp; - if (!agbp) { + if (error == -EAGAIN) { + /* Couldn't lock the AGF so skip this AG. */ ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK); ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING)); + error = 0; goto out_no_agbp; } + if (error) + goto out_no_agbp; } /* reset a padding mismatched agfl before final free space check */ @@ -2768,10 +2772,10 @@ xfs_alloc_pagf_init( xfs_buf_t *bp; int error; - if ((error = xfs_alloc_read_agf(mp, tp, agno, flags, &bp))) + error = xfs_alloc_read_agf(mp, tp, agno, flags, &bp); + if (error) return error; - if (bp) - xfs_trans_brelse(tp, bp); + xfs_trans_brelse(tp, bp); return 0; } @@ -2958,12 +2962,9 @@ xfs_read_agf( trace_xfs_read_agf(mp, agno); ASSERT(agno != NULLAGNUMBER); - error = xfs_trans_read_buf( - mp, tp, mp->m_ddev_targp, + error = xfs_trans_read_buf(mp, tp, mp->m_ddev_targp, XFS_AG_DADDR(mp, agno, XFS_AGF_DADDR(mp)), XFS_FSS_TO_BB(mp, 1), flags, bpp, &xfs_agf_buf_ops); - if (error == -EAGAIN) - return 0; if (error) return error; @@ -2995,8 +2996,6 @@ xfs_alloc_read_agf( bpp); if (error) return error; - if (!*bpp) - return 0; ASSERT(!(*bpp)->b_error); agf = XFS_BUF_TO_AGF(*bpp); diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index cfcef076c72f..10b7284cac35 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -3311,13 +3311,14 @@ xfs_bmap_longest_free_extent( pag = xfs_perag_get(mp, ag); if (!pag->pagf_init) { error = xfs_alloc_pagf_init(mp, tp, ag, XFS_ALLOC_FLAG_TRYLOCK); - if (error) - goto out; - - if (!pag->pagf_init) { + if (error == -EAGAIN) { + /* Couldn't lock the AGF, so skip this AG. */ *notinit = 1; + error = 0; goto out; } + if (error) + goto out; } longest = xfs_alloc_longest_free_extent(pag, diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c index 5f12b5d8527a..3ccdab463359 100644 --- a/fs/xfs/xfs_filestream.c +++ b/fs/xfs/xfs_filestream.c @@ -159,16 +159,17 @@ xfs_filestream_pick_ag( if (!pag->pagf_init) { err = xfs_alloc_pagf_init(mp, NULL, ag, trylock); - if (err && !trylock) { + if (err == -EAGAIN) { + /* Couldn't lock the AGF, skip this AG. */ + xfs_perag_put(pag); + continue; + } + if (err) { xfs_perag_put(pag); return err; } } - /* Might fail sometimes during the 1st pass with trylock set. */ - if (!pag->pagf_init) - goto next_ag; - /* Keep track of the AG with the most free blocks. */ if (pag->pagf_freeblks > maxfree) { maxfree = pag->pagf_freeblks;