diff mbox series

[6/8] xfs: use accessor functions for bitmap words

Message ID 169755742240.3167663.3888314487214346782.stgit@frogsfrogsfrogs (mailing list archive)
State Superseded
Headers show
Series xfs: refactor rtbitmap/summary macros | expand

Commit Message

Darrick J. Wong Oct. 17, 2023, 3:53 p.m. UTC
From: Darrick J. Wong <djwong@kernel.org>

Create get and set functions for rtbitmap words so that we can redefine
the ondisk format with a specific endianness.  Note that this requires
the definition of a distinct type for ondisk rtbitmap words so that the
compiler can perform proper typechecking as we go back and forth.

In the upcoming rtgroups feature, we're going to fix the problem that
rtwords are written in host endian order, which means we'll need the
distinct rtword/rtword_raw types.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_format.h   |    8 ++++
 fs/xfs/libxfs/xfs_rtbitmap.c |   78 +++++++++++++++++++++++++++++++-----------
 fs/xfs/libxfs/xfs_rtbitmap.h |    8 +++-
 fs/xfs/xfs_ondisk.h          |    3 ++
 4 files changed, 74 insertions(+), 23 deletions(-)

Comments

Christoph Hellwig Oct. 17, 2023, 6:53 p.m. UTC | #1
On Tue, Oct 17, 2023 at 08:53:21AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Create get and set functions for rtbitmap words so that we can redefine
> the ondisk format with a specific endianness.  Note that this requires
> the definition of a distinct type for ondisk rtbitmap words so that the
> compiler can perform proper typechecking as we go back and forth.
> 
> In the upcoming rtgroups feature, we're going to fix the problem that
> rtwords are written in host endian order, which means we'll need the
> distinct rtword/rtword_raw types.

So per the last round I'd much prefer no exposing the xfs_rtword_raw
to the callers.  I've cooked up the patch below to do this, and it
seems to survive the absolute basic testing so far.  One interesting
thing is that as far as I can tell all but one of the
xfs_trans_log_buf calls in the pre-existing code were wrong as they
were missing the usual '- 1' for the last parameter.

For reasons I can't explain the version with this patch also happens
to actually generate smaller binary code as well:

hch@brick:~/work/xfs$ size xfs_rtbitmap.o*
   text	   data	    bss	    dec	    hex	filename
   7763	      0	      0	   7763	   1e53	xfs_rtbitmap.o.new
   7833	      0	      0	   7833	   1e99	xfs_rtbitmap.o.old

---
diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index f8daaff947fce8..6ca48fe8a9e1d3 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -90,20 +90,35 @@ xfs_rtbuf_get(
 /* Convert an ondisk bitmap word to its incore representation. */
 inline xfs_rtword_t
 xfs_rtbitmap_getword(
-	struct xfs_mount	*mp,
-	union xfs_rtword_raw	*wordptr)
+	struct xfs_buf		*bp,
+	unsigned int		index)
 {
-	return wordptr->old;
+	union xfs_rtword_raw	*words = bp->b_addr;
+
+	return words[index].old;
 }
 
 /* Set an ondisk bitmap word from an incore representation. */
 inline void
 xfs_rtbitmap_setword(
-	struct xfs_mount	*mp,
-	union xfs_rtword_raw	*wordptr,
+	struct xfs_buf		*bp,
+	unsigned int		index,
 	xfs_rtword_t		incore)
 {
-	wordptr->old = incore;
+	union xfs_rtword_raw	*words = bp->b_addr;
+
+	words[index].old = incore;
+}
+
+static inline void
+xfs_trans_log_rtbitmap(
+	struct xfs_trans	*tp,
+	struct xfs_buf		*bp,
+	int			from,
+	int			to)
+{
+	xfs_trans_log_buf(tp, bp, from * sizeof(union xfs_rtword_raw),
+			  to * sizeof(union xfs_rtword_raw) - 1);
 }
 
 /*
@@ -118,7 +133,6 @@ xfs_rtfind_back(
 	xfs_rtxnum_t	limit,		/* last rtext to look at */
 	xfs_rtxnum_t	*rtx)		/* out: start rtext found */
 {
-	union xfs_rtword_raw *b;		/* current word in buffer */
 	int		bit;		/* bit number in the word */
 	xfs_fileoff_t	block;		/* bitmap block number */
 	struct xfs_buf	*bp;		/* buf for the block */
@@ -145,14 +159,13 @@ xfs_rtfind_back(
 	 * Get the first word's index & point to it.
 	 */
 	word = xfs_rtx_to_rbmword(mp, start);
-	b = xfs_rbmblock_wordptr(bp, word);
 	bit = (int)(start & (XFS_NBWORD - 1));
 	len = start - limit + 1;
 	/*
 	 * Compute match value, based on the bit at start: if 1 (free)
 	 * then all-ones, else all-zeroes.
 	 */
-	incore = xfs_rtbitmap_getword(mp, b);
+	incore = xfs_rtbitmap_getword(bp, word);
 	want = (incore & ((xfs_rtword_t)1 << bit)) ? -1 : 0;
 	/*
 	 * If the starting position is not word-aligned, deal with the
@@ -195,12 +208,6 @@ xfs_rtfind_back(
 			}
 
 			word = mp->m_blockwsize - 1;
-			b = xfs_rbmblock_wordptr(bp, word);
-		} else {
-			/*
-			 * Go on to the previous word in the buffer.
-			 */
-			b--;
 		}
 	} else {
 		/*
@@ -216,7 +223,7 @@ xfs_rtfind_back(
 		/*
 		 * Compute difference between actual and desired value.
 		 */
-		incore = xfs_rtbitmap_getword(mp, b);
+		incore = xfs_rtbitmap_getword(bp, word);
 		if ((wdiff = incore ^ want)) {
 			/*
 			 * Different, mark where we are and return.
@@ -242,12 +249,6 @@ xfs_rtfind_back(
 			}
 
 			word = mp->m_blockwsize - 1;
-			b = xfs_rbmblock_wordptr(bp, word);
-		} else {
-			/*
-			 * Go on to the previous word in the buffer.
-			 */
-			b--;
 		}
 	}
 	/*
@@ -264,7 +265,7 @@ xfs_rtfind_back(
 		/*
 		 * Compute difference between actual and desired value.
 		 */
