diff mbox series

[2/3] xfs: xfs_buf_item_size_segment() needs to pass segment offset

Message ID 20210223044636.3280862-3-david@fromorbit.com (mailing list archive)
State Superseded
Headers show
Series xfs: buffer log item optimisations | expand

Commit Message

Dave Chinner Feb. 23, 2021, 4:46 a.m. UTC 
From: Dave Chinner <dchinner@redhat.com>

Otherwise it doesn't correctly calculate the number of vectors
in a logged buffer that has a contiguous map that gets split into
multiple regions because the range spans discontigous memory.

Probably never been hit in practice - we don't log contiguous ranges
on unmapped buffers (inode clusters).

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_buf_item.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

Comments

Darrick J. Wong Feb. 24, 2021, 9:34 p.m. UTC | #1 
On Tue, Feb 23, 2021 at 03:46:35PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Otherwise it doesn't correctly calculate the number of vectors
> in a logged buffer that has a contiguous map that gets split into
> multiple regions because the range spans discontigous memory.
> 
> Probably never been hit in practice - we don't log contiguous ranges
> on unmapped buffers (inode clusters).
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Subtle.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_buf_item.c | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 0628a65d9c55..91dc7d8c9739 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -55,6 +55,18 @@ xfs_buf_log_format_size(
>  			(blfp->blf_map_size * sizeof(blfp->blf_data_map[0]));
>  }
>  
> +static inline bool
> +xfs_buf_item_straddle(
> +	struct xfs_buf		*bp,
> +	uint			offset,
> +	int			next_bit,
> +	int			last_bit)
> +{
> +	return xfs_buf_offset(bp, offset + (next_bit << XFS_BLF_SHIFT)) !=
> +		(xfs_buf_offset(bp, offset + (last_bit << XFS_BLF_SHIFT)) +
> +		 XFS_BLF_CHUNK);
> +}
> +
>  /*
>   * Return the number of log iovecs and space needed to log the given buf log
>   * item segment.
> @@ -67,6 +79,7 @@ STATIC void
>  xfs_buf_item_size_segment(
>  	struct xfs_buf_log_item		*bip,
>  	struct xfs_buf_log_format	*blfp,
> +	uint				offset,
>  	int				*nvecs,
>  	int				*nbytes)
>  {
> @@ -101,12 +114,8 @@ xfs_buf_item_size_segment(
>  		 */
>  		if (next_bit == -1) {
>  			break;
> -		} else if (next_bit != last_bit + 1) {
> -			last_bit = next_bit;
> -			(*nvecs)++;
> -		} else if (xfs_buf_offset(bp, next_bit * XFS_BLF_CHUNK) !=
> -			   (xfs_buf_offset(bp, last_bit * XFS_BLF_CHUNK) +
> -			    XFS_BLF_CHUNK)) {
> +		} else if (next_bit != last_bit + 1 ||
> +		           xfs_buf_item_straddle(bp, offset, next_bit, last_bit)) {
>  			last_bit = next_bit;
>  			(*nvecs)++;
>  		} else {
> @@ -141,8 +150,10 @@ xfs_buf_item_size(
>  	int			*nbytes)
>  {
>  	struct xfs_buf_log_item	*bip = BUF_ITEM(lip);
> +	struct xfs_buf		*bp = bip->bli_buf;
>  	int			i;
>  	int			bytes;
> +	uint			offset = 0;
>  
>  	ASSERT(atomic_read(&bip->bli_refcount) > 0);
>  	if (bip->bli_flags & XFS_BLI_STALE) {
> @@ -184,8 +195,9 @@ xfs_buf_item_size(
>  	 */
>  	bytes = 0;
>  	for (i = 0; i < bip->bli_format_count; i++) {
> -		xfs_buf_item_size_segment(bip, &bip->bli_formats[i],
> +		xfs_buf_item_size_segment(bip, &bip->bli_formats[i], offset,
>  					  nvecs, &bytes);
> +		offset += BBTOB(bp->b_maps[i].bm_len);
>  	}
>  
>  	/*
> @@ -212,18 +224,6 @@ xfs_buf_item_copy_iovec(
>  			nbits * XFS_BLF_CHUNK);
>  }
>  
> -static inline bool
> -xfs_buf_item_straddle(
> -	struct xfs_buf		*bp,
> -	uint			offset,
> -	int			next_bit,
> -	int			last_bit)
> -{
> -	return xfs_buf_offset(bp, offset + (next_bit << XFS_BLF_SHIFT)) !=
> -		(xfs_buf_offset(bp, offset + (last_bit << XFS_BLF_SHIFT)) +
> -		 XFS_BLF_CHUNK);
> -}
> -
>  static void
>  xfs_buf_item_format_segment(
>  	struct xfs_buf_log_item	*bip,
> -- 
> 2.28.0
>
Brian Foster March 2, 2021, 2:37 p.m. UTC | #2 
On Tue, Feb 23, 2021 at 03:46:35PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Otherwise it doesn't correctly calculate the number of vectors
> in a logged buffer that has a contiguous map that gets split into
> multiple regions because the range spans discontigous memory.
> 
> Probably never been hit in practice - we don't log contiguous ranges
> on unmapped buffers (inode clusters).
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_buf_item.c | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 0628a65d9c55..91dc7d8c9739 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -55,6 +55,18 @@ xfs_buf_log_format_size(
>  			(blfp->blf_map_size * sizeof(blfp->blf_data_map[0]));
>  }
>  
> +static inline bool
> +xfs_buf_item_straddle(
> +	struct xfs_buf		*bp,
> +	uint			offset,
> +	int			next_bit,
> +	int			last_bit)
> +{
> +	return xfs_buf_offset(bp, offset + (next_bit << XFS_BLF_SHIFT)) !=
> +		(xfs_buf_offset(bp, offset + (last_bit << XFS_BLF_SHIFT)) +
> +		 XFS_BLF_CHUNK);
> +}
> +
>  /*
>   * Return the number of log iovecs and space needed to log the given buf log
>   * item segment.
> @@ -67,6 +79,7 @@ STATIC void
>  xfs_buf_item_size_segment(
>  	struct xfs_buf_log_item		*bip,
>  	struct xfs_buf_log_format	*blfp,
> +	uint				offset,
>  	int				*nvecs,
>  	int				*nbytes)
>  {
> @@ -101,12 +114,8 @@ xfs_buf_item_size_segment(
>  		 */
>  		if (next_bit == -1) {
>  			break;
> -		} else if (next_bit != last_bit + 1) {
> -			last_bit = next_bit;
> -			(*nvecs)++;
> -		} else if (xfs_buf_offset(bp, next_bit * XFS_BLF_CHUNK) !=
> -			   (xfs_buf_offset(bp, last_bit * XFS_BLF_CHUNK) +
> -			    XFS_BLF_CHUNK)) {
> +		} else if (next_bit != last_bit + 1 ||
> +		           xfs_buf_item_straddle(bp, offset, next_bit, last_bit)) {
>  			last_bit = next_bit;
>  			(*nvecs)++;
>  		} else {
> @@ -141,8 +150,10 @@ xfs_buf_item_size(
>  	int			*nbytes)
>  {
>  	struct xfs_buf_log_item	*bip = BUF_ITEM(lip);
> +	struct xfs_buf		*bp = bip->bli_buf;
>  	int			i;
>  	int			bytes;
> +	uint			offset = 0;
>  
>  	ASSERT(atomic_read(&bip->bli_refcount) > 0);
>  	if (bip->bli_flags & XFS_BLI_STALE) {
> @@ -184,8 +195,9 @@ xfs_buf_item_size(
>  	 */
>  	bytes = 0;
>  	for (i = 0; i < bip->bli_format_count; i++) {
> -		xfs_buf_item_size_segment(bip, &bip->bli_formats[i],
> +		xfs_buf_item_size_segment(bip, &bip->bli_formats[i], offset,
>  					  nvecs, &bytes);
> +		offset += BBTOB(bp->b_maps[i].bm_len);
>  	}
>  
>  	/*
> @@ -212,18 +224,6 @@ xfs_buf_item_copy_iovec(
>  			nbits * XFS_BLF_CHUNK);
>  }
>  
> -static inline bool
> -xfs_buf_item_straddle(
> -	struct xfs_buf		*bp,
> -	uint			offset,
> -	int			next_bit,
> -	int			last_bit)
> -{
> -	return xfs_buf_offset(bp, offset + (next_bit << XFS_BLF_SHIFT)) !=
> -		(xfs_buf_offset(bp, offset + (last_bit << XFS_BLF_SHIFT)) +
> -		 XFS_BLF_CHUNK);
> -}
> -
>  static void
>  xfs_buf_item_format_segment(
>  	struct xfs_buf_log_item	*bip,
> -- 
> 2.28.0
>
diff mbox series

Patch

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 0628a65d9c55..91dc7d8c9739 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -55,6 +55,18 @@  xfs_buf_log_format_size(
 			(blfp->blf_map_size * sizeof(blfp->blf_data_map[0]));
 }
 
+static inline bool
+xfs_buf_item_straddle(
+	struct xfs_buf		*bp,
+	uint			offset,
+	int			next_bit,
+	int			last_bit)
+{
+	return xfs_buf_offset(bp, offset + (next_bit << XFS_BLF_SHIFT)) !=
+		(xfs_buf_offset(bp, offset + (last_bit << XFS_BLF_SHIFT)) +
+		 XFS_BLF_CHUNK);
+}
+
 /*
  * Return the number of log iovecs and space needed to log the given buf log
  * item segment.
@@ -67,6 +79,7 @@  STATIC void
 xfs_buf_item_size_segment(
 	struct xfs_buf_log_item		*bip,
 	struct xfs_buf_log_format	*blfp,
+	uint				offset,
 	int				*nvecs,
 	int				*nbytes)
 {
@@ -101,12 +114,8 @@  xfs_buf_item_size_segment(
 		 */
 		if (next_bit == -1) {
 			break;
-		} else if (next_bit != last_bit + 1) {
-			last_bit = next_bit;
-			(*nvecs)++;
-		} else if (xfs_buf_offset(bp, next_bit * XFS_BLF_CHUNK) !=
-			   (xfs_buf_offset(bp, last_bit * XFS_BLF_CHUNK) +
-			    XFS_BLF_CHUNK)) {
+		} else if (next_bit != last_bit + 1 ||
+		           xfs_buf_item_straddle(bp, offset, next_bit, last_bit)) {
 			last_bit = next_bit;
 			(*nvecs)++;
 		} else {
@@ -141,8 +150,10 @@  xfs_buf_item_size(
 	int			*nbytes)
 {
 	struct xfs_buf_log_item	*bip = BUF_ITEM(lip);
+	struct xfs_buf		*bp = bip->bli_buf;
 	int			i;
 	int			bytes;
+	uint			offset = 0;
 
 	ASSERT(atomic_read(&bip->bli_refcount) > 0);
 	if (bip->bli_flags & XFS_BLI_STALE) {
@@ -184,8 +195,9 @@  xfs_buf_item_size(
 	 */
 	bytes = 0;
 	for (i = 0; i < bip->bli_format_count; i++) {
-		xfs_buf_item_size_segment(bip, &bip->bli_formats[i],
+		xfs_buf_item_size_segment(bip, &bip->bli_formats[i], offset,
 					  nvecs, &bytes);
+		offset += BBTOB(bp->b_maps[i].bm_len);
 	}
 
 	/*
@@ -212,18 +224,6 @@  xfs_buf_item_copy_iovec(
 			nbits * XFS_BLF_CHUNK);
 }
 
-static inline bool
-xfs_buf_item_straddle(
-	struct xfs_buf		*bp,
-	uint			offset,
-	int			next_bit,
-	int			last_bit)
-{
-	return xfs_buf_offset(bp, offset + (next_bit << XFS_BLF_SHIFT)) !=
-		(xfs_buf_offset(bp, offset + (last_bit << XFS_BLF_SHIFT)) +
-		 XFS_BLF_CHUNK);
-}
-
 static void
 xfs_buf_item_format_segment(
 	struct xfs_buf_log_item	*bip,