diff mbox series

[10/12] xfs: make xfs_*read_agf return EAGAIN to ALLOC_FLAG_TRYLOCK callers

Message ID 157976537480.2388944.15713995061702153624.stgit@magnolia (mailing list archive)
State Accepted
Headers show
Series xfs: make buffer functions return error codes | expand

Commit Message

Darrick J. Wong Jan. 23, 2020, 7:42 a.m. UTC 
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 |   34 +++++++++++++++-------------------
 fs/xfs/libxfs/xfs_bmap.c  |   11 ++++++-----
 fs/xfs/xfs_filestream.c   |   11 ++++++-----
 3 files changed, 27 insertions(+), 29 deletions(-)

Comments

Dave Chinner Jan. 24, 2020, 2 a.m. UTC | #1 
On Wed, Jan 22, 2020 at 11:42:54PM -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 |   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
Darrick J. Wong Jan. 24, 2020, 4:47 a.m. UTC | #2 
On Fri, Jan 24, 2020 at 01:00:54PM +1100, Dave Chinner wrote:
> On Wed, Jan 22, 2020 at 11:42:54PM -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 |   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));
> 	....

Yeah.

> > -	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.

Cool.  Will fix.

--D

> 
> -Dave
> -- 
> Dave Chinner
> david@fromorbit.com
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 7be6c8fbfcf9..ed0172850400 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -2502,12 +2502,11 @@  xfs_alloc_fix_freelist(
 
 	if (!pag->pagf_init) {
 		error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp);
-		if (error)
+		if (error) {
+			/* Couldn't lock the AGF so skip this AG. */
+			if (error == -EAGAIN)
+				error = 0;
 			goto out_no_agbp;
-		if (!pag->pagf_init) {
-			ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK);
-			ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
-			goto out_agbp_relse;
 		}
 	}
 
@@ -2533,11 +2532,10 @@  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) {
-			ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK);
-			ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
+		if (error) {
+			/* Couldn't lock the AGF so skip this AG. */
+			if (error == -EAGAIN)
+				error = 0;
 			goto out_no_agbp;
 		}
 	}
@@ -2768,11 +2766,10 @@  xfs_alloc_pagf_init(
 	xfs_buf_t		*bp;
 	int			error;
 
-	if ((error = xfs_alloc_read_agf(mp, tp, agno, flags, &bp)))
-		return error;
-	if (bp)
+	error = xfs_alloc_read_agf(mp, tp, agno, flags, &bp);
+	if (!error)
 		xfs_trans_brelse(tp, bp);
-	return 0;
+	return error;
 }
 
 /*
@@ -2961,8 +2958,6 @@  xfs_read_agf(
 	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;
 
@@ -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;
-	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 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;