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

[Christoph Hellwig <hch@infradead.org>]

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:

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.


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:


Reviewed-by: Christoph Hellwig <hch@lst.de>

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