[PATCH 0/2] discontiguous buffer patches

[PATCH 0/2] discontiguous buffer patches

[Mark Tinguely]

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:

    commit 3605431fb9739a30ccd0c6380ae8e3c6f8e670a5
    Author: Dave Chinner <dchinner@redhat.com>
    Date:   Fri Jun 22 18:50:13 2012 +1000
    xfs: use discontiguous xfs_buf support in dabuf wrappers

xfs_trans_buf_item_match() is looking for the block number of the
buffer in the single segment map area.

Futhermore, there are a couple issue with multi-segment buffer log
format.

Patch 1 cleans up the buffer map so that XFS always uses b_maps[].

Patch 2 fixes the buffer log format issues.

--Mark.


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Mark Tinguely]

[-- Attachment #1: xfs-use-xfs_buf-b_maps.patch --]
[-- Type: text/plain, Size: 2545 bytes --]

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 <tinguely@sgi.com>

---
 fs/xfs/xfs_buf.c |    8 ++++----
 fs/xfs/xfs_buf.h |    4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

Index: b/fs/xfs/xfs_buf.c
===================================================================
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -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)
Index: b/fs/xfs/xfs_buf.h
===================================================================
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Mark Tinguely]

[-- Attachment #1: xfs-fix-blf-for-multiple-segment.patch --]
[-- Type: text/plain, Size: 3705 bytes --]

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 <tinguely@sgi.com>

---
 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
===================================================================
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -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++;
@@ -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) {
 		/*
@@ -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;
 	}
 
@@ -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.
+		 */
+		blfp->blf_size = 0;
+		return(vecp);
+	}
+
+	nvecs = 1;
+	vecp++;
 	last_bit = first_bit;
 	nbits = 1;
 	for (;;) {
@@ -371,7 +379,7 @@ xfs_buf_item_format_segment(
 			nbits++;
 		}
 	}
-	bip->bli_format.blf_size = nvecs;
+	blfp->blf_size = nvecs;
 	return vecp;
 }
 
@@ -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.
 	 */
-	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);
Index: b/fs/xfs/xfs_trans_buf.c
===================================================================
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -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)));
 	bip->bli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
 	tp->t_flags |= XFS_TRANS_DIRTY;
 }


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Christoph Hellwig]

On Tue, Nov 20, 2012 at 04:41:21PM -0600, Mark Tinguely wrote:
> 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.

Should b_map be renamed to __b_map so that accesses to it are cought
more easily?

Also do hyou have a test case for the issue?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Christoph Hellwig]

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.

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

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

> @@ -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)) {
> +			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.

Also as in the last patch should we rename bli_format to __bli_format
to catch accidental references to it?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Mark Tinguely]

On 11/21/12 03:54, Christoph Hellwig wrote:
> 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.
>
>>   		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]);
>
> This change doesn't seem to be mentioned in the changelog?
>
>> @@ -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)) {
>> +			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.
>

The bli_format -> bli_formats changes are bug fixes - the description 
should say this is a bug fix not just a cleanup.

Eric's test 291 created a buffer with 4 mapped segments. It found most 
of the issues in the buffer map and in the buffer log format. Dave 
pointed out the xfs_bitmap_empty() error.

--Mark.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Dave Chinner]

On Tue, Nov 20, 2012 at 04:41:21PM -0600, Mark Tinguely wrote:
> 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 <tinguely@sgi.com>

Looks good, though I agree with Christoph that renaming b_map to
__b_map would be a good idea to prevent future confusion.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Dave Chinner]

On Tue, Nov 20, 2012 at 04:41:22PM -0600, Mark Tinguely wrote:
> 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 <tinguely@sgi.com>
> 
> ---
>  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
> ===================================================================
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -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....

> @@ -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) {
>  		/*
> @@ -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.....

>  
> @@ -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.
> +		 */
> +		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.


> @@ -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.
>  	 */
> -	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.

> +	if (empty)
>  		xfs_buf_item_relse(bp);
>  	else
>  		atomic_dec(&bip->bli_refcount);
> Index: b/fs/xfs/xfs_trans_buf.c
> ===================================================================
> --- a/fs/xfs/xfs_trans_buf.c
> +++ b/fs/xfs/xfs_trans_buf.c
> @@ -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.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Christoph Hellwig]

On Fri, Nov 23, 2012 at 12:47:14PM +1100, Dave Chinner wrote:
> 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....

As far as I know it has been unused since I started working on XFS,
so removing it sounds fine to me.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Mark Tinguely]

On 11/22/12 19:47, Dave Chinner wrote:
> On Tue, Nov 20, 2012 at 04:41:22PM -0600, Mark Tinguely wrote:
>> 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<tinguely@sgi.com>
>>
>> ---
>>   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
>> ===================================================================
>> --- a/fs/xfs/xfs_buf_item.c
>> +++ b/fs/xfs/xfs_buf_item.c
>> @@ -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....
>

Okay. I will do that separately. Want to leave the XFS_TRANS_DEBUG for 
the inode, inode_item and AIL or remove it completely?

>> @@ -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) {
>>   		/*
>> @@ -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.....
>
>>
>> @@ -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.
>> +		 */
>> +		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.
>

I see your point. I will change that.

>
>> @@ -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.
>>   	 */
>> -	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.
>

okay.

>> +	if (empty)
>>   		xfs_buf_item_relse(bp);
>>   	else
>>   		atomic_dec(&bip->bli_refcount);
>> Index: b/fs/xfs/xfs_trans_buf.c
>> ===================================================================
>> --- a/fs/xfs/xfs_trans_buf.c
>> +++ b/fs/xfs/xfs_trans_buf.c
>> @@ -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.
>

Thank-you for the feed back.

--Mark.
> Cheers,
>
> Dave.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs