Re: [PATCH 5/8] xfs: use xfs_iext_*_extent helpers in xfs_bmap_shift_extents

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Aug 29, 2017 at 07:48:32PM +0200, Christoph Hellwig wrote:
> This abstracts the function away from details of the low-level extent
> list implementation.
> 
> Note that it seems like the previous implementation of rmap for
> the merge case was completely broken, but it no seems appear to
> trigger that.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Series looks ok enough to test,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> ---
>  fs/xfs/libxfs/xfs_bmap.c | 180 +++++++++++++++++++++++------------------------
>  1 file changed, 88 insertions(+), 92 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 6af3cc1fc630..02725becedfa 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -5881,32 +5881,26 @@ xfs_bmse_merge(
>  	int				whichfork,
>  	xfs_fileoff_t			shift,		/* shift fsb */
>  	int				current_ext,	/* idx of gotp */
> -	struct xfs_bmbt_rec_host	*gotp,		/* extent to shift */
> -	struct xfs_bmbt_rec_host	*leftp,		/* preceding extent */
> +	struct xfs_bmbt_irec		*got,		/* extent to shift */
> +	struct xfs_bmbt_irec		*left,		/* preceding extent */
>  	struct xfs_btree_cur		*cur,
> -	int				*logflags)	/* output */
> +	int				*logflags,	/* output */
> +	struct xfs_defer_ops		*dfops)
>  {
> -	struct xfs_bmbt_irec		got;
> -	struct xfs_bmbt_irec		left;
> +	struct xfs_ifork		*ifp = XFS_IFORK_PTR(ip, whichfork);
> +	struct xfs_bmbt_irec		new;
>  	xfs_filblks_t			blockcount;
>  	int				error, i;
>  	struct xfs_mount		*mp = ip->i_mount;
>  
> -	xfs_bmbt_get_all(gotp, &got);
> -	xfs_bmbt_get_all(leftp, &left);
> -	blockcount = left.br_blockcount + got.br_blockcount;
> +	blockcount = left->br_blockcount + got->br_blockcount;
>  
>  	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
>  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> -	ASSERT(xfs_bmse_can_merge(&left, &got, shift));
> +	ASSERT(xfs_bmse_can_merge(left, got, shift));
>  
> -	/*
> -	 * Merge the in-core extents. Note that the host record pointers and
> -	 * current_ext index are invalid once the extent has been removed via
> -	 * xfs_iext_remove().
> -	 */
> -	xfs_bmbt_set_blockcount(leftp, blockcount);
> -	xfs_iext_remove(ip, current_ext, 1, 0);
> +	new = *left;
> +	new.br_blockcount = blockcount;
>  
>  	/*
>  	 * Update the on-disk extent count, the btree if necessary and log the
> @@ -5917,12 +5911,12 @@ xfs_bmse_merge(
>  	*logflags |= XFS_ILOG_CORE;
>  	if (!cur) {
>  		*logflags |= XFS_ILOG_DEXT;
> -		return 0;
> +		goto done;
>  	}
>  
>  	/* lookup and remove the extent to merge */
> -	error = xfs_bmbt_lookup_eq(cur, got.br_startoff, got.br_startblock,
> -				   got.br_blockcount, &i);
> +	error = xfs_bmbt_lookup_eq(cur, got->br_startoff, got->br_startblock,
> +				   got->br_blockcount, &i);
>  	if (error)
>  		return error;
>  	XFS_WANT_CORRUPTED_RETURN(mp, i == 1);
> @@ -5933,16 +5927,29 @@ xfs_bmse_merge(
>  	XFS_WANT_CORRUPTED_RETURN(mp, i == 1);
>  
>  	/* lookup and update size of the previous extent */
> -	error = xfs_bmbt_lookup_eq(cur, left.br_startoff, left.br_startblock,
> -				   left.br_blockcount, &i);
> +	error = xfs_bmbt_lookup_eq(cur, left->br_startoff, left->br_startblock,
> +				   left->br_blockcount, &i);
>  	if (error)
>  		return error;
>  	XFS_WANT_CORRUPTED_RETURN(mp, i == 1);
>  
> -	left.br_blockcount = blockcount;
> +	error = xfs_bmbt_update(cur, new.br_startoff, new.br_startblock,
> +			        new.br_blockcount, new.br_state);
> +	if (error)
> +		return error;
>  
> -	return xfs_bmbt_update(cur, left.br_startoff, left.br_startblock,
> -			       left.br_blockcount, left.br_state);
> +done:
> +	xfs_iext_update_extent(ifp, current_ext - 1, &new);
> +	xfs_iext_remove(ip, current_ext, 1, 0);
> +
> +	/* update reverse mapping */
> +	error = xfs_rmap_unmap_extent(mp, dfops, ip, whichfork, got);
> +	if (error)
> +		return error;
> +	error = xfs_rmap_unmap_extent(mp, dfops, ip, whichfork, left);
> +	if (error)
> +		return error;
> +	return xfs_rmap_map_extent(mp, dfops, ip, whichfork, &new);
>  }
>  
>  /*
> @@ -5954,7 +5961,7 @@ xfs_bmse_shift_one(
>  	int				whichfork,
>  	xfs_fileoff_t			offset_shift_fsb,
>  	int				*current_ext,
> -	struct xfs_bmbt_rec_host	*gotp,
> +	struct xfs_bmbt_irec		*got,
>  	struct xfs_btree_cur		*cur,
>  	int				*logflags,
>  	enum shift_direction		direction,
> @@ -5963,9 +5970,7 @@ xfs_bmse_shift_one(
>  	struct xfs_ifork		*ifp;
>  	struct xfs_mount		*mp;
>  	xfs_fileoff_t			startoff;
> -	struct xfs_bmbt_rec_host	*adj_irecp;
> -	struct xfs_bmbt_irec		got;
> -	struct xfs_bmbt_irec		adj_irec;
> +	struct xfs_bmbt_irec		adj_irec, new;
>  	int				error;
>  	int				i;
>  	int				total_extents;
> @@ -5974,13 +5979,11 @@ xfs_bmse_shift_one(
>  	ifp = XFS_IFORK_PTR(ip, whichfork);
>  	total_extents = xfs_iext_count(ifp);
>  
> -	xfs_bmbt_get_all(gotp, &got);
> -
>  	/* delalloc extents should be prevented by caller */
> -	XFS_WANT_CORRUPTED_RETURN(mp, !isnullstartblock(got.br_startblock));
> +	XFS_WANT_CORRUPTED_RETURN(mp, !isnullstartblock(got->br_startblock));
>  
>  	if (direction == SHIFT_LEFT) {
> -		startoff = got.br_startoff - offset_shift_fsb;
> +		startoff = got->br_startoff - offset_shift_fsb;
>  
>  		/*
>  		 * Check for merge if we've got an extent to the left,
> @@ -5988,46 +5991,39 @@ xfs_bmse_shift_one(
>  		 * of the file for the shift.
>  		 */
>  		if (!*current_ext) {
> -			if (got.br_startoff < offset_shift_fsb)
> +			if (got->br_startoff < offset_shift_fsb)
>  				return -EINVAL;
>  			goto update_current_ext;
>  		}
> +
>  		/*
> -		 * grab the left extent and check for a large
> -		 * enough hole.
> +		 * grab the left extent and check for a large enough hole.
>  		 */
> -		adj_irecp = xfs_iext_get_ext(ifp, *current_ext - 1);
> -		xfs_bmbt_get_all(adj_irecp, &adj_irec);
> -
> -		if (startoff <
> -		    adj_irec.br_startoff + adj_irec.br_blockcount)
> +		xfs_iext_get_extent(ifp, *current_ext - 1, &adj_irec);
> +		if (startoff < adj_irec.br_startoff + adj_irec.br_blockcount)
>  			return -EINVAL;
>  
>  		/* check whether to merge the extent or shift it down */
> -		if (xfs_bmse_can_merge(&adj_irec, &got,
> -				       offset_shift_fsb)) {
> -			error = xfs_bmse_merge(ip, whichfork, offset_shift_fsb,
> -					       *current_ext, gotp, adj_irecp,
> -					       cur, logflags);
> -			if (error)
> -				return error;
> -			adj_irec = got;
> -			goto update_rmap;
> +		if (xfs_bmse_can_merge(&adj_irec, got, offset_shift_fsb)) {
> +			return xfs_bmse_merge(ip, whichfork, offset_shift_fsb,
> +					      *current_ext, got, &adj_irec,
> +					      cur, logflags, dfops);
>  		}
>  	} else {
> -		startoff = got.br_startoff + offset_shift_fsb;
> +		startoff = got->br_startoff + offset_shift_fsb;
>  		/* nothing to move if this is the last extent */
>  		if (*current_ext >= (total_extents - 1))
>  			goto update_current_ext;
> +
>  		/*
>  		 * If this is not the last extent in the file, make sure there
>  		 * is enough room between current extent and next extent for
>  		 * accommodating the shift.
>  		 */
> -		adj_irecp = xfs_iext_get_ext(ifp, *current_ext + 1);
> -		xfs_bmbt_get_all(adj_irecp, &adj_irec);
> -		if (startoff + got.br_blockcount > adj_irec.br_startoff)
> +		xfs_iext_get_extent(ifp, *current_ext + 1, &adj_irec);
> +		if (startoff + got->br_blockcount > adj_irec.br_startoff)
>  			return -EINVAL;
> +
>  		/*
>  		 * Unlike a left shift (which involves a hole punch),
>  		 * a right shift does not modify extent neighbors
> @@ -6035,45 +6031,48 @@ xfs_bmse_shift_one(
>  		 * in this scenario. Check anyways and warn if we
>  		 * encounter two extents that could be one.
>  		 */
> -		if (xfs_bmse_can_merge(&got, &adj_irec, offset_shift_fsb))
> +		if (xfs_bmse_can_merge(got, &adj_irec, offset_shift_fsb))
>  			WARN_ON_ONCE(1);
>  	}
> +
>  	/*
>  	 * Increment the extent index for the next iteration, update the start
>  	 * offset of the in-core extent and update the btree if applicable.
>  	 */
>  update_current_ext:
> -	if (direction == SHIFT_LEFT)
> -		(*current_ext)++;
> -	else
> -		(*current_ext)--;
> -	xfs_bmbt_set_startoff(gotp, startoff);
>  	*logflags |= XFS_ILOG_CORE;
> -	adj_irec = got;
> -	if (!cur) {
> +
> +	new = *got;
> +	new.br_startoff = startoff;
> +
> +	if (cur) {
> +		error = xfs_bmbt_lookup_eq(cur, got->br_startoff,
> +				got->br_startblock, got->br_blockcount, &i);
> +		if (error)
> +			return error;
> +		XFS_WANT_CORRUPTED_RETURN(mp, i == 1);
> +
> +		error = xfs_bmbt_update(cur, new.br_startoff,
> +				new.br_startblock, new.br_blockcount,
> +				new.br_state);
> +		if (error)
> +			return error;
> +	} else {
>  		*logflags |= XFS_ILOG_DEXT;
> -		goto update_rmap;
>  	}
>  
> -	error = xfs_bmbt_lookup_eq(cur, got.br_startoff, got.br_startblock,
> -				   got.br_blockcount, &i);
> -	if (error)
> -		return error;
> -	XFS_WANT_CORRUPTED_RETURN(mp, i == 1);
> +	xfs_iext_update_extent(ifp, *current_ext, &new);
>  
> -	got.br_startoff = startoff;
> -	error = xfs_bmbt_update(cur, got.br_startoff, got.br_startblock,
> -			got.br_blockcount, got.br_state);
> -	if (error)
> -		return error;
> +	if (direction == SHIFT_LEFT)
> +		(*current_ext)++;
> +	else
> +		(*current_ext)--;
>  
> -update_rmap:
>  	/* update reverse mapping */
> -	error = xfs_rmap_unmap_extent(mp, dfops, ip, whichfork, &adj_irec);
> +	error = xfs_rmap_unmap_extent(mp, dfops, ip, whichfork, got);
>  	if (error)
>  		return error;
> -	adj_irec.br_startoff = startoff;
> -	return xfs_rmap_map_extent(mp, dfops, ip, whichfork, &adj_irec);
> +	return xfs_rmap_map_extent(mp, dfops, ip, whichfork, &new);
>  }
>  
>  /*
> @@ -6100,7 +6099,6 @@ xfs_bmap_shift_extents(
>  	int			num_exts)
>  {
>  	struct xfs_btree_cur		*cur = NULL;
> -	struct xfs_bmbt_rec_host	*gotp;
>  	struct xfs_bmbt_irec            got;
>  	struct xfs_mount		*mp = ip->i_mount;
>  	struct xfs_ifork		*ifp;
> @@ -6161,25 +6159,23 @@ xfs_bmap_shift_extents(
>  		ASSERT(direction == SHIFT_RIGHT);
>  
>  		current_ext = total_extents - 1;
> -		gotp = xfs_iext_get_ext(ifp, current_ext);
> -		xfs_bmbt_get_all(gotp, &got);
> -		*next_fsb = got.br_startoff;
> -		if (stop_fsb > *next_fsb) {
> +		xfs_iext_get_extent(ifp, current_ext, &got);
> +		if (stop_fsb > got.br_startoff) {
>  			*done = 1;
>  			goto del_cursor;
>  		}
> +		*next_fsb = got.br_startoff;
>  	} else {
>  		/*
>  		 * Look up the extent index for the fsb where we start shifting. We can
>  		 * henceforth iterate with current_ext as extent list changes are locked
>  		 * out via ilock.
>  		 *
> -		 * gotp can be null in 2 cases: 1) if there are no extents or 2)
> -		 * *next_fsb lies in a hole beyond which there are no extents. Either
> -		 * way, we are done.
> +		 * If next_fsb lies in a hole beyond which there are no extents we are
> +		 * done.
>  		 */
> -		gotp = xfs_iext_bno_to_ext(ifp, *next_fsb, &current_ext);
> -		if (!gotp) {
> +		if (!xfs_iext_lookup_extent(ip, ifp, *next_fsb, &current_ext,
> +				&got)) {
>  			*done = 1;
>  			goto del_cursor;
>  		}
> @@ -6187,7 +6183,9 @@ xfs_bmap_shift_extents(
>  
>  	/* Lookup the extent index at which we have to stop */
>  	if (direction == SHIFT_RIGHT) {
> -		xfs_iext_bno_to_ext(ifp, stop_fsb, &stop_extent);
> +		struct xfs_bmbt_irec s;
> +
> +		xfs_iext_lookup_extent(ip, ifp, stop_fsb, &stop_extent, &s);
>  		/* Make stop_extent exclusive of shift range */
>  		stop_extent--;
>  		if (current_ext <= stop_extent) {
> @@ -6204,7 +6202,7 @@ xfs_bmap_shift_extents(
>  
>  	while (nexts++ < num_exts) {
>  		error = xfs_bmse_shift_one(ip, whichfork, offset_shift_fsb,
> -					   &current_ext, gotp, cur, &logflags,
> +					   &current_ext, &got, cur, &logflags,
>  					   direction, dfops);
>  		if (error)
>  			goto del_cursor;
> @@ -6222,13 +6220,11 @@ xfs_bmap_shift_extents(
>  			*next_fsb = NULLFSBLOCK;
>  			break;
>  		}
> -		gotp = xfs_iext_get_ext(ifp, current_ext);
> +		xfs_iext_get_extent(ifp, current_ext, &got);
>  	}
>  
> -	if (!*done) {
> -		xfs_bmbt_get_all(gotp, &got);
> +	if (!*done)
>  		*next_fsb = got.br_startoff;
> -	}
>  
>  del_cursor:
>  	if (cur)
> -- 
> 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