Skip to contentSkip to conversation

discontiguous buffer patches

PATCH Superseded Mark Tinguely 3 participants 13 messages Nov 28, 2012 – Dec 2, 2012 XFS filesystem

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

v2 cover discontiguous buffer patches

Eric Sundeen's "userspace vs. fragmented multiblock dir2", xfstest 291 commit 2a4ed, causes a xfs_buf lock hang:

[<ffffffff81065d87>] down+0x47/0x50
[<ffffffffa03a25c6>] xfs_buf_lock+0x66/0xe0 [xfs]
[<ffffffffa03a33c9>] _xfs_buf_find+0x1f9/0x320 [xfs]
[<ffffffffa03a393f>] xfs_buf_get_map+0x2f/0x170 [xfs]
[<ffffffffa03a3f7a>] xfs_buf_read_map+0x2a/0x100 [xfs]
[<ffffffffa0411630>] xfs_trans_read_buf_map+0x3b0/0x590 [xfs]
[<ffffffffa03e1c5e>] xfs_da_read_buf+0xbe/0x230 [xfs]
[<ffffffffa03e78dc>] xfs_dir2_block_addname+0x7c/0x980 [xfs]
[<ffffffffa03f1468>] xfs_dir2_sf_addname+0x3e8/0x450 [xfs]
[<ffffffffa03e634c>] xfs_dir_createname+0x17c/0x1e0 [xfs]
[<ffffffffa03b9fe2>] xfs_create+0x4c2/0x5f0 [xfs]
[<ffffffffa03b0c8a>] xfs_vn_mknod+0x8a/0x1a0 [xfs]
[<ffffffffa03b0dce>] xfs_vn_create+0xe/0x10 [xfs]
[<ffffffff8115695c>] vfs_create+0xac/0xd0
[<ffffffff811587de>] do_last+0x8be/0x960
[<ffffffff8115940c>] path_openat+0xdc/0x410
[<ffffffff81159853>] do_filp_open+0x43/0xa0
[<ffffffff8114a502>] do_sys_open+0x152/0x1e0
[<ffffffff8114a5cc>] sys_open+0x1c/0x20
[<ffffffff81423df9>] system_call_fastpath+0x16/0x1b

[<ffffffffffffffff>] 0xffffffffffffffff

That bisect a problem to the discontiguous xfs_buf support series. xfs_trans_buf_item_match() is looking for the block number of the buffer in the single segment map area. After fixing the b_map issue, there were couple asserts associated with the buffer log format not correctly using the bli_formats[] structure.

Changes to the series from version 1:

Patch 1: Moved b_map structure to __b_map to avoid future confusion
per Christoph's suggestion.

Patch 2: Moved bli_format structure to __bli_format to avoid future
confusion per Christoph's suggestion.

Removed the buffer XFS_TRANS_DEBUG routines per Dave's suggestion.

Patch 3: Moved the detection of an empty format to a new file and
add parenthesis per Dave's suggestion.

Cleared all buffer log formats per Dave's suggestion.

--Mark.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#1 lore raw

PATCH v2 1/3 xfs: use b_maps[] for discontiguous buffers

Commits starting at 77c1a08 introduced a multiple segment support to xfs_buf. xfs_trans_buf_item_match() could not find a multi-segment buffer in the transaction because it was looking at the single segment block number rather than the multi-segment b_maps[0].bm.bn. This results on a recursive buffer lock that can never be satisfied.

This patch:
1) Changed the remaining b_map accesses to be b_maps[0] accesses.
2) Renames the single segment b_map structure to __b_map to avoid future confusion.

---
 fs/xfs/xfs_buf.c |   12 ++++++------
 fs/xfs/xfs_buf.h |    6 +++---
 2 files changed, 9 insertions(+), 9 deletions(-)
