On Fri, Aug 26, 2011 at 04:11:32AM -0400, Christoph Hellwig wrote:
> > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> > index c57836d..594cea5 100644
> > --- a/fs/xfs/xfs_buf.c
> > +++ b/fs/xfs/xfs_buf.c
> > @@ -171,10 +171,16 @@ STATIC void
> > _xfs_buf_initialize(
> > xfs_buf_t *bp,
> > xfs_buftarg_t *target,
> > - xfs_off_t range_base,
> > - size_t range_length,
> > + xfs_off_t bno,
> > + size_t num_blocks,
> > xfs_buf_flags_t flags)
> > {
> > + xfs_off_t range_base;
> > + size_t range_length;
> > +
> > + range_base = BBTOB(bno);
> > + range_length = BBTOB(num_blocks);
>
> What is the point of changing the mostly unrelated _xfs_buf_initialize
> prototype in this patch?
We were converting units backwards and forwards inconsistently, some
functions taking bytes, some basic blocks, and conversions were
being done all over the place.
> I think it (and the other renaming changes related to it) are fine,
> but should be a separate patch.
OK, fine, I can do that.
> And once you touch _xfs_buf_initialize
> after the core of this patch, please merge it with xfs_buf_allocate into
> a new xfs_buf_alloc that does the full allocation + initialization and
> can also replace xfs_buf_get_empty.
Not right now. That restructing can be done separately, probably in
the same patch set that fixes the API types problems...
> > + bp = _xfs_buf_find(target, bno, num_blocks, 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;
> > + } else
> > xfs_buf_deallocate(new_bp);
>
> I'd recommend moving the call to xfs_buf_allocate_memory into
> _xfs_buf_find so that it returns a fully allocated buffer. In fact I'd
> also move the xfs_buf_deallocate(new_bp) into the found side of
> _xfs_buf_find, avoiding any conditionals in xfs_buf_get.
<sigh>
This code s pretty much as you requested it after the first time I
posted it.
http://oss.sgi.com/archives/xfs/2011-08/msg00146.html
I'll go rewrite this again, but IMO all you are asking for is for me
to put a different colour on the bike shed....
> >
> > - XFS_STATS_INC(xb_get);
> > -
> > /*
> > - * Always fill in the block number now, the mapped cases can do
> > - * their own overlay of this later.
> > + * Now we have a workable buffer, fill in the block number so
> > + * that we can do IO on it.
> > */
> > - bp->b_bn = ioff;
> > - bp->b_count_desired = bp->b_buffer_length;
> > + bp->b_bn = bno;
>
> Note that we only need this if we did not find an existing buffer. It's
> not strictly related to the patch, but given that you stop assigning
> b_count_desired and redo this whole area it might be worth shifting it
> into the if (bp == new_bp) conditional area.
OK.
> >
> > +found:
> > + ASSERT(bp->b_flags & XBF_MAPPED);
>
> This doesn't look right to me. Various buffers like inode or remoate attrs
> are unmapped, and I can't see any reason why we would assert not beeing
> allowed to find them here.
Yeah, a bit of a thinko, but it never tripped on me....
> Thinking about it more I'm also not sure skipping the code to map
> buffers on a straight cache hit is a good idea - there's nothing
> inherent to requiring a given buffer to be mapped for all callers.
OK, will fix.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
