Re: [PATCH 2/4] xfs: use unhashed buffers for size checks

[Dave Chinner <david@fromorbit.com>]

On Wed, Sep 08, 2010 at 09:38:07PM -0400, Christoph Hellwig wrote:
> > +struct xfs_buf *
> > +xfs_buf_read_uncached(
> > +	struct xfs_mount	*mp,
> > +	struct xfs_buftarg	*target,
> > +	xfs_daddr_t		daddr,
> > +	size_t			length)
> > +{
> > +	xfs_buf_t	*bp;
> > +	int		error;
> 
> struct xfs_buf and the same indentation as the parameters, please.
> 
> > +
> > +	bp = xfs_buf_get_noaddr(length, target);
> 
> I think both the buf_get and buf_read interfaces for the non-hash
> buffers should have the same name.  Either your uncached or maybe better
> unhashed?  (And certainly no noaddr, which is not very useful)

I'll rename it *_uncached, because the hash is going away ;)

> 
> > +	if (!bp || XFS_BUF_ISERROR(bp))
> > +		goto fail;
> 
> xfs_buf_get_noaddr never returns an error in the buffer.

I'll fix all these - they are just CNP from the previous patch.
> 
> Also this one returns the buffer locked, while buf_get_noaddr doesn't.
> I suspect we should also change buf_get_noaddr to return a locked buffer
> to make it consistant with all other buf_read/get interfaces.

None of the other callers require locked buffers. I'll leave this
for a separate patch set for the moment.

> > +struct xfs_buf * xfs_buf_read_uncached(struct xfs_mount *mp,
> > +				struct xfs_buftarg *target,
> > +				xfs_daddr_t daddr, size_t length);
> 
> wrong placement of the *
> 
> 
> This patch, or at least the introduction of the new read helper should
> be moved before patch 1 so that we don't add code that gets removed a
> little later.

Yes, I plan to do that.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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