Skip to contentSkip to conversation

refactor xfs_btree_readahead

PATCH In review Christoph Hellwig 2 participants 5 messages Jul 23, 2008 – Jul 30, 2008 XFS filesystem

lore
Share & tools
lore

Christoph Hellwig

author
#0 lore raw

PATCH 7/15 refactor xfs_btree_readahead

From: Dave Chinner <[email protected]>

Refactor xfs_btree_readahead to make it more readable:

(a) remove the inline xfs_btree_readahead wrapper and move all checks out of line into the main routine.
(b) factor out helpers for short/long form btrees
(c) move check for root in inodes from the callers into xfs_btree_readahead

[hch: split out from a big patch and minor cleanups]

Signed-off-by Christoph Hellwig <[email protected]>
fs/xfs/xfs_btree.c +68 -50
Index: linux-2.6-xfs/fs/xfs/xfs_btree.c
===================================================================
@@ -709,66 +709,84 @@ xfs_btree_reada_bufs(
 	xfs_baread(mp->m_ddev_targp, d, mp->m_bsize * count);
 }
 
+STATIC int
+xfs_btree_reada_corel(
+	struct xfs_btree_cur	*cur,
+	int			lr,
+	struct xfs_btree_lblock	*lb)
+{
+	int			rval = 0;
+	xfs_fsblock_t		left = be64_to_cpu(lb->bb_leftsib);
+	xfs_fsblock_t		right = be64_to_cpu(lb->bb_rightsib);
+
+	if ((lr & XFS_BTCUR_LEFTRA) && left != NULLDFSBNO) {
+		xfs_btree_reada_bufl(cur->bc_mp, left, 1);
+		rval++;
+	}
+
+	if ((lr & XFS_BTCUR_RIGHTRA) && right != NULLDFSBNO) {
+		xfs_btree_reada_bufl(cur->bc_mp, right, 1);
+		rval++;
+	}
+
+	return rval;
+}
+
+STATIC int
+xfs_btree_reada_cores(
+	struct xfs_btree_cur	*cur,
+	int			lr,
+	struct xfs_btree_sblock *sb)
+{
+	int			rval = 0;
+	xfs_agblock_t		left = be32_to_cpu(sb->bb_leftsib);
+	xfs_agblock_t		right = be32_to_cpu(sb->bb_rightsib);
+
+
+	if ((lr & XFS_BTCUR_LEFTRA) && left != NULLAGBLOCK) {
+		xfs_btree_reada_bufs(cur->bc_mp, cur->bc_private.a.agno,
+				     left, 1);
+		rval++;
+	}
+
+	if ((lr & XFS_BTCUR_RIGHTRA) && right != NULLAGBLOCK) {
+		xfs_btree_reada_bufs(cur->bc_mp, cur->bc_private.a.agno,
+				     right, 1);
+		rval++;
+	}
+
+	return rval;
+}
+
 /*
  * Read-ahead btree blocks, at the given level.
  * Bits in lr are set from XFS_BTCUR_{LEFT,RIGHT}RA.
  */
 int
