Skip to contentSkip to conversation

implement generic xfs_btree_split

PATCH In review Christoph Hellwig 2 participants 4 messages Jul 29, 2008 – Aug 2, 2008 1 message not in the archive XFS filesystem

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

#1 lore raw

PATCH 17/21 implement generic xfs_btree_split

Make the btree split code generic. Based on a patch from David Chinner with lots of changes to follow the original btree implementations more closely. While this loses some of the generic helper routines for inserting/moving/removing records it also solves some of the one off bugs in the original code and makes it easier to verify.

fs/xfs/xfs_btree.c +268 -0
Index: linux-2.6-xfs/fs/xfs/xfs_btree.c
===================================================================
@@ -844,6 +844,48 @@ xfs_btree_get_sibling(
 	}
 }
 
+STATIC void
+xfs_btree_set_sibling(
+	struct xfs_btree_cur	*cur,
+	struct xfs_btree_block	*block,
+	union xfs_btree_ptr	*ptr,
+	int			lr)
+{
+	ASSERT(lr == XFS_BB_LEFTSIB || lr == XFS_BB_RIGHTSIB);
+
+	if (cur->bc_flags & XFS_BTREE_LONG_PTRS) {
+		if (lr == XFS_BB_RIGHTSIB)
+			block->bb_u.l.bb_rightsib = ptr->l;
+		else
+			block->bb_u.l.bb_leftsib = ptr->l;
+	} else {
+		if (lr == XFS_BB_RIGHTSIB)
+			block->bb_u.s.bb_rightsib = ptr->s;
+		else
+			block->bb_u.s.bb_leftsib = ptr->s;
+	}
+}
+
+STATIC void
+xfs_btree_init_block(
+	struct xfs_btree_cur	*cur,
+	int			level,
+	int			numrecs,
+	struct xfs_btree_block	*new)	/* new block */
+{
+	new->bb_h.bb_magic = cpu_to_be32(xfs_magics[cur->bc_btnum]);
+	new->bb_h.bb_level = cpu_to_be16(level);
+	new->bb_h.bb_numrecs = cpu_to_be16(numrecs);
+
+	if (cur->bc_flags & XFS_BTREE_LONG_PTRS) {
+		new->bb_u.l.bb_leftsib = cpu_to_be64(NULLFSBLOCK);
+		new->bb_u.l.bb_rightsib = cpu_to_be64(NULLFSBLOCK);
+	} else {
+		new->bb_u.s.bb_leftsib = cpu_to_be32(NULLAGBLOCK);
+		new->bb_u.s.bb_rightsib = cpu_to_be32(NULLAGBLOCK);
+	}
+}
+
 /*
  * Return true if ptr is the last record in the btree and
  * we need to track updateѕ to this record.  The decision
@@ -880,6 +922,21 @@ xfs_btree_buf_to_block(
 	return XFS_BUF_TO_BLOCK(bp);
 }
 
+STATIC void
+xfs_btree_buf_to_ptr(
+	struct xfs_btree_cur	*cur,
+	struct xfs_buf		*bp,
+	union xfs_btree_ptr	*ptr)
+{
+	if (cur->bc_flags & XFS_BTREE_LONG_PTRS)
+		ptr->l = cpu_to_be64(XFS_DADDR_TO_FSB(cur->bc_mp,
+					XFS_BUF_ADDR(bp)));
+	else {
+		ptr->s = cpu_to_be32(XFS_DADDR_TO_AGBNO(cur->bc_mp,
+					XFS_BUF_ADDR(bp)));
+	}
+}
+
 STATIC xfs_daddr_t
 xfs_btree_ptr_to_daddr(
 	struct xfs_btree_cur	*cur,
@@ -919,6 +976,31 @@ xfs_btree_set_refs(
 	}
 }
 
+STATIC int
+xfs_btree_get_buf_block(
+	struct xfs_btree_cur	*cur,
+	union xfs_btree_ptr	*ptr,
+	int			flags,
+	struct xfs_btree_block	**block,
+	struct xfs_buf		**bpp)
+{
+	struct xfs_mount	*mp = cur->bc_mp;
+	xfs_daddr_t		d;
+
+	/* need to sort out how callers deal with failures first */
+	ASSERT(!(flags & XFS_BUF_TRYLOCK));
+
+	d = xfs_btree_ptr_to_daddr(cur, ptr);
+	*bpp = xfs_trans_get_buf(cur->bc_tp, mp->m_ddev_targp, d,
+				 mp->m_bsize, flags);
+
+	ASSERT(*bpp);
+	ASSERT(!XFS_BUF_GETERROR(*bpp));
+
+	*block = XFS_BUF_TO_BLOCK(*bpp);
+	return 0;
+}
+
 /*
  * Read in the buffer at the given ptr and return the buffer and
  * the block pointer within the buffer.
@@ -1968,3 +2050,189 @@ error1:
 	xfs_btree_del_cursor(tcur, XFS_BTREE_ERROR);
 	return error;
 }
+
+/*
+ * Split cur/level block in half.
+ * Return new block number and the key to its first
+ * record (to be inserted into parent).
+ */
+int						/* error */
+xfs_btree_split(
+	struct xfs_btree_cur	*cur,
+	int			level,
+	union xfs_btree_ptr	*ptrp,
+	union xfs_btree_key	*key,
+	struct xfs_btree_cur	**curp,
+	int			*stat)		/* success/failure */
+{
+	union xfs_btree_ptr	lptr;		/* left sibling block ptr */
+	struct xfs_buf		*lbp;		/* left buffer pointer */
+	struct xfs_btree_block	*left;		/* left btree block */
+	union xfs_btree_ptr	rptr;		/* right sibling block ptr */
+	struct xfs_buf		*rbp;		/* right buffer pointer */
+	struct xfs_btree_block	*right;		/* right btree block */
+	union xfs_btree_ptr	rrptr;		/* right-right sibling ptr */
+	struct xfs_buf		*rrbp;		/* right-right buffer pointer */
+	struct xfs_btree_block	*rrblock;	/* right-right btree block */
+	int			lrecs;
+	int			rrecs;
+	int			src_index;
+	int			error;		/* error return value */
+#ifdef DEBUG
+	int			i;
+#endif
+
+	XFS_BTREE_TRACE_CURSOR(cur, XBT_ENTRY);
+	XFS_BTREE_TRACE_ARGIPK(cur, level, *ptrp, key);
+
+	XFS_BTREE_STATS_INC(cur, split);
+
+	/* Set up left block (current one). */
+	left = xfs_btree_get_block(cur, level, &lbp);
+
+#ifdef DEBUG
+	error = xfs_btree_check_block(cur, left, level, lbp);
+	if (error)
+		goto error0;
+#endif
+
+	xfs_btree_buf_to_ptr(cur, lbp, &lptr);
+
+	/* Allocate the new block. If we can't do it, we're toast. Give up. */
+	error = cur->bc_ops->alloc_block(cur, &lptr, &rptr, 1, stat);
+	if (error)
+		goto error0;
+	if (*stat == 0)
+		goto out0;
+	XFS_BTREE_STATS_INC(cur, alloc);
+
+	/* Set up the new block as "right". */
+	error = xfs_btree_get_buf_block(cur, &rptr, 0, &right, &rbp);
+	if (error)
+		goto error0;
+
+	/* Fill in the btree header for the new right block. */
+	xfs_btree_init_block(cur, be16_to_cpu(left->bb_h.bb_level), 0, right);
+
+	/*
+	 * Split the entries between the old and the new block evenly.
+	 * Make sure that if there's an odd number of entries now, that
+	 * each new block will have the same number of entries.
+	 */
+	lrecs = xfs_btree_get_numrecs(left);
+	rrecs = lrecs / 2;
+	if ((lrecs & 1) && cur->bc_ptrs[level] <= rrecs + 1)
+		rrecs++;
+	src_index = (lrecs - rrecs + 1);
+
+	XFS_BTREE_STATS_ADD(cur, moves, rrecs);
+
+	/*
+	 * Copy btree block entries from the left block over to the
+	 * new block, the right. Update the right block and log the
+	 * changes.
+	 */
+	if (level > 0) {
+		/* It's a non-leaf.  Move keys and pointers. */
+		union xfs_btree_key	*lkp;	/* left btree key */
+		union xfs_btree_ptr	*lpp;	/* left address pointer */
+		union xfs_btree_key	*rkp;	/* right btree key */
+		union xfs_btree_ptr	*rpp;	/* right address pointer */
+
+		lkp = cur->bc_ops->key_addr(cur, src_index, left);
+		lpp = cur->bc_ops->ptr_addr(cur, src_index, left);
+		rkp = cur->bc_ops->key_addr(cur, 1, right);
+		rpp = cur->bc_ops->ptr_addr(cur, 1, right);
+
+#ifdef DEBUG
+		for (i = src_index; i < rrecs; i++) {
+			error = xfs_btree_check_ptr(cur, lpp, i, level);
+			if (error)
+				goto error0;
+		}
+#endif
+
+		cur->bc_ops->copy_keys(cur, lkp, rkp, rrecs);
+		xfs_btree_copy_ptrs(cur, lpp, rpp, rrecs);
+
+		cur->bc_ops->log_keys(cur, rbp, 1, rrecs);
+		xfs_btree_log_ptrs(cur, rbp, 1, rrecs);
+
+		/* Grab the keys to the entries moved to the right block */
+		cur->bc_ops->copy_keys(cur, rkp, key, 1);
+	} else {
+		/* It's a leaf.  Move records.  */
+		union xfs_btree_rec	*lrp;	/* left record pointer */
+		union xfs_btree_rec	*rrp;	/* right record pointer */
+
+		lrp = cur->bc_ops->rec_addr(cur, src_index, left);
+		rrp = cur->bc_ops->rec_addr(cur, 1, right);
+
+		cur->bc_ops->copy_recs(cur, lrp, rrp, rrecs);
+		cur->bc_ops->log_recs(cur, rbp, 1, rrecs);
+
+		cur->bc_ops->init_key_from_rec(cur, key,
+			cur->bc_ops->rec_addr(cur, 1, right));
+	}
+
+
+	/*
+	 * Find the left block number by looking in the buffer.
+	 * Adjust numrecs, sibling pointers.
+	 */
+	xfs_btree_get_sibling(cur, left, &rrptr, XFS_BB_RIGHTSIB);
+	xfs_btree_set_sibling(cur, right, &rrptr, XFS_BB_RIGHTSIB);
+	xfs_btree_set_sibling(cur, right, &lptr, XFS_BB_LEFTSIB);
+	xfs_btree_set_sibling(cur, left, &rptr, XFS_BB_RIGHTSIB);
+
+	lrecs -= rrecs;
+	xfs_btree_set_numrecs(left, lrecs);
+	xfs_btree_set_numrecs(right, xfs_btree_get_numrecs(right) + rrecs);
+
+	xfs_btree_log_block(cur, rbp, XFS_BB_ALL_BITS);
+	xfs_btree_log_block(cur, lbp, XFS_BB_NUMRECS | XFS_BB_RIGHTSIB);
+
+	/*
+	 * If there's a block to the new block's right, make that block
+	 * point back to right instead of to left.
+	 */
+	if (!xfs_btree_ptr_is_null(cur, &rrptr)) {
+		error = xfs_btree_read_buf_block(cur, &rrptr, level,
+							0, &rrblock, &rrbp);
+		if (error)
+			goto error0;
+		xfs_btree_set_sibling(cur, rrblock, &rptr, XFS_BB_LEFTSIB);
+		xfs_btree_log_block(cur, rrbp, XFS_BB_LEFTSIB);
+	}
+	/*
+	 * If the cursor is really in the right block, move it there.
+	 * If it's just pointing past the last entry in left, then we'll
+	 * insert there, so don't change anything in that case.
+	 */
+	if (cur->bc_ptrs[level] > lrecs + 1) {
+		xfs_btree_setbuf(cur, level, rbp);
+		cur->bc_ptrs[level] -= lrecs;
+	}
+	/*
+	 * If there are more levels, we'll need another cursor which refers
+	 * the right block, no matter where this cursor was.
+	 */
+	if (level + 1 < cur->bc_nlevels) {
+		error = xfs_btree_dup_cursor(cur, curp);
+		if (error)
+			goto error0;
+		(*curp)->bc_ptrs[level + 1]++;
+	}
+	*ptrp = rptr;
+	XFS_BTREE_TRACE_CURSOR(cur, XBT_EXIT);
+	*stat = 1;
+	return 0;
+out0:
+	XFS_BTREE_TRACE_CURSOR(cur, XBT_EXIT);
+	*stat = 0;
+	return 0;
+
+error0:
+	XFS_BTREE_TRACE_CURSOR(cur, XBT_ERROR);
+	return error;
+}
fs/xfs/xfs_btree.h +8 -0
Index: linux-2.6-xfs/fs/xfs/xfs_btree.h
===================================================================
@@ -191,6 +191,12 @@ struct xfs_btree_ops {
 	/* get inode rooted btree root */
 	struct xfs_btree_block *(*get_root_from_inode)(struct xfs_btree_cur *);
 
+	/* block allocation / freeing */
+	int	(*alloc_block)(struct xfs_btree_cur *cur,
+			       union xfs_btree_ptr *sbno,
+			       union xfs_btree_ptr *nbno,
+			       int length, int *stat);
+
 	/* updated last record information */
 	void	(*update_lastrec)(struct xfs_btree_cur *,
 				  struct xfs_btree_block *,
@@ -577,6 +583,8 @@ int xfs_btree_updkey(struct xfs_btree_cu
 int xfs_btree_update(struct xfs_btree_cur *, union xfs_btree_rec *);
 int xfs_btree_lshift(struct xfs_btree_cur *, int, int *);
 int xfs_btree_rshift(struct xfs_btree_cur *, int, int *);
+int xfs_btree_split(struct xfs_btree_cur *, int, union xfs_btree_ptr *,
+		union xfs_btree_key *, struct xfs_btree_cur **, int *);
 
 
 /*
fs/xfs/xfs_alloc_btree.c +41 -158

236 lines not on this page · raw message

fs/xfs/xfs_ialloc_btree.c +48 -163

248 lines not on this page · raw message

fs/xfs/xfs_bmap_btree.c +93 -182

310 lines not on this page · raw message

#2 lore raw
Quoted 7 lines · Christoph Hellwig

Make the btree split code generic. Based on a patch from David Chinner with lots of changes to follow the original btree implementations more closely. While this loses some of the generic helper routines for inserting/moving/removing records it also solves some of the one off bugs in the original code and makes it easier to verify.

.....

Quoted 9 lines
fs/xfs/xfs_btree.c +9 -0
+STATIC void
+xfs_btree_init_block(
+	struct xfs_btree_cur	*cur,
+	int			level,
+	int			numrecs,
+	struct xfs_btree_block	*new)	/* new block */
+{
+	new->bb_h.bb_magic = cpu_to_be32(xfs_magics[cur->bc_btnum]);
+	new->bb_h.bb_level = cpu_to_be16(level);

fs/xfs/xfs_btree.c xfs_btree_get_sibling( · line 887

Quoted 10 lines
fs/xfs/xfs_btree.c +10 -0
+	new->bb_h.bb_numrecs = cpu_to_be16(numrecs);
+
+	if (cur->bc_flags & XFS_BTREE_LONG_PTRS) {
+		new->bb_u.l.bb_leftsib = cpu_to_be64(NULLFSBLOCK);
+		new->bb_u.l.bb_rightsib = cpu_to_be64(NULLFSBLOCK);
+	} else {
+		new->bb_u.s.bb_leftsib = cpu_to_be32(NULLAGBLOCK);
+		new->bb_u.s.bb_rightsib = cpu_to_be32(NULLAGBLOCK);
+	}
+}

This is where I begin to question this approach (i.e. using helpers like this rather than specific ops like I did). It's taken me 4 ŏr 5 patches to put my finger on it.

The intent of this factorisation is to make implementing new btree structures easy, not making the current code better or more managable. The first thing we need is is btrees with different header blocks (self describing information, CRCs, etc). This above function will suddenly have four combinations to deal with - long and short, version 1 and version 2 header formats. The more we change, the more this complicates these helpers. That is why I pushed seemingly trivial stuff out to operations vectors - because of the future flexibility it allowed in implementation of new btrees.....

I don't see this a problem for this patch series, but I can see that some of this work will end up being converted back to ops vectors as soon as we start modifying between structures....

Quoted 10 lines
fs/xfs/xfs_btree.c +10 -0
+STATIC int
+xfs_btree_get_buf_block(
+	struct xfs_btree_cur	*cur,
+	union xfs_btree_ptr	*ptr,
+	int			flags,
+	struct xfs_btree_block	**block,
+	struct xfs_buf		**bpp)
+{
+	struct xfs_mount	*mp = cur->bc_mp;
+	xfs_daddr_t		d;

fs/xfs/xfs_btree.c xfs_btree_set_refs( · line 998

Quoted 10 lines
fs/xfs/xfs_btree.c +10 -0
+
+	/* need to sort out how callers deal with failures first */
+	ASSERT(!(flags & XFS_BUF_TRYLOCK));
+
+	d = xfs_btree_ptr_to_daddr(cur, ptr);
+	*bpp = xfs_trans_get_buf(cur->bc_tp, mp->m_ddev_targp, d,
+				 mp->m_bsize, flags);
+
+	ASSERT(*bpp);
+	ASSERT(!XFS_BUF_GETERROR(*bpp));

xfs_trans_get_buf() can return NULL, right?

fs/xfs/xfs_btree.h struct xfs_btree_ops { · line 197

Quoted 4 lines
fs/xfs/xfs_btree.h +4 -0
+	/* block allocation / freeing */
+	int	(*alloc_block)(struct xfs_btree_cur *cur,
+			       union xfs_btree_ptr *sbno,
+			       union xfs_btree_ptr *nbno,

start_bno, new_bno.

Otherwise looks good.

Cheers,

Dave.

Signature · 2 lines
Dave Chinner
[email protected]
#3 lore raw
Quoted 14 lines

This is where I begin to question this approach (i.e. using helpers like this rather than specific ops like I did). It's taken me 4 ??r 5 patches to put my finger on it.

The intent of this factorisation is to make implementing new btree structures easy, not making the current code better or more managable. The first thing we need is is btrees with different header blocks (self describing information, CRCs, etc). This above function will suddenly have four combinations to deal with - long and short, version 1 and version 2 header formats. The more we change, the more this complicates these helpers. That is why I pushed seemingly trivial stuff out to operations vectors - because of the future flexibility it allowed in implementation of new btrees.....

Quoted 3 lines

I don't see this a problem for this patch series, but I can see that some of this work will end up being converted back to ops vectors as soon as we start modifying between structures....

Maybe. But even when we convert it to ops vectors it should not be the btree implementation vector, but a btree_block_ops that's implemented once instead of duplicated for the alloc vs ialloc btree. And for now having all this in xfs_btree.c makes reading and working on the patch series easier, so..

Quoted 11 lines

fs/xfs/xfs_btree.c xfs_btree_set_refs( · line 998

Quoted 9 lines
fs/xfs/xfs_btree.c +9 -0
+	/* need to sort out how callers deal with failures first */
+	ASSERT(!(flags & XFS_BUF_TRYLOCK));
+
+	d = xfs_btree_ptr_to_daddr(cur, ptr);
+	*bpp = xfs_trans_get_buf(cur->bc_tp, mp->m_ddev_targp, d,
+				 mp->m_bsize, flags);
+
+	ASSERT(*bpp);
+	ASSERT(!XFS_BUF_GETERROR(*bpp));

xfs_trans_get_buf() can return NULL, right?

Only when XFS_BUF_TRYLOCK is set, which it never is for the btree code, and the assert above makes sure we catch any new caller early.

Quoted 6 lines

fs/xfs/xfs_btree.h struct xfs_btree_ops { · line 197

Quoted 4 lines
fs/xfs/xfs_btree.h +4 -0
+	/* block allocation / freeing */
+	int	(*alloc_block)(struct xfs_btree_cur *cur,
+			       union xfs_btree_ptr *sbno,
+			       union xfs_btree_ptr *nbno,

start_bno, new_bno.

Done.

#4 lore raw
Quoted 14 lines · Christoph Hellwig
Quoted 14 lines

This is where I begin to question this approach (i.e. using helpers like this rather than specific ops like I did). It's taken me 4 ??r 5 patches to put my finger on it.

The intent of this factorisation is to make implementing new btree structures easy, not making the current code better or more managable. The first thing we need is is btrees with different header blocks (self describing information, CRCs, etc). This above function will suddenly have four combinations to deal with - long and short, version 1 and version 2 header formats. The more we change, the more this complicates these helpers. That is why I pushed seemingly trivial stuff out to operations vectors - because of the future flexibility it allowed in implementation of new btrees.....

Quoted 8 lines
Quoted 3 lines

I don't see this a problem for this patch series, but I can see that some of this work will end up being converted back to ops vectors as soon as we start modifying between structures....

Maybe. But even when we convert it to ops vectors it should not be the btree implementation vector, but a btree_block_ops that's implemented once instead of duplicated for the alloc vs ialloc btree.

Yes, that makes sense - I had sort of headed that way, but it was not fully though out...

Quoted 2 lines

And for now having all this in xfs_btree.c makes reading and working on the patch series easier, so..

Fair enough - do the work when we implement a new btree ;)

Cheers,

Dave.

Signature · 2 lines
Dave Chinner
[email protected]