[PATCH 0/2] fix up indlen reservations on extent split

[PATCH 0/2] fix up indlen reservations on extent split

[Brian Foster]

Hi all,

Here's a couple patches to fix up the indirect block reservation
problem when splitting delalloc extents, described in more detail in the
patch 2 commit log.

This runs through xfstests without any explosions and quiets down
generic/033 when used in combination with the zero range rework (current
for-next includes the writeback on zero range workaround that also
prevents generic/033 asserts).

Brian

v1:
- xfs_bunmapi() code into independent patch.
- Refactor fix into separate helper function.
rfc: http://oss.sgi.com/archives/xfs/2014-09/msg00337.html

Brian Foster (2):
  xfs: update icsb freeblocks counter after extent deletion
  xfs: borrow indirect blocks from freed extent when available

 fs/xfs/libxfs/xfs_bmap.c | 159 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 116 insertions(+), 43 deletions(-)

-- 
1.8.3.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/2] xfs: update icsb freeblocks counter after extent deletion

[Brian Foster]

xfs_bunmapi() currently updates the icsb fdblocks counter, unreserves
quota, etc. before the extent is deleted by xfs_bmap_del_extent(). The
function has problems dividing up the indirect reserved blocks for
scenarios where a single delalloc extent is split in two. Particularly,
there aren't always enough blocks reserved for multiple extents in a
single extent reservation.

The solution to this problem is to allow the extent removal code to
steal from the deleted extent to meet indirect reservation requirements.
Move the block of code in xfs_bmapi() that updates the icsb fdblocks
counter to after the call to xfs_bmap_del_extent() to allow the codepath
to update the extent record before the free blocks are accounted. Also,
reshuffle the code slightly so the delalloc accounting occurs near the
xfs_bmap_del_extent() call to provide context for the comments.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_bmap.c | 59 ++++++++++++++++++++++++++++--------------------
 1 file changed, 35 insertions(+), 24 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 79c9819..5abd22b 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5277,9 +5277,37 @@ xfs_bunmapi(
 				goto nodelete;
 			}
 		}