fs/xfs/xfs_buf.c +6 -6
Index: b/fs/xfs/xfs_buf.c
===================================================================
@@ -175,7 +175,7 @@ xfs_buf_get_maps(
 	bp->b_map_count = map_count;
 
 	if (map_count == 1) {
-		bp->b_maps = &bp->b_map;
+		bp->b_maps = &bp->__b_map;
 		return 0;
 	}
 
@@ -193,7 +193,7 @@ static void
 xfs_buf_free_maps(
 	struct xfs_buf	*bp)
 {
-	if (bp->b_maps != &bp->b_map) {
+	if (bp->b_maps != &bp->__b_map) {
 		kmem_free(bp->b_maps);
 		bp->b_maps = NULL;
 	}
@@ -377,8 +377,8 @@ xfs_buf_allocate_memory(
 	}
 
 use_alloc_page:
-	start = BBTOB(bp->b_map.bm_bn) >> PAGE_SHIFT;
-	end = (BBTOB(bp->b_map.bm_bn + bp->b_length) + PAGE_SIZE - 1)
+	start = BBTOB(bp->b_maps[0].bm_bn) >> PAGE_SHIFT;
+	end = (BBTOB(bp->b_maps[0].bm_bn + bp->b_length) + PAGE_SIZE - 1)
 								>> PAGE_SHIFT;
 	page_count = end - start;
 	error = _xfs_buf_get_pages(bp, page_count, flags);
@@ -640,7 +640,7 @@ _xfs_buf_read(
 	xfs_buf_flags_t		flags)
 {
 	ASSERT(!(flags & XBF_WRITE));
-	ASSERT(bp->b_map.bm_bn != XFS_BUF_DADDR_NULL);
+	ASSERT(bp->b_maps[0].bm_bn != XFS_BUF_DADDR_NULL);
 
 	bp->b_flags &= ~(XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD);
 	bp->b_flags |= flags & (XBF_READ | XBF_ASYNC | XBF_READ_AHEAD);
@@ -1709,7 +1709,7 @@ xfs_buf_cmp(
 	struct xfs_buf	*bp = container_of(b, struct xfs_buf, b_list);
 	xfs_daddr_t		diff;
 
-	diff = ap->b_map.bm_bn - bp->b_map.bm_bn;
+	diff = ap->b_maps[0].bm_bn - bp->b_maps[0].bm_bn;
 	if (diff < 0)
 		return -1;
 	if (diff > 0)
fs/xfs/xfs_buf.h +3 -3
Index: b/fs/xfs/xfs_buf.h
===================================================================
@@ -151,7 +151,7 @@ typedef struct xfs_buf {
 	struct page		**b_pages;	/* array of page pointers */
 	struct page		*b_page_array[XB_PAGES]; /* inline pages */
 	struct xfs_buf_map	*b_maps;	/* compound buffer map */
-	struct xfs_buf_map	b_map;		/* inline compound buffer map */
+	struct xfs_buf_map	__b_map;	/* inline compound buffer map */
 	int			b_map_count;
 	int			b_io_length;	/* IO size in BBs */
 	atomic_t		b_pin_count;	/* pin count */
@@ -330,8 +330,8 @@ void xfs_buf_stale(struct xfs_buf *bp);
  * In future, uncached buffers will pass the block number directly to the io
  * request function and hence these macros will go away at that point.
  */
-#define XFS_BUF_ADDR(bp)		((bp)->b_map.bm_bn)
-#define XFS_BUF_SET_ADDR(bp, bno)	((bp)->b_map.bm_bn = (xfs_daddr_t)(bno))
+#define XFS_BUF_ADDR(bp)		((bp)->b_maps[0].bm_bn)
+#define XFS_BUF_SET_ADDR(bp, bno)	((bp)->b_maps[0].bm_bn = (xfs_daddr_t)(bno))
 
 static inline void xfs_buf_set_ref(struct xfs_buf *bp, int lru_ref)
 {

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#2 lore raw

Re: [PATCH v2 1/3] xfs: use b_maps[] for discontiguous buffers

fs/xfs/xfs_buf.c xfs_buf_allocate_memory( · line 381

Quoted 5 lines

use_alloc_page:

fs/xfs/xfs_buf.c +2 -2
-	start = BBTOB(bp->b_map.bm_bn) >> PAGE_SHIFT;
-	end = (BBTOB(bp->b_map.bm_bn + bp->b_length) + PAGE_SIZE - 1)
+	start = BBTOB(bp->b_maps[0].bm_bn) >> PAGE_SHIFT;
+	end = (BBTOB(bp->b_maps[0].bm_bn + bp->b_length) + PAGE_SIZE - 1)

Btw, how is this logic supposed to work for discontiguous buffers?

Each of them might straddle boundaries individually, so doing the start/end calculation for the number of pages isn't going to be correct.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#3 lore raw

Re: [PATCH v2 1/3] xfs: use b_maps[] for discontiguous buffers

Quoted 11 lines · Christoph Hellwig

fs/xfs/xfs_buf.c xfs_buf_allocate_memory( · line 381

Quoted 5 lines

use_alloc_page:

fs/xfs/xfs_buf.c +2 -2
-	start = BBTOB(bp->b_map.bm_bn)>>  PAGE_SHIFT;
-	end = (BBTOB(bp->b_map.bm_bn + bp->b_length) + PAGE_SIZE - 1)
+	start = BBTOB(bp->b_maps[0].bm_bn)>>  PAGE_SHIFT;
+	end = (BBTOB(bp->b_maps[0].bm_bn + bp->b_length) + PAGE_SIZE - 1)

Btw, how is this logic supposed to work for discontiguous buffers?

Each of them might straddle boundaries individually, so doing the start/end calculation for the number of pages isn't going to be correct.

eeek. yep, I will loop through and count the pages needed in each segment.

--Mark.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#4 lore raw

Re: [PATCH v2 1/3] xfs: use b_maps[] for discontiguous buffers

Quoted 9 lines · Mark Tinguely
Quoted 8 lines · Christoph Hellwig

fs/xfs/xfs_buf.c xfs_buf_allocate_memory( · line 381

Quoted 5 lines

use_alloc_page:

fs/xfs/xfs_buf.c +2 -2
-	start = BBTOB(bp->b_map.bm_bn)>>  PAGE_SHIFT;
-	end = (BBTOB(bp->b_map.bm_bn + bp->b_length) + PAGE_SIZE - 1)
+	start = BBTOB(bp->b_maps[0].bm_bn)>>  PAGE_SHIFT;
+	end = (BBTOB(bp->b_maps[0].bm_bn + bp->b_length) + PAGE_SIZE - 1)

Btw, how is this logic supposed to work for discontiguous buffers?

Quoted 6 lines
Quoted 3 lines

Each of them might straddle boundaries individually, so doing the start/end calculation for the number of pages isn't going to be correct.

eeek. yep, I will loop through and count the pages needed in each segment.

That's wrong - the pages must be mappable as a contiguous memory range. That's how this code avoids copying the data from discontiguous page ranges into a contiguous mapped memory range. If you separate each IO into to it's own set of pages, then you have to completely rewrite _xfs_buf_ioapply(), which AFAICT works just fine with the page allocation that is done right now....

Cheers,

Dave.

Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#5 lore raw

Re: [PATCH v2 1/3] xfs: use b_maps[] for discontiguous buffers

Quoted 9 lines · Dave Chinner
Quoted 2 lines

eeek. yep, I will loop through and count the pages needed in each segment.

That's wrong - the pages must be mappable as a contiguous memory range. That's how this code avoids copying the data from discontiguous page ranges into a contiguous mapped memory range. If you separate each IO into to it's own set of pages, then you have to completely rewrite _xfs_buf_ioapply(), which AFAICT works just fine with the page allocation that is done right now....

Yeah, you're right.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#6 lore raw

Re: [PATCH v2 1/3] xfs: use b_maps[] for discontiguous buffers

In reply to Christoph Hellwig
Quoted 10 lines · Christoph Hellwig

fs/xfs/xfs_buf.c xfs_buf_allocate_memory( · line 381

Quoted 5 lines

use_alloc_page:

fs/xfs/xfs_buf.c +2 -2
-	start = BBTOB(bp->b_map.bm_bn) >> PAGE_SHIFT;
-	end = (BBTOB(bp->b_map.bm_bn + bp->b_length) + PAGE_SIZE - 1)
+	start = BBTOB(bp->b_maps[0].bm_bn) >> PAGE_SHIFT;
+	end = (BBTOB(bp->b_maps[0].bm_bn + bp->b_length) + PAGE_SIZE - 1)

Btw, how is this logic supposed to work for discontiguous buffers?

Each of them might straddle boundaries individually, so doing the start/end calculation for the number of pages isn't going to be correct.

AFAICT it is correct - the first page is always aligned to the start of the buffer (i.e. b_offset = 0), and the end is extended to ensure that it covers any partial tail page. e.g:

Page 0 1 2

	+---------------+---------------+---------------+
map[0]	|----------|
map[1]		   |------------|
map[2]				|--------------|
        |---------------bp->b_length-----------|
	|						|
	start						end

Maps are always multiples of basic blocks, so every segment boundary aligns to something that IO can be done on, and the array of pages always spans the entire range of the maps.

Hence I'm not sure what the problem is here, Christoph. Can you expand on the issue?

Cheers,

Dave.

Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Reviewed-by on v2 1/3
#7 lore raw

Re: [PATCH v2 1/3] xfs: use b_maps[] for discontiguous buffers

In reply to Mark Tinguely
Quoted 6 lines · Mark Tinguely

Commits starting at 77c1a08 introduced a multiple segment support to xfs_buf. xfs_trans_buf_item_match() could not find a multi-segment buffer in the transaction because it was looking at the single segment block number rather than the multi-segment b_maps[0].bm.bn. This results on a recursive buffer lock that can never be satisfied.

Quoted 6 lines

This patch:
1) Changed the remaining b_map accesses to be b_maps[0] accesses.
2) Renames the single segment b_map structure to __b_map to avoid future confusion.

Looks good.

Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#8 lore raw

PATCH v2 2/3 xfs: fix the buffer log format for contiguous buffers

In reply to Mark Tinguely

Not every segment in a multi-segment buffer is dirty in a transaction and they will not be outputted. The assert in xfs_buf_item_format_segment() that checks for the at least one chunk of data in the segment to be used is not necessary true for multi-segmented buffers.

This patch:

1) Change xfs_buf_item_format_segment() to skip over non-dirty segments.
2) Change the bli_format structure to __bli_format so it is not accidently confused with the bli_formats pointer.
3) Remove the buffer XFS_TRANS_DEBUG routines xfs_buf_item_log_debug() and xfs_buf_item_log_check(). They are not used and are not multi-segment aware.

---
 fs/xfs/xfs_buf_item.c  |  151 +++++++++----------------------------------------
 fs/xfs/xfs_buf_item.h  |    2 
 fs/xfs/xfs_trans_buf.c |   24 +++----
 3 files changed, 41 insertions(+), 136 deletions(-)
fs/xfs/xfs_buf_item.c +28 -123
Index: b/fs/xfs/xfs_buf_item.c
===================================================================
@@ -37,109 +37,6 @@ static inline struct xfs_buf_log_item *B
 	return container_of(lip, struct xfs_buf_log_item, bli_item);
 }
 
-
-#ifdef XFS_TRANS_DEBUG
-/*
- * This function uses an alternate strategy for tracking the bytes
- * that the user requests to be logged.  This can then be used
- * in conjunction with the bli_orig array in the buf log item to
- * catch bugs in our callers' code.
- *
- * We also double check the bits set in xfs_buf_item_log using a
- * simple algorithm to check that every byte is accounted for.
- */
-STATIC void
-xfs_buf_item_log_debug(
-	xfs_buf_log_item_t	*bip,
-	uint			first,
-	uint			last)
-{
-	uint	x;
-	uint	byte;
-	uint	nbytes;
-	uint	chunk_num;
-	uint	word_num;
-	uint	bit_num;
-	uint	bit_set;
-	uint	*wordp;
-
-	ASSERT(bip->bli_logged != NULL);
-	byte = first;
-	nbytes = last - first + 1;
-	bfset(bip->bli_logged, first, nbytes);
-	for (x = 0; x < nbytes; x++) {
-		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]);
-		bit_set = *wordp & (1 << bit_num);
-		ASSERT(bit_set);
-		byte++;
-	}
-}
-
-/*
- * This function is called when we flush something into a buffer without
- * logging it.  This happens for things like inodes which are logged
- * separately from the buffer.
- */
-void
-xfs_buf_item_flush_log_debug(
-	xfs_buf_t	*bp,
-	uint		first,
-	uint		last)
-{
-	xfs_buf_log_item_t	*bip = bp->b_fspriv;
-	uint			nbytes;
-
-	if (bip == NULL || (bip->bli_item.li_type != XFS_LI_BUF))
-		return;
-
-	ASSERT(bip->bli_logged != NULL);
-	nbytes = last - first + 1;
-	bfset(bip->bli_logged, first, nbytes);
-}
-
-/*
- * This function is called to verify that our callers have logged
- * all the bytes that they changed.
- *
- * It does this by comparing the original copy of the buffer stored in
- * the buf log item's bli_orig array to the current copy of the buffer
- * and ensuring that all bytes which mismatch are set in the bli_logged
- * array of the buf log item.
- */
-STATIC void
-xfs_buf_item_log_check(
-	xfs_buf_log_item_t	*bip)
-{
-	char		*orig;
-	char		*buffer;
-	int		x;
-	xfs_buf_t	*bp;
-
-	ASSERT(bip->bli_orig != NULL);
-	ASSERT(bip->bli_logged != NULL);
-
-	bp = bip->bli_buf;
-	ASSERT(bp->b_length > 0);
-	ASSERT(bp->b_addr != NULL);
-	orig = bip->bli_orig;
-	buffer = bp->b_addr;
-	for (x = 0; x < BBTOB(bp->b_length); x++) {
-		if (orig[x] != buffer[x] && !btst(bip->bli_logged, x)) {
-			xfs_emerg(bp->b_mount,
-				"%s: bip %x buffer %x orig %x index %d",
-				__func__, bip, bp, orig, x);
-			ASSERT(0);
-		}
-	}
-}
-#else
-#define		xfs_buf_item_log_debug(x,y,z)
-#define		xfs_buf_item_log_check(x)
-#endif
-
 STATIC void	xfs_buf_do_callbacks(struct xfs_buf *bp);
 
 /*
@@ -237,7 +134,7 @@ xfs_buf_item_size(
 		 * cancel flag in it.
 		 */
 		trace_xfs_buf_item_size_stale(bip);
-		ASSERT(bip->bli_format.blf_flags & XFS_BLF_CANCEL);
+		ASSERT(bip->__bli_format.blf_flags & XFS_BLF_CANCEL);
 		return bip->bli_format_count;
 	}
 
@@ -278,7 +175,7 @@ xfs_buf_item_format_segment(
 	uint		buffer_offset;
 
 	/* copy the flags across from the base format item */
-	blfp->blf_flags = bip->bli_format.blf_flags;
+	blfp->blf_flags = bip->__bli_format.blf_flags;
 
 	/*
 	 * Base size is the actual size of the ondisk structure - it reflects
@@ -287,11 +184,6 @@ xfs_buf_item_format_segment(
 	 */
 	base_size = offsetof(struct xfs_buf_log_format, blf_data_map) +
 			(blfp->blf_map_size * sizeof(blfp->blf_data_map[0]));
-	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) {
 		/*
@@ -301,7 +193,11 @@ xfs_buf_item_format_segment(
 		 */
 		trace_xfs_buf_item_format_stale(bip);
 		ASSERT(blfp->blf_flags & XFS_BLF_CANCEL);
-		blfp->blf_size = nvecs;
+		vecp->i_addr = blfp;
+		vecp->i_len = base_size;
+		vecp->i_type = XLOG_REG_TYPE_BFORMAT;
+		vecp++;
+		blfp->blf_size = 1;
 		return vecp;
 	}
 
@@ -309,7 +205,19 @@ 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.
+		 */
+		blfp->blf_size = 0;
+		return(vecp);
+	}
+
+	vecp->i_addr = blfp;
+	vecp->i_len = base_size;
+	vecp->i_type = XLOG_REG_TYPE_BFORMAT;
+	vecp++;
+	nvecs = 1;
 	last_bit = first_bit;
 	nbits = 1;
 	for (;;) {
@@ -371,7 +279,7 @@ xfs_buf_item_format_segment(
 			nbits++;
 		}
 	}
-	bip->bli_format.blf_size = nvecs;
+	blfp->blf_size = nvecs;
 	return vecp;
 }
 
