minleft fixes V3

minleft fixes V3

[Christoph Hellwig]

his is my attempt to fix the problems with rmap and reflink file system
running out of space during delayed extent conversions.  It turns out
the way minleft has been handled has always been bogus, but the rmap
enablement made it even worse.

Changes since V2:
 - dropped "xfs: fix the alignment fallback in xfs_bmap_btalloc"
 - added a maxlen >= minlen assert
 - a few commit log improvements

Changes since V1:
 - new patch to fix the noalign case in xfs_bmap_btalloc
 - new patch to fix xfs_alloc_set_aside
 - dropped the patch to increase the minleft value for bmap allocations,
   we only need the current minimal values as we only allocate one
   extent per call
 - minor style fixes per review of the last round

[PATCH 1/4] xfs: bump up reserved blocks in xfs_alloc_set_aside

[Christoph Hellwig]

Setting aside 4 blocks globally for bmbt splits isn't all that useful,
as different threads can allocate space in parallel.  Bump it to 4
blocks per AG to allow each thread that is currently doing an
allocation to dip into it separately.  Without that we may no have
enough reserved blocks if there are enough parallel transactions
in an almost out space file system that all run into bmap btree
splits.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_alloc.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 5050056..0a46f84 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -95,10 +95,7 @@ unsigned int
 xfs_alloc_set_aside(
 	struct xfs_mount	*mp)
 {
-	unsigned int		blocks;
-
-	blocks = 4 + (mp->m_sb.sb_agcount * XFS_ALLOC_AGFL_RESERVE);
-	return blocks;
+	return mp->m_sb.sb_agcount * (XFS_ALLOC_AGFL_RESERVE + 4);
 }
 
 /*
-- 
2.1.4

[PATCH 2/4] xfs: fix bogus minleft manipulations

[Christoph Hellwig]

We can't just set minleft to 0 when we're low on space - that's exactly
what we need minleft for: to protect space in the AG for btree block
allocations when we are low on free space.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_alloc.c      | 24 +++++++-----------------
 fs/xfs/libxfs/xfs_bmap.c       |  3 ---
 fs/xfs/libxfs/xfs_bmap_btree.c |  3 +--
 3 files changed, 8 insertions(+), 22 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 0a46f84..fe92570 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -2635,12 +2635,10 @@ xfs_alloc_vextent(
 	xfs_agblock_t	agsize;	/* allocation group size */
 	int		error;
 	int		flags;	/* XFS_ALLOC_FLAG_... locking flags */
-	xfs_extlen_t	minleft;/* minimum left value, temp copy */
 	xfs_mount_t	*mp;	/* mount structure pointer */
 	xfs_agnumber_t	sagno;	/* starting allocation group number */
 	xfs_alloctype_t	type;	/* input allocation type */
 	int		bump_rotor = 0;
-	int		no_min = 0;
 	xfs_agnumber_t	rotorstep = xfs_rotorstep; /* inode32 agf stepper */
 
 	mp = args->mp;
@@ -2669,7 +2667,6 @@ xfs_alloc_vextent(
 		trace_xfs_alloc_vextent_badargs(args);
 		return 0;
 	}
