Re: [PATCH 1/9] xfs: separate buffer indexing from block map

From: Christoph Hellwig <hch@infradead.org>
To: Dave Chinner <david@fromorbit.com>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 1/9] xfs: separate buffer indexing from block map
Date: Wed, 20 Jun 2012 02:44:51 -0400	[thread overview]
Message-ID: <20120620064451.GB14760@infradead.org> (raw)
In-Reply-To: <1339133914-11148-2-git-send-email-david@fromorbit.com>

On Fri, Jun 08, 2012 at 03:38:26PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> To support discontiguous buffers in the buffer cache, we need to
> separate the cache index variables from the I/O map. While this is
> currently a 1:1 mapping, discontiguous buffer support will break
> this relationship.
> 
> However, for caching purposes, we can still treat them the same as a
> contiguous buffer - the block number of the first block and the
> length of the buffer - as that is still a unique representation.
> Also, the only way we will ever access the discontiguous regions of
> buffers is via bulding the complete buffer in the first place, so
> using the initial block number and entire buffer length is a sane
> way to index the buffers.
> 
> Add a block mapping vector construct to the xfs_buf and use it in
> the places where we are doing IO instead of the current
> b_bn/b_length variables.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_buf.c |   21 ++++++++++++---------
>  fs/xfs/xfs_buf.h |   17 +++++++++++++----
>  2 files changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index a4beb42..90c5b6a 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -199,9 +199,11 @@ xfs_buf_alloc(
>  	 * most cases but may be reset (e.g. XFS recovery).
>  	 */
>  	bp->b_length = numblks;
> +	bp->b_map.bm_len = numblks;
>  	bp->b_io_length = numblks;
>  	bp->b_flags = flags;
>  	bp->b_bn = blkno;
> +	bp->b_map.bm_bn = blkno;

nipick: any reason not to initialize the two fields of b_map next
to each other?

> -	bp->b_io_length = bp->b_length;
> -

oh, I just mentioned that we can remove this in reply to Jan's patch,
so this is already taken care of, too.

>  #ifdef XFS_BUF_LOCK_TRACKING
>  	int			b_last_holder;
>  #endif
> @@ -233,8 +242,8 @@ void xfs_buf_stale(struct xfs_buf *bp);
>  #define XFS_BUF_UNWRITE(bp)	((bp)->b_flags &= ~XBF_WRITE)
>  #define XFS_BUF_ISWRITE(bp)	((bp)->b_flags & XBF_WRITE)
>  
> -#define XFS_BUF_ADDR(bp)		((bp)->b_bn)
> -#define XFS_BUF_SET_ADDR(bp, bno)	((bp)->b_bn = (xfs_daddr_t)(bno))
> +#define XFS_BUF_ADDR(bp)		((bp)->b_map.bm_bn)
> +#define XFS_BUF_SET_ADDR(bp, bno)	((bp)->b_map.bm_bn = (xfs_daddr_t)(bno))

It's not really obvious why XFS_BUF_SET_ADDR should not set b_bn from
it's defintion.  Looking at the callers it seems because it's only used
for uncached buffers, which never use b_bn, but it's still confusing.

I'm fine with keeping it for now bith a big comment to get this series
in, but XFS_BUF_SET_ADDR and b_io_length probably should go away ASAP in
favor or a variant of xfs_buf_iorequest that takes a bn/len pair .

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs