Skip to contentSkip to conversation

move xfs_bmbt_killroot to common code

PATCH In review Christoph Hellwig 3 participants 4 messages Aug 4, 2008 – Aug 5, 2008 XFS filesystem

lore
Share & tools
lore
#0 lore raw

PATCH 22/26 move xfs_bmbt_killroot to common code

xfs_bmbt_killroot is a mostly generic implementation of moving from a real block based root to an inode based root. So move it to xfs_btree.c where it can use all the nice infrastructure there and make it pointer size agnostic

The new name for it is xfs_btree_root_to_iroot which is not very nice but at least slightly more descriptive than the old name.

fs/xfs/xfs_btree.c +106 -0
Index: linux-2.6-xfs/fs/xfs/xfs_btree.c
===================================================================
@@ -2838,3 +2838,109 @@ error0:
 	XFS_BTREE_TRACE_CURSOR(cur, XBT_ERROR);
 	return error;
 }
+
+int
+xfs_btree_root_to_iroot(
+	struct xfs_btree_cur	*cur)
+{
+	int			whichfork = cur->bc_private.b.whichfork;
+	struct xfs_inode	*ip = cur->bc_private.b.ip;
+	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
+	struct xfs_btree_block	*block;
+	struct xfs_btree_block	*cblock;
+	union xfs_btree_key	*kp;
+	union xfs_btree_key	*ckp;
+	union xfs_btree_ptr	*pp;
+	union xfs_btree_ptr	*cpp;
+	struct xfs_buf		*cbp;
+	int			level;
+	int			index;
+	int			numrecs;
+#ifdef DEBUG
+	union xfs_btree_ptr	ptr;
+	int			i;
+#endif
+
+	XFS_BTREE_TRACE_CURSOR(cur, XBT_ENTRY);
+	level = cur->bc_nlevels - 1;
+	ASSERT(level >= 1);
+
+	/*
+	 * Don't deal with the root block needs to be a leaf case.
+	 * We're just going to turn the thing back into extents anyway.
+	 */
+	if (level == 1)
+		goto out0;
+
+	block = xfs_btree_get_block(cur, level, &cbp);
+
+	/*
+	 * Give up if the root has multiple children.
+	 */
+	if (xfs_btree_get_numrecs(block) != 1)
+		goto out0;
+
+	/*
+	 * Only do this if the next level will fit.
+	 * Then the data must be copied up to the inode,
+	 * instead of freeing the root you free the next level.
+	 */
+	cblock = xfs_btree_get_block(cur, level - 1, &cbp);
+	numrecs = xfs_btree_get_numrecs(cblock);
+
+	if (numrecs > cur->bc_ops->get_dmaxrecs(cur, level))
+	    	goto out0;
+
+	XFS_BTREE_STATS_INC(cur, killroot);
+
+#ifdef DEBUG
+	xfs_btree_get_sibling(cur, block, &ptr, XFS_BB_LEFTSIB);
+	ASSERT(xfs_btree_ptr_is_null(cur, &ptr));
+	xfs_btree_get_sibling(cur, block, &ptr, XFS_BB_RIGHTSIB);
+	ASSERT(xfs_btree_ptr_is_null(cur, &ptr));
+
+	// XXX(hch): this assert is bmap btree specific
+ 	ASSERT(cur->bc_ops->get_maxrecs(cur, level) ==
+	       XFS_BMAP_BROOT_MAXRECS(ifp->if_broot_bytes));
+#endif
+
+	index = numrecs - cur->bc_ops->get_maxrecs(cur, level);
+	if (index) {
+		cur->bc_ops->realloc_root(cur, index);
+		block = (struct xfs_btree_block *)ifp->if_broot;
+	}
+
+	be16_add(&block->bb_numrecs, index);
+	ASSERT(block->bb_numrecs == numrecs);
+
+	kp = cur->bc_ops->key_addr(cur, 1, block);
+	ckp = cur->bc_ops->key_addr(cur, 1, cblock);
+	cur->bc_ops->copy_keys(cur, ckp, kp, numrecs);
+
+	pp = cur->bc_ops->ptr_addr(cur, 1, block);
+	cpp = cur->bc_ops->ptr_addr(cur, 1, cblock);
+#ifdef DEBUG
+	for (i = 0; i < numrecs; i++) {
+		int		error;
+
+		error = xfs_btree_check_ptr(cur, cpp, i, level - 1);
+		if (error) {
+			XFS_BTREE_TRACE_CURSOR(cur, XBT_ERROR);
+			return error;
+		}
+	}
+#endif
+	xfs_btree_copy_ptrs(cur, cpp, pp, numrecs);
+
+	cur->bc_ops->free_block(cur, cbp, 1);
+	XFS_BTREE_STATS_INC(cur, free);
+
+	cur->bc_bufs[level - 1] = NULL;
+	be16_add(&block->bb_level, -1);
+	xfs_trans_log_inode(cur->bc_tp, ip,
+		XFS_ILOG_CORE | XFS_ILOG_FBROOT(cur->bc_private.b.whichfork));
+	cur->bc_nlevels--;
+out0:
+	XFS_BTREE_TRACE_CURSOR(cur, XBT_EXIT);
+	return 0;
+}
fs/xfs/xfs_alloc_btree.c +34 -0
Index: linux-2.6-xfs/fs/xfs/xfs_alloc_btree.c
===================================================================
@@ -781,6 +781,39 @@ xfs_allocbt_alloc_block(
 	return 0;
 }
 
