a few extent lookup cleanups

a few extent lookup cleanups

[Christoph Hellwig]

Hi all,

this series contains some relatively minor refactoring to hide
details of the extent list implementation.  With it the raw extent
records aren't exposed outside the actual implementation and the
low-level add/del extent routines.

[PATCH 1/8] xfs: add a xfs_iext_update_extent helper

[Christoph Hellwig]

This helper is used to update an extent record based on the extent index,
and can be used to provide a level of abstractions between callers that
want to modify in-core extent records and the details of the extent list
implementation.

Also switch all users of the xfs_bmbt_set_all(xfs_iext_get_ext(...))
pattern to this new helper.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c       | 12 ++++++------
 fs/xfs/libxfs/xfs_inode_fork.c | 12 ++++++++++++
 fs/xfs/libxfs/xfs_inode_fork.h |  2 ++
 3 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index c09c16b1ad3b..502f531e4634 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4918,7 +4918,7 @@ xfs_bmap_del_extent_delay(
 		da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip,
 				got->br_blockcount), da_old);
 		got->br_startblock = nullstartblock((int)da_new);
-		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
+		xfs_iext_update_extent(ifp, *idx, got);
 		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
 		break;
 	case BMAP_RIGHT_CONTIG:
@@ -4930,7 +4930,7 @@ xfs_bmap_del_extent_delay(
 		da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip,
 				got->br_blockcount), da_old);
 		got->br_startblock = nullstartblock((int)da_new);
-		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
+		xfs_iext_update_extent(ifp, *idx, got);
 		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
 		break;
 	case 0:
@@ -4956,7 +4956,7 @@ xfs_bmap_del_extent_delay(
 						       del->br_blockcount);
 
 		got->br_startblock = nullstartblock((int)got_indlen);
-		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
+		xfs_iext_update_extent(ifp, *idx, got);
 		trace_xfs_bmap_post_update(ip, *idx, 0, _THIS_IP_);
 
 		new.br_startoff = del_endoff;
@@ -5026,7 +5026,7 @@ xfs_bmap_del_extent_cow(
 		got->br_startoff = del_endoff;
 		got->br_blockcount -= del->br_blockcount;
 		got->br_startblock = del->br_startblock + del->br_blockcount;
-		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
+		xfs_iext_update_extent(ifp, *idx, got);
 		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
 		break;
 	case BMAP_RIGHT_CONTIG:
@@ -5035,7 +5035,7 @@ xfs_bmap_del_extent_cow(
 		 */
 		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
 		got->br_blockcount -= del->br_blockcount;
-		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
+		xfs_iext_update_extent(ifp, *idx, got);
 		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
 		break;
 	case 0:
@@ -5044,7 +5044,7 @@ xfs_bmap_del_extent_cow(
 		 */
 		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
 		got->br_blockcount = del->br_startoff - got->br_startoff;
-		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
+		xfs_iext_update_extent(ifp, *idx, got);
 		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
 
 		new.br_startoff = del_endoff;
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 0e80f34fe97c..fb310d08dc82 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -2023,3 +2023,15 @@ xfs_iext_get_extent(
 	xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), gotp);
 	return true;
 }
+
+void
+xfs_iext_update_extent(
+	struct xfs_ifork	*ifp,
+	xfs_extnum_t		idx,
+	struct xfs_bmbt_irec	*gotp)
+{
+	ASSERT(idx >= 0);
+	ASSERT(idx < xfs_iext_count(ifp));
+
+	xfs_bmbt_set_all(xfs_iext_get_ext(ifp, idx), gotp);
+}
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 7fb8365326d1..11af705219f6 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -187,6 +187,8 @@ bool		xfs_iext_lookup_extent(struct xfs_inode *ip,
 			xfs_extnum_t *idxp, struct xfs_bmbt_irec *gotp);
 bool		xfs_iext_get_extent(struct xfs_ifork *ifp, xfs_extnum_t idx,
 			struct xfs_bmbt_irec *gotp);
+void		xfs_iext_update_extent(struct xfs_ifork *ifp, xfs_extnum_t idx,
+			struct xfs_bmbt_irec *gotp);
 
 extern struct kmem_zone	*xfs_ifork_zone;
 
-- 
2.11.0

Re: [PATCH 1/8] xfs: add a xfs_iext_update_extent helper

[Darrick J. Wong]

On Tue, Aug 29, 2017 at 07:48:28PM +0200, Christoph Hellwig wrote:
> This helper is used to update an extent record based on the extent index,
> and can be used to provide a level of abstractions between callers that
> want to modify in-core extent records and the details of the extent list
> implementation.
> 
> Also switch all users of the xfs_bmbt_set_all(xfs_iext_get_ext(...))
> pattern to this new helper.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

> ---
>  fs/xfs/libxfs/xfs_bmap.c       | 12 ++++++------
>  fs/xfs/libxfs/xfs_inode_fork.c | 12 ++++++++++++
>  fs/xfs/libxfs/xfs_inode_fork.h |  2 ++
>  3 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index c09c16b1ad3b..502f531e4634 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4918,7 +4918,7 @@ xfs_bmap_del_extent_delay(
>  		da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip,
>  				got->br_blockcount), da_old);
>  		got->br_startblock = nullstartblock((int)da_new);
> -		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> +		xfs_iext_update_extent(ifp, *idx, got);
>  		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
>  		break;
>  	case BMAP_RIGHT_CONTIG:
> @@ -4930,7 +4930,7 @@ xfs_bmap_del_extent_delay(
>  		da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip,
>  				got->br_blockcount), da_old);
>  		got->br_startblock = nullstartblock((int)da_new);
> -		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> +		xfs_iext_update_extent(ifp, *idx, got);
>  		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
>  		break;
>  	case 0:
> @@ -4956,7 +4956,7 @@ xfs_bmap_del_extent_delay(
>  						       del->br_blockcount);
>  
>  		got->br_startblock = nullstartblock((int)got_indlen);
> -		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> +		xfs_iext_update_extent(ifp, *idx, got);
>  		trace_xfs_bmap_post_update(ip, *idx, 0, _THIS_IP_);
>  
>  		new.br_startoff = del_endoff;
> @@ -5026,7 +5026,7 @@ xfs_bmap_del_extent_cow(
>  		got->br_startoff = del_endoff;
>  		got->br_blockcount -= del->br_blockcount;
>  		got->br_startblock = del->br_startblock + del->br_blockcount;
> -		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> +		xfs_iext_update_extent(ifp, *idx, got);
>  		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
>  		break;
>  	case BMAP_RIGHT_CONTIG:
> @@ -5035,7 +5035,7 @@ xfs_bmap_del_extent_cow(
>  		 */
>  		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
>  		got->br_blockcount -= del->br_blockcount;
> -		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> +		xfs_iext_update_extent(ifp, *idx, got);
>  		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
>  		break;
>  	case 0:
> @@ -5044,7 +5044,7 @@ xfs_bmap_del_extent_cow(
>  		 */
>  		trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
>  		got->br_blockcount = del->br_startoff - got->br_startoff;
> -		xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> +		xfs_iext_update_extent(ifp, *idx, got);
>  		trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
>  
>  		new.br_startoff = del_endoff;
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 0e80f34fe97c..fb310d08dc82 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -2023,3 +2023,15 @@ xfs_iext_get_extent(
>  	xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), gotp);
>  	return true;
>  }
> +
> +void
> +xfs_iext_update_extent(
> +	struct xfs_ifork	*ifp,
> +	xfs_extnum_t		idx,
> +	struct xfs_bmbt_irec	*gotp)
> +{
> +	ASSERT(idx >= 0);
> +	ASSERT(idx < xfs_iext_count(ifp));
> +
> +	xfs_bmbt_set_all(xfs_iext_get_ext(ifp, idx), gotp);
> +}
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 7fb8365326d1..11af705219f6 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -187,6 +187,8 @@ bool		xfs_iext_lookup_extent(struct xfs_inode *ip,
>  			xfs_extnum_t *idxp, struct xfs_bmbt_irec *gotp);
>  bool		xfs_iext_get_extent(struct xfs_ifork *ifp, xfs_extnum_t idx,
>  			struct xfs_bmbt_irec *gotp);
> +void		xfs_iext_update_extent(struct xfs_ifork *ifp, xfs_extnum_t idx,
> +			struct xfs_bmbt_irec *gotp);
>  
>  extern struct kmem_zone	*xfs_ifork_zone;
>  
> -- 
> 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 2/8] xfs: switch xfs_bmap_local_to_extents to use xfs_iext_insert