@@ -405,7 +313,7 @@ xfs_buf_item_format(
 	if (bip->bli_flags & XFS_BLI_INODE_BUF) {
 		if (!((bip->bli_flags & XFS_BLI_INODE_ALLOC_BUF) &&
 		      xfs_log_item_in_current_chkpt(lip)))
-			bip->bli_format.blf_flags |= XFS_BLF_INODE_BUF;
+			bip->__bli_format.blf_flags |= XFS_BLF_INODE_BUF;
 		bip->bli_flags &= ~XFS_BLI_INODE_BUF;
 	}
 
@@ -419,7 +327,6 @@ xfs_buf_item_format(
 	 * Check to make sure everything is consistent.
 	 */
 	trace_xfs_buf_item_format(bip);
-	xfs_buf_item_log_check(bip);
 }
 
 /*
@@ -485,7 +392,7 @@ xfs_buf_item_unpin(
 		ASSERT(bip->bli_flags & XFS_BLI_STALE);
 		ASSERT(xfs_buf_islocked(bp));
 		ASSERT(XFS_BUF_ISSTALE(bp));
-		ASSERT(bip->bli_format.blf_flags & XFS_BLF_CANCEL);
+		ASSERT(bip->__bli_format.blf_flags & XFS_BLF_CANCEL);
 
 		trace_xfs_buf_item_unpin_stale(bip);
 
@@ -631,7 +538,7 @@ xfs_buf_item_unlock(
 	 */
 	if (bip->bli_flags & XFS_BLI_STALE) {
 		trace_xfs_buf_item_unlock_stale(bip);
-		ASSERT(bip->bli_format.blf_flags & XFS_BLF_CANCEL);
+		ASSERT(bip->__bli_format.blf_flags & XFS_BLF_CANCEL);
 		if (!aborted) {
 			atomic_dec(&bip->bli_refcount);
 			return;
@@ -644,8 +551,8 @@ 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))
+	if (xfs_bitmap_empty(bip->__bli_format.blf_data_map,
+			     bip->__bli_format.blf_map_size))
 		xfs_buf_item_relse(bp);
 	else
 		atomic_dec(&bip->bli_refcount);
@@ -716,7 +623,7 @@ xfs_buf_item_get_format(
 	bip->bli_format_count = count;
 
 	if (count == 1) {
-		bip->bli_formats = &bip->bli_format;
+		bip->bli_formats = &bip->__bli_format;
 		return 0;
 	}
 
@@ -731,7 +638,7 @@ STATIC void
 xfs_buf_item_free_format(
 	struct xfs_buf_log_item	*bip)
 {
-	if (bip->bli_formats != &bip->bli_format) {
+	if (bip->bli_formats != &bip->__bli_format) {
 		kmem_free(bip->bli_formats);
 		bip->bli_formats = NULL;
 	}
@@ -898,8 +805,6 @@ xfs_buf_item_log_segment(
 		mask = (1 << end_bit) - 1;
 		*wordp |= mask;
 	}
-
-	xfs_buf_item_log_debug(bip, first, last);
 }
 
 /*
fs/xfs/xfs_buf_item.h +1 -1
Index: b/fs/xfs/xfs_buf_item.h
===================================================================
@@ -104,7 +104,7 @@ typedef struct xfs_buf_log_item {
 #endif
 	int			bli_format_count;	/* count of headers */
 	struct xfs_buf_log_format *bli_formats;	/* array of in-log header ptrs */
-	struct xfs_buf_log_format bli_format;	/* embedded in-log header */
+	struct xfs_buf_log_format __bli_format;	/* embedded in-log header */
 } xfs_buf_log_item_t;
 
 void	xfs_buf_item_init(struct xfs_buf *, struct xfs_mount *);
fs/xfs/xfs_trans_buf.c +12 -12
Index: b/fs/xfs/xfs_trans_buf.c
===================================================================
@@ -93,7 +93,7 @@ _xfs_trans_bjoin(
 	xfs_buf_item_init(bp, tp->t_mountp);
 	bip = bp->b_fspriv;
 	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
-	ASSERT(!(bip->bli_format.blf_flags & XFS_BLF_CANCEL));
+	ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
 	ASSERT(!(bip->bli_flags & XFS_BLI_LOGGED));
 	if (reset_recur)
 		bip->bli_recur = 0;
@@ -432,7 +432,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
 	bip = bp->b_fspriv;
 	ASSERT(bip->bli_item.li_type == XFS_LI_BUF);
 	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
-	ASSERT(!(bip->bli_format.blf_flags & XFS_BLF_CANCEL));
+	ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
 	ASSERT(atomic_read(&bip->bli_refcount) > 0);
 
 	trace_xfs_trans_brelse(bip);
@@ -519,7 +519,7 @@ xfs_trans_bhold(xfs_trans_t	*tp,
 	ASSERT(bp->b_transp == tp);
 	ASSERT(bip != NULL);
 	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
-	ASSERT(!(bip->bli_format.blf_flags & XFS_BLF_CANCEL));
+	ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
 	ASSERT(atomic_read(&bip->bli_refcount) > 0);
 
 	bip->bli_flags |= XFS_BLI_HOLD;
@@ -539,7 +539,7 @@ xfs_trans_bhold_release(xfs_trans_t	*tp,
 	ASSERT(bp->b_transp == tp);
 	ASSERT(bip != NULL);
 	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
-	ASSERT(!(bip->bli_format.blf_flags & XFS_BLF_CANCEL));
+	ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
 	ASSERT(atomic_read(&bip->bli_refcount) > 0);
 	ASSERT(bip->bli_flags & XFS_BLI_HOLD);
 
@@ -598,7 +598,7 @@ xfs_trans_log_buf(xfs_trans_t	*tp,
 		bip->bli_flags &= ~XFS_BLI_STALE;
 		ASSERT(XFS_BUF_ISSTALE(bp));
 		XFS_BUF_UNSTALE(bp);
-		bip->bli_format.blf_flags &= ~XFS_BLF_CANCEL;
+		bip->__bli_format.blf_flags &= ~XFS_BLF_CANCEL;
 	}
 
 	tp->t_flags |= XFS_TRANS_DIRTY;
@@ -657,8 +657,8 @@ xfs_trans_binval(
 		 */
 		ASSERT(XFS_BUF_ISSTALE(bp));
 		ASSERT(!(bip->bli_flags & (XFS_BLI_LOGGED | XFS_BLI_DIRTY)));
-		ASSERT(!(bip->bli_format.blf_flags & XFS_BLF_INODE_BUF));
-		ASSERT(bip->bli_format.blf_flags & XFS_BLF_CANCEL);
+		ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_INODE_BUF));
+		ASSERT(bip->__bli_format.blf_flags & XFS_BLF_CANCEL);
 		ASSERT(bip->bli_item.li_desc->lid_flags & XFS_LID_DIRTY);
 		ASSERT(tp->t_flags & XFS_TRANS_DIRTY);
 		return;
@@ -668,10 +668,10 @@ xfs_trans_binval(
 
 	bip->bli_flags |= XFS_BLI_STALE;
 	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)));
+	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)));
 	bip->bli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
 	tp->t_flags |= XFS_TRANS_DIRTY;
 }
