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