Quantcast

[PATCH 0/2] discontiguous buffer patches

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 0/2] discontiguous buffer patches

Mark Tinguely-3
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 <[hidden email]>
    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
[hidden email]
http://oss.sgi.com/mailman/listinfo/xfs
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mark Tinguely-3
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 <[hidden email]>

---
 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
[hidden email]
http://oss.sgi.com/mailman/listinfo/xfs
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mark Tinguely-3
In reply to this post by Mark Tinguely-3
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 <[hidden email]>

---
 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
[hidden email]
http://oss.sgi.com/mailman/listinfo/xfs
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Christoph Hellwig
In reply to this post by Mark Tinguely-3
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
[hidden email]
http://oss.sgi.com/mailman/listinfo/xfs
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Christoph Hellwig
In reply to this post by Mark Tinguely-3
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
[hidden email]
http://oss.sgi.com/mailman/listinfo/xfs
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mark Tinguely-3
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
[hidden email]
http://oss.sgi.com/mailman/listinfo/xfs
Loading...