@@ -775,5 +775,5 @@ xfs_trans_dquot_buf(
 	       type == XFS_BLF_GDQUOT_BUF);
 	ASSERT(atomic_read(&bip->bli_refcount) > 0);
 
-	bip->bli_format.blf_flags |= type;
+	bip->__bli_format.blf_flags |= type;
 }

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#9 lore raw

Re: [PATCH v2 2/3] xfs: fix the buffer log format for contiguous buffers

Quoted 7 lines · Mark Tinguely

Not every segment in a multi-segment buffer is dirty in a transaction and they will not be outputted. The assert in xfs_buf_item_format_segment() that checks for the at least one chunk of data in the segment to be used is not necessary true for multi-segmented buffers.

This patch:

Has three separate, unrelated changes. That usually means 3 separate patches....

Quoted 2 lines

1) Change xfs_buf_item_format_segment() to skip over non-dirty segments.

Comments about this below.

Quoted 2 lines

2) Change the bli_format structure to __bli_format so it is not accidently confused with the bli_formats pointer.

This change looks ok.

Quoted 3 lines

3) Remove the buffer XFS_TRANS_DEBUG routines xfs_buf_item_log_debug() and xfs_buf_item_log_check(). They are not used and are not multi-segment aware.

Removing XFS_TRANS_DEBUG should definitely be done as a separate patch and it should remove it from everywhere (there's several stale uses) rather than just this one place. If we don't use it here, we don't use it anywhere....

....

Quoted 26 lines
fs/xfs/xfs_buf_item.c +5 -6
@@ -287,11 +184,6 @@ xfs_buf_item_format_segment(
 	 */
 	base_size = offsetof(struct xfs_buf_log_format, blf_data_map) +
 			(blfp->blf_map_size * sizeof(blfp->blf_data_map[0]));
-	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) {
 		/*
@@ -301,7 +193,11 @@ xfs_buf_item_format_segment(
 		 */
 		trace_xfs_buf_item_format_stale(bip);
 		ASSERT(blfp->blf_flags & XFS_BLF_CANCEL);
-		blfp->blf_size = nvecs;
+		vecp->i_addr = blfp;
+		vecp->i_len = base_size;
+		vecp->i_type = XLOG_REG_TYPE_BFORMAT;
+		vecp++;
+		blfp->blf_size = 1;
 		return vecp;
 	}
 

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

Quoted 8 lines
fs/xfs/xfs_buf_item.c +4 -1
@@ -309,7 +205,19 @@ 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.
+		 */

Comment format.

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

Quoted 2 lines
fs/xfs/xfs_buf_item.c +2 -0
+		blfp->blf_size = 0;
+		return(vecp);
		return vecp;

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

Quoted 7 lines
fs/xfs/xfs_buf_item.c +7 -0
+	}
+
+	vecp->i_addr = blfp;
+	vecp->i_len = base_size;
+	vecp->i_type = XLOG_REG_TYPE_BFORMAT;
+	vecp++;
+	nvecs = 1;

