discontiguous buffer patches

This patch sets all the b_bmap accesses to be b_maps[0]. b_maps[0] works for single and multiple segment buffers.

This fixes a bug where xfs_trans_buf_item_match() could not find a multi-segment buffer associated with the transaction because it was looking for the block number in the single segment location b_map.bm.bn rather than the new generic b_maps[0].bm.bn. This resulted in recursive buffer lock that can never be satisfied.

This patch sets all the b_bmap accesses to be b_maps[0]. b_maps[0] works for single and multiple segment buffers.

This fixes a bug where xfs_trans_buf_item_match() could not find a multi-segment buffer associated with the transaction because it was looking for the block number in the single segment location b_map.bm.bn rather than the new generic b_maps[0].bm.bn. This resulted in recursive buffer lock that can never be satisfied.

Signed-off-by Mark Tinguely <[email protected]>

fs/xfs/xfs_buf_item.c +0 -1

@@ -644,8 +652,14 @@ xfs_buf_item_unlock(
 	 * If the buf item isn't tracking any data, free it, otherwise drop the
 	 * reference we hold to it.
 	 */
-	if (xfs_bitmap_empty(bip->bli_format.blf_data_map,

Do we have proper test coverage for the xfs_buf_item_format_segment changes? Given that they change what gets written into the log I'd prefer them separate from a big cleanup patch, and including test coverage verifying everything works fine when hitting these corner cases.

Review

Quoted 5 lines

  		chunk_num = byte>>  XFS_BLF_SHIFT;
  		word_num = chunk_num>>  BIT_TO_WORD_SHIFT;
  		bit_num = chunk_num&  (NBWORD - 1);

(quoted diff) +1 -1

-		wordp =&(bip->bli_format.blf_data_map[word_num]);
+		wordp =&(bip->bli_formats[0].blf_data_map[word_num]);

This change doesn't seem to be mentioned in the changelog?

Quoted 10 lines

fs/xfs/xfs_buf_item.c +4 -2

@@ -644,8 +652,14 @@ xfs_buf_item_unlock(
  	 * If the buf item isn't tracking any data, free it, otherwise drop the
  	 * reference we hold to it.
  	 */
-	if (xfs_bitmap_empty(bip->bli_format.blf_data_map,
-			     bip->bli_format.blf_map_size))
+	empty = 1;
+	for (i = 0; i<  bip->bli_format_count; i++)
+		if (!xfs_bitmap_empty(bip->bli_formats[i].blf_data_map,
+			     bip->bli_formats[i].blf_map_size)) {

fs/xfs/xfs_buf_item.c xfs_buf_item_unlock( · line 663

Quoted 5 lines

fs/xfs/xfs_buf_item.c +4 -0

+			empty = 0;
+			break;
+		}
+	if (empty)
  		xfs_buf_item_relse(bp);

This looks like a bug fix, not just a cleanup. Again I'd prefer this to be a patch on its own, with a good description.

A few buffer log format clean-ups for contiguous buffers.

In xfs_buf_item_format_segment(), when a segment is not dirty in the transaction, do not increment format vector pointer.

In xfs_buf_item_unlock(), every segment must be empty before considering the buf item empty.

In xfs_trans_binval(), clear the correct buffer log format structure.

Signed-off-by Mark Tinguely <[email protected]>

---
 fs/xfs/xfs_buf_item.c  |   32 +++++++++++++++++++++++---------
 fs/xfs/xfs_trans_buf.c |    4 ++--
 2 files changed, 25 insertions(+), 11 deletions(-)

Index: b/fs/xfs/xfs_buf_item.c
===================================================================

fs/xfs/xfs_buf_item.c +0 -2

@@ -290,8 +290,6 @@ xfs_buf_item_format_segment(
 	vecp->i_addr = blfp;
 	vecp->i_len = base_size;
 	vecp->i_type = XLOG_REG_TYPE_BFORMAT;
-	vecp++;
-	nvecs = 1;
 
 	if (bip->bli_flags & XFS_BLI_STALE) {
 		/*

fs/xfs/xfs_buf_item.c +1 -1

@@ -601,7 +609,7 @@ xfs_buf_item_unlock(
 {
 	struct xfs_buf_log_item	*bip = BUF_ITEM(lip);
 	struct xfs_buf		*bp = bip->bli_buf;
-	int			aborted;
+	int			aborted, empty, i;
 	uint			hold;
 
 	/* Clear the buffer's association with this transaction. */

@@ -644,8 +652,14 @@ xfs_buf_item_unlock(
 	 * If the buf item isn't tracking any data, free it, otherwise drop the
 	 * reference we hold to it.

fs/xfs/xfs_buf_item.c +1 -0

+	if (empty)
 		xfs_buf_item_relse(bp);
 	else
 		atomic_dec(&bip->bli_refcount);

Index: b/fs/xfs/xfs_trans_buf.c
===================================================================

This debug code is a lot more broken than just this. It's completely unaware of discontiguous buffers....

I'm wondering if we should just remove the XFS_TRANS_DEBUG code because it doesn't ever get used....

Quoted 22 lines · Mark Tinguely

A few buffer log format clean-ups for contiguous buffers.

In xfs_buf_item_format_segment(), when a segment is not dirty in the transaction, do not increment format vector pointer.

In xfs_buf_item_unlock(), every segment must be empty before considering the buf item empty.

In xfs_trans_binval(), clear the correct buffer log format structure.

Signed-off-by Mark Tinguely<[email protected]>

---
  fs/xfs/xfs_buf_item.c  |   32 +++++++++++++++++++++++---------
  fs/xfs/xfs_trans_buf.c |    4 ++--
  2 files changed, 25 insertions(+), 11 deletions(-)

Index: b/fs/xfs/xfs_buf_item.c
===================================================================

fs/xfs/xfs_buf_item.c xfs_buf_item_log_debug( · line 77

Quoted 8 lines

fs/xfs/xfs_buf_item.c +1 -1

@@ -71,7 +71,7 @@ xfs_buf_item_log_debug(
  		chunk_num = byte>>  XFS_BLF_SHIFT;
  		word_num = chunk_num>>  BIT_TO_WORD_SHIFT;
  		bit_num = chunk_num&  (NBWORD - 1);
-		wordp =&(bip->bli_format.blf_data_map[word_num]);
+		wordp =&(bip->bli_formats[0].blf_data_map[word_num]);
  		bit_set = *wordp&  (1<<  bit_num);
  		ASSERT(bit_set);
  		byte++;

This debug code is a lot more broken than just this. It's completely unaware of discontiguous buffers....

I'm wondering if we should just remove the XFS_TRANS_DEBUG code because it doesn't ever get used....

Quoted 10 lines

fs/xfs/xfs_buf_item.c +0 -2

@@ -290,8 +290,6 @@ xfs_buf_item_format_segment(
  	vecp->i_addr = blfp;
  	vecp->i_len = base_size;
  	vecp->i_type = XLOG_REG_TYPE_BFORMAT;
-	vecp++;
-	nvecs = 1;
 
  	if (bip->bli_flags&  XFS_BLI_STALE) {
  		/*

fs/xfs/xfs_buf_item.c xfs_buf_item_format_segment( · line 305

Quoted 8 lines

fs/xfs/xfs_buf_item.c +2 -1

@@ -301,7 +299,8 @@ xfs_buf_item_format_segment(
  		 */
  		trace_xfs_buf_item_format_stale(bip);
  		ASSERT(blfp->blf_flags&  XFS_BLF_CANCEL);
-		blfp->blf_size = nvecs;
+		vecp++;
+		blfp->blf_size = 1;
  		return vecp;
  	}

I don't really like this separation of the blf vector initialisation and the decision to use the vector. If we are not going to use it, then we should really be initialising it.....

Quoted 10 lines

fs/xfs/xfs_buf_item.c +4 -1

@@ -309,7 +308,16 @@ xfs_buf_item_format_segment(
  	 * Fill in an iovec for each set of contiguous chunks.
  	 */
  	first_bit = xfs_next_bit(blfp->blf_data_map, blfp->blf_map_size, 0);
-	ASSERT(first_bit != -1);
+	if (first_bit == -1) {
+		/* If the map is not be dirty in the transaction, mark
+		 * the size as zero and do not advance the vector pointer.
+		 */

fs/xfs/xfs_buf_item.c xfs_buf_item_format_segment( · line 317

Quoted 3 lines

fs/xfs/xfs_buf_item.c +3 -0

+		blfp->blf_size = 0;
+		return(vecp);
+	}

Indeed, if this is the last segment in the discontiguous buffer, the above vecp initialisation could be writing beyond then end of the vector space that has been allocated. i.e. we should only initialise it if we are going to consume the vector.

Quoted 13 lines

fs/xfs/xfs_buf_item.c +1 -1

@@ -601,7 +609,7 @@ xfs_buf_item_unlock(
  {
  	struct xfs_buf_log_item	*bip = BUF_ITEM(lip);
  	struct xfs_buf		*bp = bip->bli_buf;
-	int			aborted;
+	int			aborted, empty, i;
  	uint			hold;
 
  	/* Clear the buffer's association with this transaction. */

@@ -644,8 +652,14 @@ xfs_buf_item_unlock(
  	 * If the buf item isn't tracking any data, free it, otherwise drop the
  	 * reference we hold to it.

fs/xfs/xfs_buf_item.c xfs_buf_item_unlock( · line 661

Quoted 10 lines

fs/xfs/xfs_buf_item.c +7 -2

  	 */
-	if (xfs_bitmap_empty(bip->bli_format.blf_data_map,
-			     bip->bli_format.blf_map_size))
+	empty = 1;
+	for (i = 0; i<  bip->bli_format_count; i++)
+		if (!xfs_bitmap_empty(bip->bli_formats[i].blf_data_map,
+			     bip->bli_formats[i].blf_map_size)) {
+			empty = 0;
+			break;
+		}

Landmine warning!

Please put {} around the body of the for loop.

FWIW this is a separate problem so it should be in it's own patch.

Quoted 9 lines

fs/xfs/xfs_buf_item.c +1 -0

+	if (empty)
  		xfs_buf_item_relse(bp);
  	else
  		atomic_dec(&bip->bli_refcount);

Index: b/fs/xfs/xfs_trans_buf.c
===================================================================

fs/xfs/xfs_trans_buf.c xfs_trans_binval( · line 674

Quoted 7 lines

fs/xfs/xfs_trans_buf.c +2 -2

@@ -670,8 +670,8 @@ xfs_trans_binval(
  	bip->bli_flags&= ~(XFS_BLI_INODE_BUF | XFS_BLI_LOGGED | XFS_BLI_DIRTY);
  	bip->bli_format.blf_flags&= ~XFS_BLF_INODE_BUF;
  	bip->bli_format.blf_flags |= XFS_BLF_CANCEL;
-	memset((char *)(bip->bli_format.blf_data_map), 0,
-	      (bip->bli_format.blf_map_size * sizeof(uint)));
+	memset((char *)(bip->bli_formats[0].blf_data_map), 0,
+	      (bip->bli_formats[0].blf_map_size * sizeof(uint)));

This is also wrong. i.e. Why is the code only zeroing a single map and not all maps? If the above xfs_bitmap_empty() check is checking all the segments, then we have to ensure that we zero all the segments here. This is why the current code works and doesn't cause problems - matching bugs that cancel each other out - but only fixing one of the bugs will cause regressions. This should also be in the patch that fixes the xfs_bitmap_empty() problem.

Cheers,

Dave.