Re: [PATCH v3 4/5] xfs: fix the multi-segment log buffer format

[Dave Chinner <david@fromorbit.com>]

On Sat, Dec 08, 2012 at 07:38:21AM -0500, Christoph Hellwig wrote:
> On Tue, Dec 04, 2012 at 05:18:05PM -0600, Mark Tinguely wrote:
> > 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.
> > 
> > Signed-off-by: Mark Tinguely <tinguely@sgi.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > ---
> > fs/xfs/xfs_buf_item.c  |   13 ++++++++++---
> >  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
> > ===================================================================
> > --- a/fs/xfs/xfs_buf_item.c
> > +++ b/fs/xfs/xfs_buf_item.c
> > @@ -611,7 +611,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, clean, i;
> >  	uint			hold;
> >  
> >  	/* Clear the buffer's association with this transaction. */
> > @@ -654,8 +654,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))
> > +	clean = 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)) {
> > +			clean = 0;
> > +			break;
> > +		}
> > +	}
> > +	if (clean)
> >  		xfs_buf_item_relse(bp);
> >  	else
> >  		atomic_dec(&bip->bli_refcount);
> 
> Looks ok, although avoiding the clean variable would be even better:
> 
> 	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)) {
> 			atomic_dec(&bip->bli_refcount);
> 			goto out;
> 		}
> 	}
> 
> 	xfs_buf_item_relse(bp);
> out:

Definitely better.

> 
> bu that might be getting a bit too much into bikeshedding.
> 
> What I'm worried more about is how we semi-duplicate this bli_refcount
> decrement vs xfs_buf_item_relse in xfs_trans_brelse, but use the
> xfs_buf_item_dirty (aka XFS_BLI_DIRTY) check there instead.

Well, we only get to the case in xfs_trans_brelse() if the buffer
was not modified in this transaction. Hence the check is for whether
it was modified in a previous transaction (and hence is in the AIL)
and not whether the buffer has any changes in the bitmap or not.

So to me the checks seem to be for two different cases - one if so
whether the buffer has physical changes, the other for whether it is
currently in the AIL.

A further complication is that the XFS_BLI_DIRTY flag is cleared
when the buffer is marked stale, so any path that looks at this flag
needs to specifically handle the XFS_BLI_STALE case before the dirty
case.

It seems to me that the one place that XFS_BLI_DIRTY is checked
could actually be replaced with a:

	if (!(bip->bli_item.li_flags & XFS_LI_IN_AIL)) {
		....
	}

check and hence remove the reason for it's existence completely.
At that point, the flag could be repurposed as you suggest here:

> It seems like the proper fix might be to:
> 
>  - only set XFS_BLI_DIRTY in xfs_buf_item_log if we actually set
>    any bits in a bitmap
>  - use the XFS_BLI_DIRTY check in xfs_buf_item_unlock as well
>  - kill the useless xfs_buf_item_dirty wrapper
> 
> Probably both of these aren't worth doing it for now as we'll need to
> get fixes into Linus tree quickly, so:

Agreed.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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