+
+		/*
+		 * If it's the case where the directory code is running
+		 * with no block reservation, and the deleted block is in
+		 * the middle of its extent, and the resulting insert
+		 * of an extent would cause transformation to btree format,
+		 * then reject it.  The calling code will then swap
+		 * blocks around instead.
+		 * We have to do this now, rather than waiting for the
+		 * conversion to btree format, since the transaction
+		 * will be dirty.
+		 */
+		if (!wasdel && xfs_trans_get_block_res(tp) == 0 &&
+		    XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_EXTENTS &&
+		    XFS_IFORK_NEXTENTS(ip, whichfork) >= /* Note the >= */
+			XFS_IFORK_MAXEXT(ip, whichfork) &&
+		    del.br_startoff > got.br_startoff &&
+		    del.br_startoff + del.br_blockcount <
+		    got.br_startoff + got.br_blockcount) {
+			error = -ENOSPC;
+			goto error0;
+		}
+
+		/*
+		 * Unreserve quota and update realtime free space, if
+		 * appropriate. If delayed allocation, update the inode delalloc
+		 * counter now and wait to update the sb counters as
+		 * xfs_bmap_del_extent() might need to borrow some blocks.
+		 */
 		if (wasdel) {
 			ASSERT(startblockval(del.br_startblock) > 0);
-			/* Update realtime/data freespace, unreserve quota */
 			if (isrt) {
 				xfs_filblks_t rtexts;
 
@@ -5291,8 +5319,6 @@ xfs_bunmapi(
 					ip, -((long)del.br_blockcount), 0,
 					XFS_QMOPT_RES_RTBLKS);
 			} else {
-				xfs_icsb_modify_counters(mp, XFS_SBS_FDBLOCKS,
-						(int64_t)del.br_blockcount, 0);
 				(void)xfs_trans_reserve_quota_nblks(NULL,
 					ip, -((long)del.br_blockcount), 0,
 					XFS_QMOPT_RES_REGBLKS);
@@ -5303,32 +5329,17 @@ xfs_bunmapi(
 					XFS_BTCUR_BPRV_WASDEL;
 		} else if (cur)
 			cur->bc_private.b.flags &= ~XFS_BTCUR_BPRV_WASDEL;
-		/*
-		 * If it's the case where the directory code is running
-		 * with no block reservation, and the deleted block is in
-		 * the middle of its extent, and the resulting insert
-		 * of an extent would cause transformation to btree format,
-		 * then reject it.  The calling code will then swap
-		 * blocks around instead.
-		 * We have to do this now, rather than waiting for the
-		 * conversion to btree format, since the transaction
-		 * will be dirty.
-		 */
-		if (!wasdel && xfs_trans_get_block_res(tp) == 0 &&
-		    XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_EXTENTS &&
-		    XFS_IFORK_NEXTENTS(ip, whichfork) >= /* Note the >= */
-			XFS_IFORK_MAXEXT(ip, whichfork) &&
-		    del.br_startoff > got.br_startoff &&
-		    del.br_startoff + del.br_blockcount <
-		    got.br_startoff + got.br_blockcount) {
-			error = -ENOSPC;
-			goto error0;
-		}
+
 		error = xfs_bmap_del_extent(ip, tp, &lastx, flist, cur, &del,
 				&tmp_logflags, whichfork);
 		logflags |= tmp_logflags;
 		if (error)
 			goto error0;
+
+		if (!isrt && wasdel)
+			xfs_icsb_modify_counters(mp, XFS_SBS_FDBLOCKS,
+				(int64_t) del.br_blockcount, 0);
+
 		bno = del.br_startoff - 1;
 nodelete:
 		/*
-- 
1.8.3.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/2] xfs: borrow indirect blocks from freed extent when available

[Brian Foster]

xfs_bmap_del_extent() handles extent removal from the in-core and
on-disk extent lists. When removing a delalloc range, it updates the
indirect block reservation appropriately based on the removal. It
currently enforces that the new indirect block reservation is less than
or equal to the original. This is normally the case in all situations
except for in certain cases when the removed range creates a hole in a
single delalloc extent, thus splitting a single delalloc extent in two.

It is possible with small enough extents to split an indlen==1 extent
into two such slightly smaller extents. This leaves one extent with 0
indirect blocks and leads to assert failures in other areas (e.g.,
xfs_bunmapi() if the extent happens to be removed).

Refactor the indirect reservation code into a new helper function
invoked by xfs_bunmapi(). Allow the helper to steal blocks from the
deleted extent if necessary to satisfy the worst case indirect
reservation of the new extents. This is safe now that the caller does
not update icsb counters until the extent is removed (such stolen blocks
simply remain accounted as allocated). Fall back to the original
reservation using the existing algorithm and warn if we end up with
extents without any reservation at all to detect this more easily in the
future.

This allows generic/033 to run on XFS without assert failures.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_bmap.c | 100 ++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 81 insertions(+), 19 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 5abd22b..64b88b6 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4701,6 +4701,68 @@ error0:
 }
 
 /*
+ * When a delalloc extent is split (e.g., due to a hole punch), the original
+ * indlen reservation must be shared across the two new extents that are left
+ * behind.
+ *
+ * Provided the worst case indlen for two extents, limit the total reservation
+ * against that of the original extent. We may want to steal blocks from any
+ * extra that might be available according to the caller to make up a deficiency
+ * (e.g., if the original reservation consists of one block). The number of such
+ * stolen blocks is returned. The availability and accounting of stealable
+ * blocks is the responsibility of the caller.
+ */
+static xfs_filblks_t
+xfs_bmap_limit_indlen(
+	xfs_filblks_t			ores,		/* original res. */
+	xfs_filblks_t			*indlen1,	/* ext1 worst indlen */
+	xfs_filblks_t			*indlen2,	/* ext2 worst indlen */
+	xfs_filblks_t			avail)		/* stealable blocks */
+{
+	xfs_filblks_t			nres;		/* new total res. */
+	xfs_filblks_t			temp;
+	xfs_filblks_t			temp2;
+	xfs_filblks_t			stolen = 0;
+
+	temp = *indlen1;
+	temp2 = *indlen2;
+	nres = temp + temp2;
+
+	/*
+	 * Steal as many blocks as we can to try and satisfy the worst case
+	 * indlen of both new extents.
+	 */
+	while (nres > ores && avail) {
+		nres--;
+		avail--;
+		stolen++;
+	}
+
+	/*
+	 * If we've exhausted stealable blocks and still haven't satisfied the
+	 * worst case reservation, we have no choice but to pare back until
+	 * covered by the original reservation.
+	 */
+	while (nres > ores) {
+		if (temp) {
+			temp--;
+			nres--;
+		}
+		if (nres == ores)
+			break;
+		if (temp2) {
+			temp2--;
+			nres--;
+		}
+	}
+
+	*indlen1 = temp;
+	*indlen2 = temp2;
+
+	return stolen;
+}
+
+/*
  * Called by xfs_bmapi to update file extent records and the btree
  * after removing space (or undoing a delayed allocation).
  */
@@ -4963,28 +5025,28 @@ xfs_bmap_del_extent(
 			XFS_IFORK_NEXT_SET(ip, whichfork,
 				XFS_IFORK_NEXTENTS(ip, whichfork) + 1);
 		} else {
+			xfs_filblks_t	stolen;
 			ASSERT(whichfork == XFS_DATA_FORK);
-			temp = xfs_bmap_worst_indlen(ip, temp);
+
+			/*
+			 * Fix up the indlen reservations. We can safely steal
+			 * blocks from the deleted extent as this simply fudges
+			 * the icsb fdblocks accounting in xfs_bunmapi().
+			 */
+			temp = xfs_bmap_worst_indlen(ip, got.br_blockcount);
+			temp2 = xfs_bmap_worst_indlen(ip, new.br_blockcount);
+			stolen = xfs_bmap_limit_indlen(da_old, &temp, &temp2,
+						       del->br_blockcount);
+			da_new = temp + temp2 - stolen;
+			del->br_blockcount -= stolen;
+
+			/*
+			 * Set the reservation for each extent. Warn if either
+			 * is zero as this can lead to delalloc problems.
+			 */
+			WARN_ON_ONCE(!temp || !temp2);
 			xfs_bmbt_set_startblock(ep, nullstartblock((int)temp));
-			temp2 = xfs_bmap_worst_indlen(ip, temp2);
 			new.br_startblock = nullstartblock((int)temp2);
-			da_new = temp + temp2;
-			while (da_new > da_old) {
-				if (temp) {
-					temp--;
-					da_new--;
-					xfs_bmbt_set_startblock(ep,
-						nullstartblock((int)temp));
-				}
-				if (da_new == da_old)
-					break;
-				if (temp2) {
-					temp2--;
-					da_new--;
-					new.br_startblock =
-						nullstartblock((int)temp2);
-				}
-			}
 		}
 		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
 		xfs_iext_insert(ip, *idx + 1, 1, &new, state);
-- 
1.8.3.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 0/2] fix up indlen reservations on extent split

[Brian Foster]

On Tue, Oct 14, 2014 at 05:59:13PM -0400, Brian Foster wrote:
> Hi all,
> 
> Here's a couple patches to fix up the indirect block reservation
> problem when splitting delalloc extents, described in more detail in the
> patch 2 commit log.
> 
> This runs through xfstests without any explosions and quiets down
> generic/033 when used in combination with the zero range rework (current
> for-next includes the writeback on zero range workaround that also
> prevents generic/033 asserts).
> 
> Brian
> 
> v1:
> - xfs_bunmapi() code into independent patch.
> - Refactor fix into separate helper function.
> rfc: http://oss.sgi.com/archives/xfs/2014-09/msg00337.html
>  

ping?

> Brian Foster (2):
>   xfs: update icsb freeblocks counter after extent deletion
>   xfs: borrow indirect blocks from freed extent when available
> 
>  fs/xfs/libxfs/xfs_bmap.c | 159 ++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 116 insertions(+), 43 deletions(-)
> 
> -- 
> 1.8.3.1
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 0/2] fix up indlen reservations on extent split

[Dave Chinner]

On Wed, Oct 29, 2014 at 12:38:27PM -0400, Brian Foster wrote:
> On Tue, Oct 14, 2014 at 05:59:13PM -0400, Brian Foster wrote:
> > Hi all,
> > 
> > Here's a couple patches to fix up the indirect block reservation
> > problem when splitting delalloc extents, described in more detail in the
> > patch 2 commit log.
> > 
> > This runs through xfstests without any explosions and quiets down
> > generic/033 when used in combination with the zero range rework (current
> > for-next includes the writeback on zero range workaround that also
> > prevents generic/033 asserts).
> > 
> > Brian
> > 
> > v1:
> > - xfs_bunmapi() code into independent patch.
> > - Refactor fix into separate helper function.
> > rfc: http://oss.sgi.com/archives/xfs/2014-09/msg00337.html
> >  
> 
> ping?

In my list of things to look at once I've got the bulkstat problems
sorted out.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs