split the reflink remap from the block allocation path V2

split the reflink remap from the block allocation path V2

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

Changes since V1:
 - warn when encountering invalid forks in bmap item recovery
 - tidy up the xfs_bmap_add_extent_hole_real declaration
 - trivial indentation change in xfs_bmapi_remap

[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 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..b95e66fd0935 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 (WARN_ON_ONCE(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

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

[Darrick J. Wong]

On Tue, Apr 11, 2017 at 01:10:07PM +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>

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

> ---
>  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..b95e66fd0935 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 (WARN_ON_ONCE(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
> 
> --
> 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 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 b95e66fd0935..f32b2dab16a4 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,
+	struct xfs_inode	*ip,
+	int			whichfork,
+	xfs_extnum_t		*idx,
+	struct xfs_btree_cur	**curp,
+	struct xfs_bmbt_irec	*new,
+	xfs_fsblock_t		*first,
+	struct xfs_defer_ops	*dfops,
+	int			*logflagsp)
 {
-	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

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

[Darrick J. Wong]

On Tue, Apr 11, 2017 at 01:10:08PM +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>

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

> ---
>  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 b95e66fd0935..f32b2dab16a4 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,
> +	struct xfs_inode	*ip,
> +	int			whichfork,
> +	xfs_extnum_t		*idx,
> +	struct xfs_btree_cur	**curp,
> +	struct xfs_bmbt_irec	*new,
> +	xfs_fsblock_t		*first,
> +	struct xfs_defer_ops	*dfops,
> +	int			*logflagsp)
>  {
> -	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
> 
> --
> 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/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 f32b2dab16a4..faf7cb0a28f9 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

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

[Darrick J. Wong]

On Tue, Apr 11, 2017 at 01:10:09PM +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>

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

> ---
>  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 f32b2dab16a4..faf7cb0a28f9 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
> 
> --
> 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 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 faf7cb0a28f9..414cfa91af01 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

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

[Darrick J. Wong]

On Tue, Apr 11, 2017 at 01:10:10PM +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.

Looks ok, will go test.  By the way, what release were you targeting
with this patchset?  AFAICT the only behavioral change is that we no
longer ensure the AGFL in the remap step prior to (if necessary)
ensuring the AGFL again in the subsequent rmap step.

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

--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 faf7cb0a28f9..414cfa91af01 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: [PATCH 5/6] xfs: remove xfs_bmap_remap_alloc

[Christoph Hellwig]

On Tue, Apr 11, 2017 at 04:02:46PM -0700, Darrick J. Wong wrote:
> On Tue, Apr 11, 2017 at 01:10:10PM +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.
> 
> Looks ok, will go test.  By the way, what release were you targeting
> with this patchset?

The patches are against for-next.  Given how late we are in 4.11 I didn't
dare to send them for 4.11, although I'd like to backport it to 4.11-stable
and 4.9-stable eventually.

> AFAICT the only behavioral change is that we no
> longer ensure the AGFL in the remap step prior to (if necessary)
> ensuring the AGFL again in the subsequent rmap step.

Yes.

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

[Darrick J. Wong]

On Wed, Apr 12, 2017 at 07:38:39AM +0200, Christoph Hellwig wrote:
> On Tue, Apr 11, 2017 at 04:02:46PM -0700, Darrick J. Wong wrote:
> > On Tue, Apr 11, 2017 at 01:10:10PM +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.
> > 
> > Looks ok, will go test.  By the way, what release were you targeting
> > with this patchset?
> 
> The patches are against for-next.  Given how late we are in 4.11 I didn't
> dare to send them for 4.11, although I'd like to backport it to 4.11-stable
> and 4.9-stable eventually.
> 
> > AFAICT the only behavioral change is that we no
> > longer ensure the AGFL in the remap step prior to (if necessary)
> > ensuring the AGFL again in the subsequent rmap step.
> 
> Yes.

Hmmm... I got a crash in generic/187 with:

MKFS_OPTIONS="-f -m reflink=1,rmapbt=1, -i sparse=1, -b size=1024, /dev/sdf"

generic/187      - output mismatch (see /tmp/xfstests/results//generic/187.out.bad)
    --- tests/generic/187.out   2017-02-28 09:23:56.058066197 -0800
    +++ /tmp/xfstests/results//generic/187.out.bad      2017-04-11 18:14:40.715071575 -0700
    @@ -7,8 +7,11 @@
     8d9ea4925db533da10a45d2919d8d2d9  SCRATCH_MNT/test-187/file3
     8d9ea4925db533da10a45d2919d8d2d9  SCRATCH_MNT/test-187/file3.chk
     CoW with multiple extents?
    +pwrite: No space left on device
    +/opt/test-187/file3.chk: Input/output error
    +./common/rc: line 192: 24965 Segmentation fault      $MOUNT_PROG '_mount_ops_filter $*'
     Compare files
    ...
    (Run 'diff -u tests/generic/187.out /tmp/xfstests/results//generic/187.out.bad'  to see the entire diff)
_check_xfs_filesystem: filesystem on /dev/sdf has dirty log
(see /tmp/xfstests/results//generic/187.full for details)

[ 4307.852659] run fstests generic/187 at 2017-04-11 18:13:44
[ 4308.507527] XFS (sdf): Unmounting Filesystem
[ 4308.627786] XFS (sdf): EXPERIMENTAL reverse mapping btree feature enabled. Use at your own risk!
[ 4308.628673] XFS (sdf): EXPERIMENTAL reflink feature enabled. Use at your own risk!
[ 4308.629822] XFS (sdf): Mounting V5 Filesystem
[ 4308.634470] XFS (sdf): Ending clean mount
[ 4308.692509] XFS (sdf): Unmounting Filesystem
[ 4308.876024] XFS (sdf): EXPERIMENTAL reverse mapping btree feature enabled. Use at your own risk!
[ 4308.877205] XFS (sdf): EXPERIMENTAL reflink feature enabled. Use at your own risk!
[ 4308.878274] XFS (sdf): Mounting V5 Filesystem
[ 4308.883306] XFS (sdf): Ending clean mount
[ 4328.077062] XFS (sdf): Unmounting Filesystem
[ 4328.128459] XFS (sdf): EXPERIMENTAL reverse mapping btree feature enabled. Use at your own risk!
[ 4328.129473] XFS (sdf): EXPERIMENTAL reflink feature enabled. Use at your own risk!
[ 4328.130719] XFS (sdf): Mounting V5 Filesystem
[ 4328.166304] XFS (sdf): Ending clean mount
[ 4362.064634] XFS (sdf): Unmounting Filesystem
[ 4362.157219] XFS (sdf): EXPERIMENTAL reverse mapping btree feature enabled. Use at your own risk!
[ 4362.159172] XFS (sdf): EXPERIMENTAL reflink feature enabled. Use at your own risk!
[ 4362.161274] XFS (sdf): Mounting V5 Filesystem
[ 4362.223441] XFS (sdf): Ending clean mount
[ 4363.236809] XFS (sdf): xfs_do_force_shutdown(0x1) called from line 236 of file /raid/home/djwong/cdev/work/linux-dgc/fs/xfs/libxfs/xfs_defer.c.  Return address = 0xffffffffa0203400
[ 4363.240663] XFS (sdf): I/O Error Detected. Shutting down filesystem
[ 4363.241532] XFS (sdf): Please umount the filesystem and rectify the problem(s)
[ 4363.375513] XFS (sdf): Unmounting Filesystem
[ 4363.422278] XFS (sdf): EXPERIMENTAL reverse mapping btree feature enabled. Use at your own risk!
[ 4363.432790] XFS (sdf): EXPERIMENTAL reflink feature enabled. Use at your own risk!
[ 4363.435026] XFS (sdf): Mounting V5 Filesystem
[ 4363.538385] XFS (sdf): Starting recovery (logdev: internal)
[ 4363.630364] XFS: Assertion failed: ifp->if_flags & XFS_IFEXTENTS, file: /raid/home/djwong/cdev/work/linux-dgc/fs/xfs/libxfs/xfs_bmap.c, line: 4706
[ 4363.632502] ------------[ cut here ]------------
[ 4363.633162] kernel BUG at /raid/home/djwong/cdev/work/linux-dgc/fs/xfs/xfs_message.c:113!
[ 4363.634304] invalid opcode: 0000 [#1] PREEMPT SMP
[ 4363.634969] Dumping ftrace buffer:
[ 4363.635463]    (ftrace buffer empty)
[ 4363.635974] Modules linked in: deadline_iosched dm_snapshot dm_bufio ext4 jbd2 mbcache dm_flakey xfs libcrc32c dax_pmem nd_pmem dax sch_fq_codel af_packet [last unloaded: scsi_debug]
[ 4363.638224] CPU: 3 PID: 24965 Comm: mount Tainted: G        W       4.11.0-rc4-dgc #1
[ 4363.639316] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS Ubuntu-1.8.2-1ubuntu1djwong0 04/01/2014
[ 4363.640688] task: ffff880074aaa780 task.stack: ffffc90002e50000
[ 4363.641580] RIP: 0010:assfail+0x20/0x30 [xfs]
[ 4363.642205] RSP: 0018:ffffc90002e53ac8 EFLAGS: 00010282
[ 4363.642942] RAX: 00000000ffffffea RBX: ffff880072629b00 RCX: 0000000000000001
[ 4363.643927] RDX: 00000000ffffffc0 RSI: 000000000000000a RDI: ffffffffa02a7e3a
[ 4363.644989] RBP: ffffc90002e53ac8 R08: 0000000000000000 R09: 0000000000000000
[ 4363.645939] R10: 000000000000000a R11: f000000000000000 R12: 0000000000006740
[ 4363.646964] R13: ffff88007497d000 R14: 00000000000350ee R15: ffff880072629b48
[ 4363.648019] FS:  00007f16ba56d840(0000) GS:ffff88007f600000(0000) knlGS:0000000000000000
[ 4363.649091] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 4363.649851] CR2: 00007f16b9d95170 CR3: 0000000069983000 CR4: 00000000000006e0
[ 4363.650796] Call Trace:
[ 4363.651182]  xfs_bmap_finish_one+0x5fa/0x6f0 [xfs]
[ 4363.651947]  ? rcu_read_lock_sched_held+0x72/0x80
[ 4363.652724]  ? kmem_zone_alloc+0x81/0x120 [xfs]
[ 4363.653373]  xfs_trans_log_finish_bmap_update+0x40/0x60 [xfs]
[ 4363.654209]  xfs_bui_recover+0x265/0x5d0 [xfs]
[ 4363.654897]  xlog_recover_process_intents+0x293/0x2b0 [xfs]
[ 4363.655723]  xlog_recover_finish+0x1d/0xa0 [xfs]
[ 4363.656423]  xfs_log_mount_finish+0x3a/0x80 [xfs]
[ 4363.657125]  xfs_mountfs+0x65a/0xad0 [xfs]
[ 4363.657745]  xfs_fs_fill_super+0x483/0x610 [xfs]
[ 4363.658419]  mount_bdev+0x180/0x1b0
[ 4363.658954]  ? xfs_finish_flags+0x150/0x150 [xfs]
[ 4363.659660]  xfs_fs_mount+0x15/0x20 [xfs]
[ 4363.660256]  mount_fs+0x14/0xa0
[ 4363.660724]  vfs_kern_mount+0x6b/0x160
[ 4363.661269]  do_mount+0x195/0xd30
[ 4363.661754]  ? _copy_from_user+0x8c/0xd0
[ 4363.662328]  ? memdup_user+0x60/0x90
[ 4363.662846]  SyS_mount+0x95/0xe0
[ 4363.663326]  entry_SYSCALL_64_fastpath+0x1f/0xc2
[ 4363.663988] RIP: 0033:0x7f16b9e4efaa
[ 4363.664490] RSP: 002b:00007ffe9202d4a8 EFLAGS: 00000202 ORIG_RAX: 00000000000000a5
[ 4363.665515] RAX: ffffffffffffffda RBX: 00007f16ba14763a RCX: 00007f16b9e4efaa
[ 4363.666484] RDX: 0000000002579240 RSI: 0000000002579280 RDI: 0000000002579260
[ 4363.667452] RBP: 0000000002579120 R08: 0000000000000000 R09: 0000000000000012
[ 4363.668431] R10: 00000000c0ed0000 R11: 0000000000000202 R12: 00007f16ba35783c
[ 4363.669370] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000003
[ 4363.670319] Code: 00 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 90 55 48 89 f1 41 89 d0 48 c7 c6 d0 66 2b a0 48 89 fa 31 ff 48 89 e5 e8 50 fa ff ff <0f> 0b 0f 1f 40 00 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 90 
[ 4363.673053] RIP: assfail+0x20/0x30 [xfs] RSP: ffffc90002e53ac8
[ 4363.674170] ---[ end trace 6f71a70ec6fdf3f5 ]---

Looks like _bmapi_remap needs to be able to _iread_extents() if the
data fork hasn't been loaded during log recovery.

--D

> --
> 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 5/6] xfs: remove xfs_bmap_remap_alloc

[Christoph Hellwig]

> Looks like _bmapi_remap needs to be able to _iread_extents() if the
> data fork hasn't been loaded during log recovery.

Yeah, probably.  I'll respin once more with that included.

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

[Darrick J. Wong]

On Wed, Apr 12, 2017 at 08:00:26AM +0200, Christoph Hellwig wrote:
> > Looks like _bmapi_remap needs to be able to _iread_extents() if the
> > data fork hasn't been loaded during log recovery.
> 
> Yeah, probably.  I'll respin once more with that included.

Unfortunately, even after adding in the necessary loading code I still
get -ENOSPC back from xfs_bmap_add_extent_hole_real which causes log
recovery to fail.

--D

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

[Christoph Hellwig]

On Tue, Apr 11, 2017 at 11:44:20PM -0700, Darrick J. Wong wrote:
> On Wed, Apr 12, 2017 at 08:00:26AM +0200, Christoph Hellwig wrote:
> > > Looks like _bmapi_remap needs to be able to _iread_extents() if the
> > > data fork hasn't been loaded during log recovery.
> > 
> > Yeah, probably.  I'll respin once more with that included.
> 
> Unfortunately, even after adding in the necessary loading code I still
> get -ENOSPC back from xfs_bmap_add_extent_hole_real which causes log
> recovery to fail.

The test in your configuration already fails with -ENOSPC for me on
for-next..

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

[Darrick J. Wong]

On Wed, Apr 12, 2017 at 10:00:59AM +0200, Christoph Hellwig wrote:
> On Tue, Apr 11, 2017 at 11:44:20PM -0700, Darrick J. Wong wrote:
> > On Wed, Apr 12, 2017 at 08:00:26AM +0200, Christoph Hellwig wrote:
> > > > Looks like _bmapi_remap needs to be able to _iread_extents() if the
> > > > data fork hasn't been loaded during log recovery.
> > > 
> > > Yeah, probably.  I'll respin once more with that included.
> > 
> > Unfortunately, even after adding in the necessary loading code I still
> > get -ENOSPC back from xfs_bmap_add_extent_hole_real which causes log
> > recovery to fail.
> 
> The test in your configuration already fails with -ENOSPC for me on
> for-next..

Crap, there are actually /two/ problems here:

The first problem is that xfs_reflink_end_cow reserves only enough
blocks to handle adding 1 extent.  This is problematic if we fragment
free space, have to do CoW, and then have to perform multiple bmap btree
expansions, which g/187 seems to hit.

The second problem is that the BUI recovery routine doesn't reserve
/any/ blocks to handle btree splits, so when the first problem takes the
fs down, recovery also fails.

Evidently with 1k blocks we can hit this fairly often in g/187 if the
scratch fs is big enough.

Ok, patch soon. <sigh>

--D

> --
> 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 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>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 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 414cfa91af01..cc9feb2f5731 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

split the reflink remap from the block allocation path V3

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

Changes since V2:
 - add a missing call to xfs_iread_extents

Changes since V1:
 - warn when encountering invalid forks in bmap item recovery
 - tidy up the xfs_bmap_add_extent_hole_real declaration
 - trivial indentation change in xfs_bmapi_remap

[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>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 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..b95e66fd0935 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 (WARN_ON_ONCE(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

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

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