This results in duplicating the code. Perhaps it woul dbe better to rearrange the code slightly to avoid needing to do that. i.e:

{
	nvecs = 0;
	first_bit = xfs_next_bit(blfp->blf_data_map, blfp->blf_map_size, 0);

	if (!(bip->bli_flags & XFS_BLI_STALE) && 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.
		 */
		goto out;
	}

	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) {
		.....
		goto out;
	}
	.....

out:

	blfp->blf_size = nvecs;
	return vecp;
}

Cheers,

Dave.

Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#10 lore raw

PATCH v2 3/3 xfs: fix the multi-segment log buffer format

In reply to Mark Tinguely

Per Dave Chinner suggestion, this patch:
1) Corrects the detection of whether a multi-segment buffer is still tracking data.
2) Clears all the buffer log formats for a multi-segment buffer.

---
 fs/xfs/xfs_buf_item.c  |   13 ++++++++++---
 fs/xfs/xfs_trans_buf.c |    7 +++++--
 2 files changed, 15 insertions(+), 5 deletions(-)
fs/xfs/xfs_buf_item.c +10 -3
Index: b/fs/xfs/xfs_buf_item.c
===================================================================
@@ -508,7 +508,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. */
@@ -551,8 +551,15 @@ 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)) {
+			empty = 0;
+			break;
+		}
+	}
+	if (empty)
 		xfs_buf_item_relse(bp);
 	else
 		atomic_dec(&bip->bli_refcount);