[Christoph Hellwig]

Use the helper instead of open coding it, to provide a better abstraction
for the scalable extent list work.  This also gets an additional assert
and trace point for free.

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

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 502f531e4634..bbde43c99323 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -880,7 +880,7 @@ xfs_bmap_local_to_extents(
 	xfs_ifork_t	*ifp;		/* inode fork pointer */
 	xfs_alloc_arg_t	args;		/* allocation arguments */
 	xfs_buf_t	*bp;		/* buffer for extent block */
-	xfs_bmbt_rec_host_t *ep;	/* extent record pointer */
+	struct xfs_bmbt_irec rec;
 
 	/*
 	 * We don't want to deal with the case of keeping inode data inline yet.
@@ -943,9 +943,12 @@ xfs_bmap_local_to_extents(
 	xfs_bmap_local_to_extents_empty(ip, whichfork);
 	flags |= XFS_ILOG_CORE;
 
-	xfs_iext_add(ifp, 0, 1);
-	ep = xfs_iext_get_ext(ifp, 0);
-	xfs_bmbt_set_allf(ep, 0, args.fsbno, 1, XFS_EXT_NORM);
+	rec.br_startoff = 0;
+	rec.br_startblock = args.fsbno;
+	rec.br_blockcount = 1;
+	rec.br_state = XFS_EXT_NORM;
+	xfs_iext_insert(ip, 0, 1, &rec, 0);
+
 	trace_xfs_bmap_post_update(ip, 0,
 			whichfork == XFS_ATTR_FORK ? BMAP_ATTRFORK : 0,
 			_THIS_IP_);
-- 
2.11.0

Re: [PATCH 2/8] xfs: switch xfs_bmap_local_to_extents to use xfs_iext_insert

[Darrick J. Wong]

On Tue, Aug 29, 2017 at 07:48:29PM +0200, Christoph Hellwig wrote:
> Use the helper instead of open coding it, to provide a better abstraction
> for the scalable extent list work.  This also gets an additional assert
> and trace point for free.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

> ---
>  fs/xfs/libxfs/xfs_bmap.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 502f531e4634..bbde43c99323 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -880,7 +880,7 @@ xfs_bmap_local_to_extents(
>  	xfs_ifork_t	*ifp;		/* inode fork pointer */
>  	xfs_alloc_arg_t	args;		/* allocation arguments */
>  	xfs_buf_t	*bp;		/* buffer for extent block */
> -	xfs_bmbt_rec_host_t *ep;	/* extent record pointer */
> +	struct xfs_bmbt_irec rec;
>  
>  	/*
>  	 * We don't want to deal with the case of keeping inode data inline yet.
> @@ -943,9 +943,12 @@ xfs_bmap_local_to_extents(
>  	xfs_bmap_local_to_extents_empty(ip, whichfork);
>  	flags |= XFS_ILOG_CORE;
>  
> -	xfs_iext_add(ifp, 0, 1);
> -	ep = xfs_iext_get_ext(ifp, 0);
> -	xfs_bmbt_set_allf(ep, 0, args.fsbno, 1, XFS_EXT_NORM);
> +	rec.br_startoff = 0;
> +	rec.br_startblock = args.fsbno;
> +	rec.br_blockcount = 1;
> +	rec.br_state = XFS_EXT_NORM;
> +	xfs_iext_insert(ip, 0, 1, &rec, 0);
> +
>  	trace_xfs_bmap_post_update(ip, 0,
>  			whichfork == XFS_ATTR_FORK ? BMAP_ATTRFORK : 0,
>  			_THIS_IP_);
> -- 
> 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/8] xfs: use xfs_iext_get_extent in xfs_bmap_first_unused

[Christoph Hellwig]

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

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index bbde43c99323..c944987c485d 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1359,7 +1359,6 @@ xfs_bmap_first_unused(
 	xfs_fileoff_t	lastaddr;		/* last block number seen */
 	xfs_fileoff_t	lowest;			/* lowest useful block */
 	xfs_fileoff_t	max;			/* starting useful block */
-	xfs_fileoff_t	off;			/* offset for this block */
 	xfs_extnum_t	nextents;		/* number of extent entries */
 
 	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_BTREE ||
@@ -1376,16 +1375,19 @@ xfs_bmap_first_unused(
 	lowest = *first_unused;
 	nextents = xfs_iext_count(ifp);
 	for (idx = 0, lastaddr = 0, max = lowest; idx < nextents; idx++) {
-		xfs_bmbt_rec_host_t *ep = xfs_iext_get_ext(ifp, idx);
-		off = xfs_bmbt_get_startoff(ep);
+		struct xfs_bmbt_irec got;
+
+		xfs_iext_get_extent(ifp, idx, &got);
+
 		/*
 		 * See if the hole before this extent will work.
 		 */
-		if (off >= lowest + len && off - max >= len) {
+		if (got.br_startoff >= lowest + len &&
+		    got.br_startoff - max >= len) {
 			*first_unused = max;
 			return 0;
 		}
-		lastaddr = off + xfs_bmbt_get_blockcount(ep);
+		lastaddr = got.br_startoff + got.br_blockcount;
 		max = XFS_FILEOFF_MAX(lastaddr, lowest);
 	}
 	*first_unused = max;
-- 
2.11.0

Re: [PATCH 3/8] xfs: use xfs_iext_get_extent in xfs_bmap_first_unused

[Darrick J. Wong]

This probably ought to have a justification in the changelog, such as
"Use the bmap abstraction instead of open-coding bmbt details here."

Code otherwise looks ok.

--D

On Tue, Aug 29, 2017 at 07:48:30PM +0200, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index bbde43c99323..c944987c485d 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -1359,7 +1359,6 @@ xfs_bmap_first_unused(
>  	xfs_fileoff_t	lastaddr;		/* last block number seen */
>  	xfs_fileoff_t	lowest;			/* lowest useful block */
>  	xfs_fileoff_t	max;			/* starting useful block */
> -	xfs_fileoff_t	off;			/* offset for this block */
>  	xfs_extnum_t	nextents;		/* number of extent entries */
>  
>  	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_BTREE ||
> @@ -1376,16 +1375,19 @@ xfs_bmap_first_unused(
>  	lowest = *first_unused;
>  	nextents = xfs_iext_count(ifp);
>  	for (idx = 0, lastaddr = 0, max = lowest; idx < nextents; idx++) {
> -		xfs_bmbt_rec_host_t *ep = xfs_iext_get_ext(ifp, idx);
> -		off = xfs_bmbt_get_startoff(ep);
> +		struct xfs_bmbt_irec got;
> +
> +		xfs_iext_get_extent(ifp, idx, &got);
> +
>  		/*
>  		 * See if the hole before this extent will work.
>  		 */
> -		if (off >= lowest + len && off - max >= len) {
> +		if (got.br_startoff >= lowest + len &&
> +		    got.br_startoff - max >= len) {
>  			*first_unused = max;
>  			return 0;
>  		}
> -		lastaddr = off + xfs_bmbt_get_blockcount(ep);
> +		lastaddr = got.br_startoff + got.br_blockcount;
>  		max = XFS_FILEOFF_MAX(lastaddr, lowest);
>  	}
>  	*first_unused = max;
> -- 
> 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/8] xfs: use xfs_iext_get_extent in xfs_bmap_first_unused

[Christoph Hellwig]

On Tue, Aug 29, 2017 at 11:41:48AM -0700, Darrick J. Wong wrote:
> This probably ought to have a justification in the changelog, such as
> "Use the bmap abstraction instead of open-coding bmbt details here."
> 
> Code otherwise looks ok.

Sure.  Do you just want to add the above?

Re: [PATCH 3/8] xfs: use xfs_iext_get_extent in xfs_bmap_first_unused

[Darrick J. Wong]

On Fri, Sep 01, 2017 at 10:06:21PM +0200, Christoph Hellwig wrote:
> On Tue, Aug 29, 2017 at 11:41:48AM -0700, Darrick J. Wong wrote:
> > This probably ought to have a justification in the changelog, such as
> > "Use the bmap abstraction instead of open-coding bmbt details here."
> > 
> > Code otherwise looks ok.
> 
> Sure.  Do you just want to add the above?

Done.

--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 4/8] xfs: move some code around inside xfs_bmap_shift_extents

[Christoph Hellwig]

For the first right move we need to look up next_fsb.  That means
our last fsb that contains next_fsb must also be the current extent,
so take advantage of that by moving the code around a bit.

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

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index c944987c485d..6af3cc1fc630 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -6127,7 +6127,6 @@ xfs_bmap_shift_extents(
 	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 	ASSERT(direction == SHIFT_LEFT || direction == SHIFT_RIGHT);
-	ASSERT(*next_fsb != NULLFSBLOCK || direction == SHIFT_RIGHT);
 
 	ifp = XFS_IFORK_PTR(ip, whichfork);
 	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
@@ -6159,43 +6158,48 @@ xfs_bmap_shift_extents(
 	 * In case of first right shift, we need to initialize next_fsb
 	 */
 	if (*next_fsb == NULLFSBLOCK) {
-		gotp = xfs_iext_get_ext(ifp, total_extents - 1);
+		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) {
 			*done = 1;
 			goto del_cursor;
 		}
+	} 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.
+		 */
+		gotp = xfs_iext_bno_to_ext(ifp, *next_fsb, &current_ext);
+		if (!gotp) {
+			*done = 1;
+			goto del_cursor;
+		}
 	}
 
 	/* Lookup the extent index at which we have to stop */
 	if (direction == SHIFT_RIGHT) {
-		gotp = xfs_iext_bno_to_ext(ifp, stop_fsb, &stop_extent);
+		xfs_iext_bno_to_ext(ifp, stop_fsb, &stop_extent);
 		/* Make stop_extent exclusive of shift range */
 		stop_extent--;
-	} else
+		if (current_ext <= stop_extent) {
+			error = -EIO;
+			goto del_cursor;
+		}
+	} else {
 		stop_extent = total_extents;
-
-	/*
-	 * 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.
-	 */
-	gotp = xfs_iext_bno_to_ext(ifp, *next_fsb, &current_ext);
-	if (!gotp) {
-		*done = 1;
-		goto del_cursor;
-	}
-
-	/* some sanity checking before we finally start shifting extents */
-	if ((direction == SHIFT_LEFT && current_ext >= stop_extent) ||
-	     (direction == SHIFT_RIGHT && current_ext <= stop_extent)) {
-		error = -EIO;
-		goto del_cursor;
+		if (current_ext >= stop_extent) {
+			error = -EIO;
+			goto del_cursor;
+		}
 	}
 
 	while (nexts++ < num_exts) {
-- 
2.11.0

Re: [PATCH 4/8] xfs: move some code around inside xfs_bmap_shift_extents

[Darrick J. Wong]

On Tue, Aug 29, 2017 at 07:48:31PM +0200, Christoph Hellwig wrote:
> For the first right move we need to look up next_fsb.  That means
> our last fsb that contains next_fsb must also be the current extent,
> so take advantage of that by moving the code around a bit.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Though I gotta ask, for the cases where we check current_ext against
stop_extent and return -EIO, isn't that only possible if there's
something seriously wrong with the extent map?  In which case
-EFSCORRUPTED might be a more appropriate choice?

(Separate fix, obviously...)

--D

> ---
>  fs/xfs/libxfs/xfs_bmap.c | 54 ++++++++++++++++++++++++++----------------------
>  1 file changed, 29 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index c944987c485d..6af3cc1fc630 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -6127,7 +6127,6 @@ xfs_bmap_shift_extents(
>  	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
>  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>  	ASSERT(direction == SHIFT_LEFT || direction == SHIFT_RIGHT);
> -	ASSERT(*next_fsb != NULLFSBLOCK || direction == SHIFT_RIGHT);
>  
>  	ifp = XFS_IFORK_PTR(ip, whichfork);
>  	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> @@ -6159,43 +6158,48 @@ xfs_bmap_shift_extents(
>  	 * In case of first right shift, we need to initialize next_fsb
>  	 */
>  	if (*next_fsb == NULLFSBLOCK) {
> -		gotp = xfs_iext_get_ext(ifp, total_extents - 1);
> +		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) {
>  			*done = 1;
>  			goto del_cursor;
>  		}
> +	} 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.
> +		 */
> +		gotp = xfs_iext_bno_to_ext(ifp, *next_fsb, &current_ext);
> +		if (!gotp) {
> +			*done = 1;
> +			goto del_cursor;
> +		}
>  	}
>  
>  	/* Lookup the extent index at which we have to stop */
>  	if (direction == SHIFT_RIGHT) {
> -		gotp = xfs_iext_bno_to_ext(ifp, stop_fsb, &stop_extent);
> +		xfs_iext_bno_to_ext(ifp, stop_fsb, &stop_extent);
>  		/* Make stop_extent exclusive of shift range */
>  		stop_extent--;
> -	} else
> +		if (current_ext <= stop_extent) {
> +			error = -EIO;
> +			goto del_cursor;
> +		}
> +	} else {
>  		stop_extent = total_extents;
> -
> -	/*
> -	 * 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.
> -	 */
> -	gotp = xfs_iext_bno_to_ext(ifp, *next_fsb, &current_ext);
> -	if (!gotp) {
> -		*done = 1;
> -		goto del_cursor;
> -	}
> -
> -	/* some sanity checking before we finally start shifting extents */
> -	if ((direction == SHIFT_LEFT && current_ext >= stop_extent) ||
> -	     (direction == SHIFT_RIGHT && current_ext <= stop_extent)) {
> -		error = -EIO;
> -		goto del_cursor;
> +		if (current_ext >= stop_extent) {
> +			error = -EIO;
> +			goto del_cursor;
> +		}
>  	}
>  
>  	while (nexts++ < num_exts) {
> -- 
> 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/8] xfs: use xfs_iext_*_extent helpers in xfs_bmap_shift_extents

[Christoph Hellwig]

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

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

[Darrick J. Wong]

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

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

[Darrick J. Wong]

On Tue, Aug 29, 2017 at 05:05:41PM -0700, Darrick J. Wong wrote:
> 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>

Tests ok, but I've since noticed the following:

<scroll down>

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

So one subtlety of the xfs_rmap_{,un}map_extent methods is that because
they take as input an inode fork mapping (as opposed to an rmap record),
these functions are free to merge rmap records under the hood.  That's
why the old code worked fine, even if the way the call sites were
structured was (quite obviously) misleading.

You can replace that whole hunk of code with:

	/* update reverse mapping */
	error = xfs_rmap_unmap_extent(mp, dfops, ip, whichfork, got);
	if (error)
		return error;
	memcpy(&new, got, sizeof(new));
	new.br_startoff = left->br_startoff + left->br_blockcount;
	return xfs_rmap_map_extent(mp, dfops, ip, whichfork, &new);

And it works fine, at least for all the fcollapse tests including the
new xfs/706.  Plus we save one deferred op.

--D

> >  }
> >  
> >  /*
> > @@ -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
> --
> 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/8] xfs: use xfs_iext_*_extent helpers in xfs_bmap_split_extent_at

[Christoph Hellwig]

This abstracts the function away from details of the low-level extent
list implementation.

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

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 02725becedfa..51571113b270 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -6253,7 +6253,6 @@ xfs_bmap_split_extent_at(
 {
 	int				whichfork = XFS_DATA_FORK;
 	struct xfs_btree_cur		*cur = NULL;
-	struct xfs_bmbt_rec_host	*gotp;
 	struct xfs_bmbt_irec		got;
 	struct xfs_bmbt_irec		new; /* split extent */
 	struct xfs_mount		*mp = ip->i_mount;
@@ -6285,21 +6284,10 @@ xfs_bmap_split_extent_at(
 	}
 
 	/*
-	 * gotp can be null in 2 cases: 1) if there are no extents
-	 * or 2) split_fsb lies in a hole beyond which there are
-	 * no extents. Either way, we are done.
+	 * If there are not extents, or split_fsb lies in a hole we are done.
 	 */
-	gotp = xfs_iext_bno_to_ext(ifp, split_fsb, &current_ext);
-	if (!gotp)
-		return 0;
-
-	xfs_bmbt_get_all(gotp, &got);
-
-	/*
-	 * Check split_fsb lies in a hole or the start boundary offset
-	 * of the extent.
-	 */
-	if (got.br_startoff >= split_fsb)
+	if (!xfs_iext_lookup_extent(ip, ifp, split_fsb, &current_ext, &got) ||
+	    got.br_startoff >= split_fsb)
 		return 0;
 
 	gotblkcnt = split_fsb - got.br_startoff;
@@ -6322,8 +6310,8 @@ xfs_bmap_split_extent_at(
 		XFS_WANT_CORRUPTED_GOTO(mp, i == 1, del_cursor);
 	}
 
-	xfs_bmbt_set_blockcount(gotp, gotblkcnt);
 	got.br_blockcount = gotblkcnt;
+	xfs_iext_update_extent(ifp, current_ext, &got);
 
 	logflags = XFS_ILOG_CORE;
 	if (cur) {
-- 
2.11.0

Re: [PATCH 6/8] xfs: use xfs_iext_*_extent helpers in xfs_bmap_split_extent_at

[Darrick J. Wong]

On Tue, Aug 29, 2017 at 07:48:33PM +0200, Christoph Hellwig wrote:
> This abstracts the function away from details of the low-level extent
> list implementation.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

> ---
>  fs/xfs/libxfs/xfs_bmap.c | 20 ++++----------------
>  1 file changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 02725becedfa..51571113b270 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -6253,7 +6253,6 @@ xfs_bmap_split_extent_at(
>  {
>  	int				whichfork = XFS_DATA_FORK;
>  	struct xfs_btree_cur		*cur = NULL;
> -	struct xfs_bmbt_rec_host	*gotp;
>  	struct xfs_bmbt_irec		got;
>  	struct xfs_bmbt_irec		new; /* split extent */
>  	struct xfs_mount		*mp = ip->i_mount;
> @@ -6285,21 +6284,10 @@ xfs_bmap_split_extent_at(
>  	}
>  
>  	/*
> -	 * gotp can be null in 2 cases: 1) if there are no extents
> -	 * or 2) split_fsb lies in a hole beyond which there are
> -	 * no extents. Either way, we are done.
> +	 * If there are not extents, or split_fsb lies in a hole we are done.
>  	 */
> -	gotp = xfs_iext_bno_to_ext(ifp, split_fsb, &current_ext);
> -	if (!gotp)
> -		return 0;
> -
> -	xfs_bmbt_get_all(gotp, &got);
> -
> -	/*
> -	 * Check split_fsb lies in a hole or the start boundary offset
> -	 * of the extent.
> -	 */
> -	if (got.br_startoff >= split_fsb)
> +	if (!xfs_iext_lookup_extent(ip, ifp, split_fsb, &current_ext, &got) ||
> +	    got.br_startoff >= split_fsb)
>  		return 0;
>  
>  	gotblkcnt = split_fsb - got.br_startoff;
> @@ -6322,8 +6310,8 @@ xfs_bmap_split_extent_at(
>  		XFS_WANT_CORRUPTED_GOTO(mp, i == 1, del_cursor);
>  	}
>  
> -	xfs_bmbt_set_blockcount(gotp, gotblkcnt);
>  	got.br_blockcount = gotblkcnt;
> +	xfs_iext_update_extent(ifp, current_ext, &got);
>  
>  	logflags = XFS_ILOG_CORE;
>  	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

[PATCH 7/8] xfs: rewrite xfs_bmap_count_leaves using xfs_iext_get_extent

[Christoph Hellwig]

This avoids poking into the internals of the extent list.  Also return
the number of extents as the return value instead of an additional
by reference argument, and make it available to callers outside of
xfs_bmap_util.c

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_bmap_util.c | 21 ++++++++++-----------
 fs/xfs/xfs_bmap_util.h |  1 +
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 93e955262d07..ea737ebafc53 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -222,22 +222,21 @@ xfs_bmap_eof(
  * Count leaf blocks given a range of extent records.  Delayed allocation
  * extents are not counted towards the totals.
  */
-STATIC void
+xfs_extnum_t
 xfs_bmap_count_leaves(
 	struct xfs_ifork	*ifp,
-	xfs_extnum_t		*numrecs,
 	xfs_filblks_t		*count)
 {
-	xfs_extnum_t		i;
-	xfs_extnum_t		nr_exts = xfs_iext_count(ifp);
-
-	for (i = 0; i < nr_exts; i++) {
-		xfs_bmbt_rec_host_t *frp = xfs_iext_get_ext(ifp, i);
-		if (!isnullstartblock(xfs_bmbt_get_startblock(frp))) {
-			(*numrecs)++;
-			*count += xfs_bmbt_get_blockcount(frp);
+	struct xfs_bmbt_irec	got;
+	xfs_extnum_t		numrecs = 0, i = 0;
+
+	while (xfs_iext_get_extent(ifp, i++, &got)) {
+		if (!isnullstartblock(got.br_startblock)) {
+			*count += got.br_blockcount;
+			numrecs++;
 		}
 	}
+	return numrecs;
 }
 
 /*
@@ -370,7 +369,7 @@ xfs_bmap_count_blocks(
 
 	switch (XFS_IFORK_FORMAT(ip, whichfork)) {
 	case XFS_DINODE_FMT_EXTENTS:
-		xfs_bmap_count_leaves(ifp, nextents, count);
+		*nextents = xfs_bmap_count_leaves(ifp, count);
 		return 0;
 	case XFS_DINODE_FMT_BTREE:
 		if (!(ifp->if_flags & XFS_IFEXTENTS)) {
diff --git a/fs/xfs/xfs_bmap_util.h b/fs/xfs/xfs_bmap_util.h
index 0cede1043571..0eaa81dc49be 100644
--- a/fs/xfs/xfs_bmap_util.h
+++ b/fs/xfs/xfs_bmap_util.h
@@ -70,6 +70,7 @@ int	xfs_swap_extents(struct xfs_inode *ip, struct xfs_inode *tip,
 
 xfs_daddr_t xfs_fsb_to_db(struct xfs_inode *ip, xfs_fsblock_t fsb);
 
+xfs_extnum_t xfs_bmap_count_leaves(struct xfs_ifork *ifp, xfs_filblks_t *count);
 int xfs_bmap_count_blocks(struct xfs_trans *tp, struct xfs_inode *ip,
 			  int whichfork, xfs_extnum_t *nextents,
 			  xfs_filblks_t *count);
-- 
2.11.0

Re: [PATCH 7/8] xfs: rewrite xfs_bmap_count_leaves using xfs_iext_get_extent

[Darrick J. Wong]

On Tue, Aug 29, 2017 at 07:48:34PM +0200, Christoph Hellwig wrote:
> This avoids poking into the internals of the extent list.  Also return
> the number of extents as the return value instead of an additional
> by reference argument, and make it available to callers outside of
> xfs_bmap_util.c
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

> ---
>  fs/xfs/xfs_bmap_util.c | 21 ++++++++++-----------
>  fs/xfs/xfs_bmap_util.h |  1 +
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 93e955262d07..ea737ebafc53 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -222,22 +222,21 @@ xfs_bmap_eof(
>   * Count leaf blocks given a range of extent records.  Delayed allocation
>   * extents are not counted towards the totals.
>   */
> -STATIC void
> +xfs_extnum_t
>  xfs_bmap_count_leaves(
>  	struct xfs_ifork	*ifp,
> -	xfs_extnum_t		*numrecs,
>  	xfs_filblks_t		*count)
>  {
> -	xfs_extnum_t		i;
> -	xfs_extnum_t		nr_exts = xfs_iext_count(ifp);
> -
> -	for (i = 0; i < nr_exts; i++) {
> -		xfs_bmbt_rec_host_t *frp = xfs_iext_get_ext(ifp, i);
> -		if (!isnullstartblock(xfs_bmbt_get_startblock(frp))) {
> -			(*numrecs)++;
> -			*count += xfs_bmbt_get_blockcount(frp);
> +	struct xfs_bmbt_irec	got;
> +	xfs_extnum_t		numrecs = 0, i = 0;
> +
> +	while (xfs_iext_get_extent(ifp, i++, &got)) {
> +		if (!isnullstartblock(got.br_startblock)) {
> +			*count += got.br_blockcount;
> +			numrecs++;
>  		}
>  	}
> +	return numrecs;
>  }
>  
>  /*
> @@ -370,7 +369,7 @@ xfs_bmap_count_blocks(
>  
>  	switch (XFS_IFORK_FORMAT(ip, whichfork)) {
>  	case XFS_DINODE_FMT_EXTENTS:
> -		xfs_bmap_count_leaves(ifp, nextents, count);
> +		*nextents = xfs_bmap_count_leaves(ifp, count);
>  		return 0;
>  	case XFS_DINODE_FMT_BTREE:
>  		if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> diff --git a/fs/xfs/xfs_bmap_util.h b/fs/xfs/xfs_bmap_util.h
> index 0cede1043571..0eaa81dc49be 100644
> --- a/fs/xfs/xfs_bmap_util.h
> +++ b/fs/xfs/xfs_bmap_util.h
> @@ -70,6 +70,7 @@ int	xfs_swap_extents(struct xfs_inode *ip, struct xfs_inode *tip,
>  
>  xfs_daddr_t xfs_fsb_to_db(struct xfs_inode *ip, xfs_fsblock_t fsb);
>  
> +xfs_extnum_t xfs_bmap_count_leaves(struct xfs_ifork *ifp, xfs_filblks_t *count);
>  int xfs_bmap_count_blocks(struct xfs_trans *tp, struct xfs_inode *ip,
>  			  int whichfork, xfs_extnum_t *nextents,
>  			  xfs_filblks_t *count);
> -- 
> 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 8/8] xfs: replace xfs_qm_get_rtblks with a direct call to xfs_bmap_count_leaves

[Christoph Hellwig]

Use the existing functionality instead of directly poking into the extent
list.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_qm.c | 44 ++++++++++++--------------------------------
 1 file changed, 12 insertions(+), 32 deletions(-)

diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 15751dc2a27d..010a13a201aa 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -31,6 +31,7 @@
 #include "xfs_error.h"
 #include "xfs_bmap.h"
 #include "xfs_bmap_btree.h"
+#include "xfs_bmap_util.h"
 #include "xfs_trans.h"
 #include "xfs_trans_space.h"
 #include "xfs_qm.h"
@@ -1120,31 +1121,6 @@ xfs_qm_quotacheck_dqadjust(
 	return 0;
 }
 
-STATIC int
-xfs_qm_get_rtblks(
-	xfs_inode_t	*ip,
-	xfs_qcnt_t	*O_rtblks)
-{
-	xfs_filblks_t	rtblks;			/* total rt blks */
-	xfs_extnum_t	idx;			/* extent record index */
-	xfs_ifork_t	*ifp;			/* inode fork pointer */
-	xfs_extnum_t	nextents;		/* number of extent entries */
-	int		error;
-
-	ASSERT(XFS_IS_REALTIME_INODE(ip));
-	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
-	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
-		if ((error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK)))
-			return error;
-	}
-	rtblks = 0;
-	nextents = xfs_iext_count(ifp);
-	for (idx = 0; idx < nextents; idx++)
-		rtblks += xfs_bmbt_get_blockcount(xfs_iext_get_ext(ifp, idx));
-	*O_rtblks = (xfs_qcnt_t)rtblks;
-	return 0;
-}
-
 /*
  * callback routine supplied to bulkstat(). Given an inumber, find its
  * dquots and update them to account for resources taken by that inode.
@@ -1160,7 +1136,8 @@ xfs_qm_dqusage_adjust(
 	int		*res)		/* result code value */
 {
 	xfs_inode_t	*ip;
-	xfs_qcnt_t	nblks, rtblks = 0;
+	xfs_qcnt_t	nblks;
+	xfs_filblks_t	rtblks = 0;	/* total rt blks */
 	int		error;
 
 	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
@@ -1190,12 +1167,15 @@ xfs_qm_dqusage_adjust(
 	ASSERT(ip->i_delayed_blks == 0);
 
 	if (XFS_IS_REALTIME_INODE(ip)) {
-		/*
-		 * Walk thru the extent list and count the realtime blocks.
-		 */
-		error = xfs_qm_get_rtblks(ip, &rtblks);
-		if (error)
-			goto error0;
+		struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
+
+		if (!(ifp->if_flags & XFS_IFEXTENTS)) {
+			error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK);
+			if (error)
+				goto error0;
+		}
+
+		xfs_bmap_count_leaves(ifp, &rtblks);
 	}
 
 	nblks = (xfs_qcnt_t)ip->i_d.di_nblocks - rtblks;
-- 
2.11.0

Re: [PATCH 8/8] xfs: replace xfs_qm_get_rtblks with a direct call to xfs_bmap_count_leaves

[Darrick J. Wong]

On Tue, Aug 29, 2017 at 07:48:35PM +0200, Christoph Hellwig wrote:
> Use the existing functionality instead of directly poking into the extent
> list.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

> ---
>  fs/xfs/xfs_qm.c | 44 ++++++++++++--------------------------------
>  1 file changed, 12 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 15751dc2a27d..010a13a201aa 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -31,6 +31,7 @@
>  #include "xfs_error.h"
>  #include "xfs_bmap.h"
>  #include "xfs_bmap_btree.h"
> +#include "xfs_bmap_util.h"
>  #include "xfs_trans.h"
>  #include "xfs_trans_space.h"
>  #include "xfs_qm.h"
> @@ -1120,31 +1121,6 @@ xfs_qm_quotacheck_dqadjust(
>  	return 0;
>  }
>  
> -STATIC int
> -xfs_qm_get_rtblks(
> -	xfs_inode_t	*ip,
> -	xfs_qcnt_t	*O_rtblks)
> -{
> -	xfs_filblks_t	rtblks;			/* total rt blks */
> -	xfs_extnum_t	idx;			/* extent record index */
> -	xfs_ifork_t	*ifp;			/* inode fork pointer */
> -	xfs_extnum_t	nextents;		/* number of extent entries */
> -	int		error;
> -
> -	ASSERT(XFS_IS_REALTIME_INODE(ip));
> -	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> -	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> -		if ((error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK)))
> -			return error;
> -	}
> -	rtblks = 0;
> -	nextents = xfs_iext_count(ifp);
> -	for (idx = 0; idx < nextents; idx++)
> -		rtblks += xfs_bmbt_get_blockcount(xfs_iext_get_ext(ifp, idx));
> -	*O_rtblks = (xfs_qcnt_t)rtblks;
> -	return 0;
> -}
> -
>  /*
>   * callback routine supplied to bulkstat(). Given an inumber, find its
>   * dquots and update them to account for resources taken by that inode.
> @@ -1160,7 +1136,8 @@ xfs_qm_dqusage_adjust(
>  	int		*res)		/* result code value */
>  {
>  	xfs_inode_t	*ip;
> -	xfs_qcnt_t	nblks, rtblks = 0;
> +	xfs_qcnt_t	nblks;
> +	xfs_filblks_t	rtblks = 0;	/* total rt blks */
>  	int		error;
>  
>  	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
> @@ -1190,12 +1167,15 @@ xfs_qm_dqusage_adjust(
>  	ASSERT(ip->i_delayed_blks == 0);
>  
>  	if (XFS_IS_REALTIME_INODE(ip)) {
> -		/*
> -		 * Walk thru the extent list and count the realtime blocks.
> -		 */
> -		error = xfs_qm_get_rtblks(ip, &rtblks);
> -		if (error)
> -			goto error0;
> +		struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> +
> +		if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> +			error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK);
> +			if (error)
> +				goto error0;
> +		}
> +
> +		xfs_bmap_count_leaves(ifp, &rtblks);
>  	}
>  
>  	nblks = (xfs_qcnt_t)ip->i_d.di_nblocks - rtblks;
> -- 
> 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 9/8] xfs: simplify the rmap code in xfs_bmse_merge

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In Christoph's patch to refactor xfs_bmse_merge, the updated rmap code
does more work than it needs to (because map-extent auto-merges
records).  Remove the unnecessary unmap and save ourselves a deferred
op.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_bmap.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 9b87702..9558f5e 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5942,13 +5942,12 @@ xfs_bmse_merge(
 	xfs_iext_update_extent(ifp, current_ext - 1, &new);
 	xfs_iext_remove(ip, current_ext, 1, 0);
 
-	/* update reverse mapping */
+	/* update reverse mapping. rmap functions merge the rmaps for us */
 	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;
+	memcpy(&new, got, sizeof(new));
+	new.br_startoff = left->br_startoff + left->br_blockcount;
 	return xfs_rmap_map_extent(mp, dfops, ip, whichfork, &new);
 }

Re: [PATCH 9/8] xfs: simplify the rmap code in xfs_bmse_merge

[Christoph Hellwig]

On Wed, Aug 30, 2017 at 04:10:47PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In Christoph's patch to refactor xfs_bmse_merge, the updated rmap code
> does more work than it needs to (because map-extent auto-merges
> records).  Remove the unnecessary unmap and save ourselves a deferred
> op.

Looks fine,

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