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@xxxxxxx> > > --- > 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@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs