xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/4] xfs: use unhashed buffers for size checks
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 10 Sep 2010 13:14:52 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20100909013806.GC29825@xxxxxxxxxxxxx>
References: <1283958778-28610-1-git-send-email-david@xxxxxxxxxxxxx> <1283958778-28610-3-git-send-email-david@xxxxxxxxxxxxx> <20100909013806.GC29825@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
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

<Prev in Thread] Current Thread [Next in Thread>