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

[Dave Chinner <david@fromorbit.com>]

On Wed, Nov 28, 2012 at 04:23:11PM -0600, Mark Tinguely wrote:
> Not every segment in a multi-segment buffer is dirty in a
> transaction and they will not be outputted. The assert in
> xfs_buf_item_format_segment() that checks for the at least
> one chunk of data in the segment to be used is not necessary
> true for multi-segmented buffers.
> 
> This patch:

Has three separate, unrelated changes. That usually means 3
separate patches....

>  1) Change xfs_buf_item_format_segment() to skip over non-dirty
>     segments.

Comments about this below.

>  2) Change the bli_format structure to __bli_format so it is not
>     accidently confused with the bli_formats pointer.

This change looks ok.

>  3) Remove the buffer XFS_TRANS_DEBUG routines xfs_buf_item_log_debug()
>     and xfs_buf_item_log_check(). They are not used and are not
>     multi-segment aware.

Removing XFS_TRANS_DEBUG should definitely be done as a separate
patch and it should remove it from everywhere (there's several stale
uses) rather than just this one place. If we don't use it here, we
don't use it anywhere....

....

> @@ -287,11 +184,6 @@ xfs_buf_item_format_segment(
>  	 */
>  	base_size = offsetof(struct xfs_buf_log_format, blf_data_map) +
>  			(blfp->blf_map_size * sizeof(blfp->blf_data_map[0]));
> -	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 +193,11 @@ xfs_buf_item_format_segment(
>  		 */
>  		trace_xfs_buf_item_format_stale(bip);
>  		ASSERT(blfp->blf_flags & XFS_BLF_CANCEL);
> -		blfp->blf_size = nvecs;
> +		vecp->i_addr = blfp;
> +		vecp->i_len = base_size;
> +		vecp->i_type = XLOG_REG_TYPE_BFORMAT;
> +		vecp++;
> +		blfp->blf_size = 1;
>  		return vecp;
>  	}
>  
> @@ -309,7 +205,19 @@ 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.
> +		 */

Comment format.

> +		blfp->blf_size = 0;
> +		return(vecp);

		return vecp;
> +	}
> +
> +	vecp->i_addr = blfp;
> +	vecp->i_len = base_size;
> +	vecp->i_type = XLOG_REG_TYPE_BFORMAT;
> +	vecp++;
> +	nvecs = 1;

This results in duplicating the code. Perhaps it woul dbe better to
rearrange the code slightly to avoid needing to do that. i.e:

{
	nvecs = 0;
	first_bit = xfs_next_bit(blfp->blf_data_map, blfp->blf_map_size, 0);

	if (!(bip->bli_flags & XFS_BLI_STALE) && 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.
		 */
		goto out;
	}

	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) {
		.....
		goto out;
	}
	.....

out:
	blfp->blf_size = nvecs;
	return vecp;
}

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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