split the reflink remap from the block allocation path

split the reflink remap from the block allocation path

[Christoph Hellwig]

We've run into various problems due to the fact that the reflink remap
code reuses the xfs_bmapi_write codepath for setting up the extent list
entries and abuses the firstblock field for that purpose.

This series fixes this by creating an entirely separate xfs_bmapi_remap
path that is much simpler than xfs_bmapi_write and does not need to
overload the firstblock field.

[PATCH 1/6] xfs: fix integer truncation in xfs_bmap_remap_alloc

[Christoph Hellwig]

bno should be a xfs_fsblock_t, which is 64-bit wides instead of a
xfs_aglock_t, which truncates the value to 32 bits.

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

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 9bd104f32908..2a426d127e05 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3863,7 +3863,7 @@ xfs_bmap_remap_alloc(
 {
 	struct xfs_trans	*tp = ap->tp;
 	struct xfs_mount	*mp = tp->t_mountp;
-	xfs_agblock_t		bno;
+	xfs_fsblock_t		bno;
 	struct xfs_alloc_arg	args;
 	int			error;
 
-- 
2.11.0

[PATCH 2/6] xfs: remove attr fork handling in xfs_bmap_finish_one

[Christoph Hellwig]

We never do COW operations for the attr fork, so don't pretend we handle
them.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 2a426d127e05..47b909c27bb3 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -6491,7 +6491,6 @@ xfs_bmap_finish_one(
 	struct xfs_bmbt_irec		bmap;
 	int				nimaps = 1;
 	xfs_fsblock_t			firstfsb;
-	int				flags = XFS_BMAPI_REMAP;
 	int				done;
 	int				error = 0;
 
@@ -6505,10 +6504,8 @@ xfs_bmap_finish_one(
 			XFS_FSB_TO_AGBNO(tp->t_mountp, startblock),
 			ip->i_ino, whichfork, startoff, blockcount, state);
 
-	if (whichfork != XFS_DATA_FORK && whichfork != XFS_ATTR_FORK)
+	if (whichfork != XFS_DATA_FORK)
 		return -EFSCORRUPTED;
-	if (whichfork == XFS_ATTR_FORK)
-		flags |= XFS_BMAPI_ATTRFORK;
 
 	if (XFS_TEST_ERROR(false, tp->t_mountp,
 			XFS_ERRTAG_BMAP_FINISH_ONE,
@@ -6519,13 +6516,13 @@ xfs_bmap_finish_one(
 	case XFS_BMAP_MAP:
 		firstfsb = bmap.br_startblock;
 		error = xfs_bmapi_write(tp, ip, bmap.br_startoff,
-					bmap.br_blockcount, flags, &firstfsb,
+					bmap.br_blockcount, XFS_BMAPI_REMAP, &firstfsb,
 					bmap.br_blockcount, &bmap, &nimaps,
 					dfops);
 		break;
 	case XFS_BMAP_UNMAP:
 		error = xfs_bunmapi(tp, ip, bmap.br_startoff,
-				bmap.br_blockcount, flags, 1, &firstfsb,
+				bmap.br_blockcount, XFS_BMAPI_REMAP, 1, &firstfsb,
 				dfops, &done);
 		ASSERT(done);
 		break;
-- 
2.11.0

[PATCH 3/6] xfs: pass individual arguments to xfs_bmap_add_extent_hole_real

[Christoph Hellwig]

For the reflink case we'd much rather pass the required arguments than
faking up a struct xfs_bmalloca.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c | 123 ++++++++++++++++++++++++-----------------------
 1 file changed, 64 insertions(+), 59 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 47b909c27bb3..d404dee7ede4 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -2879,27 +2879,30 @@ xfs_bmap_add_extent_hole_delay(
  */
 STATIC int				/* error */
 xfs_bmap_add_extent_hole_real(
-	struct xfs_bmalloca	*bma,
-	int			whichfork)
+	struct xfs_trans	*tp,
+	xfs_inode_t		*ip,	/* incore inode pointer */
+	int			whichfork,
+	xfs_extnum_t		*idx,	/* extent number to update/insert */
+	xfs_btree_cur_t		**curp,	/* if *curp is null, not a btree */
+	xfs_bmbt_irec_t		*new,	/* new data to add to file extents */
+	xfs_fsblock_t		*first,	/* pointer to firstblock variable */
+	struct xfs_defer_ops	*dfops,	/* list of extents to be freed */
+	int			*logflagsp) /* inode logging flags */
 {
-	struct xfs_bmbt_irec	*new = &bma->got;
+	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_btree_cur	*cur = *curp;
 	int			error;	/* error return value */
 	int			i;	/* temp state */
-	xfs_ifork_t		*ifp;	/* inode fork pointer */
 	xfs_bmbt_irec_t		left;	/* left neighbor extent entry */
 	xfs_bmbt_irec_t		right;	/* right neighbor extent entry */
 	int			rval=0;	/* return value (logging flags) */
 	int			state;	/* state bits, accessed thru macros */
-	struct xfs_mount	*mp;
-
-	mp = bma->ip->i_mount;
-	ifp = XFS_IFORK_PTR(bma->ip, whichfork);
 
-	ASSERT(bma->idx >= 0);
-	ASSERT(bma->idx <= xfs_iext_count(ifp));
+	ASSERT(*idx >= 0);
+	ASSERT(*idx <= xfs_iext_count(ifp));
 	ASSERT(!isnullstartblock(new->br_startblock));
-	ASSERT(!bma->cur ||
-	       !(bma->cur->bc_private.b.flags & XFS_BTCUR_BPRV_WASDEL));
+	ASSERT(!cur || !(cur->bc_private.b.flags & XFS_BTCUR_BPRV_WASDEL));
 
 	XFS_STATS_INC(mp, xs_add_exlist);
 
@@ -2912,9 +2915,9 @@ xfs_bmap_add_extent_hole_real(
 	/*
 	 * Check and set flags if this segment has a left neighbor.
 	 */
-	if (bma->idx > 0) {
+	if (*idx > 0) {
 		state |= BMAP_LEFT_VALID;
-		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, bma->idx - 1), &left);
+		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, *idx - 1), &left);
 		if (isnullstartblock(left.br_startblock))
 			state |= BMAP_LEFT_DELAY;
 	}
@@ -2923,9 +2926,9 @@ xfs_bmap_add_extent_hole_real(
 	 * Check and set flags if this segment has a current value.
 	 * Not true if we're inserting into the "hole" at eof.
 	 */
-	if (bma->idx < xfs_iext_count(ifp)) {
+	if (*idx < xfs_iext_count(ifp)) {
 		state |= BMAP_RIGHT_VALID;
-		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, bma->idx), &right);
+		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, *idx), &right);
 		if (isnullstartblock(right.br_startblock))
 			state |= BMAP_RIGHT_DELAY;
 	}
@@ -2962,36 +2965,36 @@ xfs_bmap_add_extent_hole_real(
 		 * left and on the right.
 		 * Merge all three into a single extent record.
 		 */
-		--bma->idx;
-		trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_);
-		xfs_bmbt_set_blockcount(xfs_iext_get_ext(ifp, bma->idx),
+		--*idx;
+		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
+		xfs_bmbt_set_blockcount(xfs_iext_get_ext(ifp, *idx),
 			left.br_blockcount + new->br_blockcount +
 			right.br_blockcount);
-		trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_);
+		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
 
-		xfs_iext_remove(bma->ip, bma->idx + 1, 1, state);
+		xfs_iext_remove(ip, *idx + 1, 1, state);
 
-		XFS_IFORK_NEXT_SET(bma->ip, whichfork,
-			XFS_IFORK_NEXTENTS(bma->ip, whichfork) - 1);
-		if (bma->cur == NULL) {
+		XFS_IFORK_NEXT_SET(ip, whichfork,
+			XFS_IFORK_NEXTENTS(ip, whichfork) - 1);
+		if (cur == NULL) {
 			rval = XFS_ILOG_CORE | xfs_ilog_fext(whichfork);
 		} else {
 			rval = XFS_ILOG_CORE;
-			error = xfs_bmbt_lookup_eq(bma->cur, right.br_startoff,
+			error = xfs_bmbt_lookup_eq(cur, right.br_startoff,
 					right.br_startblock, right.br_blockcount,
 					&i);
 			if (error)
 				goto done;
 			XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done);
-			error = xfs_btree_delete(bma->cur, &i);
+			error = xfs_btree_delete(cur, &i);
 			if (error)
 				goto done;
 			XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done);
-			error = xfs_btree_decrement(bma->cur, 0, &i);
+			error = xfs_btree_decrement(cur, 0, &i);
 			if (error)
 				goto done;
 			XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done);
-			error = xfs_bmbt_update(bma->cur, left.br_startoff,
+			error = xfs_bmbt_update(cur, left.br_startoff,
 					left.br_startblock,
 					left.br_blockcount +
 						new->br_blockcount +
@@ -3008,23 +3011,23 @@ xfs_bmap_add_extent_hole_real(
 		 * on the left.
 		 * Merge the new allocation with the left neighbor.
 		 */
-		--bma->idx;
-		trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_);
-		xfs_bmbt_set_blockcount(xfs_iext_get_ext(ifp, bma->idx),
+		--*idx;
+		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
+		xfs_bmbt_set_blockcount(xfs_iext_get_ext(ifp, *idx),
 			left.br_blockcount + new->br_blockcount);
-		trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_);
+		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
 
-		if (bma->cur == NULL) {
+		if (cur == NULL) {
 			rval = xfs_ilog_fext(whichfork);
 		} else {
 			rval = 0;
-			error = xfs_bmbt_lookup_eq(bma->cur, left.br_startoff,
+			error = xfs_bmbt_lookup_eq(cur, left.br_startoff,
 					left.br_startblock, left.br_blockcount,
 					&i);
 			if (error)
 				goto done;
 			XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done);
-			error = xfs_bmbt_update(bma->cur, left.br_startoff,
+			error = xfs_bmbt_update(cur, left.br_startoff,
 					left.br_startblock,
 					left.br_blockcount +
 						new->br_blockcount,
@@ -3040,25 +3043,25 @@ xfs_bmap_add_extent_hole_real(
 		 * on the right.
 		 * Merge the new allocation with the right neighbor.
 		 */
-		trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_);
-		xfs_bmbt_set_allf(xfs_iext_get_ext(ifp, bma->idx),
+		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
+		xfs_bmbt_set_allf(xfs_iext_get_ext(ifp, *idx),
 			new->br_startoff, new->br_startblock,
 			new->br_blockcount + right.br_blockcount,
 			right.br_state);
-		trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_);
+		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
 
-		if (bma->cur == NULL) {
+		if (cur == NULL) {
 			rval = xfs_ilog_fext(whichfork);
 		} else {
 			rval = 0;
-			error = xfs_bmbt_lookup_eq(bma->cur,
+			error = xfs_bmbt_lookup_eq(cur,
 					right.br_startoff,
 					right.br_startblock,
 					right.br_blockcount, &i);
 			if (error)
 				goto done;
 			XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done);
-			error = xfs_bmbt_update(bma->cur, new->br_startoff,
+			error = xfs_bmbt_update(cur, new->br_startoff,
 					new->br_startblock,
 					new->br_blockcount +
 						right.br_blockcount,
@@ -3074,22 +3077,22 @@ xfs_bmap_add_extent_hole_real(
 		 * real allocation.
 		 * Insert a new entry.
 		 */
-		xfs_iext_insert(bma->ip, bma->idx, 1, new, state);
-		XFS_IFORK_NEXT_SET(bma->ip, whichfork,
-			XFS_IFORK_NEXTENTS(bma->ip, whichfork) + 1);
-		if (bma->cur == NULL) {
+		xfs_iext_insert(ip, *idx, 1, new, state);
+		XFS_IFORK_NEXT_SET(ip, whichfork,
+			XFS_IFORK_NEXTENTS(ip, whichfork) + 1);
+		if (cur == NULL) {
 			rval = XFS_ILOG_CORE | xfs_ilog_fext(whichfork);
 		} else {
 			rval = XFS_ILOG_CORE;
-			error = xfs_bmbt_lookup_eq(bma->cur,
+			error = xfs_bmbt_lookup_eq(cur,
 					new->br_startoff,
 					new->br_startblock,
 					new->br_blockcount, &i);
 			if (error)
 				goto done;
 			XFS_WANT_CORRUPTED_GOTO(mp, i == 0, done);
-			bma->cur->bc_rec.b.br_state = new->br_state;
-			error = xfs_btree_insert(bma->cur, &i);
+			cur->bc_rec.b.br_state = new->br_state;
+			error = xfs_btree_insert(cur, &i);
 			if (error)
 				goto done;
 			XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done);
@@ -3098,30 +3101,30 @@ xfs_bmap_add_extent_hole_real(
 	}
 
 	/* add reverse mapping */
-	error = xfs_rmap_map_extent(mp, bma->dfops, bma->ip, whichfork, new);
+	error = xfs_rmap_map_extent(mp, dfops, ip, whichfork, new);
 	if (error)
 		goto done;
 
 	/* convert to a btree if necessary */
-	if (xfs_bmap_needs_btree(bma->ip, whichfork)) {
+	if (xfs_bmap_needs_btree(ip, whichfork)) {
 		int	tmp_logflags;	/* partial log flag return val */
 
-		ASSERT(bma->cur == NULL);
-		error = xfs_bmap_extents_to_btree(bma->tp, bma->ip,
-				bma->firstblock, bma->dfops, &bma->cur,
+		ASSERT(cur == NULL);
+		error = xfs_bmap_extents_to_btree(tp, ip, first, dfops, curp,
 				0, &tmp_logflags, whichfork);
-		bma->logflags |= tmp_logflags;
+		*logflagsp |= tmp_logflags;
+		cur = *curp;
 		if (error)
 			goto done;
 	}
 
 	/* clear out the allocated field, done with it now in any case. */
-	if (bma->cur)
-		bma->cur->bc_private.b.allocated = 0;
+	if (cur)
+		cur->bc_private.b.allocated = 0;
 
-	xfs_bmap_check_leaf_extents(bma->cur, bma->ip, whichfork);
+	xfs_bmap_check_leaf_extents(cur, ip, whichfork);
 done:
-	bma->logflags |= rval;
+	*logflagsp |= rval;
 	return error;
 }
 
@@ -4386,7 +4389,9 @@ xfs_bmapi_allocate(
 	if (bma->wasdel)
 		error = xfs_bmap_add_extent_delay_real(bma, whichfork);
 	else
-		error = xfs_bmap_add_extent_hole_real(bma, whichfork);
+		error = xfs_bmap_add_extent_hole_real(bma->tp, bma->ip,
+				whichfork, &bma->idx, &bma->cur, &bma->got,
+				bma->firstblock, bma->dfops, &bma->logflags);
 
 	bma->logflags |= tmp_logflags;
 	if (error)
-- 
2.11.0

[PATCH 4/6] xfs: introduce xfs_bmapi_remap

[Christoph Hellwig]

Add a new helper to be used for reflink extent list additions instead of
funneling them through xfs_bmapi_write and overloading the firstblock
member in struct xfs_bmalloca and struct xfs_alloc_args.

With some small changes to xfs_bmap_remap_alloc this also means we do
not need a xfs_bmalloca structure for this case at all.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c | 152 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 107 insertions(+), 45 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index d404dee7ede4..caadfe27af39 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3862,11 +3862,12 @@ xfs_bmap_btalloc(
  */
 STATIC int
 xfs_bmap_remap_alloc(
-	struct xfs_bmalloca	*ap)
+	struct xfs_trans	*tp,
+	struct xfs_inode	*ip,
+	xfs_fsblock_t		startblock,
+	xfs_extlen_t		length)
 {
-	struct xfs_trans	*tp = ap->tp;
 	struct xfs_mount	*mp = tp->t_mountp;
-	xfs_fsblock_t		bno;
 	struct xfs_alloc_arg	args;
 	int			error;
 
@@ -3875,24 +3876,24 @@ xfs_bmap_remap_alloc(
 	 * and handle a silent filesystem corruption rather than crashing.
 	 */
 	memset(&args, 0, sizeof(struct xfs_alloc_arg));
-	args.tp = ap->tp;
-	args.mp = ap->tp->t_mountp;
-	bno = *ap->firstblock;
-	args.agno = XFS_FSB_TO_AGNO(mp, bno);
-	args.agbno = XFS_FSB_TO_AGBNO(mp, bno);
+	args.tp = tp;
+	args.mp = mp;
+	args.agno = XFS_FSB_TO_AGNO(mp, startblock);
+	args.agbno = XFS_FSB_TO_AGBNO(mp, startblock);
+
 	if (args.agno >= mp->m_sb.sb_agcount ||
 	    args.agbno >= mp->m_sb.sb_agblocks)
 		return -EFSCORRUPTED;
 
 	/* "Allocate" the extent from the range we passed in. */
-	trace_xfs_bmap_remap_alloc(ap->ip, *ap->firstblock, ap->length);
-	ap->blkno = bno;
-	ap->ip->i_d.di_nblocks += ap->length;
-	xfs_trans_log_inode(ap->tp, ap->ip, XFS_ILOG_CORE);
+	trace_xfs_bmap_remap_alloc(ip, startblock, length);
+
+	ip->i_d.di_nblocks += length;
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 
 	/* Fix the freelist, like a real allocator does. */
-	args.datatype = ap->datatype;
-	args.pag = xfs_perag_get(args.mp, args.agno);
+	args.datatype = XFS_ALLOC_USERDATA | XFS_ALLOC_NOBUSY;
+	args.pag = xfs_perag_get(mp, args.agno);
 	ASSERT(args.pag);
 
 	/*
@@ -3905,7 +3906,7 @@ xfs_bmap_remap_alloc(
 	error = xfs_alloc_fix_freelist(&args, XFS_ALLOC_FLAG_FREEING);
 	xfs_perag_put(args.pag);
 	if (error)
-		trace_xfs_bmap_remap_alloc_error(ap->ip, error, _RET_IP_);
+		trace_xfs_bmap_remap_alloc_error(ip, error, _RET_IP_);
 	return error;
 }
 
@@ -3917,8 +3918,6 @@ STATIC int
 xfs_bmap_alloc(
 	struct xfs_bmalloca	*ap)	/* bmap alloc argument struct */
 {
-	if (ap->flags & XFS_BMAPI_REMAP)
-		return xfs_bmap_remap_alloc(ap);
 	if (XFS_IS_REALTIME_INODE(ap->ip) &&
 	    xfs_alloc_is_userdata(ap->datatype))
 		return xfs_bmap_rtalloc(ap);
@@ -4554,9 +4553,7 @@ xfs_bmapi_write(
 	ASSERT(len > 0);
 	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL);
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
-	ASSERT(!(flags & XFS_BMAPI_REMAP) || whichfork == XFS_DATA_FORK);
-	ASSERT(!(flags & XFS_BMAPI_PREALLOC) || !(flags & XFS_BMAPI_REMAP));
-	ASSERT(!(flags & XFS_BMAPI_CONVERT) || !(flags & XFS_BMAPI_REMAP));
+	ASSERT(!(flags & XFS_BMAPI_REMAP));
 
 	/* zeroing is for currently only for data extents, not metadata */
 	ASSERT((flags & (XFS_BMAPI_METADATA | XFS_BMAPI_ZERO)) !=
@@ -4640,13 +4637,8 @@ xfs_bmapi_write(
 			} else {
 				need_alloc = true;
 			}
-		} else {
-			/*
-			 * Make sure we only reflink into a hole.
-			 */
-			ASSERT(!(flags & XFS_BMAPI_REMAP));
-			if (isnullstartblock(bma.got.br_startblock))
-				wasdelay = true;
+		} else if (isnullstartblock(bma.got.br_startblock)) {
+			wasdelay = true;
 		}
 
 		/*
@@ -4775,6 +4767,89 @@ xfs_bmapi_write(
 	return error;
 }
 
+static int
+xfs_bmapi_remap(
+	struct xfs_trans	*tp,
+	struct xfs_inode	*ip,
+	xfs_fileoff_t		bno,
+	xfs_filblks_t		len,
+	xfs_fsblock_t		startblock,
+	struct xfs_defer_ops	*dfops)
+{
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
+	struct xfs_btree_cur	*cur = NULL;
+	xfs_fsblock_t		firstblock = NULLFSBLOCK;
+	struct xfs_bmbt_irec	got;
+	xfs_extnum_t		idx;
+	int			logflags = 0, error;
+
+	ASSERT(len > 0);
+	ASSERT(len <= (xfs_filblks_t)MAXEXTLEN);
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(ifp->if_flags & XFS_IFEXTENTS);
+
+	if (unlikely(XFS_TEST_ERROR(
+	    (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_EXTENTS &&
+	     XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE),
+	     mp, XFS_ERRTAG_BMAPIFORMAT, XFS_RANDOM_BMAPIFORMAT))) {
+		XFS_ERROR_REPORT("xfs_bmapi_remap", XFS_ERRLEVEL_LOW, mp);
+		return -EFSCORRUPTED;
+	}
+
+	if (XFS_FORCED_SHUTDOWN(mp))
+		return -EIO;
+
+	if (xfs_iext_lookup_extent(ip, ifp, bno, &idx, &got)) {
+		/* make sure we only reflink into a hole. */
+		ASSERT(got.br_startoff > bno);
+		ASSERT(got.br_startoff - bno >= len);
+	}
+
+	error = xfs_bmap_remap_alloc(tp, ip, startblock, len);
+	if (error)
+		goto error0;
+
+	if (ifp->if_flags & XFS_IFBROOT) {
+		cur = xfs_bmbt_init_cursor(mp, tp, ip, XFS_DATA_FORK);
+		cur->bc_private.b.firstblock = firstblock;
+		cur->bc_private.b.dfops = dfops;
+		cur->bc_private.b.flags = 0;
+	}
+
+	got.br_startoff = bno;
+	got.br_startblock = startblock;
+	got.br_blockcount = len;
+	got.br_state = XFS_EXT_NORM;
+
+	error = xfs_bmap_add_extent_hole_real(tp, ip, XFS_DATA_FORK, &idx, &cur,
+			&got, &firstblock, dfops, &logflags);
+	if (error)
+		goto error0;
+
+	if (xfs_bmap_wants_extents(ip, XFS_DATA_FORK)) {
+		int		tmp_logflags = 0;
+
+		error = xfs_bmap_btree_to_extents(tp, ip, cur,
+			&tmp_logflags, XFS_DATA_FORK);
+		logflags |= tmp_logflags;
+	}
+
+error0:
+	if (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS)
+		logflags &= ~XFS_ILOG_DEXT;
+	else if (ip->i_d.di_format != XFS_DINODE_FMT_BTREE)
+		logflags &= ~XFS_ILOG_DBROOT;
+
+	if (logflags)
+		xfs_trans_log_inode(tp, ip, logflags);
+	if (cur) {
+		xfs_btree_del_cursor(cur,
+			error ? XFS_BTREE_ERROR : XFS_BTREE_NOERROR);
+	}
+	return error;
+}
+
 /*
  * 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
@@ -6493,16 +6568,7 @@ xfs_bmap_finish_one(
 	xfs_filblks_t			blockcount,
 	xfs_exntst_t			state)
 {
-	struct xfs_bmbt_irec		bmap;
-	int				nimaps = 1;
-	xfs_fsblock_t			firstfsb;
-	int				done;
-	int				error = 0;
-
-	bmap.br_startblock = startblock;
-	bmap.br_startoff = startoff;
-	bmap.br_blockcount = blockcount;
-	bmap.br_state = state;
+	int				error = 0, done;
 
 	trace_xfs_bmap_deferred(tp->t_mountp,
 			XFS_FSB_TO_AGNO(tp->t_mountp, startblock), type,
@@ -6519,16 +6585,12 @@ xfs_bmap_finish_one(
 
 	switch (type) {
 	case XFS_BMAP_MAP:
-		firstfsb = bmap.br_startblock;
-		error = xfs_bmapi_write(tp, ip, bmap.br_startoff,
-					bmap.br_blockcount, XFS_BMAPI_REMAP, &firstfsb,
-					bmap.br_blockcount, &bmap, &nimaps,
-					dfops);
+		error = xfs_bmapi_remap(tp, ip, startoff, blockcount,
+				startblock, dfops);
 		break;
 	case XFS_BMAP_UNMAP:
-		error = xfs_bunmapi(tp, ip, bmap.br_startoff,
-				bmap.br_blockcount, XFS_BMAPI_REMAP, 1, &firstfsb,
-				dfops, &done);
+		error = xfs_bunmapi(tp, ip, startoff, blockcount,
+				XFS_BMAPI_REMAP, 1, &startblock, dfops, &done);
 		ASSERT(done);
 		break;
 	default:
-- 
2.11.0

[PATCH 5/6] xfs: remove xfs_bmap_remap_alloc

[Christoph Hellwig]

The main thing that xfs_bmap_remap_alloc does is fixing the AGFL, similar
to what we do in the space allocator.  But the reflink code doesn't touch
the allocation btree unlike the normal space allocator, so we couldn't
care less about the state of the AGFL.

So remove xfs_bmap_remap_alloc and just handle the di_nblocks update in
the caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c | 60 ++----------------------------------------------
 fs/xfs/xfs_trace.h       | 25 --------------------
 2 files changed, 2 insertions(+), 83 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index caadfe27af39..0035f96a6e5a 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3856,61 +3856,6 @@ xfs_bmap_btalloc(
 }
 
 /*
- * For a remap operation, just "allocate" an extent at the address that the
- * caller passed in, and ensure that the AGFL is the right size.  The caller
- * will then map the "allocated" extent into the file somewhere.
- */
-STATIC int
-xfs_bmap_remap_alloc(
-	struct xfs_trans	*tp,
-	struct xfs_inode	*ip,
-	xfs_fsblock_t		startblock,
-	xfs_extlen_t		length)
-{
-	struct xfs_mount	*mp = tp->t_mountp;
-	struct xfs_alloc_arg	args;
-	int			error;
-
-	/*
-	 * validate that the block number is legal - the enables us to detect
-	 * and handle a silent filesystem corruption rather than crashing.
-	 */
-	memset(&args, 0, sizeof(struct xfs_alloc_arg));
-	args.tp = tp;
-	args.mp = mp;
-	args.agno = XFS_FSB_TO_AGNO(mp, startblock);
-	args.agbno = XFS_FSB_TO_AGBNO(mp, startblock);
-
-	if (args.agno >= mp->m_sb.sb_agcount ||
-	    args.agbno >= mp->m_sb.sb_agblocks)
-		return -EFSCORRUPTED;
-
-	/* "Allocate" the extent from the range we passed in. */
-	trace_xfs_bmap_remap_alloc(ip, startblock, length);
-
-	ip->i_d.di_nblocks += length;
-	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
-
-	/* Fix the freelist, like a real allocator does. */
-	args.datatype = XFS_ALLOC_USERDATA | XFS_ALLOC_NOBUSY;
-	args.pag = xfs_perag_get(mp, args.agno);
-	ASSERT(args.pag);
-
-	/*
-	 * The freelist fixing code will decline the allocation if
-	 * the size and shape of the free space doesn't allow for
-	 * allocating the extent and updating all the metadata that
-	 * happens during an allocation.  We're remapping, not
-	 * allocating, so skip that check by pretending to be freeing.
-	 */
-	error = xfs_alloc_fix_freelist(&args, XFS_ALLOC_FLAG_FREEING);
-	xfs_perag_put(args.pag);
-	if (error)
-		trace_xfs_bmap_remap_alloc_error(ip, error, _RET_IP_);
-	return error;
-}
-
-/*
  * xfs_bmap_alloc is called by xfs_bmapi to allocate an extent for a file.
  * It figures out where to ask the underlying allocator to put the new extent.
  */
@@ -4806,9 +4751,8 @@ xfs_bmapi_remap(
 		ASSERT(got.br_startoff - bno >= len);
 	}
 
-	error = xfs_bmap_remap_alloc(tp, ip, startblock, len);
-	if (error)
-		goto error0;
+	ip->i_d.di_nblocks += len;
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 
 	if (ifp->if_flags & XFS_IFBROOT) {
 		cur = xfs_bmbt_init_cursor(mp, tp, ip, XFS_DATA_FORK);
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 4f96dc953fbe..cba10daf8391 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -3003,31 +3003,6 @@ DEFINE_EVENT(xfs_inode_error_class, name, \
 		 unsigned long caller_ip), \
 	TP_ARGS(ip, error, caller_ip))
 
-/* reflink allocator */
-TRACE_EVENT(xfs_bmap_remap_alloc,
-	TP_PROTO(struct xfs_inode *ip, xfs_fsblock_t fsbno,
-		 xfs_extlen_t len),
-	TP_ARGS(ip, fsbno, len),
-	TP_STRUCT__entry(
-		__field(dev_t, dev)
-		__field(xfs_ino_t, ino)
-		__field(xfs_fsblock_t, fsbno)
-		__field(xfs_extlen_t, len)
-	),
-	TP_fast_assign(
-		__entry->dev = VFS_I(ip)->i_sb->s_dev;
-		__entry->ino = ip->i_ino;
-		__entry->fsbno = fsbno;
-		__entry->len = len;
-	),
-	TP_printk("dev %d:%d ino 0x%llx fsbno 0x%llx len %x",
-		  MAJOR(__entry->dev), MINOR(__entry->dev),
-		  __entry->ino,
-		  __entry->fsbno,
-		  __entry->len)
-);
-DEFINE_INODE_ERROR_EVENT(xfs_bmap_remap_alloc_error);
-
 /* reflink tracepoint classes */
 
 /* two-file io tracepoint class */
-- 
2.11.0

[PATCH 6/6] xfs: remove bmap block allocation retries

[Christoph Hellwig]

Now that reflink operations don't set the firstblock value we don't
need the workarounds for non-NULL firstblock values without a prior
allocation.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c       | 29 -----------------------------
 fs/xfs/libxfs/xfs_bmap_btree.c | 17 -----------------
 2 files changed, 46 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 0035f96a6e5a..c66e9c25e3c2 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -764,7 +764,6 @@ xfs_bmap_extents_to_btree(
 		args.fsbno = XFS_INO_TO_FSB(mp, ip->i_ino);
 	} else if (dfops->dop_low) {
 		args.type = XFS_ALLOCTYPE_START_BNO;
-try_another_ag:
 		args.fsbno = *firstblock;
 	} else {
 		args.type = XFS_ALLOCTYPE_NEAR_BNO;
@@ -779,20 +778,6 @@ xfs_bmap_extents_to_btree(
 		return error;
 	}
 
-	/*
-	 * During a CoW operation, the allocation and bmbt updates occur in
-	 * different transactions.  The mapping code tries to put new bmbt
-	 * blocks near extents being mapped, but the only way to guarantee this
-	 * is if the alloc and the mapping happen in a single transaction that
-	 * has a block reservation.  That isn't the case here, so if we run out
-	 * of space we'll try again with another AG.
-	 */
-	if (xfs_sb_version_hasreflink(&cur->bc_mp->m_sb) &&
-	    args.fsbno == NULLFSBLOCK &&
-	    args.type == XFS_ALLOCTYPE_NEAR_BNO) {
-		args.type = XFS_ALLOCTYPE_FIRST_AG;
-		goto try_another_ag;
-	}
 	if (WARN_ON_ONCE(args.fsbno == NULLFSBLOCK)) {
 		xfs_iroot_realloc(ip, -1, whichfork);
 		xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
@@ -925,7 +910,6 @@ xfs_bmap_local_to_extents(
 	 * file currently fits in an inode.
 	 */
 	if (*firstblock == NULLFSBLOCK) {
-try_another_ag:
 		args.fsbno = XFS_INO_TO_FSB(args.mp, ip->i_ino);
 		args.type = XFS_ALLOCTYPE_START_BNO;
 	} else {
@@ -938,19 +922,6 @@ xfs_bmap_local_to_extents(
 	if (error)
 		goto done;
 
-	/*
-	 * During a CoW operation, the allocation and bmbt updates occur in
-	 * different transactions.  The mapping code tries to put new bmbt
-	 * blocks near extents being mapped, but the only way to guarantee this
-	 * is if the alloc and the mapping happen in a single transaction that
-	 * has a block reservation.  That isn't the case here, so if we run out
-	 * of space we'll try again with another AG.
-	 */
-	if (xfs_sb_version_hasreflink(&ip->i_mount->m_sb) &&
-	    args.fsbno == NULLFSBLOCK &&
-	    args.type == XFS_ALLOCTYPE_NEAR_BNO) {
-		goto try_another_ag;
-	}
 	/* Can't fail, the space was reserved. */
 	ASSERT(args.fsbno != NULLFSBLOCK);
 	ASSERT(args.len == 1);
diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
index fd55db479385..3e17ceda038c 100644
--- a/fs/xfs/libxfs/xfs_bmap_btree.c
+++ b/fs/xfs/libxfs/xfs_bmap_btree.c
@@ -448,7 +448,6 @@ xfs_bmbt_alloc_block(
 	if (args.fsbno == NULLFSBLOCK) {
 		args.fsbno = be64_to_cpu(start->l);
 		args.type = XFS_ALLOCTYPE_START_BNO;
-try_another_ag:
 		/*
 		 * Make sure there is sufficient room left in the AG to
 		 * complete a full tree split for an extent insert.  If
@@ -477,22 +476,6 @@ xfs_bmbt_alloc_block(
 	if (error)
 		goto error0;
 
-	/*
-	 * During a CoW operation, the allocation and bmbt updates occur in
-	 * different transactions.  The mapping code tries to put new bmbt
-	 * blocks near extents being mapped, but the only way to guarantee this
-	 * is if the alloc and the mapping happen in a single transaction that
-	 * has a block reservation.  That isn't the case here, so if we run out
-	 * of space we'll try again with another AG.
-	 */
-	if (xfs_sb_version_hasreflink(&cur->bc_mp->m_sb) &&
-	    args.fsbno == NULLFSBLOCK &&
-	    args.type == XFS_ALLOCTYPE_NEAR_BNO) {
-		args.fsbno = cur->bc_private.b.firstblock;
-		args.type = XFS_ALLOCTYPE_FIRST_AG;
-		goto try_another_ag;
-	}
-
 	if (args.fsbno == NULLFSBLOCK && args.minleft) {
 		/*
 		 * Could not find an AG with enough free space to satisfy
-- 
2.11.0

Re: [PATCH 5/6] xfs: remove xfs_bmap_remap_alloc

[Darrick J. Wong]

On Mon, Apr 03, 2017 at 02:18:32PM +0200, Christoph Hellwig wrote:
> The main thing that xfs_bmap_remap_alloc does is fixing the AGFL, similar
> to what we do in the space allocator.  But the reflink code doesn't touch
> the allocation btree unlike the normal space allocator, so we couldn't
> care less about the state of the AGFL.
> 
> So remove xfs_bmap_remap_alloc and just handle the di_nblocks update in
> the caller.

Looking at my notes, _bmap_remap_alloc was modified to fix the freelist
to avoid running out of AGFL blocks and crashing on a rmapbt split.
Nowadays _rmap_finish_one should take care of that before any deferred
rmap changes, so I think we're covered.

I /think/ this looks ok, but I'll reread the series after lunch.

--D

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 60 ++----------------------------------------------
>  fs/xfs/xfs_trace.h       | 25 --------------------
>  2 files changed, 2 insertions(+), 83 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index caadfe27af39..0035f96a6e5a 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3856,61 +3856,6 @@ xfs_bmap_btalloc(
>  }
>  
>  /*
> - * For a remap operation, just "allocate" an extent at the address that the
> - * caller passed in, and ensure that the AGFL is the right size.  The caller
> - * will then map the "allocated" extent into the file somewhere.
> - */
> -STATIC int
> -xfs_bmap_remap_alloc(
> -	struct xfs_trans	*tp,
> -	struct xfs_inode	*ip,
> -	xfs_fsblock_t		startblock,
> -	xfs_extlen_t		length)
> -{
> -	struct xfs_mount	*mp = tp->t_mountp;
> -	struct xfs_alloc_arg	args;
> -	int			error;
> -
> -	/*
> -	 * validate that the block number is legal - the enables us to detect
> -	 * and handle a silent filesystem corruption rather than crashing.
> -	 */
> -	memset(&args, 0, sizeof(struct xfs_alloc_arg));
> -	args.tp = tp;
> -	args.mp = mp;
> -	args.agno = XFS_FSB_TO_AGNO(mp, startblock);
> -	args.agbno = XFS_FSB_TO_AGBNO(mp, startblock);
> -
> -	if (args.agno >= mp->m_sb.sb_agcount ||
> -	    args.agbno >= mp->m_sb.sb_agblocks)
> -		return -EFSCORRUPTED;
> -
> -	/* "Allocate" the extent from the range we passed in. */
> -	trace_xfs_bmap_remap_alloc(ip, startblock, length);
> -
> -	ip->i_d.di_nblocks += length;
> -	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> -
> -	/* Fix the freelist, like a real allocator does. */
> -	args.datatype = XFS_ALLOC_USERDATA | XFS_ALLOC_NOBUSY;
> -	args.pag = xfs_perag_get(mp, args.agno);
> -	ASSERT(args.pag);
> -
> -	/*
> -	 * The freelist fixing code will decline the allocation if
> -	 * the size and shape of the free space doesn't allow for
> -	 * allocating the extent and updating all the metadata that
> -	 * happens during an allocation.  We're remapping, not
> -	 * allocating, so skip that check by pretending to be freeing.
> -	 */
> -	error = xfs_alloc_fix_freelist(&args, XFS_ALLOC_FLAG_FREEING);
> -	xfs_perag_put(args.pag);
> -	if (error)
> -		trace_xfs_bmap_remap_alloc_error(ip, error, _RET_IP_);
> -	return error;
> -}
> -
> -/*
>   * xfs_bmap_alloc is called by xfs_bmapi to allocate an extent for a file.
>   * It figures out where to ask the underlying allocator to put the new extent.
>   */
> @@ -4806,9 +4751,8 @@ xfs_bmapi_remap(
>  		ASSERT(got.br_startoff - bno >= len);
>  	}
>  
> -	error = xfs_bmap_remap_alloc(tp, ip, startblock, len);
> -	if (error)
> -		goto error0;
> +	ip->i_d.di_nblocks += len;
> +	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>  
>  	if (ifp->if_flags & XFS_IFBROOT) {
>  		cur = xfs_bmbt_init_cursor(mp, tp, ip, XFS_DATA_FORK);
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 4f96dc953fbe..cba10daf8391 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -3003,31 +3003,6 @@ DEFINE_EVENT(xfs_inode_error_class, name, \
>  		 unsigned long caller_ip), \
>  	TP_ARGS(ip, error, caller_ip))
>  
> -/* reflink allocator */
> -TRACE_EVENT(xfs_bmap_remap_alloc,
> -	TP_PROTO(struct xfs_inode *ip, xfs_fsblock_t fsbno,
> -		 xfs_extlen_t len),
> -	TP_ARGS(ip, fsbno, len),
> -	TP_STRUCT__entry(
> -		__field(dev_t, dev)
> -		__field(xfs_ino_t, ino)
> -		__field(xfs_fsblock_t, fsbno)
> -		__field(xfs_extlen_t, len)
> -	),
> -	TP_fast_assign(
> -		__entry->dev = VFS_I(ip)->i_sb->s_dev;
> -		__entry->ino = ip->i_ino;
> -		__entry->fsbno = fsbno;
> -		__entry->len = len;
> -	),
> -	TP_printk("dev %d:%d ino 0x%llx fsbno 0x%llx len %x",
> -		  MAJOR(__entry->dev), MINOR(__entry->dev),
> -		  __entry->ino,
> -		  __entry->fsbno,
> -		  __entry->len)
> -);
> -DEFINE_INODE_ERROR_EVENT(xfs_bmap_remap_alloc_error);
> -
>  /* reflink tracepoint classes */
>  
>  /* two-file io tracepoint class */
> -- 
> 2.11.0
> 
> --
> 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: split the reflink remap from the block allocation path

[Christoph Hellwig]

Any chance to get some reviews on this series?

On Mon, Apr 03, 2017 at 02:18:27PM +0200, Christoph Hellwig wrote:
> We've run into various problems due to the fact that the reflink remap
> code reuses the xfs_bmapi_write codepath for setting up the extent list
> entries and abuses the firstblock field for that purpose.
> 
> This series fixes this by creating an entirely separate xfs_bmapi_remap
> path that is much simpler than xfs_bmapi_write and does not need to
> overload the firstblock field.
> --
> 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
---end quoted text---

Re: [PATCH 1/6] xfs: fix integer truncation in xfs_bmap_remap_alloc

[Darrick J. Wong]

On Mon, Apr 03, 2017 at 02:18:28PM +0200, Christoph Hellwig wrote:
> bno should be a xfs_fsblock_t, which is 64-bit wides instead of a
> xfs_aglock_t, which truncates the value to 32 bits.

Looks good,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 9bd104f32908..2a426d127e05 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3863,7 +3863,7 @@ xfs_bmap_remap_alloc(
>  {
>  	struct xfs_trans	*tp = ap->tp;
>  	struct xfs_mount	*mp = tp->t_mountp;
> -	xfs_agblock_t		bno;
> +	xfs_fsblock_t		bno;
>  	struct xfs_alloc_arg	args;
>  	int			error;
>  
> -- 
> 2.11.0
> 
> --
> 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/6] xfs: remove attr fork handling in xfs_bmap_finish_one

[Darrick J. Wong]

On Mon, Apr 03, 2017 at 02:18:29PM +0200, Christoph Hellwig wrote:
> We never do COW operations for the attr fork, so don't pretend we handle
> them.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 2a426d127e05..47b909c27bb3 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -6491,7 +6491,6 @@ xfs_bmap_finish_one(
>  	struct xfs_bmbt_irec		bmap;
>  	int				nimaps = 1;
>  	xfs_fsblock_t			firstfsb;
> -	int				flags = XFS_BMAPI_REMAP;
>  	int				done;
>  	int				error = 0;
>  
> @@ -6505,10 +6504,8 @@ xfs_bmap_finish_one(
>  			XFS_FSB_TO_AGBNO(tp->t_mountp, startblock),
>  			ip->i_ino, whichfork, startoff, blockcount, state);
>  
> -	if (whichfork != XFS_DATA_FORK && whichfork != XFS_ATTR_FORK)
> +	if (whichfork != XFS_DATA_FORK)
>  		return -EFSCORRUPTED;
> -	if (whichfork == XFS_ATTR_FORK)
> -		flags |= XFS_BMAPI_ATTRFORK;

Hmmmm.  We never schedule deferred bmap operations for the attr fork,
but all that stuff got plumbed all the way through to the bmap log item
that is written to disk.  Therefore, it's possible that we may some day
replay a log from a future XFS with a deferred attr fork bmap item in
the log, at which point log recovery blows up here.

I wonder if this might be worth a WARN_ON_ONCE just to make it obvious
that recovery stopped because it doesn't handle deferred attr fork bmap
updates?

--D

>  	if (XFS_TEST_ERROR(false, tp->t_mountp,
>  			XFS_ERRTAG_BMAP_FINISH_ONE,
> @@ -6519,13 +6516,13 @@ xfs_bmap_finish_one(
>  	case XFS_BMAP_MAP:
>  		firstfsb = bmap.br_startblock;
>  		error = xfs_bmapi_write(tp, ip, bmap.br_startoff,
> -					bmap.br_blockcount, flags, &firstfsb,
> +					bmap.br_blockcount, XFS_BMAPI_REMAP, &firstfsb,
>  					bmap.br_blockcount, &bmap, &nimaps,
>  					dfops);
>  		break;
>  	case XFS_BMAP_UNMAP:
>  		error = xfs_bunmapi(tp, ip, bmap.br_startoff,
> -				bmap.br_blockcount, flags, 1, &firstfsb,
> +				bmap.br_blockcount, XFS_BMAPI_REMAP, 1, &firstfsb,
>  				dfops, &done);
>  		ASSERT(done);
>  		break;
> -- 
> 2.11.0
> 
> --
> 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 3/6] xfs: pass individual arguments to xfs_bmap_add_extent_hole_real

[Darrick J. Wong]

On Mon, Apr 03, 2017 at 02:18:30PM +0200, Christoph Hellwig wrote:
> For the reflink case we'd much rather pass the required arguments than
> faking up a struct xfs_bmalloca.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 123 ++++++++++++++++++++++++-----------------------
>  1 file changed, 64 insertions(+), 59 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 47b909c27bb3..d404dee7ede4 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -2879,27 +2879,30 @@ xfs_bmap_add_extent_hole_delay(
>   */
>  STATIC int				/* error */
>  xfs_bmap_add_extent_hole_real(
> -	struct xfs_bmalloca	*bma,
> -	int			whichfork)
> +	struct xfs_trans	*tp,
> +	xfs_inode_t		*ip,	/* incore inode pointer */
> +	int			whichfork,
> +	xfs_extnum_t		*idx,	/* extent number to update/insert */
> +	xfs_btree_cur_t		**curp,	/* if *curp is null, not a btree */
> +	xfs_bmbt_irec_t		*new,	/* new data to add to file extents */

struct xfs_inode vs. xfs_inode_t ?

I was under the impression that we don't add comments to the function
arguments anymore, but TBH they don't bother me.

> +	xfs_fsblock_t		*first,	/* pointer to firstblock variable */
> +	struct xfs_defer_ops	*dfops,	/* list of extents to be freed */
> +	int			*logflagsp) /* inode logging flags */
>  {
> -	struct xfs_bmbt_irec	*new = &bma->got;
> +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct xfs_btree_cur	*cur = *curp;
>  	int			error;	/* error return value */
>  	int			i;	/* temp state */
> -	xfs_ifork_t		*ifp;	/* inode fork pointer */
>  	xfs_bmbt_irec_t		left;	/* left neighbor extent entry */
>  	xfs_bmbt_irec_t		right;	/* right neighbor extent entry */
>  	int			rval=0;	/* return value (logging flags) */
>  	int			state;	/* state bits, accessed thru macros */
> -	struct xfs_mount	*mp;
> -
> -	mp = bma->ip->i_mount;
> -	ifp = XFS_IFORK_PTR(bma->ip, whichfork);
>  
> -	ASSERT(bma->idx >= 0);
> -	ASSERT(bma->idx <= xfs_iext_count(ifp));
> +	ASSERT(*idx >= 0);
> +	ASSERT(*idx <= xfs_iext_count(ifp));
>  	ASSERT(!isnullstartblock(new->br_startblock));
> -	ASSERT(!bma->cur ||
> -	       !(bma->cur->bc_private.b.flags & XFS_BTCUR_BPRV_WASDEL));
> +	ASSERT(!cur || !(cur->bc_private.b.flags & XFS_BTCUR_BPRV_WASDEL));
>  
>  	XFS_STATS_INC(mp, xs_add_exlist);
>  
> @@ -2912,9 +2915,9 @@ xfs_bmap_add_extent_hole_real(
>  	/*
>  	 * Check and set flags if this segment has a left neighbor.
>  	 */
> -	if (bma->idx > 0) {
> +	if (*idx > 0) {
>  		state |= BMAP_LEFT_VALID;
> -		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, bma->idx - 1), &left);
> +		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, *idx - 1), &left);
>  		if (isnullstartblock(left.br_startblock))
>  			state |= BMAP_LEFT_DELAY;
>  	}
> @@ -2923,9 +2926,9 @@ xfs_bmap_add_extent_hole_real(
>  	 * Check and set flags if this segment has a current value.
>  	 * Not true if we're inserting into the "hole" at eof.
>  	 */
> -	if (bma->idx < xfs_iext_count(ifp)) {
> +	if (*idx < xfs_iext_count(ifp)) {
>  		state |= BMAP_RIGHT_VALID;
> -		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, bma->idx), &right);
> +		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, *idx), &right);
>  		if (isnullstartblock(right.br_startblock))
>  			state |= BMAP_RIGHT_DELAY;
>  	}
> @@ -2962,36 +2965,36 @@ xfs_bmap_add_extent_hole_real(
>  		 * left and on the right.
>  		 * Merge all three into a single extent record.
>  		 */
> -		--bma->idx;
> -		trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_);
> -		xfs_bmbt_set_blockcount(xfs_iext_get_ext(ifp, bma->idx),
> +		--*idx;

I'm pretty sure this is the same as --(*idx); but... ick.

--D

> +		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> +		xfs_bmbt_set_blockcount(xfs_iext_get_ext(ifp, *idx),
>  			left.br_blockcount + new->br_blockcount +
>  			right.br_blockcount);
> -		trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_);
> +		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
>  
> -		xfs_iext_remove(bma->ip, bma->idx + 1, 1, state);
> +		xfs_iext_remove(ip, *idx + 1, 1, state);
>  
> -		XFS_IFORK_NEXT_SET(bma->ip, whichfork,
> -			XFS_IFORK_NEXTENTS(bma->ip, whichfork) - 1);
> -		if (bma->cur == NULL) {
> +		XFS_IFORK_NEXT_SET(ip, whichfork,
> +			XFS_IFORK_NEXTENTS(ip, whichfork) - 1);
> +		if (cur == NULL) {
>  			rval = XFS_ILOG_CORE | xfs_ilog_fext(whichfork);
>  		} else {
>  			rval = XFS_ILOG_CORE;
> -			error = xfs_bmbt_lookup_eq(bma->cur, right.br_startoff,
> +			error = xfs_bmbt_lookup_eq(cur, right.br_startoff,
>  					right.br_startblock, right.br_blockcount,
>  					&i);
>  			if (error)
>  				goto done;
>  			XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done);
> -			error = xfs_btree_delete(bma->cur, &i);
> +			error = xfs_btree_delete(cur, &i);
>  			if (error)
>  				goto done;
>  			XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done);
> -			error = xfs_btree_decrement(bma->cur, 0, &i);
> +			error = xfs_btree_decrement(cur, 0, &i);
>  			if (error)
>  				goto done;
>  			XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done);
> -			error = xfs_bmbt_update(bma->cur, left.br_startoff,
> +			error = xfs_bmbt_update(cur, left.br_startoff,
>  					left.br_startblock,
>  					left.br_blockcount +
>  						new->br_blockcount +
> @@ -3008,23 +3011,23 @@ xfs_bmap_add_extent_hole_real(
>  		 * on the left.
>  		 * Merge the new allocation with the left neighbor.
>  		 */
> -		--bma->idx;
> -		trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_);
> -		xfs_bmbt_set_blockcount(xfs_iext_get_ext(ifp, bma->idx),
> +		--*idx;
> +		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> +		xfs_bmbt_set_blockcount(xfs_iext_get_ext(ifp, *idx),
>  			left.br_blockcount + new->br_blockcount);
> -		trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_);
> +		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
>  
> -		if (bma->cur == NULL) {
> +		if (cur == NULL) {
>  			rval = xfs_ilog_fext(whichfork);
>  		} else {
>  			rval = 0;
> -			error = xfs_bmbt_lookup_eq(bma->cur, left.br_startoff,
> +			error = xfs_bmbt_lookup_eq(cur, left.br_startoff,
>  					left.br_startblock, left.br_blockcount,
>  					&i);
>  			if (error)
>  				goto done;
>  			XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done);
> -			error = xfs_bmbt_update(bma->cur, left.br_startoff,
> +			error = xfs_bmbt_update(cur, left.br_startoff,
>  					left.br_startblock,
>  					left.br_blockcount +
>  						new->br_blockcount,
> @@ -3040,25 +3043,25 @@ xfs_bmap_add_extent_hole_real(
>  		 * on the right.
>  		 * Merge the new allocation with the right neighbor.
>  		 */
> -		trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_);
> -		xfs_bmbt_set_allf(xfs_iext_get_ext(ifp, bma->idx),
> +		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> +		xfs_bmbt_set_allf(xfs_iext_get_ext(ifp, *idx),
>  			new->br_startoff, new->br_startblock,
>  			new->br_blockcount + right.br_blockcount,
>  			right.br_state);
> -		trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_);
> +		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
>  
> -		if (bma->cur == NULL) {
> +		if (cur == NULL) {
>  			rval = xfs_ilog_fext(whichfork);
>  		} else {
>  			rval = 0;
> -			error = xfs_bmbt_lookup_eq(bma->cur,
> +			error = xfs_bmbt_lookup_eq(cur,
>  					right.br_startoff,
>  					right.br_startblock,
>  					right.br_blockcount, &i);
>  			if (error)
>  				goto done;
>  			XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done);
> -			error = xfs_bmbt_update(bma->cur, new->br_startoff,
> +			error = xfs_bmbt_update(cur, new->br_startoff,
>  					new->br_startblock,
>  					new->br_blockcount +
>  						right.br_blockcount,
> @@ -3074,22 +3077,22 @@ xfs_bmap_add_extent_hole_real(
>  		 * real allocation.
>  		 * Insert a new entry.
>  		 */
> -		xfs_iext_insert(bma->ip, bma->idx, 1, new, state);
> -		XFS_IFORK_NEXT_SET(bma->ip, whichfork,
> -			XFS_IFORK_NEXTENTS(bma->ip, whichfork) + 1);
> -		if (bma->cur == NULL) {
> +		xfs_iext_insert(ip, *idx, 1, new, state);
> +		XFS_IFORK_NEXT_SET(ip, whichfork,
> +			XFS_IFORK_NEXTENTS(ip, whichfork) + 1);
> +		if (cur == NULL) {
>  			rval = XFS_ILOG_CORE | xfs_ilog_fext(whichfork);
>  		} else {
>  			rval = XFS_ILOG_CORE;
> -			error = xfs_bmbt_lookup_eq(bma->cur,
> +			error = xfs_bmbt_lookup_eq(cur,
>  					new->br_startoff,
>  					new->br_startblock,
>  					new->br_blockcount, &i);
>  			if (error)
>  				goto done;
>  			XFS_WANT_CORRUPTED_GOTO(mp, i == 0, done);
> -			bma->cur->bc_rec.b.br_state = new->br_state;
> -			error = xfs_btree_insert(bma->cur, &i);
> +			cur->bc_rec.b.br_state = new->br_state;
> +			error = xfs_btree_insert(cur, &i);
>  			if (error)
>  				goto done;
>  			XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done);
> @@ -3098,30 +3101,30 @@ xfs_bmap_add_extent_hole_real(
>  	}
>  
>  	/* add reverse mapping */
> -	error = xfs_rmap_map_extent(mp, bma->dfops, bma->ip, whichfork, new);
> +	error = xfs_rmap_map_extent(mp, dfops, ip, whichfork, new);
>  	if (error)
>  		goto done;
>  
>  	/* convert to a btree if necessary */
> -	if (xfs_bmap_needs_btree(bma->ip, whichfork)) {
> +	if (xfs_bmap_needs_btree(ip, whichfork)) {
>  		int	tmp_logflags;	/* partial log flag return val */
>  
> -		ASSERT(bma->cur == NULL);
> -		error = xfs_bmap_extents_to_btree(bma->tp, bma->ip,
> -				bma->firstblock, bma->dfops, &bma->cur,
> +		ASSERT(cur == NULL);
> +		error = xfs_bmap_extents_to_btree(tp, ip, first, dfops, curp,
>  				0, &tmp_logflags, whichfork);
> -		bma->logflags |= tmp_logflags;
> +		*logflagsp |= tmp_logflags;
> +		cur = *curp;
>  		if (error)
>  			goto done;
>  	}
>  
>  	/* clear out the allocated field, done with it now in any case. */
> -	if (bma->cur)
> -		bma->cur->bc_private.b.allocated = 0;
> +	if (cur)
> +		cur->bc_private.b.allocated = 0;
>  
> -	xfs_bmap_check_leaf_extents(bma->cur, bma->ip, whichfork);
> +	xfs_bmap_check_leaf_extents(cur, ip, whichfork);
>  done:
> -	bma->logflags |= rval;
> +	*logflagsp |= rval;
>  	return error;
>  }
>  
> @@ -4386,7 +4389,9 @@ xfs_bmapi_allocate(
>  	if (bma->wasdel)
>  		error = xfs_bmap_add_extent_delay_real(bma, whichfork);
>  	else
> -		error = xfs_bmap_add_extent_hole_real(bma, whichfork);
> +		error = xfs_bmap_add_extent_hole_real(bma->tp, bma->ip,
> +				whichfork, &bma->idx, &bma->cur, &bma->got,
> +				bma->firstblock, bma->dfops, &bma->logflags);
>  
>  	bma->logflags |= tmp_logflags;
>  	if (error)
> -- 
> 2.11.0
> 
> --
> 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/6] xfs: remove attr fork handling in xfs_bmap_finish_one

[Christoph Hellwig]

On Mon, Apr 10, 2017 at 10:05:39AM -0700, Darrick J. Wong wrote:
> Hmmmm.  We never schedule deferred bmap operations for the attr fork,
> but all that stuff got plumbed all the way through to the bmap log item
> that is written to disk.  Therefore, it's possible that we may some day
> replay a log from a future XFS with a deferred attr fork bmap item in
> the log, at which point log recovery blows up here.

If we add that it's a format change that needs a incompat feature
flag.

> I wonder if this might be worth a WARN_ON_ONCE just to make it obvious
> that recovery stopped because it doesn't handle deferred attr fork bmap
> updates?

Sure, although we should never actually hit it.

Re: [PATCH 3/6] xfs: pass individual arguments to xfs_bmap_add_extent_hole_real

[Christoph Hellwig]

On Mon, Apr 10, 2017 at 10:13:16AM -0700, Darrick J. Wong wrote:
> > -	struct xfs_bmalloca	*bma,
> > -	int			whichfork)
> > +	struct xfs_trans	*tp,
> > +	xfs_inode_t		*ip,	/* incore inode pointer */
> > +	int			whichfork,
> > +	xfs_extnum_t		*idx,	/* extent number to update/insert */
> > +	xfs_btree_cur_t		**curp,	/* if *curp is null, not a btree */
> > +	xfs_bmbt_irec_t		*new,	/* new data to add to file extents */
> 
> struct xfs_inode vs. xfs_inode_t ?

Sure, I can fix this up.

> 
> I was under the impression that we don't add comments to the function
> arguments anymore, but TBH they don't bother me.

It's copy and paste from the other function in the family that
takes individual parameters.  I can remove it.

Re: [PATCH 4/6] xfs: introduce xfs_bmapi_remap

[Darrick J. Wong]

On Mon, Apr 03, 2017 at 02:18:31PM +0200, Christoph Hellwig wrote:
> Add a new helper to be used for reflink extent list additions instead of
> funneling them through xfs_bmapi_write and overloading the firstblock
> member in struct xfs_bmalloca and struct xfs_alloc_args.
> 
> With some small changes to xfs_bmap_remap_alloc this also means we do
> not need a xfs_bmalloca structure for this case at all.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 152 +++++++++++++++++++++++++++++++++--------------
>  1 file changed, 107 insertions(+), 45 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index d404dee7ede4..caadfe27af39 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3862,11 +3862,12 @@ xfs_bmap_btalloc(
>   */
>  STATIC int
>  xfs_bmap_remap_alloc(
> -	struct xfs_bmalloca	*ap)
> +	struct xfs_trans	*tp,
> +	struct xfs_inode	*ip,
> +	xfs_fsblock_t		startblock,
> +	xfs_extlen_t		length)

Why change all this if the next patch removes the whole function except:

ip->i_d.di_nblocks += length;
xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

Wouldn't it make more sense to have this patch add _bmapi_remap (as it
appears in the next patch) and change the callers to use it; and then
the next patch removes the old _bmap_remap_alloc and its callers?

>  {
> -	struct xfs_trans	*tp = ap->tp;
>  	struct xfs_mount	*mp = tp->t_mountp;
> -	xfs_fsblock_t		bno;
>  	struct xfs_alloc_arg	args;
>  	int			error;
>  
> @@ -3875,24 +3876,24 @@ xfs_bmap_remap_alloc(
>  	 * and handle a silent filesystem corruption rather than crashing.
>  	 */
>  	memset(&args, 0, sizeof(struct xfs_alloc_arg));
> -	args.tp = ap->tp;
> -	args.mp = ap->tp->t_mountp;
> -	bno = *ap->firstblock;
> -	args.agno = XFS_FSB_TO_AGNO(mp, bno);
> -	args.agbno = XFS_FSB_TO_AGBNO(mp, bno);
> +	args.tp = tp;
> +	args.mp = mp;
> +	args.agno = XFS_FSB_TO_AGNO(mp, startblock);
> +	args.agbno = XFS_FSB_TO_AGBNO(mp, startblock);
> +
>  	if (args.agno >= mp->m_sb.sb_agcount ||
>  	    args.agbno >= mp->m_sb.sb_agblocks)
>  		return -EFSCORRUPTED;
>  
>  	/* "Allocate" the extent from the range we passed in. */
> -	trace_xfs_bmap_remap_alloc(ap->ip, *ap->firstblock, ap->length);
> -	ap->blkno = bno;
> -	ap->ip->i_d.di_nblocks += ap->length;
> -	xfs_trans_log_inode(ap->tp, ap->ip, XFS_ILOG_CORE);
> +	trace_xfs_bmap_remap_alloc(ip, startblock, length);
> +
> +	ip->i_d.di_nblocks += length;
> +	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>  
>  	/* Fix the freelist, like a real allocator does. */
> -	args.datatype = ap->datatype;
> -	args.pag = xfs_perag_get(args.mp, args.agno);
> +	args.datatype = XFS_ALLOC_USERDATA | XFS_ALLOC_NOBUSY;
> +	args.pag = xfs_perag_get(mp, args.agno);
>  	ASSERT(args.pag);
>  
>  	/*
> @@ -3905,7 +3906,7 @@ xfs_bmap_remap_alloc(
>  	error = xfs_alloc_fix_freelist(&args, XFS_ALLOC_FLAG_FREEING);
>  	xfs_perag_put(args.pag);
>  	if (error)
> -		trace_xfs_bmap_remap_alloc_error(ap->ip, error, _RET_IP_);
> +		trace_xfs_bmap_remap_alloc_error(ip, error, _RET_IP_);
>  	return error;
>  }
>  
> @@ -3917,8 +3918,6 @@ STATIC int
>  xfs_bmap_alloc(
>  	struct xfs_bmalloca	*ap)	/* bmap alloc argument struct */
>  {
> -	if (ap->flags & XFS_BMAPI_REMAP)
> -		return xfs_bmap_remap_alloc(ap);
>  	if (XFS_IS_REALTIME_INODE(ap->ip) &&
>  	    xfs_alloc_is_userdata(ap->datatype))
>  		return xfs_bmap_rtalloc(ap);
> @@ -4554,9 +4553,7 @@ xfs_bmapi_write(
>  	ASSERT(len > 0);
>  	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL);
>  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> -	ASSERT(!(flags & XFS_BMAPI_REMAP) || whichfork == XFS_DATA_FORK);
> -	ASSERT(!(flags & XFS_BMAPI_PREALLOC) || !(flags & XFS_BMAPI_REMAP));
> -	ASSERT(!(flags & XFS_BMAPI_CONVERT) || !(flags & XFS_BMAPI_REMAP));
> +	ASSERT(!(flags & XFS_BMAPI_REMAP));
>  
>  	/* zeroing is for currently only for data extents, not metadata */
>  	ASSERT((flags & (XFS_BMAPI_METADATA | XFS_BMAPI_ZERO)) !=
> @@ -4640,13 +4637,8 @@ xfs_bmapi_write(
>  			} else {
>  				need_alloc = true;
>  			}
> -		} else {
> -			/*
> -			 * Make sure we only reflink into a hole.
> -			 */
> -			ASSERT(!(flags & XFS_BMAPI_REMAP));
> -			if (isnullstartblock(bma.got.br_startblock))
> -				wasdelay = true;
> +		} else if (isnullstartblock(bma.got.br_startblock)) {
> +			wasdelay = true;
>  		}
>  
>  		/*
> @@ -4775,6 +4767,89 @@ xfs_bmapi_write(
>  	return error;
>  }
>  
> +static int
> +xfs_bmapi_remap(
> +	struct xfs_trans	*tp,
> +	struct xfs_inode	*ip,
> +	xfs_fileoff_t		bno,
> +	xfs_filblks_t		len,
> +	xfs_fsblock_t		startblock,
> +	struct xfs_defer_ops	*dfops)
> +{
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> +	struct xfs_btree_cur	*cur = NULL;
> +	xfs_fsblock_t		firstblock = NULLFSBLOCK;
> +	struct xfs_bmbt_irec	got;
> +	xfs_extnum_t		idx;
> +	int			logflags = 0, error;
> +
> +	ASSERT(len > 0);
> +	ASSERT(len <= (xfs_filblks_t)MAXEXTLEN);
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> +	ASSERT(ifp->if_flags & XFS_IFEXTENTS);
> +
> +	if (unlikely(XFS_TEST_ERROR(
> +	    (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_EXTENTS &&
> +	     XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE),
> +	     mp, XFS_ERRTAG_BMAPIFORMAT, XFS_RANDOM_BMAPIFORMAT))) {
> +		XFS_ERROR_REPORT("xfs_bmapi_remap", XFS_ERRLEVEL_LOW, mp);
> +		return -EFSCORRUPTED;
> +	}
> +
> +	if (XFS_FORCED_SHUTDOWN(mp))
> +		return -EIO;
> +
> +	if (xfs_iext_lookup_extent(ip, ifp, bno, &idx, &got)) {
> +		/* make sure we only reflink into a hole. */
> +		ASSERT(got.br_startoff > bno);
> +		ASSERT(got.br_startoff - bno >= len);
> +	}
> +
> +	error = xfs_bmap_remap_alloc(tp, ip, startblock, len);
> +	if (error)
> +		goto error0;
> +
> +	if (ifp->if_flags & XFS_IFBROOT) {
> +		cur = xfs_bmbt_init_cursor(mp, tp, ip, XFS_DATA_FORK);
> +		cur->bc_private.b.firstblock = firstblock;
> +		cur->bc_private.b.dfops = dfops;
> +		cur->bc_private.b.flags = 0;
> +	}
> +
> +	got.br_startoff = bno;
> +	got.br_startblock = startblock;
> +	got.br_blockcount = len;
> +	got.br_state = XFS_EXT_NORM;
> +
> +	error = xfs_bmap_add_extent_hole_real(tp, ip, XFS_DATA_FORK, &idx, &cur,
> +			&got, &firstblock, dfops, &logflags);
> +	if (error)
> +		goto error0;
> +
> +	if (xfs_bmap_wants_extents(ip, XFS_DATA_FORK)) {
> +		int		tmp_logflags = 0;
> +
> +		error = xfs_bmap_btree_to_extents(tp, ip, cur,
> +			&tmp_logflags, XFS_DATA_FORK);
> +		logflags |= tmp_logflags;
> +	}
> +
> +error0:
> +	if (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS)
> +		logflags &= ~XFS_ILOG_DEXT;
> +	else if (ip->i_d.di_format != XFS_DINODE_FMT_BTREE)
> +		logflags &= ~XFS_ILOG_DBROOT;
> +
> +	if (logflags)
> +		xfs_trans_log_inode(tp, ip, logflags);
> +	if (cur) {
> +		xfs_btree_del_cursor(cur,
> +			error ? XFS_BTREE_ERROR : XFS_BTREE_NOERROR);

Double indent here?

xfs_btree_del_cursor(cur,
		error ? ...);

Especially since you do that in one of the subsequent hunks.

> +	}
> +	return error;

I think the code in this function looks ok aside from my earlier
comments.

--D

> +}
> +
>  /*
>   * 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
> @@ -6493,16 +6568,7 @@ xfs_bmap_finish_one(
>  	xfs_filblks_t			blockcount,
>  	xfs_exntst_t			state)
>  {
> -	struct xfs_bmbt_irec		bmap;
> -	int				nimaps = 1;
> -	xfs_fsblock_t			firstfsb;
> -	int				done;
> -	int				error = 0;
> -
> -	bmap.br_startblock = startblock;
> -	bmap.br_startoff = startoff;
> -	bmap.br_blockcount = blockcount;
> -	bmap.br_state = state;
> +	int				error = 0, done;
>  
>  	trace_xfs_bmap_deferred(tp->t_mountp,
>  			XFS_FSB_TO_AGNO(tp->t_mountp, startblock), type,
> @@ -6519,16 +6585,12 @@ xfs_bmap_finish_one(
>  
>  	switch (type) {
>  	case XFS_BMAP_MAP:
> -		firstfsb = bmap.br_startblock;
> -		error = xfs_bmapi_write(tp, ip, bmap.br_startoff,
> -					bmap.br_blockcount, XFS_BMAPI_REMAP, &firstfsb,
> -					bmap.br_blockcount, &bmap, &nimaps,
> -					dfops);
> +		error = xfs_bmapi_remap(tp, ip, startoff, blockcount,
> +				startblock, dfops);
>  		break;
>  	case XFS_BMAP_UNMAP:
> -		error = xfs_bunmapi(tp, ip, bmap.br_startoff,
> -				bmap.br_blockcount, XFS_BMAPI_REMAP, 1, &firstfsb,
> -				dfops, &done);
> +		error = xfs_bunmapi(tp, ip, startoff, blockcount,
> +				XFS_BMAPI_REMAP, 1, &startblock, dfops, &done);
>  		ASSERT(done);
>  		break;
>  	default:
> -- 
> 2.11.0
> 
> --
> 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 6/6] xfs: remove bmap block allocation retries

[Darrick J. Wong]

On Mon, Apr 03, 2017 at 02:18:33PM +0200, Christoph Hellwig wrote:
> Now that reflink operations don't set the firstblock value we don't
> need the workarounds for non-NULL firstblock values without a prior
> allocation.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_bmap.c       | 29 -----------------------------
>  fs/xfs/libxfs/xfs_bmap_btree.c | 17 -----------------
>  2 files changed, 46 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 0035f96a6e5a..c66e9c25e3c2 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -764,7 +764,6 @@ xfs_bmap_extents_to_btree(
>  		args.fsbno = XFS_INO_TO_FSB(mp, ip->i_ino);
>  	} else if (dfops->dop_low) {
>  		args.type = XFS_ALLOCTYPE_START_BNO;
> -try_another_ag:
>  		args.fsbno = *firstblock;
>  	} else {
>  		args.type = XFS_ALLOCTYPE_NEAR_BNO;
> @@ -779,20 +778,6 @@ xfs_bmap_extents_to_btree(
>  		return error;
>  	}
>  
> -	/*
> -	 * During a CoW operation, the allocation and bmbt updates occur in
> -	 * different transactions.  The mapping code tries to put new bmbt
> -	 * blocks near extents being mapped, but the only way to guarantee this
> -	 * is if the alloc and the mapping happen in a single transaction that
> -	 * has a block reservation.  That isn't the case here, so if we run out
> -	 * of space we'll try again with another AG.
> -	 */
> -	if (xfs_sb_version_hasreflink(&cur->bc_mp->m_sb) &&
> -	    args.fsbno == NULLFSBLOCK &&
> -	    args.type == XFS_ALLOCTYPE_NEAR_BNO) {
> -		args.type = XFS_ALLOCTYPE_FIRST_AG;
> -		goto try_another_ag;
> -	}
>  	if (WARN_ON_ONCE(args.fsbno == NULLFSBLOCK)) {
>  		xfs_iroot_realloc(ip, -1, whichfork);
>  		xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
> @@ -925,7 +910,6 @@ xfs_bmap_local_to_extents(
>  	 * file currently fits in an inode.
>  	 */
>  	if (*firstblock == NULLFSBLOCK) {
> -try_another_ag:
>  		args.fsbno = XFS_INO_TO_FSB(args.mp, ip->i_ino);
>  		args.type = XFS_ALLOCTYPE_START_BNO;
>  	} else {
> @@ -938,19 +922,6 @@ xfs_bmap_local_to_extents(
>  	if (error)
>  		goto done;
>  
> -	/*
> -	 * During a CoW operation, the allocation and bmbt updates occur in
> -	 * different transactions.  The mapping code tries to put new bmbt
> -	 * blocks near extents being mapped, but the only way to guarantee this
> -	 * is if the alloc and the mapping happen in a single transaction that
> -	 * has a block reservation.  That isn't the case here, so if we run out
> -	 * of space we'll try again with another AG.
> -	 */
> -	if (xfs_sb_version_hasreflink(&ip->i_mount->m_sb) &&
> -	    args.fsbno == NULLFSBLOCK &&
> -	    args.type == XFS_ALLOCTYPE_NEAR_BNO) {
> -		goto try_another_ag;
> -	}
>  	/* Can't fail, the space was reserved. */
>  	ASSERT(args.fsbno != NULLFSBLOCK);
>  	ASSERT(args.len == 1);
> diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
> index fd55db479385..3e17ceda038c 100644
> --- a/fs/xfs/libxfs/xfs_bmap_btree.c
> +++ b/fs/xfs/libxfs/xfs_bmap_btree.c
> @@ -448,7 +448,6 @@ xfs_bmbt_alloc_block(
>  	if (args.fsbno == NULLFSBLOCK) {
>  		args.fsbno = be64_to_cpu(start->l);
>  		args.type = XFS_ALLOCTYPE_START_BNO;
> -try_another_ag:
>  		/*
>  		 * Make sure there is sufficient room left in the AG to
>  		 * complete a full tree split for an extent insert.  If
> @@ -477,22 +476,6 @@ xfs_bmbt_alloc_block(
>  	if (error)
>  		goto error0;
>  
> -	/*
> -	 * During a CoW operation, the allocation and bmbt updates occur in
> -	 * different transactions.  The mapping code tries to put new bmbt
> -	 * blocks near extents being mapped, but the only way to guarantee this
> -	 * is if the alloc and the mapping happen in a single transaction that
> -	 * has a block reservation.  That isn't the case here, so if we run out
> -	 * of space we'll try again with another AG.
> -	 */
> -	if (xfs_sb_version_hasreflink(&cur->bc_mp->m_sb) &&
> -	    args.fsbno == NULLFSBLOCK &&
> -	    args.type == XFS_ALLOCTYPE_NEAR_BNO) {
> -		args.fsbno = cur->bc_private.b.firstblock;
> -		args.type = XFS_ALLOCTYPE_FIRST_AG;
> -		goto try_another_ag;
> -	}
> -
>  	if (args.fsbno == NULLFSBLOCK && args.minleft) {
>  		/*
>  		 * Could not find an AG with enough free space to satisfy
> -- 
> 2.11.0
> 
> --
> 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/6] xfs: introduce xfs_bmapi_remap

[Christoph Hellwig]

On Mon, Apr 10, 2017 at 12:50:04PM -0700, Darrick J. Wong wrote:
> Why change all this if the next patch removes the whole function except:
> 
> ip->i_d.di_nblocks += length;
> xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> 
> Wouldn't it make more sense to have this patch add _bmapi_remap (as it
> appears in the next patch) and change the callers to use it; and then
> the next patch removes the old _bmap_remap_alloc and its callers?

I'd rather keep steps as small and self-explaining as possible,
so one is factoring out a new helper, and the other is removing a call
that's no needed.

> > +	else if (ip->i_d.di_format != XFS_DINODE_FMT_BTREE)
> > +		logflags &= ~XFS_ILOG_DBROOT;
> > +
> > +	if (logflags)
> > +		xfs_trans_log_inode(tp, ip, logflags);
> > +	if (cur) {
> > +		xfs_btree_del_cursor(cur,
> > +			error ? XFS_BTREE_ERROR : XFS_BTREE_NOERROR);
> 
> Double indent here?

Sure..

[PATCH 1/6] xfs: fix integer truncation in xfs_bmap_remap_alloc

[Christoph Hellwig]

bno should be a xfs_fsblock_t, which is 64-bit wides instead of a
xfs_aglock_t, which truncates the value to 32 bits.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_bmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 9bd104f32908..2a426d127e05 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3863,7 +3863,7 @@ xfs_bmap_remap_alloc(
 {
 	struct xfs_trans	*tp = ap->tp;
 	struct xfs_mount	*mp = tp->t_mountp;
-	xfs_agblock_t		bno;
+	xfs_fsblock_t		bno;
 	struct xfs_alloc_arg	args;
 	int			error;
 
-- 
2.11.0

[PATCH 1/6] xfs: fix integer truncation in xfs_bmap_remap_alloc

[Christoph Hellwig]

bno should be a xfs_fsblock_t, which is 64-bit wides instead of a
xfs_aglock_t, which truncates the value to 32 bits.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_bmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 9bd104f32908..2a426d127e05 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3863,7 +3863,7 @@ xfs_bmap_remap_alloc(
 {
 	struct xfs_trans	*tp = ap->tp;
 	struct xfs_mount	*mp = tp->t_mountp;
-	xfs_agblock_t		bno;
+	xfs_fsblock_t		bno;
 	struct xfs_alloc_arg	args;
 	int			error;
 
-- 
2.11.0

[PATCH 1/6] xfs: fix integer truncation in xfs_bmap_remap_alloc

[Christoph Hellwig]

bno should be a xfs_fsblock_t, which is 64-bit wides instead of a
xfs_aglock_t, which truncates the value to 32 bits.

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

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 9bd104f32908..2a426d127e05 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3863,7 +3863,7 @@ xfs_bmap_remap_alloc(
 {
 	struct xfs_trans	*tp = ap->tp;
 	struct xfs_mount	*mp = tp->t_mountp;
-	xfs_agblock_t		bno;
+	xfs_fsblock_t		bno;
 	struct xfs_alloc_arg	args;
 	int			error;
 
-- 
2.11.0