Re: [PATCH 02/10] xfs: separate buffer indexing from block map

[Dave Chinner <david@fromorbit.com>]

On Tue, May 01, 2012 at 08:16:59AM -0500, Mark Tinguely wrote:
> On 04/30/12 18:24, Dave Chinner wrote:
> >On Mon, Apr 30, 2012 at 02:28:44PM -0500, Mark Tinguely wrote:
> >>On 04/24/12 01:33, 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>
> >>...
> >>>+struct xfs_buf_map {
> >>>+	xfs_daddr_t		bm_bn;	/* block number for I/O */
> >>>+	int			bm_len;	/* size of I/O */
> >>>+};
> >>>+
> >>>  typedef struct xfs_buf {
> >>>  	/*
> >>>  	 * first cacheline holds all the fields needed for an uncontended cache
> >>>@@ -107,7 +114,7 @@ typedef struct xfs_buf {
> >>>  	 * fast-path on locking.
> >>>  	 */
> >>>  	struct rb_node		b_rbnode;	/* rbtree node */
> >>>-	xfs_daddr_t		b_bn;		/* block number for I/O */
> >>>+	xfs_daddr_t		b_bn;		/* block number of buffer */
> >>>  	int			b_length;	/* size of buffer in BBs */
> >>
> >>Looks good.
> >>Do you plan to eventually remove b_bn and b_length from xfs_buf?
> >
> >No. b_bn is a fast way of identifying unique buffers for cache
> >lookups and is located in the same cacheline as the tree node so we
> >don't take an extra cache miss on every buffer we traverse during
> >tree walks in _xfs_buf_find(). Also, b_length is used so often it is
> >much cleaner to keep it around than it s to iterate over all the
> >maps to calculate it every time it is needed.
> 
> Thanks for the reply. I was thinking that maybe the "inline" map
> could do both of those things. It is not used if there are multiple
> maps and is the same value as the original variables if there is
> only one map.

I thought about doing that, but then the code gets harder to follow.
e.g. why do we use bp->b_map.b_bn in some places, and
bp->b_maps[0].b_bn in others when most of the time they both point
to the same thing? It just maps it too easy to
misunderstand/misuse/confuse the difference between the buffer
identifiers and the real IO block addresses. I prefer the clarity of
purpose that retaining b_bn/b_length gives, even though there is a
slight increase in memory usage....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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