On Wed, Jan 22, 2020 at 11:42:54PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_alloc.c | 34 +++++++++++++++-------------------
> fs/xfs/libxfs/xfs_bmap.c | 11 ++++++-----
> fs/xfs/xfs_filestream.c | 11 ++++++-----
> 3 files changed, 27 insertions(+), 29 deletions(-)
.....
> @@ -2992,10 +2987,11 @@ xfs_alloc_read_agf(
> error = xfs_read_agf(mp, tp, agno,
> (flags & XFS_ALLOC_FLAG_TRYLOCK) ? XBF_TRYLOCK : 0,
> bpp);
> - if (error)
> + if (error) {
> + /* We don't support trylock when freeing. */
> + ASSERT(error != -EAGAIN || !(flags & XFS_ALLOC_FLAG_FREEING));
> return error;
Shouldn't we check this with asserts before we call xfs_read_agf()?
i.e.
/* We don't support trylock when freeing. */
ASSERT((flags & (XFS_ALLOC_FLAG_FREEING | XFS_ALLOC_FLAG_TRYLOCK)) !=
(XFS_ALLOC_FLAG_FREEING | XFS_ALLOC_FLAG_TRYLOCK));
....
> - 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..9a6d7a84689a 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3311,11 +3311,12 @@ 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) {
> - *notinit = 1;
> + if (error) {
> + /* Couldn't lock the AGF, so skip this AG. */
> + if (error == -EAGAIN) {
> + *notinit = 1;
> + error = 0;
> + }
> goto out;
> }
> }
> 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 neater to do:
if (!pag->pagf_init) {
err = xfs_alloc_pagf_init(mp, NULL, ag, trylock);
if (err) {
xfs_perag_put(pag);
if (err != -EAGAIN)
return err;
/* Couldn't lock the AGF, skip this AG. */
continue;
}
}
Otherwise it all looks ok.
-Dave
--
Dave Chinner
david@xxxxxxxxxxxxx
