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

[Dave Chinner <david@fromorbit.com>]

On Tue, Aug 09, 2011 at 03:51:51AM -0400, Christoph Hellwig wrote:
> 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:

Makes sense.

> 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.

I'll leave that problem for when RCU-izing the lookup - I haven't
quite worked out what the best way of doing that is yet, 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

OK.

>  - there should be no need to re-assign b_bn and b_count_desired
>    when we find a buffer

Yes, definitely.

I'm also tempted to rename some variables to make it more obvious
what units they are in and to reduce the conversions we do from
one type to another.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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