-		incore = xfs_rtbitmap_getword(mp, b);
+		incore = xfs_rtbitmap_getword(bp, word);
 		if ((wdiff = (incore ^ want) & mask)) {
 			/*
 			 * Different, mark where we are and return.
@@ -296,7 +297,6 @@ xfs_rtfind_forw(
 	xfs_rtxnum_t	limit,		/* last rtext to look at */
 	xfs_rtxnum_t	*rtx)		/* out: start rtext found */
 {
-	union xfs_rtword_raw *b;		/* current word in buffer */
 	int		bit;		/* bit number in the word */
 	xfs_fileoff_t	block;		/* bitmap block number */
 	struct xfs_buf	*bp;		/* buf for the block */
@@ -323,14 +323,13 @@ xfs_rtfind_forw(
 	 * Get the first word's index & point to it.
 	 */
 	word = xfs_rtx_to_rbmword(mp, start);
-	b = xfs_rbmblock_wordptr(bp, word);
 	bit = (int)(start & (XFS_NBWORD - 1));
 	len = limit - start + 1;
 	/*
 	 * Compute match value, based on the bit at start: if 1 (free)
 	 * then all-ones, else all-zeroes.
 	 */
-	incore = xfs_rtbitmap_getword(mp, b);
+	incore = xfs_rtbitmap_getword(bp, word);
 	want = (incore & ((xfs_rtword_t)1 << bit)) ? -1 : 0;
 	/*
 	 * If the starting position is not word-aligned, deal with the
@@ -372,12 +371,6 @@ xfs_rtfind_forw(
 			}
 
 			word = 0;
-			b = xfs_rbmblock_wordptr(bp, word);
-		} else {
-			/*
-			 * Go on to the previous word in the buffer.
-			 */
-			b++;
 		}
 	} else {
 		/*
@@ -393,7 +386,7 @@ xfs_rtfind_forw(
 		/*
 		 * Compute difference between actual and desired value.
 		 */
-		incore = xfs_rtbitmap_getword(mp, b);
+		incore = xfs_rtbitmap_getword(bp, word);
 		if ((wdiff = incore ^ want)) {
 			/*
 			 * Different, mark where we are and return.
@@ -419,12 +412,6 @@ xfs_rtfind_forw(
 			}
 
 			word = 0;
-			b = xfs_rbmblock_wordptr(bp, word);
-		} else {
-			/*
-			 * Go on to the next word in the buffer.
-			 */
-			b++;
 		}
 	}
 	/*
@@ -439,7 +426,7 @@ xfs_rtfind_forw(
 		/*
 		 * Compute difference between actual and desired value.
 		 */