-xfs_btree_readahead_core(
-	xfs_btree_cur_t		*cur,		/* btree cursor */
+xfs_btree_readahead(
+	struct xfs_btree_cur	*cur,		/* btree cursor */
 	int			lev,		/* level in btree */
 	int			lr)		/* left/right bits */
 {
-	xfs_alloc_block_t	*a;
-	xfs_bmbt_block_t	*b;
-	xfs_inobt_block_t	*i;
-	int			rval = 0;
+	struct xfs_buf		*bp;
+
+	/*
+	 * No readahead needed if we are at the root level and the
+	 * btree root is stored in the inode.
+	 */
+	if ((cur->bc_flags & XFS_BTREE_ROOT_IN_INODE) &&
+	    (lev == cur->bc_nlevels - 1))
+		return 0;
+
+	if ((cur->bc_ra[lev] | lr) == cur->bc_ra[lev])
+		return 0;
 
-	ASSERT(cur->bc_bufs[lev] != NULL);
 	cur->bc_ra[lev] |= lr;
-	switch (cur->bc_btnum) {
-	case XFS_BTNUM_BNO:
-	case XFS_BTNUM_CNT:
-		a = XFS_BUF_TO_ALLOC_BLOCK(cur->bc_bufs[lev]);
-		if ((lr & XFS_BTCUR_LEFTRA) && be32_to_cpu(a->bb_leftsib) != NULLAGBLOCK) {
-			xfs_btree_reada_bufs(cur->bc_mp, cur->bc_private.a.agno,
-				be32_to_cpu(a->bb_leftsib), 1);
-			rval++;
-		}
-		if ((lr & XFS_BTCUR_RIGHTRA) && be32_to_cpu(a->bb_rightsib) != NULLAGBLOCK) {
-			xfs_btree_reada_bufs(cur->bc_mp, cur->bc_private.a.agno,
-				be32_to_cpu(a->bb_rightsib), 1);
-			rval++;
-		}
-		break;
-	case XFS_BTNUM_BMAP:
-		b = XFS_BUF_TO_BMBT_BLOCK(cur->bc_bufs[lev]);
-		if ((lr & XFS_BTCUR_LEFTRA) && be64_to_cpu(b->bb_leftsib) != NULLDFSBNO) {
-			xfs_btree_reada_bufl(cur->bc_mp, be64_to_cpu(b->bb_leftsib), 1);
-			rval++;
-		}
-		if ((lr & XFS_BTCUR_RIGHTRA) && be64_to_cpu(b->bb_rightsib) != NULLDFSBNO) {
-			xfs_btree_reada_bufl(cur->bc_mp, be64_to_cpu(b->bb_rightsib), 1);
-			rval++;
-		}
-		break;
-	case XFS_BTNUM_INO:
-		i = XFS_BUF_TO_INOBT_BLOCK(cur->bc_bufs[lev]);
-		if ((lr & XFS_BTCUR_LEFTRA) && be32_to_cpu(i->bb_leftsib) != NULLAGBLOCK) {
-			xfs_btree_reada_bufs(cur->bc_mp, cur->bc_private.a.agno,
-				be32_to_cpu(i->bb_leftsib), 1);
-			rval++;
-		}
-		if ((lr & XFS_BTCUR_RIGHTRA) && be32_to_cpu(i->bb_rightsib) != NULLAGBLOCK) {
-			xfs_btree_reada_bufs(cur->bc_mp, cur->bc_private.a.agno,
-				be32_to_cpu(i->bb_rightsib), 1);
-			rval++;
-		}
-		break;
-	default:
-		ASSERT(0);
-	}
-	return rval;
+	bp = cur->bc_bufs[lev];
+
+	if (cur->bc_flags & XFS_BTREE_LONG_PTRS)
+		return xfs_btree_reada_corel(cur, lr, XFS_BUF_TO_LBLOCK(bp));
+	return xfs_btree_reada_cores(cur, lr, XFS_BUF_TO_SBLOCK(bp));
 }
 
 /*
fs/xfs/xfs_btree.h +1 -14
Index: linux-2.6-xfs/fs/xfs/xfs_btree.h
===================================================================
@@ -429,23 +429,10 @@ xfs_btree_reada_bufs(
  * Bits in lr are set from XFS_BTCUR_{LEFT,RIGHT}RA.
  */
 int					/* readahead block count */
-xfs_btree_readahead_core(
-	xfs_btree_cur_t		*cur,	/* btree cursor */
-	int			lev,	/* level in btree */
-	int			lr);	/* left/right bits */
-
-static inline int			/* readahead block count */
 xfs_btree_readahead(
 	xfs_btree_cur_t		*cur,	/* btree cursor */
 	int			lev,	/* level in btree */
-	int			lr)	/* left/right bits */
-{
-	if ((cur->bc_ra[lev] | lr) == cur->bc_ra[lev])
-		return 0;
-
-	return xfs_btree_readahead_core(cur, lev, lr);
-}
-
+	int			lr);	/* left/right bits */
 
 /*
  * Set the buffer for level "lev" in the cursor to bp, releasing
fs/xfs/xfs_bmap_btree.c +7 -8
Index: linux-2.6-xfs/fs/xfs/xfs_bmap_btree.c
===================================================================
@@ -1721,8 +1721,9 @@ xfs_bmbt_decrement(
 	XFS_BMBT_TRACE_CURSOR(cur, ENTRY);
 	XFS_BMBT_TRACE_ARGI(cur, level);
 	ASSERT(level < cur->bc_nlevels);
-	if (level < cur->bc_nlevels - 1)
-		xfs_btree_readahead(cur, level, XFS_BTCUR_LEFTRA);
+
+	xfs_btree_readahead(cur, level, XFS_BTCUR_LEFTRA);
+
 	if (--cur->bc_ptrs[level] > 0) {
 		XFS_BMBT_TRACE_CURSOR(cur, EXIT);
 		*stat = 1;

@@ -1743,8 +1744,7 @@ xfs_bmbt_decrement(
 	for (lev = level + 1; lev < cur->bc_nlevels; lev++) {
 		if (--cur->bc_ptrs[lev] > 0)
 			break;
-		if (lev < cur->bc_nlevels - 1)
-			xfs_btree_readahead(cur, lev, XFS_BTCUR_LEFTRA);
+		xfs_btree_readahead(cur, lev, XFS_BTCUR_LEFTRA);
 	}
 	if (lev == cur->bc_nlevels) {
 		XFS_BMBT_TRACE_CURSOR(cur, EXIT);

@@ -1995,8 +1995,8 @@ xfs_bmbt_increment(
 	XFS_BMBT_TRACE_CURSOR(cur, ENTRY);
 	XFS_BMBT_TRACE_ARGI(cur, level);
 	ASSERT(level < cur->bc_nlevels);
-	if (level < cur->bc_nlevels - 1)
-		xfs_btree_readahead(cur, level, XFS_BTCUR_RIGHTRA);
+
+	xfs_btree_readahead(cur, level, XFS_BTCUR_RIGHTRA);
 	block = xfs_bmbt_get_block(cur, level, &bp);
 #ifdef DEBUG
 	if ((error = xfs_btree_check_lblock(cur, block, level, bp))) {

@@ -2024,8 +2024,7 @@ xfs_bmbt_increment(
 #endif
 		if (++cur->bc_ptrs[lev] <= be16_to_cpu(block->bb_numrecs))
 			break;
-		if (lev < cur->bc_nlevels - 1)
-			xfs_btree_readahead(cur, lev, XFS_BTCUR_RIGHTRA);
+		xfs_btree_readahead(cur, lev, XFS_BTCUR_RIGHTRA);
 	}
 	if (lev == cur->bc_nlevels) {
 		XFS_BMBT_TRACE_CURSOR(cur, EXIT);
#1 lore raw
Quoted 1 line · Christoph Hellwig

From: Dave Chinner <[email protected]>

Can i review my own code? ;)

Quoted 18 lines

Refactor xfs_btree_readahead to make it more readable:

(a) remove the inline xfs_btree_readahead wrapper and move all checks out of line into the main routine.
(b) factor out helpers for short/long form btrees
(c) move check for root in inodes from the callers into xfs_btree_readahead

[hch: split out from a big patch and minor cleanups]

Signed-off-by Christoph Hellwig <[email protected]>

Index: linux-2.6-xfs/fs/xfs/xfs_btree.c
===================================================================

fs/xfs/xfs_btree.c xfs_btree_reada_bufs( · line 713

Quoted 5 lines
fs/xfs/xfs_btree.c +2 -0
@@ -709,66 +709,84 @@ xfs_btree_reada_bufs(
 	xfs_baread(mp->m_ddev_targp, d, mp->m_bsize * count);
 }
 
+STATIC int
+xfs_btree_reada_corel(

I've been wondering if this is the best naming convention - appending a single s or l to indicate short or long btree ops. Perhaps this would be better as xfs_btree_readahead_short() and xfs_btree_readahead_long(), esp. as we dropped the 'core' from the caller to make it xfs_btree_readahead().

Otherwise it looks good.

Cheers,

Dave.

Signature · 2 lines
Dave Chinner
[email protected]
author
#2 lore raw
Quoted 4 lines · Dave Chinner
Quoted 1 line · Christoph Hellwig

From: Dave Chinner <[email protected]>

Can i review my own code? ;)

Sure..

Quoted 10 lines

fs/xfs/xfs_btree.c xfs_btree_reada_bufs( · line 713

Quoted 2 lines
fs/xfs/xfs_btree.c +2 -0
+STATIC int
+xfs_btree_reada_corel(

I've been wondering if this is the best naming convention - appending a single s or l to indicate short or long btree ops. Perhaps this would be better as xfs_btree_readahead_short() and xfs_btree_readahead_long(), esp. as we dropped the 'core' from the caller to make it xfs_btree_readahead().

Otherwise it looks good.

I'll see if I can find a saner name.

author
#3 lore raw
In reply to Dave Chinner
Quoted 5 lines · Dave Chinner

I've been wondering if this is the best naming convention - appending a single s or l to indicate short or long btree ops. Perhaps this would be better as xfs_btree_readahead_short() and xfs_btree_readahead_long(), esp. as we dropped the 'core' from the caller to make it xfs_btree_readahead().

I've renamedthe helpers to xfs_btree_readahead_lblock / xfs_btree_readahead_slblock to match the naming for the other short / long block helpers.

#4 lore raw
Quoted 10 lines · Christoph Hellwig
Quoted 5 lines · Dave Chinner

I've been wondering if this is the best naming convention - appending a single s or l to indicate short or long btree ops. Perhaps this would be better as xfs_btree_readahead_short() and xfs_btree_readahead_long(), esp. as we dropped the 'core' from the caller to make it xfs_btree_readahead().

I've renamedthe helpers to xfs_btree_readahead_lblock / xfs_btree_readahead_slblock to match the naming for the other short / long block helpers.

Ok, that sounds reasonable.

Cheers,

Dave.

Signature · 2 lines
Dave Chinner
[email protected]