xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/4] xfs: Don't allocate new buffers on every call to _xfs_buf_find
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Fri, 26 Aug 2011 04:11:32 -0400
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1314341497-23910-2-git-send-email-david@xxxxxxxxxxxxx>
References: <1314341497-23910-1-git-send-email-david@xxxxxxxxxxxxx> <1314341497-23910-2-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
> 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?

I think it (and the other renaming changes related to it) are fine,
but should be a separate patch.  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.

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


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


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

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.

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