-		incore = xfs_rtbitmap_getword(mp, b);
+		incore = xfs_rtbitmap_getword(bp, word);
 		if ((wdiff = (incore ^ want) & mask)) {
 			/*
 			 * Different, mark where we are and return.
@@ -566,12 +553,11 @@ xfs_rtmodify_range(
 	xfs_rtxlen_t	len,		/* length of extent to modify */
 	int		val)		/* 1 for free, 0 for allocated */
 {
-	union xfs_rtword_raw *b;		/* current word in buffer */
 	int		bit;		/* bit number in the word */
 	xfs_fileoff_t	block;		/* bitmap block number */
 	struct xfs_buf	*bp;		/* buf for the block */
 	int		error;		/* error value */
-	union xfs_rtword_raw *first;		/* first used word in the buffer */
+	int		first;		/* first used word in the buffer */
 	int		i;		/* current bit number rel. to start */
 	int		lastbit;	/* last useful bit in word */
 	xfs_rtword_t	mask;		/* mask o frelevant bits for value */
@@ -593,8 +579,7 @@ xfs_rtmodify_range(
 	/*
 	 * Compute the starting word's address, and starting bit.
 	 */
-	word = xfs_rtx_to_rbmword(mp, start);
-	first = b = xfs_rbmblock_wordptr(bp, word);
+	first = word = xfs_rtx_to_rbmword(mp, start);
 	bit = (int)(start & (XFS_NBWORD - 1));
 	/*
 	 * 0 (allocated) => all zeroes; 1 (free) => all ones.
@@ -613,12 +598,12 @@ xfs_rtmodify_range(
 		/*
 		 * Set/clear the active bits.
 		 */
-		incore = xfs_rtbitmap_getword(mp, b);
+		incore = xfs_rtbitmap_getword(bp, word);
 		if (val)
 			incore |= mask;
 		else
 			incore &= ~mask;
-		xfs_rtbitmap_setword(mp, b, incore);
+		xfs_rtbitmap_setword(bp, word, incore);
 		i = lastbit - bit;
 		/*
 		 * Go on to the next block if that's where the next word is
@@ -629,21 +614,13 @@ xfs_rtmodify_range(
 			 * Log the changed part of this block.
 			 * Get the next one.
 			 */
-			xfs_trans_log_buf(tp, bp,
-				(uint)((char *)first - (char *)bp->b_addr),
-				(uint)((char *)b - (char *)bp->b_addr));
+			xfs_trans_log_rtbitmap(tp, bp, first, word);
 			error = xfs_rtbuf_get(mp, tp, ++block, 0, &bp);
 			if (error) {
 				return error;
 			}
 
-			word = 0;
-			first = b = xfs_rbmblock_wordptr(bp, word);
-		} else {
-			/*
-			 * Go on to the next word in the buffer
-			 */
-			b++;
+			first = word = 0;
 		}
 	} else {
 		/*
@@ -659,7 +636,7 @@ xfs_rtmodify_range(
 		/*
 		 * Set the word value correctly.
 		 */
-		xfs_rtbitmap_setword(mp, b, val);
+		xfs_rtbitmap_setword(bp, word, val);
 		i += XFS_NBWORD;
 		/*
 		 * Go on to the next block if that's where the next word is
@@ -670,21 +647,13 @@ xfs_rtmodify_range(
 			 * Log the changed part of this block.
 			 * Get the next one.
 			 */
-			xfs_trans_log_buf(tp, bp,
-				(uint)((char *)first - (char *)bp->b_addr),
-				(uint)((char *)b - (char *)bp->b_addr));
+			xfs_trans_log_rtbitmap(tp, bp, first, word);
 			error = xfs_rtbuf_get(mp, tp, ++block, 0, &bp);
 			if (error) {
 				return error;
 			}
 
-			word = 0;
-			first = b = xfs_rbmblock_wordptr(bp, word);
-		} else {
-			/*
-			 * Go on to the next word in the buffer
-			 */
-			b++;
+			first = word = 0;
 		}
 	}
 	/*
@@ -699,21 +668,19 @@ xfs_rtmodify_range(
 		/*
 		 * Set/clear the active bits.
 		 */
-		incore = xfs_rtbitmap_getword(mp, b);
+		incore = xfs_rtbitmap_getword(bp, word);
 		if (val)
 			incore |= mask;
 		else
 			incore &= ~mask;
-		xfs_rtbitmap_setword(mp, b, incore);
-		b++;
+		xfs_rtbitmap_setword(bp, word, incore);
+		word++;
 	}
 	/*
 	 * Log any remaining changed bytes.
 	 */
-	if (b > first)
-		xfs_trans_log_buf(tp, bp,
-			(uint)((char *)first - (char *)bp->b_addr),
-			(uint)((char *)b - (char *)bp->b_addr - 1));
+	if (word > first)
+		xfs_trans_log_rtbitmap(tp, bp, first, word);
 	return 0;
 }
 