-	minleft = args->minleft;
 
 	switch (type) {
 	case XFS_ALLOCTYPE_THIS_AG:
@@ -2680,9 +2677,7 @@ xfs_alloc_vextent(
 		 */
 		args->agno = XFS_FSB_TO_AGNO(mp, args->fsbno);
 		args->pag = xfs_perag_get(mp, args->agno);
-		args->minleft = 0;
 		error = xfs_alloc_fix_freelist(args, 0);
-		args->minleft = minleft;
 		if (error) {
 			trace_xfs_alloc_vextent_nofix(args);
 			goto error0;
@@ -2747,9 +2742,7 @@ xfs_alloc_vextent(
 		 */
 		for (;;) {
 			args->pag = xfs_perag_get(mp, args->agno);
-			if (no_min) args->minleft = 0;
 			error = xfs_alloc_fix_freelist(args, flags);
-			args->minleft = minleft;
 			if (error) {
 				trace_xfs_alloc_vextent_nofix(args);
 				goto error0;
@@ -2789,20 +2782,17 @@ xfs_alloc_vextent(
 			 * or switch to non-trylock mode.
 			 */
 			if (args->agno == sagno) {
-				if (no_min == 1) {
+				if (flags == 0) {
 					args->agbno = NULLAGBLOCK;
 					trace_xfs_alloc_vextent_allfailed(args);
 					break;
 				}
-				if (flags == 0) {
-					no_min = 1;
-				} else {
-					flags = 0;
-					if (type == XFS_ALLOCTYPE_START_BNO) {
-						args->agbno = XFS_FSB_TO_AGBNO(mp,
-							args->fsbno);
-						args->type = XFS_ALLOCTYPE_NEAR_BNO;
-					}
+
+				flags = 0;
+				if (type == XFS_ALLOCTYPE_START_BNO) {
+					args->agbno = XFS_FSB_TO_AGBNO(mp,
+						args->fsbno);
+					args->type = XFS_ALLOCTYPE_NEAR_BNO;
 				}
 			}
 			xfs_perag_put(args->pag);
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 2760bc3..44773c9 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3812,7 +3812,6 @@ xfs_bmap_btalloc(
 		args.fsbno = 0;
 		args.type = XFS_ALLOCTYPE_FIRST_AG;
 		args.total = ap->minlen;
-		args.minleft = 0;
 		if ((error = xfs_alloc_vextent(&args)))
 			return error;
 		ap->dfops->dop_low = true;
@@ -4344,8 +4343,6 @@ xfs_bmapi_allocate(
 	if (error)
 		return error;
 
-	if (bma->dfops->dop_low)
-		bma->minleft = 0;
 	if (bma->cur)
 		bma->cur->bc_private.b.firstblock = *bma->firstblock;
 	if (bma->blkno == NULLFSBLOCK)
diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
index d6330c2..d9be241 100644
--- a/fs/xfs/libxfs/xfs_bmap_btree.c
+++ b/fs/xfs/libxfs/xfs_bmap_btree.c
@@ -502,12 +502,11 @@ xfs_bmbt_alloc_block(
 	if (args.fsbno == NULLFSBLOCK && args.minleft) {
 		/*
 		 * Could not find an AG with enough free space to satisfy
-		 * a full btree split.  Try again without minleft and if
+		 * a full btree split.  Try again and if
 		 * successful activate the lowspace algorithm.
 		 */
 		args.fsbno = 0;
 		args.type = XFS_ALLOCTYPE_FIRST_AG;
-		args.minleft = 0;
 		error = xfs_alloc_vextent(&args);
 		if (error)
 			goto error0;
-- 
2.1.4

[PATCH 3/4] xfs: adjust allocation length in xfs_alloc_space_available

[Christoph Hellwig]

We must decide in xfs_alloc_fix_freelist if we can perform an
allocation from a given AG is possible or not based on the available
space, and should not fail the allocation past that point on a
healthy file system.

But currently we have two additional places that second-guess
xfs_alloc_fix_freelist: xfs_alloc_ag_vextent tries to adjust the
maxlen parameter to remove the reservation before doing the
allocation (but ignores the various minium freespace requirements),
and xfs_alloc_fix_minleft tries to fix up the allocated length
after we've found an extent, but ignores the reservations and also
doesn't take the AGFL into account (and thus fails allocations
for not matching minlen in some cases).

Remove all these later fixups and just correct the maxlen argument
inside xfs_alloc_fix_freelist once we have the AGF buffer locked.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_alloc.c | 81 ++++++++++-------------------------------------
 fs/xfs/libxfs/xfs_alloc.h |  2 +-
 2 files changed, 18 insertions(+), 65 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index fe92570..f2e7eb6 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -362,36 +362,12 @@ xfs_alloc_fix_len(
 		return;
 	ASSERT(rlen >= args->minlen && rlen <= args->maxlen);
 	ASSERT(rlen % args->prod == args->mod);
+	ASSERT(args->pag->pagf_freeblks + args->pag->pagf_flcount >=
+		rlen + args->minleft);
 	args->len = rlen;
 }
 
 /*
- * Fix up length if there is too little space left in the a.g.
- * Return 1 if ok, 0 if too little, should give up.
- */
-STATIC int
-xfs_alloc_fix_minleft(
-	xfs_alloc_arg_t	*args)		/* allocation argument structure */
-{
-	xfs_agf_t	*agf;		/* a.g. freelist header */
-	int		diff;		/* free space difference */
-
-	if (args->minleft == 0)
-		return 1;
-	agf = XFS_BUF_TO_AGF(args->agbp);
-	diff = be32_to_cpu(agf->agf_freeblks)
-		- args->len - args->minleft;
-	if (diff >= 0)
-		return 1;
-	args->len += diff;		/* shrink the allocated space */
-	/* casts to (int) catch length underflows */
-	if ((int)args->len >= (int)args->minlen)
-		return 1;
-	args->agbno = NULLAGBLOCK;
-	return 0;
-}
-
-/*
  * Update the two btrees, logically removing from freespace the extent
  * starting at rbno, rlen blocks.  The extent is contained within the
  * actual (current) free extent fbno for flen blocks.
@@ -686,8 +662,6 @@ xfs_alloc_ag_vextent(
 	xfs_alloc_arg_t	*args)	/* argument structure for allocation */
 {
 	int		error=0;
-	xfs_extlen_t	reservation;
-	xfs_extlen_t	oldmax;
 
 	ASSERT(args->minlen > 0);
 	ASSERT(args->maxlen > 0);
@@ -696,20 +670,6 @@ xfs_alloc_ag_vextent(
 	ASSERT(args->alignment > 0);
 
 	/*
-	 * Clamp maxlen to the amount of free space minus any reservations
-	 * that have been made.
-	 */
-	oldmax = args->maxlen;
-	reservation = xfs_ag_resv_needed(args->pag, args->resv);
-	if (args->maxlen > args->pag->pagf_freeblks - reservation)
-		args->maxlen = args->pag->pagf_freeblks - reservation;
-	if (args->maxlen == 0) {
-		args->agbno = NULLAGBLOCK;
-		args->maxlen = oldmax;
-		return 0;
-	}
-
-	/*
 	 * Branch to correct routine based on the type.
 	 */
 	args->wasfromfl = 0;
@@ -728,8 +688,6 @@ xfs_alloc_ag_vextent(
 		/* NOTREACHED */
 	}
 
-	args->maxlen = oldmax;
-
 	if (error || args->agbno == NULLAGBLOCK)
 		return error;
 
@@ -838,9 +796,6 @@ xfs_alloc_ag_vextent_exact(
 	args->len = XFS_AGBLOCK_MIN(tend, args->agbno + args->maxlen)
 						- args->agbno;
 	xfs_alloc_fix_len(args);
-	if (!xfs_alloc_fix_minleft(args))
-		goto not_found;
-
 	ASSERT(args->agbno + args->len <= tend);
 
 	/*
@@ -1146,12 +1101,7 @@ xfs_alloc_ag_vextent_near(
 		XFS_WANT_CORRUPTED_GOTO(args->mp, i == 1, error0);
 		ASSERT(ltbno + ltlen <= be32_to_cpu(XFS_BUF_TO_AGF(args->agbp)->agf_length));
 		args->len = blen;
-		if (!xfs_alloc_fix_minleft(args)) {
-			xfs_btree_del_cursor(cnt_cur, XFS_BTREE_NOERROR);
-			trace_xfs_alloc_near_nominleft(args);
-			return 0;
-		}
-		blen = args->len;
+
 		/*
 		 * We are allocating starting at bnew for blen blocks.
 		 */
@@ -1343,12 +1293,6 @@ xfs_alloc_ag_vextent_near(
 	 */
 	args->len = XFS_EXTLEN_MIN(ltlena, args->maxlen);
 	xfs_alloc_fix_len(args);
-	if (!xfs_alloc_fix_minleft(args)) {
-		trace_xfs_alloc_near_nominleft(args);
-		xfs_btree_del_cursor(bno_cur_lt, XFS_BTREE_NOERROR);
-		xfs_btree_del_cursor(cnt_cur, XFS_BTREE_NOERROR);
-		return 0;
-	}
 	rlen = args->len;
 	(void)xfs_alloc_compute_diff(args->agbno, rlen, args->alignment,
 				     args->datatype, ltbnoa, ltlena, &ltnew);
@@ -1550,8 +1494,6 @@ xfs_alloc_ag_vextent_size(
 	}
 	xfs_alloc_fix_len(args);
 
-	if (!xfs_alloc_fix_minleft(args))
-		goto out_nominleft;
 	rlen = args->len;
 	XFS_WANT_CORRUPTED_GOTO(args->mp, rlen <= flen, error0);
 	/*
@@ -2070,10 +2012,20 @@ xfs_alloc_space_available(
 
 	/* do we have enough free space remaining for the allocation? */
 	available = (int)(pag->pagf_freeblks + pag->pagf_flcount -
-			  reservation - min_free - args->total);
-	if (available < (int)args->minleft || available <= 0)
+			  reservation - min_free - args->minleft);
+	if (available < (int)args->total)
 		return false;
 
+	/*
+	 * Clamp maxlen to the amount of free space available for the actual
+	 * extent allocation.
+	 */
+	if (available < (int)args->maxlen && !(flags & XFS_ALLOC_FLAG_CHECK)) {
+		args->maxlen = available;
+		ASSERT(args->maxlen > 0);
+		ASSERT(args->maxlen >= args->minlen);
+	}
+
 	return true;
 }
 
@@ -2119,7 +2071,8 @@ xfs_alloc_fix_freelist(
 	}
 
 	need = xfs_alloc_min_freelist(mp, pag);
-	if (!xfs_alloc_space_available(args, need, flags))
+	if (!xfs_alloc_space_available(args, need, flags |
+			XFS_ALLOC_FLAG_CHECK))
 		goto out_agbp_relse;
 
 	/*
diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
index 7c404a6..1d0f48a 100644
--- a/fs/xfs/libxfs/xfs_alloc.h
+++ b/fs/xfs/libxfs/xfs_alloc.h
@@ -56,7 +56,7 @@ typedef unsigned int xfs_alloctype_t;
 #define	XFS_ALLOC_FLAG_FREEING	0x00000002  /* indicate caller is freeing extents*/
 #define	XFS_ALLOC_FLAG_NORMAP	0x00000004  /* don't modify the rmapbt */
 #define	XFS_ALLOC_FLAG_NOSHRINK	0x00000008  /* don't shrink the freelist */
-
+#define	XFS_ALLOC_FLAG_CHECK	0x00000010  /* test only, don't modify args */
 
 /*
  * Argument structure for xfs_alloc routines.
-- 
2.1.4

[PATCH 4/4] xfs: don't rely on ->total in xfs_alloc_space_available

[Christoph Hellwig]

->total is a bit of an odd parameter passed down to the low-level
allocator all the way from the high-level callers.  It's supposed to
contain the maximum number of blocks to be allocated for the whole
transaction [1].

But in xfs_iomap_write_allocate we only convert existing delayed
allocations and thus only have a minimal block reservation for the
current transaction, so xfs_alloc_space_available can't use it for
the allocation decisions.  Use the maximum of args->total and the
calculated block requirement to make a decision.  We probably should
get rid of args->total eventually and instead apply ->minleft more
broadly, but that will require some extensive changes all over.

[1] which creates lots of confusion as most callers don't decrement it
once doing a first allocation.  But that's for a separate series.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_alloc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index f2e7eb6..9f06a21 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -1995,7 +1995,7 @@ xfs_alloc_space_available(
 	int			flags)
 {
 	struct xfs_perag	*pag = args->pag;
-	xfs_extlen_t		longest;
+	xfs_extlen_t		alloc_len, longest;
 	xfs_extlen_t		reservation; /* blocks that are still reserved */
 	int			available;
 
@@ -2005,15 +2005,16 @@ xfs_alloc_space_available(
 	reservation = xfs_ag_resv_needed(pag, args->resv);
 
 	/* do we have enough contiguous free space for the allocation? */
+	alloc_len = args->minlen + (args->alignment - 1) + args->minalignslop;
 	longest = xfs_alloc_longest_free_extent(args->mp, pag, min_free,
 			reservation);
-	if ((args->minlen + args->alignment + args->minalignslop - 1) > longest)
+	if (longest < alloc_len)
 		return false;
 
 	/* do we have enough free space remaining for the allocation? */
 	available = (int)(pag->pagf_freeblks + pag->pagf_flcount -
 			  reservation - min_free - args->minleft);
-	if (available < (int)args->total)
+	if (available < (int)max(args->total, alloc_len))
 		return false;
 
 	/*
-- 
2.1.4

Re: [PATCH 1/4] xfs: bump up reserved blocks in xfs_alloc_set_aside

[Brian Foster]

On Mon, Jan 09, 2017 at 08:53:39PM +0100, Christoph Hellwig wrote:
> Setting aside 4 blocks globally for bmbt splits isn't all that useful,
> as different threads can allocate space in parallel.  Bump it to 4
> blocks per AG to allow each thread that is currently doing an
> allocation to dip into it separately.  Without that we may no have
> enough reserved blocks if there are enough parallel transactions
> in an almost out space file system that all run into bmap btree
> splits.
> 

Thanks.

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_alloc.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index 5050056..0a46f84 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -95,10 +95,7 @@ unsigned int
>  xfs_alloc_set_aside(
>  	struct xfs_mount	*mp)
>  {
> -	unsigned int		blocks;
> -
> -	blocks = 4 + (mp->m_sb.sb_agcount * XFS_ALLOC_AGFL_RESERVE);
> -	return blocks;
> +	return mp->m_sb.sb_agcount * (XFS_ALLOC_AGFL_RESERVE + 4);
>  }
>  
>  /*
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] xfs: fix bogus minleft manipulations

[Brian Foster]

On Mon, Jan 09, 2017 at 08:53:40PM +0100, Christoph Hellwig wrote:
> We can't just set minleft to 0 when we're low on space - that's exactly
> what we need minleft for: to protect space in the AG for btree block
> allocations when we are low on free space.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_alloc.c      | 24 +++++++-----------------
>  fs/xfs/libxfs/xfs_bmap.c       |  3 ---
>  fs/xfs/libxfs/xfs_bmap_btree.c |  3 +--
>  3 files changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index 0a46f84..fe92570 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -2635,12 +2635,10 @@ xfs_alloc_vextent(
>  	xfs_agblock_t	agsize;	/* allocation group size */
>  	int		error;
>  	int		flags;	/* XFS_ALLOC_FLAG_... locking flags */
> -	xfs_extlen_t	minleft;/* minimum left value, temp copy */
>  	xfs_mount_t	*mp;	/* mount structure pointer */
>  	xfs_agnumber_t	sagno;	/* starting allocation group number */
>  	xfs_alloctype_t	type;	/* input allocation type */
>  	int		bump_rotor = 0;
> -	int		no_min = 0;
>  	xfs_agnumber_t	rotorstep = xfs_rotorstep; /* inode32 agf stepper */
>  
>  	mp = args->mp;
> @@ -2669,7 +2667,6 @@ xfs_alloc_vextent(
>  		trace_xfs_alloc_vextent_badargs(args);
>  		return 0;
>  	}
> -	minleft = args->minleft;
>  
>  	switch (type) {
>  	case XFS_ALLOCTYPE_THIS_AG:
> @@ -2680,9 +2677,7 @@ xfs_alloc_vextent(
>  		 */
>  		args->agno = XFS_FSB_TO_AGNO(mp, args->fsbno);
>  		args->pag = xfs_perag_get(mp, args->agno);
> -		args->minleft = 0;
>  		error = xfs_alloc_fix_freelist(args, 0);
> -		args->minleft = minleft;
>  		if (error) {
>  			trace_xfs_alloc_vextent_nofix(args);
>  			goto error0;
> @@ -2747,9 +2742,7 @@ xfs_alloc_vextent(
>  		 */
>  		for (;;) {
>  			args->pag = xfs_perag_get(mp, args->agno);
> -			if (no_min) args->minleft = 0;
>  			error = xfs_alloc_fix_freelist(args, flags);
> -			args->minleft = minleft;
>  			if (error) {
>  				trace_xfs_alloc_vextent_nofix(args);
>  				goto error0;
> @@ -2789,20 +2782,17 @@ xfs_alloc_vextent(
>  			 * or switch to non-trylock mode.
>  			 */
>  			if (args->agno == sagno) {
> -				if (no_min == 1) {
> +				if (flags == 0) {
>  					args->agbno = NULLAGBLOCK;
>  					trace_xfs_alloc_vextent_allfailed(args);
>  					break;
>  				}
> -				if (flags == 0) {
> -					no_min = 1;
> -				} else {
> -					flags = 0;
> -					if (type == XFS_ALLOCTYPE_START_BNO) {
> -						args->agbno = XFS_FSB_TO_AGBNO(mp,
> -							args->fsbno);
> -						args->type = XFS_ALLOCTYPE_NEAR_BNO;
> -					}
> +
> +				flags = 0;
> +				if (type == XFS_ALLOCTYPE_START_BNO) {
> +					args->agbno = XFS_FSB_TO_AGBNO(mp,
> +						args->fsbno);
> +					args->type = XFS_ALLOCTYPE_NEAR_BNO;
>  				}
>  			}
>  			xfs_perag_put(args->pag);
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 2760bc3..44773c9 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3812,7 +3812,6 @@ xfs_bmap_btalloc(
>  		args.fsbno = 0;
>  		args.type = XFS_ALLOCTYPE_FIRST_AG;
>  		args.total = ap->minlen;
> -		args.minleft = 0;
>  		if ((error = xfs_alloc_vextent(&args)))
>  			return error;
>  		ap->dfops->dop_low = true;
> @@ -4344,8 +4343,6 @@ xfs_bmapi_allocate(
>  	if (error)
>  		return error;
>  
> -	if (bma->dfops->dop_low)
> -		bma->minleft = 0;
>  	if (bma->cur)
>  		bma->cur->bc_private.b.firstblock = *bma->firstblock;
>  	if (bma->blkno == NULLFSBLOCK)
> diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
> index d6330c2..d9be241 100644
> --- a/fs/xfs/libxfs/xfs_bmap_btree.c
> +++ b/fs/xfs/libxfs/xfs_bmap_btree.c
> @@ -502,12 +502,11 @@ xfs_bmbt_alloc_block(
>  	if (args.fsbno == NULLFSBLOCK && args.minleft) {
>  		/*
>  		 * Could not find an AG with enough free space to satisfy
> -		 * a full btree split.  Try again without minleft and if
> +		 * a full btree split.  Try again and if
>  		 * successful activate the lowspace algorithm.
>  		 */
>  		args.fsbno = 0;
>  		args.type = XFS_ALLOCTYPE_FIRST_AG;
> -		args.minleft = 0;
>  		error = xfs_alloc_vextent(&args);
>  		if (error)
>  			goto error0;
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] xfs: don't rely on ->total in xfs_alloc_space_available

[Brian Foster]

On Thu, Dec 15, 2016 at 09:55:34AM +0100, Christoph Hellwig wrote:
> On Wed, Dec 14, 2016 at 04:51:58PM -0500, Brian Foster wrote:
> > On Wed, Dec 14, 2016 at 08:38:02PM +0100, Christoph Hellwig wrote:
> > > On Wed, Dec 14, 2016 at 01:30:18PM -0500, Brian Foster wrote:
> > > > >  	/* do we have enough contiguous free space for the allocation? */
> > > > > +	alloc_len = args->minlen + (args->alignment - 1)+ args->minalignslop;
> > > > 
> > > > Whitespace nit, missing space:				^
> > > > 
> > > > Also, is the addition of braces intentional? I believe it is possible
> > > > for args->alignment == 0.
> > > 
> > > It's mostly for explaining the - 1 to the reader as it took me a while
> > > to figure out what it was for.
> > 
> > Oh, care to elaborate? :)
> 
> the alignment is the boundary we want to align to, so we'll have to add
> alignment - 1 at max to achive it. 

Gotcha, so perhaps we should check that alignment != 0?

Brian

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] xfs: don't rely on ->total in xfs_alloc_space_available

[Christoph Hellwig]

On Wed, Dec 14, 2016 at 04:51:58PM -0500, Brian Foster wrote:
> On Wed, Dec 14, 2016 at 08:38:02PM +0100, Christoph Hellwig wrote:
> > On Wed, Dec 14, 2016 at 01:30:18PM -0500, Brian Foster wrote:
> > > >  	/* do we have enough contiguous free space for the allocation? */
> > > > +	alloc_len = args->minlen + (args->alignment - 1)+ args->minalignslop;
> > > 
> > > Whitespace nit, missing space:				^
> > > 
> > > Also, is the addition of braces intentional? I believe it is possible
> > > for args->alignment == 0.
> > 
> > It's mostly for explaining the - 1 to the reader as it took me a while
> > to figure out what it was for.
> 
> Oh, care to elaborate? :)

the alignment is the boundary we want to align to, so we'll have to add
alignment - 1 at max to achive it. 

Re: [PATCH 4/4] xfs: don't rely on ->total in xfs_alloc_space_available

[Brian Foster]

On Wed, Dec 14, 2016 at 08:38:02PM +0100, Christoph Hellwig wrote:
> On Wed, Dec 14, 2016 at 01:30:18PM -0500, Brian Foster wrote:
> > >  	/* do we have enough contiguous free space for the allocation? */
> > > +	alloc_len = args->minlen + (args->alignment - 1)+ args->minalignslop;
> > 
> > Whitespace nit, missing space:				^
> > 
> > Also, is the addition of braces intentional? I believe it is possible
> > for args->alignment == 0.
> 
> It's mostly for explaining the - 1 to the reader as it took me a while
> to figure out what it was for.

Oh, care to elaborate? :)

Brian

Re: [PATCH 4/4] xfs: don't rely on ->total in xfs_alloc_space_available

[Christoph Hellwig]

On Wed, Dec 14, 2016 at 01:30:18PM -0500, Brian Foster wrote:
> >  	/* do we have enough contiguous free space for the allocation? */
> > +	alloc_len = args->minlen + (args->alignment - 1)+ args->minalignslop;
> 
> Whitespace nit, missing space:				^
> 
> Also, is the addition of braces intentional? I believe it is possible
> for args->alignment == 0.

It's mostly for explaining the - 1 to the reader as it took me a while
to figure out what it was for.

Re: [PATCH 4/4] xfs: don't rely on ->total in xfs_alloc_space_available

[Brian Foster]

On Tue, Dec 13, 2016 at 04:59:27PM +0100, Christoph Hellwig wrote:
> ->total is a bit of an odd parameter passed down to the low-level
> allocator all the way from the high-level callers.  It's supposed to
> contain the maximum number of blocks to be allocated for the whole
> transaction [1].
> 
> But in xfs_iomap_write_allocate we only convert existing delayed
> allocations and thus only have a minimal block reservation for the
> current transaction, so xfs_alloc_space_available can't use it for
> the allocation decisions.  Use the maximum of args->total and the
> calculated block requirement to make a decision.  We probably should
> get rid of args->total eventually and instead apply ->minleft more
> broadly, but that will require some extensive changes all over.
> 
> [1] which creates lots of confusion as most callers don't decrement it
> once doing a first allocation.  But that's for a separate series.
> ---
>  fs/xfs/libxfs/xfs_alloc.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index 87328a8..85039e5 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -1998,7 +1998,7 @@ xfs_alloc_space_available(
>  	int			flags)
>  {
>  	struct xfs_perag	*pag = args->pag;
> -	xfs_extlen_t		longest;
> +	xfs_extlen_t		alloc_len, longest;
>  	xfs_extlen_t		reservation; /* blocks that are still reserved */
>  	int			available;
>  
> @@ -2008,15 +2008,16 @@ xfs_alloc_space_available(
>  	reservation = xfs_ag_resv_needed(pag, args->resv);
>  
>  	/* do we have enough contiguous free space for the allocation? */
> +	alloc_len = args->minlen + (args->alignment - 1)+ args->minalignslop;

Whitespace nit, missing space:				^

Also, is the addition of braces intentional? I believe it is possible
for args->alignment == 0.

Nits aside:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  	longest = xfs_alloc_longest_free_extent(args->mp, pag, min_free,
>  			reservation);
> -	if ((args->minlen + args->alignment + args->minalignslop - 1) > longest)
> +	if (longest < alloc_len)
>  		return false;
>  
>  	/* do we have enough free space remaining for the allocation? */
>  	available = (int)(pag->pagf_freeblks + pag->pagf_flcount -
>  			  reservation - min_free - args->minleft);
> -	if (available < (int)args->total)
> +	if (available < (int)max(args->total, alloc_len))
>  		return false;
>  
>  	/*
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] xfs: don't rely on ->total in xfs_alloc_space_available

[Christoph Hellwig]

->total is a bit of an odd parameter passed down to the low-level
allocator all the way from the high-level callers.  It's supposed to
contain the maximum number of blocks to be allocated for the whole
transaction [1].

But in xfs_iomap_write_allocate we only convert existing delayed
allocations and thus only have a minimal block reservation for the
current transaction, so xfs_alloc_space_available can't use it for
the allocation decisions.  Use the maximum of args->total and the
calculated block requirement to make a decision.  We probably should
get rid of args->total eventually and instead apply ->minleft more
broadly, but that will require some extensive changes all over.

[1] which creates lots of confusion as most callers don't decrement it
once doing a first allocation.  But that's for a separate series.
---
 fs/xfs/libxfs/xfs_alloc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 87328a8..85039e5 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -1998,7 +1998,7 @@ xfs_alloc_space_available(
 	int			flags)
 {
 	struct xfs_perag	*pag = args->pag;
-	xfs_extlen_t		longest;
+	xfs_extlen_t		alloc_len, longest;
 	xfs_extlen_t		reservation; /* blocks that are still reserved */
 	int			available;
 
@@ -2008,15 +2008,16 @@ xfs_alloc_space_available(
 	reservation = xfs_ag_resv_needed(pag, args->resv);
 
 	/* do we have enough contiguous free space for the allocation? */
+	alloc_len = args->minlen + (args->alignment - 1)+ args->minalignslop;
 	longest = xfs_alloc_longest_free_extent(args->mp, pag, min_free,
 			reservation);
-	if ((args->minlen + args->alignment + args->minalignslop - 1) > longest)
+	if (longest < alloc_len)
 		return false;
 
 	/* do we have enough free space remaining for the allocation? */
 	available = (int)(pag->pagf_freeblks + pag->pagf_flcount -
 			  reservation - min_free - args->minleft);
-	if (available < (int)args->total)
+	if (available < (int)max(args->total, alloc_len))
 		return false;
 
 	/*
-- 
2.1.4