Re: [PATCH 1/2] xfs: Don't allocate new buffers on every call to _xfs_buf_find

[Christoph Hellwig <hch@infradead.org>]

On Mon, Aug 08, 2011 at 04:51:10PM +1000, Dave Chinner wrote:
> -xfs_buf_t *
> +struct xfs_buf *
>  xfs_buf_get(
> -	xfs_buftarg_t		*target,/* target for buffer		*/
> +	struct xfs_buftarg	*target,/* target for buffer		*/
>  	xfs_off_t		ioff,	/* starting offset of range	*/
>  	size_t			isize,	/* length of range		*/
>  	xfs_buf_flags_t		flags)
>  {
> -	xfs_buf_t		*bp, *new_bp;
> +	struct xfs_buf		*bp;
> +	struct xfs_buf		*new_bp = NULL;
>  	int			error = 0;
>  
> -	new_bp = xfs_buf_allocate(flags);
> -	if (unlikely(!new_bp))
> -		return NULL;
> -
> +again:
>  	bp = _xfs_buf_find(target, ioff, isize, flags, new_bp);
> -	if (bp == new_bp) {
> +	if (!(bp || new_bp)) {
> +		new_bp = xfs_buf_allocate(flags);
> +		if (unlikely(!new_bp))
> +			return NULL;
> +
> +		_xfs_buf_initialize(new_bp, target, ioff << BBSHIFT,
> +				isize << BBSHIFT, flags);
> +		goto again;
> +	}
> +
> +	if (!bp) {
> +		xfs_buf_deallocate(new_bp);
> +		return NULL;
> +	} else if (bp == new_bp) {
>  		error = xfs_buf_allocate_memory(bp, flags);
>  		if (error)
>  			goto no_buffer;
> -	} else {
> +	} else if (new_bp)
>  		xfs_buf_deallocate(new_bp);
> -		if (unlikely(bp == NULL))
> -			return NULL;
> -	}

The while loop looks a bit confusing to me.  Why not unroll it similar
to how it's done for the VFS inode cache:

	bp = _xfs_buf_find(target, ioff, isize, flags, new_bp);
	if (bp)
		goto found;

	new_bp = xfs_buf_allocate(flags);
	if (unlikely(!new_bp))
		return NULL;
	_xfs_buf_initialize(new_bp, target, ioff << BBSHIFT, isize << BBSHIFT,
			    flags);
	bp = _xfs_buf_find(target, ioff, isize, flags, new_bp);
	if (!bp) {
		xfs_buf_deallocate(new_bp);
		return NULL;
	}

	if (bp == new_bp) {
  		error = xfs_buf_allocate_memory(bp, flags);
  		if (error)
  			goto no_buffer;
	}
found:


or even better make sure all buffer initialization is done either as
part of xfs_buf_find or the caller - although we'd need a variant
with caller locking if we want to go down the latter route.  This
variant would be nessecary if we ever want to add shared/exclusive or
RCU locking for buffer lookup anyway.

There's also some other weird things here that should be cleaned up
when touching it:

 - we should never be able to find a buffer with the wrong mapped
   state, so this could be replaced by an assert
 - there should be no need to re-assign b_bn and b_count_desired
   when we find a buffer

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