fs/xfs/xfs_trans_buf.c +5 -2
Index: b/fs/xfs/xfs_trans_buf.c
===================================================================
@@ -643,6 +643,7 @@ xfs_trans_binval(
 	xfs_buf_t	*bp)
 {
 	xfs_buf_log_item_t	*bip = bp->b_fspriv;
+	int			i;
 
 	ASSERT(bp->b_transp == tp);
 	ASSERT(bip != NULL);
@@ -670,8 +671,10 @@ 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)));
+	for (i = 0; i < bip->bli_format_count; i++) {
+		memset((char *)(bip->bli_formats[i].blf_data_map), 0,
+		       (bip->bli_formats[i].blf_map_size * sizeof(uint)));
+	}
 	bip->bli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
 	tp->t_flags |= XFS_TRANS_DIRTY;
 }

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#12 lore raw

Re: [PATCH v2 3/3] xfs: fix the multi-segment log buffer format

In reply to Mark Tinguely
Quoted 33 lines · Mark Tinguely

Per Dave Chinner suggestion, this patch:
1) Corrects the detection of whether a multi-segment buffer is still tracking data.
2) Clears all the buffer log formats for a multi-segment buffer.

---
 fs/xfs/xfs_buf_item.c  |   13 ++++++++++---
 fs/xfs/xfs_trans_buf.c |    7 +++++--
 2 files changed, 15 insertions(+), 5 deletions(-)

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