@@ -807,7 +774,6 @@ xfs_rtcheck_range(
 	xfs_rtxnum_t	*new,		/* out: first rtext not matching */
 	int		*stat)		/* out: 1 for matches, 0 for not */
 {
-	union xfs_rtword_raw *b;		/* current word in buffer */
 	int		bit;		/* bit number in the word */
 	xfs_fileoff_t	block;		/* bitmap block number */
 	struct xfs_buf	*bp;		/* buf for the block */
@@ -835,7 +801,6 @@ xfs_rtcheck_range(
 	 * Compute the starting word's address, and starting bit.
 	 */
 	word = xfs_rtx_to_rbmword(mp, start);
-	b = xfs_rbmblock_wordptr(bp, word);
 	bit = (int)(start & (XFS_NBWORD - 1));
 	/*
 	 * 0 (allocated) => all zero's; 1 (free) => all one's.
@@ -857,7 +822,7 @@ xfs_rtcheck_range(
 		/*
 		 * Compute difference between actual and desired value.
 		 */
-		incore = xfs_rtbitmap_getword(mp, b);
+		incore = xfs_rtbitmap_getword(bp, word);
 		if ((wdiff = (incore ^ val) & mask)) {
 			/*
 			 * Different, compute first wrong bit and return.
@@ -884,12 +849,6 @@ xfs_rtcheck_range(
 			}
 
 			word = 0;
-			b = xfs_rbmblock_wordptr(bp, word);
-		} else {
-			/*
-			 * Go on to the next word in the buffer.
-			 */
-			b++;
 		}
 	} else {
 		/*
@@ -905,7 +864,7 @@ xfs_rtcheck_range(
 		/*
 		 * Compute difference between actual and desired value.
 		 */
-		incore = xfs_rtbitmap_getword(mp, b);
+		incore = xfs_rtbitmap_getword(bp, word);
 		if ((wdiff = incore ^ val)) {
 			/*
 			 * Different, compute first wrong bit and return.
@@ -932,12 +891,6 @@ xfs_rtcheck_range(
 			}
 
 			word = 0;
-			b = xfs_rbmblock_wordptr(bp, word);
-		} else {
-			/*
-			 * Go on to the next word in the buffer.
-			 */
-			b++;
 		}
 	}
 	/*
@@ -952,7 +905,7 @@ xfs_rtcheck_range(
 		/*
 		 * Compute difference between actual and desired value.
 		 */
-		incore = xfs_rtbitmap_getword(mp, b);
+		incore = xfs_rtbitmap_getword(bp, word);
 		if ((wdiff = (incore ^ val) & mask)) {
 			/*
 			 * Different, compute first wrong bit and return.
diff --git a/fs/xfs/libxfs/xfs_rtbitmap.h b/fs/xfs/libxfs/xfs_rtbitmap.h
index 4e33e84afa7ad6..ec14e6adb8364a 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.h
+++ b/fs/xfs/libxfs/xfs_rtbitmap.h
@@ -158,17 +158,6 @@ xfs_rbmblock_to_rtx(
 	return rbmoff << mp->m_blkbit_log;
 }
 
-/* Return a pointer to a bitmap word within a rt bitmap block. */
-static inline union xfs_rtword_raw *
-xfs_rbmblock_wordptr(
-	struct xfs_buf		*bp,
-	unsigned int		index)
-{
-	union xfs_rtword_raw	*words = bp->b_addr;
-
-	return words + index;
-}
-
 /*
  * Convert a rt extent length and rt bitmap block number to a xfs_suminfo_t
  * offset within the rt summary file.
@@ -285,10 +274,10 @@ xfs_filblks_t xfs_rtbitmap_blockcount(struct xfs_mount *mp, xfs_rtbxlen_t
 		rtextents);
 unsigned long long xfs_rtbitmap_wordcount(struct xfs_mount *mp,
 		xfs_rtbxlen_t rtextents);
-xfs_rtword_t xfs_rtbitmap_getword(struct xfs_mount *mp,
-		union xfs_rtword_raw *wordptr);
-void xfs_rtbitmap_setword(struct xfs_mount *mp,
-		union xfs_rtword_raw *wordptr, xfs_rtword_t incore);
+xfs_rtword_t xfs_rtbitmap_getword(struct xfs_buf *bp, unsigned int index);
+void xfs_rtbitmap_setword(struct xfs_buf *bp, unsigned int index,
+		xfs_rtword_t incore);
+
 #else /* CONFIG_XFS_RT */
 # define xfs_rtfree_extent(t,b,l)			(-ENOSYS)
 # define xfs_rtfree_blocks(t,rb,rl)			(-ENOSYS)
Darrick J. Wong Oct. 18, 2023, 2:01 a.m. UTC | #2
On Tue, Oct 17, 2023 at 08:53:16PM +0200, Christoph Hellwig wrote:
> On Tue, Oct 17, 2023 at 08:53:21AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Create get and set functions for rtbitmap words so that we can redefine
> > the ondisk format with a specific endianness.  Note that this requires
> > the definition of a distinct type for ondisk rtbitmap words so that the
> > compiler can perform proper typechecking as we go back and forth.
> > 
> > In the upcoming rtgroups feature, we're going to fix the problem that
> > rtwords are written in host endian order, which means we'll need the
> > distinct rtword/rtword_raw types.
> 
> So per the last round I'd much prefer no exposing the xfs_rtword_raw
> to the callers.  I've cooked up the patch below to do this, and it
> seems to survive the absolute basic testing so far.  One interesting
> thing is that as far as I can tell all but one of the
> xfs_trans_log_buf calls in the pre-existing code were wrong as they
> were missing the usual '- 1' for the last parameter.
> 
> For reasons I can't explain the version with this patch also happens
> to actually generate smaller binary code as well:
> 
> hch@brick:~/work/xfs$ size xfs_rtbitmap.o*
>    text	   data	    bss	    dec	    hex	filename
>    7763	      0	      0	   7763	   1e53	xfs_rtbitmap.o.new
>    7833	      0	      0	   7833	   1e99	xfs_rtbitmap.o.old

Probably not having to maintain b and word as separate variables...

> ---
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index f8daaff947fce8..6ca48fe8a9e1d3 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -90,20 +90,35 @@ xfs_rtbuf_get(
>  /* Convert an ondisk bitmap word to its incore representation. */
>  inline xfs_rtword_t
>  xfs_rtbitmap_getword(
> -	struct xfs_mount	*mp,
> -	union xfs_rtword_raw	*wordptr)
> +	struct xfs_buf		*bp,
> +	unsigned int		index)
>  {
> -	return wordptr->old;
> +	union xfs_rtword_raw	*words = bp->b_addr;
> +
> +	return words[index].old;
>  }
>  
>  /* Set an ondisk bitmap word from an incore representation. */
>  inline void
>  xfs_rtbitmap_setword(
> -	struct xfs_mount	*mp,
> -	union xfs_rtword_raw	*wordptr,
> +	struct xfs_buf		*bp,
> +	unsigned int		index,
>  	xfs_rtword_t		incore)
>  {
> -	wordptr->old = incore;
> +	union xfs_rtword_raw	*words = bp->b_addr;
> +
> +	words[index].old = incore;
> +}
> +
> +static inline void
> +xfs_trans_log_rtbitmap(
> +	struct xfs_trans	*tp,
> +	struct xfs_buf		*bp,
> +	int			from,
> +	int			to)
> +{
> +	xfs_trans_log_buf(tp, bp, from * sizeof(union xfs_rtword_raw),
> +			  to * sizeof(union xfs_rtword_raw) - 1);
>  }

I'll make a separate patch with new xfs_trans_log_rt{bitmap,summary}
helpers.  Dunno if we care about a Fixes: tag for that off by one error
since its only effect was to log more of the bitmap than was strictly
necessary and I don't think that's worth bothering the LTS folks about.

>  
>  /*
> @@ -118,7 +133,6 @@ xfs_rtfind_back(
>  	xfs_rtxnum_t	limit,		/* last rtext to look at */
>  	xfs_rtxnum_t	*rtx)		/* out: start rtext found */
>  {
> -	union xfs_rtword_raw *b;		/* current word in buffer */
>  	int		bit;		/* bit number in the word */
>  	xfs_fileoff_t	block;		/* bitmap block number */
>  	struct xfs_buf	*bp;		/* buf for the block */
> @@ -145,14 +159,13 @@ xfs_rtfind_back(
>  	 * Get the first word's index & point to it.
>  	 */
>  	word = xfs_rtx_to_rbmword(mp, start);
> -	b = xfs_rbmblock_wordptr(bp, word);
>  	bit = (int)(start & (XFS_NBWORD - 1));
>  	len = start - limit + 1;
>  	/*
>  	 * Compute match value, based on the bit at start: if 1 (free)
>  	 * then all-ones, else all-zeroes.
>  	 */
> -	incore = xfs_rtbitmap_getword(mp, b);
> +	incore = xfs_rtbitmap_getword(bp, word);
>  	want = (incore & ((xfs_rtword_t)1 << bit)) ? -1 : 0;
>  	/*
>  	 * If the starting position is not word-aligned, deal with the
> @@ -195,12 +208,6 @@ xfs_rtfind_back(
>  			}
>  
>  			word = mp->m_blockwsize - 1;
> -			b = xfs_rbmblock_wordptr(bp, word);
> -		} else {
> -			/*
> -			 * Go on to the previous word in the buffer.
> -			 */
> -			b--;

Yay this goes away!

>  		}
>  	} else {
>  		/*

<snip to a function that uses setword>

> @@ -566,12 +553,11 @@ xfs_rtmodify_range(
>  	xfs_rtxlen_t	len,		/* length of extent to modify */
>  	int		val)		/* 1 for free, 0 for allocated */
>  {
> -	union xfs_rtword_raw *b;		/* current word in buffer */
>  	int		bit;		/* bit number in the word */
>  	xfs_fileoff_t	block;		/* bitmap block number */
>  	struct xfs_buf	*bp;		/* buf for the block */
>  	int		error;		/* error value */
> -	union xfs_rtword_raw *first;		/* first used word in the buffer */
> +	int		first;		/* first used word in the buffer */

/me notes that the xfs_rtx_to_rbmword function returns an unsigned int,
will clean that up...

>  	int		i;		/* current bit number rel. to start */
>  	int		lastbit;	/* last useful bit in word */
>  	xfs_rtword_t	mask;		/* mask o frelevant bits for value */
> @@ -593,8 +579,7 @@ xfs_rtmodify_range(
>  	/*
>  	 * Compute the starting word's address, and starting bit.
>  	 */
> -	word = xfs_rtx_to_rbmword(mp, start);
> -	first = b = xfs_rbmblock_wordptr(bp, word);
> +	first = word = xfs_rtx_to_rbmword(mp, start);
>  	bit = (int)(start & (XFS_NBWORD - 1));
>  	/*
>  	 * 0 (allocated) => all zeroes; 1 (free) => all ones.
> @@ -613,12 +598,12 @@ xfs_rtmodify_range(
>  		/*
>  		 * Set/clear the active bits.
>  		 */
> -		incore = xfs_rtbitmap_getword(mp, b);
> +		incore = xfs_rtbitmap_getword(bp, word);
>  		if (val)
>  			incore |= mask;
>  		else
>  			incore &= ~mask;
> -		xfs_rtbitmap_setword(mp, b, incore);
> +		xfs_rtbitmap_setword(bp, word, incore);

Yay!

>  		i = lastbit - bit;
>  		/*
>  		 * Go on to the next block if that's where the next word is

<snip more of the same>

> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.h b/fs/xfs/libxfs/xfs_rtbitmap.h
> index 4e33e84afa7ad6..ec14e6adb8364a 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.h
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.h
> @@ -158,17 +158,6 @@ xfs_rbmblock_to_rtx(
>  	return rbmoff << mp->m_blkbit_log;
>  }
>  
> -/* Return a pointer to a bitmap word within a rt bitmap block. */
> -static inline union xfs_rtword_raw *
> -xfs_rbmblock_wordptr(
> -	struct xfs_buf		*bp,
> -	unsigned int		index)
> -{
> -	union xfs_rtword_raw	*words = bp->b_addr;
> -
> -	return words + index;
> -}

Note that I still need this (and the _infoptr helper) for online repair
to be able to generate an in-memory replacement of the bitmap and
summary file and then be able to memcpy the words into the new ondisk
file.

That said, I also noticed that the _rtbitmap_[gs]etword and
_suminfo_{get,add} functions can be static inlines in xfs_rtbitmap.h, so
I'll put them right after here and the compiler will (hopefully)
collapse the nested inlines into something fairly compact.

Ok, I've made all those changes and I'll resend this patchset tomorrow
after letting it test overnight.

--D

> -
>  /*
>   * Convert a rt extent length and rt bitmap block number to a xfs_suminfo_t
>   * offset within the rt summary file.
> @@ -285,10 +274,10 @@ xfs_filblks_t xfs_rtbitmap_blockcount(struct xfs_mount *mp, xfs_rtbxlen_t
>  		rtextents);
>  unsigned long long xfs_rtbitmap_wordcount(struct xfs_mount *mp,
>  		xfs_rtbxlen_t rtextents);
> -xfs_rtword_t xfs_rtbitmap_getword(struct xfs_mount *mp,
> -		union xfs_rtword_raw *wordptr);
> -void xfs_rtbitmap_setword(struct xfs_mount *mp,
> -		union xfs_rtword_raw *wordptr, xfs_rtword_t incore);
> +xfs_rtword_t xfs_rtbitmap_getword(struct xfs_buf *bp, unsigned int index);
> +void xfs_rtbitmap_setword(struct xfs_buf *bp, unsigned int index,
> +		xfs_rtword_t incore);
> +
>  #else /* CONFIG_XFS_RT */
>  # define xfs_rtfree_extent(t,b,l)			(-ENOSYS)
>  # define xfs_rtfree_blocks(t,rb,rl)			(-ENOSYS)
>
Christoph Hellwig Oct. 18, 2023, 4:50 a.m. UTC | #3
On Tue, Oct 17, 2023 at 07:01:01PM -0700, Darrick J. Wong wrote:
> Note that I still need this (and the _infoptr helper) for online repair
> to be able to generate an in-memory replacement of the bitmap and
> summary file and then be able to memcpy the words into the new ondisk
> file.
> 
> That said, I also noticed that the _rtbitmap_[gs]etword and
> _suminfo_{get,add} functions can be static inlines in xfs_rtbitmap.h, so
> I'll put them right after here and the compiler will (hopefully)
> collapse the nested inlines into something fairly compact.
> 
> Ok, I've made all those changes and I'll resend this patchset tomorrow
> after letting it test overnight.

Ok.  We'll also need to do the same for the summary file.  I still offer
to do the full work myself, but I won't have time for that until next
week.  I didn't really have time for this hack either, but I just carved
it off..
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index d48e3a395bd9..2af891d5d171 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -690,6 +690,14 @@  struct xfs_agfl {
 	    ASSERT(xfs_daddr_to_agno(mp, d) == \
 		   xfs_daddr_to_agno(mp, (d) + (len) - 1)))
 
+/*
+ * Realtime bitmap information is accessed by the word, which is currently
+ * stored in host-endian format.
+ */
+union xfs_rtword_raw {
+	__u32		old;
+};
+
 /*
  * XFS Timestamps
  * ==============
diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index ff3d10d67bde..f8daaff947fc 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -87,6 +87,25 @@  xfs_rtbuf_get(
 	return 0;
 }
 
+/* Convert an ondisk bitmap word to its incore representation. */
+inline xfs_rtword_t
+xfs_rtbitmap_getword(
+	struct xfs_mount	*mp,
+	union xfs_rtword_raw	*wordptr)
+{
+	return wordptr->old;
+}
+
+/* Set an ondisk bitmap word from an incore representation. */
+inline void
+xfs_rtbitmap_setword(
+	struct xfs_mount	*mp,
+	union xfs_rtword_raw	*wordptr,
+	xfs_rtword_t		incore)
+{
+	wordptr->old = incore;
+}
+
 /*
  * Searching backward from start to limit, find the first block whose
  * allocated/free state is different from start's.
@@ -99,7 +118,7 @@  xfs_rtfind_back(
 	xfs_rtxnum_t	limit,		/* last rtext to look at */
 	xfs_rtxnum_t	*rtx)		/* out: start rtext found */
 {
-	xfs_rtword_t	*b;		/* current word in buffer */
+	union xfs_rtword_raw *b;		/* current word in buffer */
 	int		bit;		/* bit number in the word */
 	xfs_fileoff_t	block;		/* bitmap block number */
 	struct xfs_buf	*bp;		/* buf for the block */
@@ -110,6 +129,7 @@  xfs_rtfind_back(
 	xfs_rtword_t	mask;		/* mask of relevant bits for value */
 	xfs_rtword_t	want;		/* mask for "good" values */
 	xfs_rtword_t	wdiff;		/* difference from wanted value */
+	xfs_rtword_t	incore;
 	int		word;		/* word number in the buffer */
 
 	/*
@@ -132,7 +152,8 @@  xfs_rtfind_back(
 	 * Compute match value, based on the bit at start: if 1 (free)
 	 * then all-ones, else all-zeroes.
 	 */
-	want = (*b & ((xfs_rtword_t)1 << bit)) ? -1 : 0;
+	incore = xfs_rtbitmap_getword(mp, b);
+	want = (incore & ((xfs_rtword_t)1 << bit)) ? -1 : 0;
 	/*
 	 * If the starting position is not word-aligned, deal with the
 	 * partial word.
@@ -149,7 +170,7 @@  xfs_rtfind_back(
 		 * Calculate the difference between the value there
 		 * and what we're looking for.
 		 */
-		if ((wdiff = (*b ^ want) & mask)) {
+		if ((wdiff = (incore ^ want) & mask)) {
 			/*
 			 * Different.  Mark where we are and return.
 			 */
@@ -195,7 +216,8 @@  xfs_rtfind_back(
 		/*
 		 * Compute difference between actual and desired value.
 		 */
-		if ((wdiff = *b ^ want)) {
+		incore = xfs_rtbitmap_getword(mp, b);
+		if ((wdiff = incore ^ want)) {
 			/*
 			 * Different, mark where we are and return.
 			 */
@@ -242,7 +264,8 @@  xfs_rtfind_back(
 		/*
 		 * Compute difference between actual and desired value.
 		 */
-		if ((wdiff = (*b ^ want) & mask)) {
+		incore = xfs_rtbitmap_getword(mp, b);
+		if ((wdiff = (incore ^ want) & mask)) {
 			/*
 			 * Different, mark where we are and return.
 			 */
@@ -273,7 +296,7 @@  xfs_rtfind_forw(
 	xfs_rtxnum_t	limit,		/* last rtext to look at */
 	xfs_rtxnum_t	*rtx)		/* out: start rtext found */
 {
-	xfs_rtword_t	*b;		/* current word in buffer */
+	union xfs_rtword_raw *b;		/* current word in buffer */
 	int		bit;		/* bit number in the word */
 	xfs_fileoff_t	block;		/* bitmap block number */
 	struct xfs_buf	*bp;		/* buf for the block */
@@ -284,6 +307,7 @@  xfs_rtfind_forw(
 	xfs_rtword_t	mask;		/* mask of relevant bits for value */
 	xfs_rtword_t	want;		/* mask for "good" values */
 	xfs_rtword_t	wdiff;		/* difference from wanted value */
+	xfs_rtword_t	incore;
 	int		word;		/* word number in the buffer */
 
 	/*
@@ -306,7 +330,8 @@  xfs_rtfind_forw(
 	 * Compute match value, based on the bit at start: if 1 (free)
 	 * then all-ones, else all-zeroes.
 	 */
-	want = (*b & ((xfs_rtword_t)1 << bit)) ? -1 : 0;
+	incore = xfs_rtbitmap_getword(mp, b);
+	want = (incore & ((xfs_rtword_t)1 << bit)) ? -1 : 0;
 	/*
 	 * If the starting position is not word-aligned, deal with the
 	 * partial word.
@@ -322,7 +347,7 @@  xfs_rtfind_forw(
 		 * Calculate the difference between the value there
 		 * and what we're looking for.
 		 */
-		if ((wdiff = (*b ^ want) & mask)) {
+		if ((wdiff = (incore ^ want) & mask)) {
 			/*
 			 * Different.  Mark where we are and return.
 			 */
@@ -368,7 +393,8 @@  xfs_rtfind_forw(
 		/*
 		 * Compute difference between actual and desired value.
 		 */
-		if ((wdiff = *b ^ want)) {
+		incore = xfs_rtbitmap_getword(mp, b);
+		if ((wdiff = incore ^ want)) {
 			/*
 			 * Different, mark where we are and return.
 			 */
@@ -413,7 +439,8 @@  xfs_rtfind_forw(
 		/*
 		 * Compute difference between actual and desired value.
 		 */
-		if ((wdiff = (*b ^ want) & mask)) {
+		incore = xfs_rtbitmap_getword(mp, b);
+		if ((wdiff = (incore ^ want) & mask)) {
 			/*
 			 * Different, mark where we are and return.
 			 */
@@ -539,15 +566,16 @@  xfs_rtmodify_range(
 	xfs_rtxlen_t	len,		/* length of extent to modify */
 	int		val)		/* 1 for free, 0 for allocated */
 {
-	xfs_rtword_t	*b;		/* current word in buffer */
+	union xfs_rtword_raw *b;		/* current word in buffer */
 	int		bit;		/* bit number in the word */
 	xfs_fileoff_t	block;		/* bitmap block number */
 	struct xfs_buf	*bp;		/* buf for the block */
 	int		error;		/* error value */
-	xfs_rtword_t	*first;		/* first used word in the buffer */
+	union xfs_rtword_raw *first;		/* first used word in the buffer */
 	int		i;		/* current bit number rel. to start */
 	int		lastbit;	/* last useful bit in word */
 	xfs_rtword_t	mask;		/* mask o frelevant bits for value */
+	xfs_rtword_t	incore;
 	int		word;		/* word number in the buffer */
 
 	/*
@@ -585,10 +613,12 @@  xfs_rtmodify_range(
 		/*
 		 * Set/clear the active bits.
 		 */
+		incore = xfs_rtbitmap_getword(mp, b);
 		if (val)
-			*b |= mask;
+			incore |= mask;
 		else
-			*b &= ~mask;
+			incore &= ~mask;
+		xfs_rtbitmap_setword(mp, b, incore);
 		i = lastbit - bit;
 		/*
 		 * Go on to the next block if that's where the next word is
@@ -629,7 +659,7 @@  xfs_rtmodify_range(
 		/*
 		 * Set the word value correctly.
 		 */
-		*b = val;
+		xfs_rtbitmap_setword(mp, b, val);
 		i += XFS_NBWORD;
 		/*
 		 * Go on to the next block if that's where the next word is
@@ -669,10 +699,12 @@  xfs_rtmodify_range(
 		/*
 		 * Set/clear the active bits.
 		 */
+		incore = xfs_rtbitmap_getword(mp, b);
 		if (val)
-			*b |= mask;
+			incore |= mask;
 		else
-			*b &= ~mask;
+			incore &= ~mask;
+		xfs_rtbitmap_setword(mp, b, incore);
 		b++;
 	}
 	/*
@@ -775,7 +807,7 @@  xfs_rtcheck_range(
 	xfs_rtxnum_t	*new,		/* out: first rtext not matching */
 	int		*stat)		/* out: 1 for matches, 0 for not */
 {
-	xfs_rtword_t	*b;		/* current word in buffer */
+	union xfs_rtword_raw *b;		/* current word in buffer */
 	int		bit;		/* bit number in the word */
 	xfs_fileoff_t	block;		/* bitmap block number */
 	struct xfs_buf	*bp;		/* buf for the block */
@@ -784,6 +816,7 @@  xfs_rtcheck_range(
 	xfs_rtxnum_t	lastbit;	/* last useful bit in word */
 	xfs_rtword_t	mask;		/* mask of relevant bits for value */
 	xfs_rtword_t	wdiff;		/* difference from wanted value */
+	xfs_rtword_t	incore;
 	int		word;		/* word number in the buffer */
 
 	/*
@@ -824,7 +857,8 @@  xfs_rtcheck_range(
 		/*
 		 * Compute difference between actual and desired value.
 		 */
-		if ((wdiff = (*b ^ val) & mask)) {
+		incore = xfs_rtbitmap_getword(mp, b);
+		if ((wdiff = (incore ^ val) & mask)) {
 			/*
 			 * Different, compute first wrong bit and return.
 			 */
@@ -871,7 +905,8 @@  xfs_rtcheck_range(
 		/*
 		 * Compute difference between actual and desired value.
 		 */
-		if ((wdiff = *b ^ val)) {
+		incore = xfs_rtbitmap_getword(mp, b);
+		if ((wdiff = incore ^ val)) {
 			/*
 			 * Different, compute first wrong bit and return.
 			 */
@@ -917,7 +952,8 @@  xfs_rtcheck_range(
 		/*
 		 * Compute difference between actual and desired value.
 		 */
-		if ((wdiff = (*b ^ val) & mask)) {
+		incore = xfs_rtbitmap_getword(mp, b);
+		if ((wdiff = (incore ^ val) & mask)) {
 			/*
 			 * Different, compute first wrong bit and return.
 			 */
diff --git a/fs/xfs/libxfs/xfs_rtbitmap.h b/fs/xfs/libxfs/xfs_rtbitmap.h
index 01eabb9b5516..4e33e84afa7a 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.h
+++ b/fs/xfs/libxfs/xfs_rtbitmap.h
@@ -159,12 +159,12 @@  xfs_rbmblock_to_rtx(
 }
 
 /* Return a pointer to a bitmap word within a rt bitmap block. */
-static inline xfs_rtword_t *
+static inline union xfs_rtword_raw *
 xfs_rbmblock_wordptr(
 	struct xfs_buf		*bp,
 	unsigned int		index)
 {
-	xfs_rtword_t		*words = bp->b_addr;
+	union xfs_rtword_raw	*words = bp->b_addr;
 
 	return words + index;
 }
@@ -285,6 +285,10 @@  xfs_filblks_t xfs_rtbitmap_blockcount(struct xfs_mount *mp, xfs_rtbxlen_t
 		rtextents);
 unsigned long long xfs_rtbitmap_wordcount(struct xfs_mount *mp,
 		xfs_rtbxlen_t rtextents);
+xfs_rtword_t xfs_rtbitmap_getword(struct xfs_mount *mp,
+		union xfs_rtword_raw *wordptr);
+void xfs_rtbitmap_setword(struct xfs_mount *mp,
+		union xfs_rtword_raw *wordptr, xfs_rtword_t incore);
 #else /* CONFIG_XFS_RT */
 # define xfs_rtfree_extent(t,b,l)			(-ENOSYS)
 # define xfs_rtfree_blocks(t,rb,rl)			(-ENOSYS)
diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
index c4cc99b70dd3..14d455f768d3 100644
--- a/fs/xfs/xfs_ondisk.h
+++ b/fs/xfs/xfs_ondisk.h
@@ -72,6 +72,9 @@  xfs_check_ondisk_structs(void)
 	XFS_CHECK_STRUCT_SIZE(xfs_attr_leaf_map_t,		4);
 	XFS_CHECK_STRUCT_SIZE(xfs_attr_leaf_name_local_t,	4);
 
+	/* realtime structures */
+	XFS_CHECK_STRUCT_SIZE(union xfs_rtword_raw,		4);
+
 	/*
 	 * m68k has problems with xfs_attr_leaf_name_remote_t, but we pad it to
 	 * 4 bytes anyway so it's not obviously a problem.  Hence for the moment