+STATIC int
+xfs_allocbt_free_block(
+	struct xfs_btree_cur	*cur,
+	struct xfs_buf		*bp,
+	int			size)
+{
+	struct xfs_agf		*agf = XFS_BUF_TO_AGF(cur->bc_private.a.agbp);
+	xfs_agblock_t		bno;
+	int			error;
+
+	bno = XFS_DADDR_TO_AGBNO(cur->bc_mp, XFS_BUF_ADDR(bp));
+
+	error = xfs_alloc_put_freelist(cur->bc_tp, cur->bc_private.a.agbp,
+						NULL, bno, size);
+	if (error)
+		return error;
+
+	/*
+	 * Since blocks move to the free list without the coordination used in
+	 * xfs_bmap_finish, we can't allow block to be available for
+	 * reallocation and non-transaction writing (user data) until we know
+	 * that the transaction that moved it to the free list is permanently
+	 * on disk. We track the blocks by declaring these blocks as "busy";
+	 * the busy list is maintained on a per-ag basis and each transaction
+	 * records which entries should be removed when the iclog commits to
+	 * disk. If a busy block is allocated, the iclog is pushed up to the
+	 * LSN that freed the block.
+	 */
+	xfs_alloc_mark_busy(cur->bc_tp, be32_to_cpu(agf->agf_seqno), bno, 1);
+	xfs_trans_agbtree_delta(cur->bc_tp, -1);
+	return 0;
+}
+
 /*
  * Update the longest extent in the AGF
  */
@@ -1153,6 +1186,7 @@ static const struct xfs_btree_ops xfs_al
 	.dup_cursor		= xfs_allocbt_dup_cursor,
 	.set_root		= xfs_allocbt_set_root,
 	.alloc_block		= xfs_allocbt_alloc_block,
+	.free_block		= xfs_allocbt_free_block,
 	.update_lastrec		= xfs_allocbt_update_lastrec,
 	.get_maxrecs		= xfs_allocbt_get_maxrecs,
 	.init_key_from_rec	= xfs_allocbt_init_key_from_rec,
fs/xfs/xfs_bmap_btree.c +21 -94

151 lines not on this page · raw message

fs/xfs/xfs_btree.h +3 -0
Index: linux-2.6-xfs/fs/xfs/xfs_btree.h
===================================================================
@@ -198,6 +198,8 @@ struct xfs_btree_ops {
 			       union xfs_btree_ptr *start_bno,
 			       union xfs_btree_ptr *new_bno,
 			       int length, int *stat);