fs/xfs/xfs_buf_item.c +3 -3
@@ -508,7 +508,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. */
@@ -551,8 +551,15 @@ 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++) {

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

Quoted 10 lines
fs/xfs/xfs_buf_item.c +7 -0
+		if (!xfs_bitmap_empty(bip->bli_formats[i].blf_data_map,
+			     bip->bli_formats[i].blf_map_size)) {
+			empty = 0;
+			break;
+		}
+	}
+	if (empty)
 		xfs_buf_item_relse(bp);
 	else
 		atomic_dec(&bip->bli_refcount);

I think "clean" is a better name for the temporary variable, as what we are actually checking is whether the buffer has been modified or not (i.e. whether it is clean or dirty)....

Quoted 13 lines

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

fs/xfs/xfs_trans_buf.c +1 -0
@@ -643,6 +643,7 @@ xfs_trans_binval(
 	xfs_buf_t	*bp)
 {
 	xfs_buf_log_item_t	*bip = bp->b_fspriv;
+	int			i;
 
 	ASSERT(bp->b_transp == tp);
 	ASSERT(bip != NULL);

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

Quoted 8 lines
fs/xfs/xfs_trans_buf.c +3 -2
@@ -670,8 +671,10 @@ 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)));
+	for (i = 0; i < bip->bli_format_count; i++) {
+		memset((char *)(bip->bli_formats[i].blf_data_map), 0,
+		       (bip->bli_formats[i].blf_map_size * sizeof(uint)));

No cast needed - memset() takes a void pointer....

Cheers,

Dave.

Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs