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@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