+	int	(*free_block)(struct xfs_btree_cur *cur, struct xfs_buf *bp,
+			      int length);
 
 	/* updated last record information */
 	void	(*update_lastrec)(struct xfs_btree_cur *cur,
@@ -596,6 +598,7 @@ int xfs_btree_split(struct xfs_btree_cur
 		union xfs_btree_key *, struct xfs_btree_cur **, int *);
 int xfs_btree_new_root(struct xfs_btree_cur *, int *);
 int xfs_btree_iroot_to_root(struct xfs_btree_cur *, int *, int *);
+int xfs_btree_root_to_iroot(struct xfs_btree_cur *);
 int xfs_btree_insert(struct xfs_btree_cur *, int *);
 
 
fs/xfs/xfs_ialloc_btree.c +17 -0
Index: linux-2.6-xfs/fs/xfs/xfs_ialloc_btree.c
===================================================================
@@ -710,6 +710,22 @@ xfs_inobt_alloc_block(
 	return 0;
 }
 
+STATIC int
+xfs_inobt_free_block(
+	struct xfs_btree_cur	*cur,
+	struct xfs_buf		*bp,
+	int			size)
+{
+	int			error;
+
+	error = xfs_free_extent(cur->bc_tp,
+			XFS_DADDR_TO_FSB(cur->bc_mp, XFS_BUF_ADDR(bp)), 1);
+	if (error)
+		return error;
+
+	xfs_trans_binval(cur->bc_tp, bp);
+	return 0;
+}
 
 STATIC int
 xfs_inobt_get_maxrecs(
@@ -1001,6 +1017,7 @@ static const struct xfs_btree_ops xfs_in
 	.dup_cursor		= xfs_inobt_dup_cursor,
 	.set_root		= xfs_inobt_set_root,
 	.alloc_block		= xfs_inobt_alloc_block,
+	.free_block		= xfs_inobt_free_block,
 	.get_maxrecs		= xfs_inobt_get_maxrecs,
 	.init_key_from_rec	= xfs_inobt_init_key_from_rec,
 	.init_ptr_from_cur	= xfs_inobt_init_ptr_from_cur,
#1 lore raw
Quoted 7 lines · Christoph Hellwig

xfs_bmbt_killroot is a mostly generic implementation of moving from a real block based root to an inode based root. So move it to xfs_btree.c where it can use all the nice infrastructure there and make it pointer size agnostic

The new name for it is xfs_btree_root_to_iroot which is not very nice but at least slightly more descriptive than the old name.

.....

fs/xfs/xfs_btree.c error0: · line 2866

Quoted 4 lines
fs/xfs/xfs_btree.c +4 -0
+
+	XFS_BTREE_TRACE_CURSOR(cur, XBT_ENTRY);
+	level = cur->bc_nlevels - 1;
+	ASSERT(level >= 1);

probably should assert root in inode is set here.

fs/xfs/xfs_btree.c error0: · line 2892

Quoted 5 lines
fs/xfs/xfs_btree.c +5 -0
+	cblock = xfs_btree_get_block(cur, level - 1, &cbp);
+	numrecs = xfs_btree_get_numrecs(cblock);
+
+	if (numrecs > cur->bc_ops->get_dmaxrecs(cur, level))
+	    	goto out0;
        ^^^^

Stray whitespace.

fs/xfs/xfs_btree.c error0: · line 2903

Quoted 11 lines
fs/xfs/xfs_btree.c +11 -0
+
+	XFS_BTREE_STATS_INC(cur, killroot);
+
+#ifdef DEBUG
+	xfs_btree_get_sibling(cur, block, &ptr, XFS_BB_LEFTSIB);
+	ASSERT(xfs_btree_ptr_is_null(cur, &ptr));
+	xfs_btree_get_sibling(cur, block, &ptr, XFS_BB_RIGHTSIB);
+	ASSERT(xfs_btree_ptr_is_null(cur, &ptr));
+
+	// XXX(hch): this assert is bmap btree specific
+ 	ASSERT(cur->bc_ops->get_maxrecs(cur, level) ==

^

fs/xfs/xfs_btree.c error0: · line 2904

Quoted 1 line
fs/xfs/xfs_btree.c +1 -0
+	       XFS_BMAP_BROOT_MAXRECS(ifp->if_broot_bytes));

Stray whitespace. As to the assert - what is it really trying to check? That the btree root space in the inode is large enough to fit the max number of records? If so, does it really need to be checked here (i.e. could the caller do it?)

Otherwise looks ok.

Cheers,

Dave.

Signature · 2 lines
Dave Chinner
[email protected]
#2 lore raw
Quoted 8 lines · Dave Chinner
Quoted 7 lines · Christoph Hellwig

xfs_bmbt_killroot is a mostly generic implementation of moving from a real block based root to an inode based root. So move it to xfs_btree.c where it can use all the nice infrastructure there and make it pointer size agnostic

The new name for it is xfs_btree_root_to_iroot which is not very nice but at least slightly more descriptive than the old name.

Quoted 7 lines

.....

fs/xfs/xfs_btree.c error0: · line 2866

Quoted 4 lines
fs/xfs/xfs_btree.c +4 -0
+
+	XFS_BTREE_TRACE_CURSOR(cur, XBT_ENTRY);
+	level = cur->bc_nlevels - 1;
+	ASSERT(level >= 1);

probably should assert root in inode is set here.

We have that check just before the call, so this might be a little overkill..

Quoted 8 lines

fs/xfs/xfs_btree.c error0: · line 2892

Quoted 6 lines
fs/xfs/xfs_btree.c +5 -0
+	cblock = xfs_btree_get_block(cur, level - 1, &cbp);
+	numrecs = xfs_btree_get_numrecs(cblock);
+
+	if (numrecs > cur->bc_ops->get_dmaxrecs(cur, level))
+	    	goto out0;
        ^^^^

Stray whitespace.

Already fixed before your mail after I ran checkpatch.pl over all patches - there were a few spread over the various patches.

Quoted 9 lines

fs/xfs/xfs_btree.c error0: · line 2903

Quoted 2 lines
fs/xfs/xfs_btree.c +2 -0
+	// XXX(hch): this assert is bmap btree specific
+ 	ASSERT(cur->bc_ops->get_maxrecs(cur, level) ==

^

fs/xfs/xfs_btree.c error0: · line 2904

Quoted 1 line
fs/xfs/xfs_btree.c +1 -0
+	       XFS_BMAP_BROOT_MAXRECS(ifp->if_broot_bytes));

Stray whitespace. As to the assert - what is it really trying to check? That the btree root space in the inode is large enough to fit the max number of records? If so, does it really need to be checked here (i.e. could the caller do it?)

I don't think it makes much sense at all. My preference would be to simply kill it.

#3 lore raw
Quoted 17 lines · Christoph Hellwig
Quoted 8 lines · Dave Chinner
Quoted 7 lines · Christoph Hellwig

xfs_bmbt_killroot is a mostly generic implementation of moving from a real block based root to an inode based root. So move it to xfs_btree.c where it can use all the nice infrastructure there and make it pointer size agnostic

The new name for it is xfs_btree_root_to_iroot which is not very nice but at least slightly more descriptive than the old name.

Quoted 7 lines

.....

fs/xfs/xfs_btree.c error0: · line 2866

Quoted 4 lines
fs/xfs/xfs_btree.c +4 -0
+
+	XFS_BTREE_TRACE_CURSOR(cur, XBT_ENTRY);
+	level = cur->bc_nlevels - 1;
+	ASSERT(level >= 1);

probably should assert root in inode is set here.

Quoted 2 lines

We have that check just before the call, so this might be a little overkill..

Fair enough.

Quoted 10 lines
Quoted 9 lines

fs/xfs/xfs_btree.c error0: · line 2903

Quoted 2 lines
fs/xfs/xfs_btree.c +2 -0
+	// XXX(hch): this assert is bmap btree specific
+ 	ASSERT(cur->bc_ops->get_maxrecs(cur, level) ==

^

fs/xfs/xfs_btree.c error0: · line 2904

Quoted 1 line
fs/xfs/xfs_btree.c +1 -0
+	       XFS_BMAP_BROOT_MAXRECS(ifp->if_broot_bytes));

Stray whitespace. As to the assert - what is it really trying to check? That the btree root space in the inode is large enough to fit the max number of records? If so, does it really need to be checked here (i.e. could the caller do it?)

Quoted 2 lines

I don't think it makes much sense at all. My preference would be to simply kill it.

Agreed. Kill it - I don't think it adds any value.

Cheers,

Dave.

Signature · 2 lines
Dave Chinner
[email protected]