* [PATCH 0/2] discontiguous buffer patches
@ 2012-11-20 22:41 Mark Tinguely
2012-11-20 22:41 ` [PATCH 1/2] xfs: use b_maps[] for discontiguous buffers Mark Tinguely
2012-11-20 22:41 ` [PATCH 2/2] xfs: fix the buffer log format for contiguous buffers Mark Tinguely
0 siblings, 2 replies; 10+ messages in thread
From: Mark Tinguely @ 2012-11-20 22:41 UTC (permalink / raw)
To: xfs
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
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/2] xfs: use b_maps[] for discontiguous buffers 2012-11-20 22:41 [PATCH 0/2] discontiguous buffer patches Mark Tinguely @ 2012-11-20 22:41 ` Mark Tinguely 2012-11-21 9:51 ` Christoph Hellwig 2012-11-23 1:32 ` Dave Chinner 2012-11-20 22:41 ` [PATCH 2/2] xfs: fix the buffer log format for contiguous buffers Mark Tinguely 1 sibling, 2 replies; 10+ messages in thread From: Mark Tinguely @ 2012-11-20 22:41 UTC (permalink / raw) To: xfs [-- 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] xfs: use b_maps[] for discontiguous buffers 2012-11-20 22:41 ` [PATCH 1/2] xfs: use b_maps[] for discontiguous buffers Mark Tinguely @ 2012-11-21 9:51 ` Christoph Hellwig 2012-11-23 1:32 ` Dave Chinner 1 sibling, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2012-11-21 9:51 UTC (permalink / raw) To: Mark Tinguely; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] xfs: use b_maps[] for discontiguous buffers 2012-11-20 22:41 ` [PATCH 1/2] xfs: use b_maps[] for discontiguous buffers Mark Tinguely 2012-11-21 9:51 ` Christoph Hellwig @ 2012-11-23 1:32 ` Dave Chinner 1 sibling, 0 replies; 10+ messages in thread From: Dave Chinner @ 2012-11-23 1:32 UTC (permalink / raw) To: Mark Tinguely; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] xfs: fix the buffer log format for contiguous buffers 2012-11-20 22:41 [PATCH 0/2] discontiguous buffer patches Mark Tinguely 2012-11-20 22:41 ` [PATCH 1/2] xfs: use b_maps[] for discontiguous buffers Mark Tinguely @ 2012-11-20 22:41 ` Mark Tinguely 2012-11-21 9:54 ` Christoph Hellwig 2012-11-23 1:47 ` Dave Chinner 1 sibling, 2 replies; 10+ messages in thread From: Mark Tinguely @ 2012-11-20 22:41 UTC (permalink / raw) To: xfs [-- 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] xfs: fix the buffer log format for contiguous buffers 2012-11-20 22:41 ` [PATCH 2/2] xfs: fix the buffer log format for contiguous buffers Mark Tinguely @ 2012-11-21 9:54 ` Christoph Hellwig 2012-11-21 13:21 ` Mark Tinguely 2012-11-23 1:47 ` Dave Chinner 1 sibling, 1 reply; 10+ messages in thread From: Christoph Hellwig @ 2012-11-21 9:54 UTC (permalink / raw) To: Mark Tinguely; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] xfs: fix the buffer log format for contiguous buffers 2012-11-21 9:54 ` Christoph Hellwig @ 2012-11-21 13:21 ` Mark Tinguely 0 siblings, 0 replies; 10+ messages in thread From: Mark Tinguely @ 2012-11-21 13:21 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] xfs: fix the buffer log format for contiguous buffers 2012-11-20 22:41 ` [PATCH 2/2] xfs: fix the buffer log format for contiguous buffers Mark Tinguely 2012-11-21 9:54 ` Christoph Hellwig @ 2012-11-23 1:47 ` Dave Chinner 2012-11-23 7:37 ` Christoph Hellwig 2012-11-25 18:59 ` Mark Tinguely 1 sibling, 2 replies; 10+ messages in thread From: Dave Chinner @ 2012-11-23 1:47 UTC (permalink / raw) To: Mark Tinguely; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] xfs: fix the buffer log format for contiguous buffers 2012-11-23 1:47 ` Dave Chinner @ 2012-11-23 7:37 ` Christoph Hellwig 2012-11-25 18:59 ` Mark Tinguely 1 sibling, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2012-11-23 7:37 UTC (permalink / raw) To: Dave Chinner; +Cc: Mark Tinguely, xfs 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] xfs: fix the buffer log format for contiguous buffers 2012-11-23 1:47 ` Dave Chinner 2012-11-23 7:37 ` Christoph Hellwig @ 2012-11-25 18:59 ` Mark Tinguely 1 sibling, 0 replies; 10+ messages in thread From: Mark Tinguely @ 2012-11-25 18:59 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-11-25 18:57 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-11-20 22:41 [PATCH 0/2] discontiguous buffer patches Mark Tinguely 2012-11-20 22:41 ` [PATCH 1/2] xfs: use b_maps[] for discontiguous buffers Mark Tinguely 2012-11-21 9:51 ` Christoph Hellwig 2012-11-23 1:32 ` Dave Chinner 2012-11-20 22:41 ` [PATCH 2/2] xfs: fix the buffer log format for contiguous buffers Mark Tinguely 2012-11-21 9:54 ` Christoph Hellwig 2012-11-21 13:21 ` Mark Tinguely 2012-11-23 1:47 ` Dave Chinner 2012-11-23 7:37 ` Christoph Hellwig 2012-11-25 18:59 ` Mark Tinguely
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox