[PATCH 0/3] xfs: reduce boilerplate around xfs_btree_init_block

[PATCH 0/3] xfs: reduce boilerplate around xfs_btree_init_block

[Eric Sandeen]

We have this pattern all over the kernel & userspace:

                if (xfs_sb_version_hascrc(&mp->m_sb))
                        xfs_btree_init_block(mp, bp, XFS_ABTB_CRC_MAGIC, 0, 1,
                                                agno, XFS_BTREE_CRC_BLOCKS);
                else
                        xfs_btree_init_block(mp, bp, XFS_ABTB_MAGIC, 0, 1,
                                                agno, 0);

The last flag (XFS_BTREE_CRC_BLOCKS) can be gleaned from
the features on the superblock via mp just as the if/else does,
so no need to pass that in from the caller.  That's patch 1.

Then the difference is simply CRC vs not-CRC magic.  This can also
be determined inside the called function by passing in a btree number,
and looking up the proper magic in the xfs_magics array.

patch2 makes xfs_btree_magic() more generic, taking a 
crc flag & btnum, instead of just a cursor.

patch3 then makes use of xfs_btree_magic in the block init routine
to determine the proper magic.

With those changes, the if/else can go away, and we can simply call:

xfs_btree_init_block(mp, bp, XFS_BTNUM_BNO, 0, 1, agno, 0);

and let the lower function sort out crc vs. no crc differences
by itself.

It's not a huge reduction in code, but it reduces a lot of the
boilerplate, particularly in growfs code.  This may also make it
slightly easier to factor the growfs tree init code into a loop
(not sure about that, yet) and/or to move mkfs & growfs tree init
code into a libxfs/ call.

 libxfs/xfs_bmap.c       |   19 ++++---------------
 libxfs/xfs_bmap_btree.c |   10 ++--------
 libxfs/xfs_btree.c      |   34 +++++++++++++++++++++-------------
 libxfs/xfs_btree.h      |    4 ++--
 libxfs/xfs_types.h      |    2 ++
 xfs_fsops.c             |   39 +++++++++------------------------------
 6 files changed, 40 insertions(+), 68 deletions(-)

-Eric

[PATCH 1/3] xfs: glean crc status from mp not flags in xfs_btree_init_block_int

[Eric Sandeen]

xfs_btree_init_block_int() can determine whether crcs are
in effect without the passed-in XFS_BTREE_CRC_BLOCKS flag;
the mp argument allows us to determine this from the
superblock.  Remove the flag from callers, and use
xfs_sb_version_hascrc(&mp->m_sb) internally instead.

This removes one difference between the if & else cases
in the callers.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index c27344c..e645aa8 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -727,7 +727,7 @@ static inline bool xfs_bmap_wants_extents(struct xfs_inode *ip, int whichfork)
 	if (xfs_sb_version_hascrc(&mp->m_sb))
 		xfs_btree_init_block_int(mp, block, XFS_BUF_DADDR_NULL,
 				 XFS_BMAP_CRC_MAGIC, 1, 1, ip->i_ino,
-				 XFS_BTREE_LONG_PTRS | XFS_BTREE_CRC_BLOCKS);
+				 XFS_BTREE_LONG_PTRS);
 	else
 		xfs_btree_init_block_int(mp, block, XFS_BUF_DADDR_NULL,
 				 XFS_BMAP_MAGIC, 1, 1, ip->i_ino,
@@ -804,7 +804,7 @@ static inline bool xfs_bmap_wants_extents(struct xfs_inode *ip, int whichfork)
 	if (xfs_sb_version_hascrc(&mp->m_sb))
 		xfs_btree_init_block_int(mp, ablock, abp->b_bn,
 				XFS_BMAP_CRC_MAGIC, 0, 0, ip->i_ino,
-				XFS_BTREE_LONG_PTRS | XFS_BTREE_CRC_BLOCKS);
+				XFS_BTREE_LONG_PTRS);
 	else
 		xfs_btree_init_block_int(mp, ablock, abp->b_bn,
 				XFS_BMAP_MAGIC, 0, 0, ip->i_ino,
diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
index 8007d2b..f0293d4 100644
--- a/fs/xfs/libxfs/xfs_bmap_btree.c
+++ b/fs/xfs/libxfs/xfs_bmap_btree.c
@@ -74,7 +74,7 @@
 	if (xfs_sb_version_hascrc(&mp->m_sb))
 		xfs_btree_init_block_int(mp, rblock, XFS_BUF_DADDR_NULL,
 				 XFS_BMAP_CRC_MAGIC, 0, 0, ip->i_ino,
-				 XFS_BTREE_LONG_PTRS | XFS_BTREE_CRC_BLOCKS);
+				 XFS_BTREE_LONG_PTRS);
 	else
 		xfs_btree_init_block_int(mp, rblock, XFS_BUF_DADDR_NULL,
 				 XFS_BMAP_MAGIC, 0, 0, ip->i_ino,
diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index 5c8e6f2..f49fc2f 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -1090,6 +1090,8 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
 	__u64			owner,
 	unsigned int		flags)
 {
+	int			crc = xfs_sb_version_hascrc(&mp->m_sb);
+
 	buf->bb_magic = cpu_to_be32(magic);
 	buf->bb_level = cpu_to_be16(level);
 	buf->bb_numrecs = cpu_to_be16(numrecs);
@@ -1097,7 +1099,7 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
 	if (flags & XFS_BTREE_LONG_PTRS) {
 		buf->bb_u.l.bb_leftsib = cpu_to_be64(NULLFSBLOCK);
 		buf->bb_u.l.bb_rightsib = cpu_to_be64(NULLFSBLOCK);
-		if (flags & XFS_BTREE_CRC_BLOCKS) {
+		if (crc) {
 			buf->bb_u.l.bb_blkno = cpu_to_be64(blkno);
 			buf->bb_u.l.bb_owner = cpu_to_be64(owner);
 			uuid_copy(&buf->bb_u.l.bb_uuid, &mp->m_sb.sb_meta_uuid);
@@ -1110,7 +1112,7 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
 
 		buf->bb_u.s.bb_leftsib = cpu_to_be32(NULLAGBLOCK);
 		buf->bb_u.s.bb_rightsib = cpu_to_be32(NULLAGBLOCK);
-		if (flags & XFS_BTREE_CRC_BLOCKS) {
+		if (crc) {
 			buf->bb_u.s.bb_blkno = cpu_to_be64(blkno);
 			buf->bb_u.s.bb_owner = cpu_to_be32(__owner);
 			uuid_copy(&buf->bb_u.s.bb_uuid, &mp->m_sb.sb_meta_uuid);
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 93d12fa..bd20051 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -354,7 +354,7 @@
 
 		if (xfs_sb_version_hascrc(&mp->m_sb))
 			xfs_btree_init_block(mp, bp, XFS_ABTB_CRC_MAGIC, 0, 1,
-						agno, XFS_BTREE_CRC_BLOCKS);
+						agno, 0);
 		else
 			xfs_btree_init_block(mp, bp, XFS_ABTB_MAGIC, 0, 1,
 						agno, 0);
@@ -383,7 +383,7 @@
 
 		if (xfs_sb_version_hascrc(&mp->m_sb))
 			xfs_btree_init_block(mp, bp, XFS_ABTC_CRC_MAGIC, 0, 1,
-						agno, XFS_BTREE_CRC_BLOCKS);
+						agno, 0);
 		else
 			xfs_btree_init_block(mp, bp, XFS_ABTC_MAGIC, 0, 1,
 						agno, 0);
@@ -414,7 +414,7 @@
 			}
 
 			xfs_btree_init_block(mp, bp, XFS_RMAP_CRC_MAGIC, 0, 0,
-						agno, XFS_BTREE_CRC_BLOCKS);
+						agno, 0);
 			block = XFS_BUF_TO_BLOCK(bp);
 
 
@@ -490,7 +490,7 @@
 
 		if (xfs_sb_version_hascrc(&mp->m_sb))
 			xfs_btree_init_block(mp, bp, XFS_IBT_CRC_MAGIC, 0, 0,
-						agno, XFS_BTREE_CRC_BLOCKS);
+						agno, 0);
 		else
 			xfs_btree_init_block(mp, bp, XFS_IBT_MAGIC, 0, 0,
 						agno, 0);
@@ -515,8 +515,7 @@
 
 			if (xfs_sb_version_hascrc(&mp->m_sb))
 				xfs_btree_init_block(mp, bp, XFS_FIBT_CRC_MAGIC,
-						     0, 0, agno,
-						     XFS_BTREE_CRC_BLOCKS);
+						     0, 0, agno, 0);
 			else
 				xfs_btree_init_block(mp, bp, XFS_FIBT_MAGIC, 0,
 						     0, agno, 0);
@@ -541,8 +540,7 @@
 			}
 
 			xfs_btree_init_block(mp, bp, XFS_REFC_CRC_MAGIC,
-					     0, 0, agno,
-					     XFS_BTREE_CRC_BLOCKS);
+					     0, 0, agno, 0);
 
 			error = xfs_bwrite(bp);
 			xfs_buf_relse(bp);

[PATCH 2/3] xfs: make xfs_btree_magic more generic

[Eric Sandeen]

Right now the xfs_btree_magic() define takes only a cursor;
change this to take crc and btnum args to make it more generically
useful, and move to a header file.

This will allow xfs_btree_init_block_int callers which don't
have a cursor to make use of the xfs_magics array, which will
happen in the next patch.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index f49fc2f..cdb952a 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -50,8 +50,6 @@
 	  XFS_BMAP_CRC_MAGIC, XFS_IBT_CRC_MAGIC, XFS_FIBT_CRC_MAGIC,
 	  XFS_REFC_CRC_MAGIC }
 };
-#define xfs_btree_magic(cur) \
-	xfs_magics[!!((cur)->bc_flags & XFS_BTREE_CRC_BLOCKS)][cur->bc_btnum]
 
 STATIC int				/* error (0 or EFSCORRUPTED) */
 xfs_btree_check_lblock(
@@ -62,10 +60,13 @@
 {
 	int			lblock_ok = 1; /* block passes checks */
 	struct xfs_mount	*mp;	/* file system mount point */
+	xfs_btnum_t		btnum = cur->bc_btnum;
+	int			crc;
 
 	mp = cur->bc_mp;
+	crc = xfs_sb_version_hascrc(&mp->m_sb);
 
-	if (xfs_sb_version_hascrc(&mp->m_sb)) {
+	if (crc) {
 		lblock_ok = lblock_ok &&
 			uuid_equal(&block->bb_u.l.bb_uuid,
 				   &mp->m_sb.sb_meta_uuid) &&
@@ -74,7 +75,7 @@
 	}
 
 	lblock_ok = lblock_ok &&
-		be32_to_cpu(block->bb_magic) == xfs_btree_magic(cur) &&
+		be32_to_cpu(block->bb_magic) == xfs_btree_magic(crc, btnum) &&
 		be16_to_cpu(block->bb_level) == level &&
 		be16_to_cpu(block->bb_numrecs) <=
 			cur->bc_ops->get_maxrecs(cur, level) &&
@@ -110,13 +111,16 @@
 	struct xfs_agf		*agf;	/* ag. freespace structure */
 	xfs_agblock_t		agflen;	/* native ag. freespace length */
 	int			sblock_ok = 1; /* block passes checks */
+	xfs_btnum_t		btnum = cur->bc_btnum;
+	int			crc;
 
 	mp = cur->bc_mp;
+	crc = xfs_sb_version_hascrc(&mp->m_sb);
 	agbp = cur->bc_private.a.agbp;
 	agf = XFS_BUF_TO_AGF(agbp);
 	agflen = be32_to_cpu(agf->agf_length);
 
-	if (xfs_sb_version_hascrc(&mp->m_sb)) {
+	if (crc) {
 		sblock_ok = sblock_ok &&
 			uuid_equal(&block->bb_u.s.bb_uuid,
 				   &mp->m_sb.sb_meta_uuid) &&
@@ -125,7 +129,7 @@
 	}
 
 	sblock_ok = sblock_ok &&
-		be32_to_cpu(block->bb_magic) == xfs_btree_magic(cur) &&
+		be32_to_cpu(block->bb_magic) == xfs_btree_magic(crc, btnum) &&
 		be16_to_cpu(block->bb_level) == level &&
 		be16_to_cpu(block->bb_numrecs) <=
 			cur->bc_ops->get_maxrecs(cur, level) &&
@@ -1142,7 +1146,9 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
 	int			level,
 	int			numrecs)
 {
-	__u64 owner;
+	__u64			owner;
+	int			crc = xfs_sb_version_hascrc(&cur->bc_mp->m_sb);
+	xfs_btnum_t		btnum = cur->bc_btnum;
 
 	/*
 	 * we can pull the owner from the cursor right now as the different
@@ -1156,7 +1162,7 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
 		owner = cur->bc_private.a.agno;
 
 	xfs_btree_init_block_int(cur->bc_mp, XFS_BUF_TO_BLOCK(bp), bp->b_bn,
-				 xfs_btree_magic(cur), level, numrecs,
+				 xfs_btree_magic(crc, btnum), level, numrecs,
 				 owner, cur->bc_flags);
 }
 
diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
index 8d74870..8ddacf4 100644
--- a/fs/xfs/libxfs/xfs_types.h
+++ b/fs/xfs/libxfs/xfs_types.h
@@ -113,6 +113,8 @@
 	XFS_BTNUM_INOi, XFS_BTNUM_FINOi, XFS_BTNUM_REFCi, XFS_BTNUM_MAX
 } xfs_btnum_t;
 
+#define xfs_btree_magic(crc, btnum)	xfs_magics[crc][btnum]
+
 struct xfs_name {
 	const unsigned char	*name;
 	int			len;

[PATCH 3/3] xfs: remove boilerplate around xfs_btree_init_block

[Eric Sandeen]

Now that xfs_btree_init_block_int is able to determine crc
status from the passed-in mp, we can determine the proper
magic as well if we are given a btree number, rather than
an explicit magic value.

Change xfs_btree_init_block[_int] callers to pass in the
btree number, and let xfs_btree_init_block_int use the
xfs_magics array via the xfs_btree_magic macro to determine
which magic value is needed.  This makes all of the
if (crc) / else stanzas identical, and the if/else can be
removed, leading to a single, common init_block call.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index e645aa8..b30ff0a 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -724,15 +724,9 @@ static inline bool xfs_bmap_wants_extents(struct xfs_inode *ip, int whichfork)
 	 * Fill in the root.
 	 */
 	block = ifp->if_broot;
-	if (xfs_sb_version_hascrc(&mp->m_sb))
-		xfs_btree_init_block_int(mp, block, XFS_BUF_DADDR_NULL,
-				 XFS_BMAP_CRC_MAGIC, 1, 1, ip->i_ino,
+	xfs_btree_init_block_int(mp, block, XFS_BUF_DADDR_NULL,
+				 XFS_BTNUM_BMAP, 1, 1, ip->i_ino,
 				 XFS_BTREE_LONG_PTRS);
-	else
-		xfs_btree_init_block_int(mp, block, XFS_BUF_DADDR_NULL,
-				 XFS_BMAP_MAGIC, 1, 1, ip->i_ino,
-				 XFS_BTREE_LONG_PTRS);
-
 	/*
 	 * Need a cursor.  Can't allocate until bb_level is filled in.
 	 */
@@ -801,13 +795,8 @@ static inline bool xfs_bmap_wants_extents(struct xfs_inode *ip, int whichfork)
 	 */
 	abp->b_ops = &xfs_bmbt_buf_ops;
 	ablock = XFS_BUF_TO_BLOCK(abp);
-	if (xfs_sb_version_hascrc(&mp->m_sb))
-		xfs_btree_init_block_int(mp, ablock, abp->b_bn,
-				XFS_BMAP_CRC_MAGIC, 0, 0, ip->i_ino,
-				XFS_BTREE_LONG_PTRS);
-	else
-		xfs_btree_init_block_int(mp, ablock, abp->b_bn,
-				XFS_BMAP_MAGIC, 0, 0, ip->i_ino,
+	xfs_btree_init_block_int(mp, ablock, abp->b_bn,
+				XFS_BTNUM_BMAP, 0, 0, ip->i_ino,
 				XFS_BTREE_LONG_PTRS);
 
 	arp = XFS_BMBT_REC_ADDR(mp, ablock, 1);
diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
index f0293d4..a8867a7 100644
--- a/fs/xfs/libxfs/xfs_bmap_btree.c
+++ b/fs/xfs/libxfs/xfs_bmap_btree.c
@@ -71,15 +71,9 @@
 	xfs_bmbt_key_t		*tkp;
 	__be64			*tpp;
 
-	if (xfs_sb_version_hascrc(&mp->m_sb))
-		xfs_btree_init_block_int(mp, rblock, XFS_BUF_DADDR_NULL,
-				 XFS_BMAP_CRC_MAGIC, 0, 0, ip->i_ino,
-				 XFS_BTREE_LONG_PTRS);
-	else
-		xfs_btree_init_block_int(mp, rblock, XFS_BUF_DADDR_NULL,
-				 XFS_BMAP_MAGIC, 0, 0, ip->i_ino,
+	xfs_btree_init_block_int(mp, rblock, XFS_BUF_DADDR_NULL,
+				 XFS_BTNUM_BMAP, 0, 0, ip->i_ino,
 				 XFS_BTREE_LONG_PTRS);
-
 	rblock->bb_level = dblock->bb_level;
 	ASSERT(be16_to_cpu(rblock->bb_level) > 0);
 	rblock->bb_numrecs = dblock->bb_numrecs;
diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index cdb952a..8487005 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -1088,14 +1088,16 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
 	struct xfs_mount	*mp,
 	struct xfs_btree_block	*buf,
 	xfs_daddr_t		blkno,
-	__u32			magic,
+	xfs_btnum_t		btnum,
 	__u16			level,
 	__u16			numrecs,
 	__u64			owner,
 	unsigned int		flags)
 {
 	int			crc = xfs_sb_version_hascrc(&mp->m_sb);
+	__u32			magic = xfs_btree_magic(crc, btnum);
 
+	ASSERT(magic != 0);
 	buf->bb_magic = cpu_to_be32(magic);
 	buf->bb_level = cpu_to_be16(level);
 	buf->bb_numrecs = cpu_to_be16(numrecs);
@@ -1129,14 +1131,14 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
 xfs_btree_init_block(
 	struct xfs_mount *mp,
 	struct xfs_buf	*bp,
-	__u32		magic,
+	xfs_btnum_t	btnum,
 	__u16		level,
 	__u16		numrecs,
 	__u64		owner,
 	unsigned int	flags)
 {
 	xfs_btree_init_block_int(mp, XFS_BUF_TO_BLOCK(bp), bp->b_bn,
-				 magic, level, numrecs, owner, flags);
+				 btnum, level, numrecs, owner, flags);
 }
 
 STATIC void
@@ -1147,8 +1149,6 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
 	int			numrecs)
 {
 	__u64			owner;
-	int			crc = xfs_sb_version_hascrc(&cur->bc_mp->m_sb);
-	xfs_btnum_t		btnum = cur->bc_btnum;
 
 	/*
 	 * we can pull the owner from the cursor right now as the different
@@ -1162,7 +1162,7 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
 		owner = cur->bc_private.a.agno;
 
 	xfs_btree_init_block_int(cur->bc_mp, XFS_BUF_TO_BLOCK(bp), bp->b_bn,
-				 xfs_btree_magic(crc, btnum), level, numrecs,
+				 cur->bc_btnum, level, numrecs,
 				 owner, cur->bc_flags);
 }
 
diff --git a/fs/xfs/libxfs/xfs_btree.h b/fs/xfs/libxfs/xfs_btree.h
index c2b01d1..63de8dc 100644
--- a/fs/xfs/libxfs/xfs_btree.h
+++ b/fs/xfs/libxfs/xfs_btree.h
@@ -413,7 +413,7 @@ struct xfs_buf *				/* buffer for agno/agbno */
 xfs_btree_init_block(
 	struct xfs_mount *mp,
 	struct xfs_buf	*bp,
-	__u32		magic,
+	xfs_btnum_t	btnum,
 	__u16		level,
 	__u16		numrecs,
 	__u64		owner,
@@ -424,7 +424,7 @@ struct xfs_buf *				/* buffer for agno/agbno */
 	struct xfs_mount	*mp,
 	struct xfs_btree_block	*buf,
 	xfs_daddr_t		blkno,
-	__u32			magic,
+	xfs_btnum_t		btnum,
 	__u16			level,
 	__u16			numrecs,
 	__u64			owner,
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index bd20051..e702928 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -352,12 +352,7 @@
 			goto error0;
 		}
 
-		if (xfs_sb_version_hascrc(&mp->m_sb))
-			xfs_btree_init_block(mp, bp, XFS_ABTB_CRC_MAGIC, 0, 1,
-						agno, 0);
-		else
-			xfs_btree_init_block(mp, bp, XFS_ABTB_MAGIC, 0, 1,
-						agno, 0);
+		xfs_btree_init_block(mp, bp, XFS_BTNUM_BNO, 0, 1, agno, 0);
 
 		arec = XFS_ALLOC_REC_ADDR(mp, XFS_BUF_TO_BLOCK(bp), 1);
 		arec->ar_startblock = cpu_to_be32(mp->m_ag_prealloc_blocks);
@@ -381,12 +376,7 @@
 			goto error0;
 		}
 
-		if (xfs_sb_version_hascrc(&mp->m_sb))
-			xfs_btree_init_block(mp, bp, XFS_ABTC_CRC_MAGIC, 0, 1,
-						agno, 0);
-		else
-			xfs_btree_init_block(mp, bp, XFS_ABTC_MAGIC, 0, 1,
-						agno, 0);
+		xfs_btree_init_block(mp, bp, XFS_BTNUM_CNT, 0, 1, agno, 0);
 
 		arec = XFS_ALLOC_REC_ADDR(mp, XFS_BUF_TO_BLOCK(bp), 1);
 		arec->ar_startblock = cpu_to_be32(mp->m_ag_prealloc_blocks);
@@ -413,7 +403,7 @@
 				goto error0;
 			}
 
-			xfs_btree_init_block(mp, bp, XFS_RMAP_CRC_MAGIC, 0, 0,
+			xfs_btree_init_block(mp, bp, XFS_BTNUM_RMAP, 0, 0,
 						agno, 0);
 			block = XFS_BUF_TO_BLOCK(bp);
 
@@ -488,12 +478,7 @@
 			goto error0;
 		}
 
-		if (xfs_sb_version_hascrc(&mp->m_sb))
-			xfs_btree_init_block(mp, bp, XFS_IBT_CRC_MAGIC, 0, 0,
-						agno, 0);
-		else
-			xfs_btree_init_block(mp, bp, XFS_IBT_MAGIC, 0, 0,
-						agno, 0);
+		xfs_btree_init_block(mp, bp, XFS_BTNUM_INO , 0, 0, agno, 0);
 
 		error = xfs_bwrite(bp);
 		xfs_buf_relse(bp);
@@ -513,12 +498,8 @@
 				goto error0;
 			}
 
-			if (xfs_sb_version_hascrc(&mp->m_sb))
-				xfs_btree_init_block(mp, bp, XFS_FIBT_CRC_MAGIC,
+			xfs_btree_init_block(mp, bp, XFS_BTNUM_FINO,
 						     0, 0, agno, 0);
-			else
-				xfs_btree_init_block(mp, bp, XFS_FIBT_MAGIC, 0,
-						     0, agno, 0);
 
 			error = xfs_bwrite(bp);
 			xfs_buf_relse(bp);
@@ -539,7 +520,7 @@
 				goto error0;
 			}
 
-			xfs_btree_init_block(mp, bp, XFS_REFC_CRC_MAGIC,
+			xfs_btree_init_block(mp, bp, XFS_BTNUM_REFC,
 					     0, 0, agno, 0);
 
 			error = xfs_bwrite(bp);

Re: [PATCH 2/3] xfs: make xfs_btree_magic more generic

[Darrick J. Wong]

On Thu, Dec 22, 2016 at 11:47:26AM -0600, Eric Sandeen wrote:
> Right now the xfs_btree_magic() define takes only a cursor;
> change this to take crc and btnum args to make it more generically
> useful, and move to a header file.
> 
> This will allow xfs_btree_init_block_int callers which don't
> have a cursor to make use of the xfs_magics array, which will
> happen in the next patch.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> index f49fc2f..cdb952a 100644
> --- a/fs/xfs/libxfs/xfs_btree.c
> +++ b/fs/xfs/libxfs/xfs_btree.c
> @@ -50,8 +50,6 @@
>  	  XFS_BMAP_CRC_MAGIC, XFS_IBT_CRC_MAGIC, XFS_FIBT_CRC_MAGIC,
>  	  XFS_REFC_CRC_MAGIC }
>  };
> -#define xfs_btree_magic(cur) \
> -	xfs_magics[!!((cur)->bc_flags & XFS_BTREE_CRC_BLOCKS)][cur->bc_btnum]
>  
>  STATIC int				/* error (0 or EFSCORRUPTED) */
>  xfs_btree_check_lblock(
> @@ -62,10 +60,13 @@
>  {
>  	int			lblock_ok = 1; /* block passes checks */
>  	struct xfs_mount	*mp;	/* file system mount point */
> +	xfs_btnum_t		btnum = cur->bc_btnum;
> +	int			crc;
>  
>  	mp = cur->bc_mp;
> +	crc = xfs_sb_version_hascrc(&mp->m_sb);
>  
> -	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> +	if (crc) {
>  		lblock_ok = lblock_ok &&
>  			uuid_equal(&block->bb_u.l.bb_uuid,
>  				   &mp->m_sb.sb_meta_uuid) &&
> @@ -74,7 +75,7 @@
>  	}
>  
>  	lblock_ok = lblock_ok &&
> -		be32_to_cpu(block->bb_magic) == xfs_btree_magic(cur) &&
> +		be32_to_cpu(block->bb_magic) == xfs_btree_magic(crc, btnum) &&
>  		be16_to_cpu(block->bb_level) == level &&
>  		be16_to_cpu(block->bb_numrecs) <=
>  			cur->bc_ops->get_maxrecs(cur, level) &&
> @@ -110,13 +111,16 @@
>  	struct xfs_agf		*agf;	/* ag. freespace structure */
>  	xfs_agblock_t		agflen;	/* native ag. freespace length */
>  	int			sblock_ok = 1; /* block passes checks */
> +	xfs_btnum_t		btnum = cur->bc_btnum;
> +	int			crc;
>  
>  	mp = cur->bc_mp;
> +	crc = xfs_sb_version_hascrc(&mp->m_sb);
>  	agbp = cur->bc_private.a.agbp;
>  	agf = XFS_BUF_TO_AGF(agbp);
>  	agflen = be32_to_cpu(agf->agf_length);
>  
> -	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> +	if (crc) {
>  		sblock_ok = sblock_ok &&
>  			uuid_equal(&block->bb_u.s.bb_uuid,
>  				   &mp->m_sb.sb_meta_uuid) &&
> @@ -125,7 +129,7 @@
>  	}
>  
>  	sblock_ok = sblock_ok &&
> -		be32_to_cpu(block->bb_magic) == xfs_btree_magic(cur) &&
> +		be32_to_cpu(block->bb_magic) == xfs_btree_magic(crc, btnum) &&
>  		be16_to_cpu(block->bb_level) == level &&
>  		be16_to_cpu(block->bb_numrecs) <=
>  			cur->bc_ops->get_maxrecs(cur, level) &&
> @@ -1142,7 +1146,9 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
>  	int			level,
>  	int			numrecs)
>  {
> -	__u64 owner;
> +	__u64			owner;
> +	int			crc = xfs_sb_version_hascrc(&cur->bc_mp->m_sb);
> +	xfs_btnum_t		btnum = cur->bc_btnum;
>  
>  	/*
>  	 * we can pull the owner from the cursor right now as the different
> @@ -1156,7 +1162,7 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
>  		owner = cur->bc_private.a.agno;
>  
>  	xfs_btree_init_block_int(cur->bc_mp, XFS_BUF_TO_BLOCK(bp), bp->b_bn,
> -				 xfs_btree_magic(cur), level, numrecs,
> +				 xfs_btree_magic(crc, btnum), level, numrecs,
>  				 owner, cur->bc_flags);
>  }
>  
> diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
> index 8d74870..8ddacf4 100644
> --- a/fs/xfs/libxfs/xfs_types.h
> +++ b/fs/xfs/libxfs/xfs_types.h
> @@ -113,6 +113,8 @@
>  	XFS_BTNUM_INOi, XFS_BTNUM_FINOi, XFS_BTNUM_REFCi, XFS_BTNUM_MAX
>  } xfs_btnum_t;
>  
> +#define xfs_btree_magic(crc, btnum)	xfs_magics[crc][btnum]

TBH I was expecting this to be a static inline function instead of a
macro that looks like a function.  Maybe an assert to catch things like
crc=0, btnum=XFS_BTNUM_RMAP that aren't supposed to exist.

(I see that the third patch has one assert, but why not just leave it in
the helper to catch any error case?)

--D

> +
>  struct xfs_name {
>  	const unsigned char	*name;
>  	int			len;
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3 V2] xfs: make xfs_btree_magic more generic

[Eric Sandeen]

Right now the xfs_btree_magic() define takes only a cursor;
change this to take crc and btnum args to make it more generically
useful, and move to a function.

This will allow xfs_btree_init_block_int callers which don't
have a cursor to make use of the xfs_magics array, which will
happen in the next patch.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: turn xfs_btree_magic into a function with a built-in ASSERT

diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index f49fc2f..81866dd 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -50,8 +50,15 @@
 	  XFS_BMAP_CRC_MAGIC, XFS_IBT_CRC_MAGIC, XFS_FIBT_CRC_MAGIC,
 	  XFS_REFC_CRC_MAGIC }
 };
-#define xfs_btree_magic(cur) \
-	xfs_magics[!!((cur)->bc_flags & XFS_BTREE_CRC_BLOCKS)][cur->bc_btnum]
+
+__uint32_t xfs_btree_magic(int crc, xfs_btnum_t btnum)
+{
+	__uint32_t magic = xfs_magics[crc][btnum];
+
+	/* Ensure we asked for crc for crc-only magics. */
+	ASSERT(magic != 0);
+	return magic;
+}
 
 STATIC int				/* error (0 or EFSCORRUPTED) */
 xfs_btree_check_lblock(
@@ -62,10 +69,13 @@
 {
 	int			lblock_ok = 1; /* block passes checks */
 	struct xfs_mount	*mp;	/* file system mount point */
+	xfs_btnum_t		btnum = cur->bc_btnum;
+	int			crc;
 
 	mp = cur->bc_mp;
+	crc = xfs_sb_version_hascrc(&mp->m_sb);
 
-	if (xfs_sb_version_hascrc(&mp->m_sb)) {
+	if (crc) {
 		lblock_ok = lblock_ok &&
 			uuid_equal(&block->bb_u.l.bb_uuid,
 				   &mp->m_sb.sb_meta_uuid) &&
@@ -74,7 +84,7 @@
 	}
 
 	lblock_ok = lblock_ok &&
-		be32_to_cpu(block->bb_magic) == xfs_btree_magic(cur) &&
+		be32_to_cpu(block->bb_magic) == xfs_btree_magic(crc, btnum) &&
 		be16_to_cpu(block->bb_level) == level &&
 		be16_to_cpu(block->bb_numrecs) <=
 			cur->bc_ops->get_maxrecs(cur, level) &&
@@ -110,13 +120,16 @@
 	struct xfs_agf		*agf;	/* ag. freespace structure */
 	xfs_agblock_t		agflen;	/* native ag. freespace length */
 	int			sblock_ok = 1; /* block passes checks */
+	xfs_btnum_t		btnum = cur->bc_btnum;
+	int			crc;
 
 	mp = cur->bc_mp;
+	crc = xfs_sb_version_hascrc(&mp->m_sb);
 	agbp = cur->bc_private.a.agbp;
 	agf = XFS_BUF_TO_AGF(agbp);
 	agflen = be32_to_cpu(agf->agf_length);
 
-	if (xfs_sb_version_hascrc(&mp->m_sb)) {
+	if (crc) {
 		sblock_ok = sblock_ok &&
 			uuid_equal(&block->bb_u.s.bb_uuid,
 				   &mp->m_sb.sb_meta_uuid) &&
@@ -125,7 +138,7 @@
 	}
 
 	sblock_ok = sblock_ok &&
-		be32_to_cpu(block->bb_magic) == xfs_btree_magic(cur) &&
+		be32_to_cpu(block->bb_magic) == xfs_btree_magic(crc, btnum) &&
 		be16_to_cpu(block->bb_level) == level &&
 		be16_to_cpu(block->bb_numrecs) <=
 			cur->bc_ops->get_maxrecs(cur, level) &&
@@ -1142,7 +1155,9 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
 	int			level,
 	int			numrecs)
 {
-	__u64 owner;
+	__u64			owner;
+	int			crc = xfs_sb_version_hascrc(&cur->bc_mp->m_sb);
+	xfs_btnum_t		btnum = cur->bc_btnum;
 
 	/*
 	 * we can pull the owner from the cursor right now as the different
@@ -1156,7 +1171,7 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
 		owner = cur->bc_private.a.agno;
 
 	xfs_btree_init_block_int(cur->bc_mp, XFS_BUF_TO_BLOCK(bp), bp->b_bn,
-				 xfs_btree_magic(cur), level, numrecs,
+				 xfs_btree_magic(crc, btnum), level, numrecs,
 				 owner, cur->bc_flags);
 }
 
diff --git a/fs/xfs/libxfs/xfs_btree.h b/fs/xfs/libxfs/xfs_btree.h
index c2b01d1..3aaced3 100644
--- a/fs/xfs/libxfs/xfs_btree.h
+++ b/fs/xfs/libxfs/xfs_btree.h
@@ -76,6 +76,8 @@
 #define	XFS_BTNUM_RMAP	((xfs_btnum_t)XFS_BTNUM_RMAPi)
 #define	XFS_BTNUM_REFC	((xfs_btnum_t)XFS_BTNUM_REFCi)
 
+__uint32_t xfs_btree_magic(int crc, xfs_btnum_t btnum);
+
 /*
  * For logging record fields.
  */

[PATCH 3/3 V2] xfs: remove boilerplate around xfs_btree_init_block

[Eric Sandeen]

Now that xfs_btree_init_block_int is able to determine crc
status from the passed-in mp, we can determine the proper
magic as well if we are given a btree number, rather than
an explicit magic value.

Change xfs_btree_init_block[_int] callers to pass in the
btree number, and let xfs_btree_init_block_int use the
xfs_magics array via the xfs_btree_magic macro to determine
which magic value is needed.  This makes all of the
if (crc) / else stanzas identical, and the if/else can be
removed, leading to a single, common init_block call.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: remove ASSERT after xfs_btree_magic now that it's built
into that function

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index e645aa8..b30ff0a 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -724,15 +724,9 @@ static inline bool xfs_bmap_wants_extents(struct xfs_inode *ip, int whichfork)
 	 * Fill in the root.
 	 */
 	block = ifp->if_broot;
-	if (xfs_sb_version_hascrc(&mp->m_sb))
-		xfs_btree_init_block_int(mp, block, XFS_BUF_DADDR_NULL,
-				 XFS_BMAP_CRC_MAGIC, 1, 1, ip->i_ino,
+	xfs_btree_init_block_int(mp, block, XFS_BUF_DADDR_NULL,
+				 XFS_BTNUM_BMAP, 1, 1, ip->i_ino,
 				 XFS_BTREE_LONG_PTRS);
-	else
-		xfs_btree_init_block_int(mp, block, XFS_BUF_DADDR_NULL,
-				 XFS_BMAP_MAGIC, 1, 1, ip->i_ino,
-				 XFS_BTREE_LONG_PTRS);
-
 	/*
 	 * Need a cursor.  Can't allocate until bb_level is filled in.
 	 */
@@ -801,13 +795,8 @@ static inline bool xfs_bmap_wants_extents(struct xfs_inode *ip, int whichfork)
 	 */
 	abp->b_ops = &xfs_bmbt_buf_ops;
 	ablock = XFS_BUF_TO_BLOCK(abp);
-	if (xfs_sb_version_hascrc(&mp->m_sb))
-		xfs_btree_init_block_int(mp, ablock, abp->b_bn,
-				XFS_BMAP_CRC_MAGIC, 0, 0, ip->i_ino,
-				XFS_BTREE_LONG_PTRS);
-	else
-		xfs_btree_init_block_int(mp, ablock, abp->b_bn,
-				XFS_BMAP_MAGIC, 0, 0, ip->i_ino,
+	xfs_btree_init_block_int(mp, ablock, abp->b_bn,
+				XFS_BTNUM_BMAP, 0, 0, ip->i_ino,
 				XFS_BTREE_LONG_PTRS);
 
 	arp = XFS_BMBT_REC_ADDR(mp, ablock, 1);
diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
index f0293d4..a8867a7 100644
--- a/fs/xfs/libxfs/xfs_bmap_btree.c
+++ b/fs/xfs/libxfs/xfs_bmap_btree.c
@@ -71,15 +71,9 @@
 	xfs_bmbt_key_t		*tkp;
 	__be64			*tpp;
 
-	if (xfs_sb_version_hascrc(&mp->m_sb))
-		xfs_btree_init_block_int(mp, rblock, XFS_BUF_DADDR_NULL,
-				 XFS_BMAP_CRC_MAGIC, 0, 0, ip->i_ino,
-				 XFS_BTREE_LONG_PTRS);
-	else
-		xfs_btree_init_block_int(mp, rblock, XFS_BUF_DADDR_NULL,
-				 XFS_BMAP_MAGIC, 0, 0, ip->i_ino,
+	xfs_btree_init_block_int(mp, rblock, XFS_BUF_DADDR_NULL,
+				 XFS_BTNUM_BMAP, 0, 0, ip->i_ino,
 				 XFS_BTREE_LONG_PTRS);
-
 	rblock->bb_level = dblock->bb_level;
 	ASSERT(be16_to_cpu(rblock->bb_level) > 0);
 	rblock->bb_numrecs = dblock->bb_numrecs;
diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index 81866dd..0dcc5f2 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -1097,13 +1097,14 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
 	struct xfs_mount	*mp,
 	struct xfs_btree_block	*buf,
 	xfs_daddr_t		blkno,
-	__u32			magic,
+	xfs_btnum_t		btnum,
 	__u16			level,
 	__u16			numrecs,
 	__u64			owner,
 	unsigned int		flags)
 {
 	int			crc = xfs_sb_version_hascrc(&mp->m_sb);
+	__u32			magic = xfs_btree_magic(crc, btnum);
 
 	buf->bb_magic = cpu_to_be32(magic);
 	buf->bb_level = cpu_to_be16(level);
@@ -1138,14 +1139,14 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
 xfs_btree_init_block(
 	struct xfs_mount *mp,
 	struct xfs_buf	*bp,
-	__u32		magic,
+	xfs_btnum_t	btnum,
 	__u16		level,
 	__u16		numrecs,
 	__u64		owner,
 	unsigned int	flags)
 {
 	xfs_btree_init_block_int(mp, XFS_BUF_TO_BLOCK(bp), bp->b_bn,
-				 magic, level, numrecs, owner, flags);
+				 btnum, level, numrecs, owner, flags);
 }
 
 STATIC void
@@ -1156,8 +1157,6 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
 	int			numrecs)
 {
 	__u64			owner;
-	int			crc = xfs_sb_version_hascrc(&cur->bc_mp->m_sb);
-	xfs_btnum_t		btnum = cur->bc_btnum;
 
 	/*
 	 * we can pull the owner from the cursor right now as the different
@@ -1171,7 +1170,7 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
 		owner = cur->bc_private.a.agno;
 
 	xfs_btree_init_block_int(cur->bc_mp, XFS_BUF_TO_BLOCK(bp), bp->b_bn,
-				 xfs_btree_magic(crc, btnum), level, numrecs,
+				 cur->bc_btnum, level, numrecs,
 				 owner, cur->bc_flags);
 }
 
diff --git a/fs/xfs/libxfs/xfs_btree.h b/fs/xfs/libxfs/xfs_btree.h
index 3aaced3..ae10b7b 100644
--- a/fs/xfs/libxfs/xfs_btree.h
+++ b/fs/xfs/libxfs/xfs_btree.h
@@ -415,7 +415,7 @@ struct xfs_buf *				/* buffer for agno/agbno */
 xfs_btree_init_block(
 	struct xfs_mount *mp,
 	struct xfs_buf	*bp,
-	__u32		magic,
+	xfs_btnum_t	btnum,
 	__u16		level,
 	__u16		numrecs,
 	__u64		owner,
@@ -426,7 +426,7 @@ struct xfs_buf *				/* buffer for agno/agbno */
 	struct xfs_mount	*mp,
 	struct xfs_btree_block	*buf,
 	xfs_daddr_t		blkno,
-	__u32			magic,
+	xfs_btnum_t		btnum,
 	__u16			level,
 	__u16			numrecs,
 	__u64			owner,
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index bd20051..e702928 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -352,12 +352,7 @@
 			goto error0;
 		}
 
-		if (xfs_sb_version_hascrc(&mp->m_sb))
-			xfs_btree_init_block(mp, bp, XFS_ABTB_CRC_MAGIC, 0, 1,
-						agno, 0);
-		else
-			xfs_btree_init_block(mp, bp, XFS_ABTB_MAGIC, 0, 1,
-						agno, 0);
+		xfs_btree_init_block(mp, bp, XFS_BTNUM_BNO, 0, 1, agno, 0);
 
 		arec = XFS_ALLOC_REC_ADDR(mp, XFS_BUF_TO_BLOCK(bp), 1);
 		arec->ar_startblock = cpu_to_be32(mp->m_ag_prealloc_blocks);
@@ -381,12 +376,7 @@
 			goto error0;
 		}
 
-		if (xfs_sb_version_hascrc(&mp->m_sb))
-			xfs_btree_init_block(mp, bp, XFS_ABTC_CRC_MAGIC, 0, 1,
-						agno, 0);
-		else
-			xfs_btree_init_block(mp, bp, XFS_ABTC_MAGIC, 0, 1,
-						agno, 0);
+		xfs_btree_init_block(mp, bp, XFS_BTNUM_CNT, 0, 1, agno, 0);
 
 		arec = XFS_ALLOC_REC_ADDR(mp, XFS_BUF_TO_BLOCK(bp), 1);
 		arec->ar_startblock = cpu_to_be32(mp->m_ag_prealloc_blocks);
@@ -413,7 +403,7 @@
 				goto error0;
 			}
 
-			xfs_btree_init_block(mp, bp, XFS_RMAP_CRC_MAGIC, 0, 0,
+			xfs_btree_init_block(mp, bp, XFS_BTNUM_RMAP, 0, 0,
 						agno, 0);
 			block = XFS_BUF_TO_BLOCK(bp);
 
@@ -488,12 +478,7 @@
 			goto error0;
 		}
 
-		if (xfs_sb_version_hascrc(&mp->m_sb))
-			xfs_btree_init_block(mp, bp, XFS_IBT_CRC_MAGIC, 0, 0,
-						agno, 0);
-		else
-			xfs_btree_init_block(mp, bp, XFS_IBT_MAGIC, 0, 0,
-						agno, 0);
+		xfs_btree_init_block(mp, bp, XFS_BTNUM_INO , 0, 0, agno, 0);
 
 		error = xfs_bwrite(bp);
 		xfs_buf_relse(bp);
@@ -513,12 +498,8 @@
 				goto error0;
 			}
 
-			if (xfs_sb_version_hascrc(&mp->m_sb))
-				xfs_btree_init_block(mp, bp, XFS_FIBT_CRC_MAGIC,
+			xfs_btree_init_block(mp, bp, XFS_BTNUM_FINO,
 						     0, 0, agno, 0);
-			else
-				xfs_btree_init_block(mp, bp, XFS_FIBT_MAGIC, 0,
-						     0, agno, 0);
 
 			error = xfs_bwrite(bp);
 			xfs_buf_relse(bp);
@@ -539,7 +520,7 @@
 				goto error0;
 			}
 
-			xfs_btree_init_block(mp, bp, XFS_REFC_CRC_MAGIC,
+			xfs_btree_init_block(mp, bp, XFS_BTNUM_REFC,
 					     0, 0, agno, 0);
 
 			error = xfs_bwrite(bp);

Re: [PATCH 1/3] xfs: glean crc status from mp not flags in xfs_btree_init_block_int

[Brian Foster]

On Thu, Dec 22, 2016 at 11:44:59AM -0600, Eric Sandeen wrote:
> xfs_btree_init_block_int() can determine whether crcs are
> in effect without the passed-in XFS_BTREE_CRC_BLOCKS flag;
> the mp argument allows us to determine this from the
> superblock.  Remove the flag from callers, and use
> xfs_sb_version_hascrc(&mp->m_sb) internally instead.
> 
> This removes one difference between the if & else cases
> in the callers.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index c27344c..e645aa8 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -727,7 +727,7 @@ static inline bool xfs_bmap_wants_extents(struct xfs_inode *ip, int whichfork)
>  	if (xfs_sb_version_hascrc(&mp->m_sb))
>  		xfs_btree_init_block_int(mp, block, XFS_BUF_DADDR_NULL,
>  				 XFS_BMAP_CRC_MAGIC, 1, 1, ip->i_ino,
> -				 XFS_BTREE_LONG_PTRS | XFS_BTREE_CRC_BLOCKS);
> +				 XFS_BTREE_LONG_PTRS);
>  	else
>  		xfs_btree_init_block_int(mp, block, XFS_BUF_DADDR_NULL,
>  				 XFS_BMAP_MAGIC, 1, 1, ip->i_ino,
> @@ -804,7 +804,7 @@ static inline bool xfs_bmap_wants_extents(struct xfs_inode *ip, int whichfork)
>  	if (xfs_sb_version_hascrc(&mp->m_sb))
>  		xfs_btree_init_block_int(mp, ablock, abp->b_bn,
>  				XFS_BMAP_CRC_MAGIC, 0, 0, ip->i_ino,
> -				XFS_BTREE_LONG_PTRS | XFS_BTREE_CRC_BLOCKS);
> +				XFS_BTREE_LONG_PTRS);
>  	else
>  		xfs_btree_init_block_int(mp, ablock, abp->b_bn,
>  				XFS_BMAP_MAGIC, 0, 0, ip->i_ino,
> diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
> index 8007d2b..f0293d4 100644
> --- a/fs/xfs/libxfs/xfs_bmap_btree.c
> +++ b/fs/xfs/libxfs/xfs_bmap_btree.c
> @@ -74,7 +74,7 @@
>  	if (xfs_sb_version_hascrc(&mp->m_sb))
>  		xfs_btree_init_block_int(mp, rblock, XFS_BUF_DADDR_NULL,
>  				 XFS_BMAP_CRC_MAGIC, 0, 0, ip->i_ino,
> -				 XFS_BTREE_LONG_PTRS | XFS_BTREE_CRC_BLOCKS);
> +				 XFS_BTREE_LONG_PTRS);
>  	else
>  		xfs_btree_init_block_int(mp, rblock, XFS_BUF_DADDR_NULL,
>  				 XFS_BMAP_MAGIC, 0, 0, ip->i_ino,
> diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> index 5c8e6f2..f49fc2f 100644
> --- a/fs/xfs/libxfs/xfs_btree.c
> +++ b/fs/xfs/libxfs/xfs_btree.c
> @@ -1090,6 +1090,8 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
>  	__u64			owner,
>  	unsigned int		flags)
>  {
> +	int			crc = xfs_sb_version_hascrc(&mp->m_sb);
> +
>  	buf->bb_magic = cpu_to_be32(magic);
>  	buf->bb_level = cpu_to_be16(level);
>  	buf->bb_numrecs = cpu_to_be16(numrecs);
> @@ -1097,7 +1099,7 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
>  	if (flags & XFS_BTREE_LONG_PTRS) {
>  		buf->bb_u.l.bb_leftsib = cpu_to_be64(NULLFSBLOCK);
>  		buf->bb_u.l.bb_rightsib = cpu_to_be64(NULLFSBLOCK);
> -		if (flags & XFS_BTREE_CRC_BLOCKS) {
> +		if (crc) {
>  			buf->bb_u.l.bb_blkno = cpu_to_be64(blkno);
>  			buf->bb_u.l.bb_owner = cpu_to_be64(owner);
>  			uuid_copy(&buf->bb_u.l.bb_uuid, &mp->m_sb.sb_meta_uuid);
> @@ -1110,7 +1112,7 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
>  
>  		buf->bb_u.s.bb_leftsib = cpu_to_be32(NULLAGBLOCK);
>  		buf->bb_u.s.bb_rightsib = cpu_to_be32(NULLAGBLOCK);
> -		if (flags & XFS_BTREE_CRC_BLOCKS) {
> +		if (crc) {
>  			buf->bb_u.s.bb_blkno = cpu_to_be64(blkno);
>  			buf->bb_u.s.bb_owner = cpu_to_be32(__owner);
>  			uuid_copy(&buf->bb_u.s.bb_uuid, &mp->m_sb.sb_meta_uuid);
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index 93d12fa..bd20051 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -354,7 +354,7 @@
>  
>  		if (xfs_sb_version_hascrc(&mp->m_sb))
>  			xfs_btree_init_block(mp, bp, XFS_ABTB_CRC_MAGIC, 0, 1,
> -						agno, XFS_BTREE_CRC_BLOCKS);
> +						agno, 0);
>  		else
>  			xfs_btree_init_block(mp, bp, XFS_ABTB_MAGIC, 0, 1,
>  						agno, 0);
> @@ -383,7 +383,7 @@
>  
>  		if (xfs_sb_version_hascrc(&mp->m_sb))
>  			xfs_btree_init_block(mp, bp, XFS_ABTC_CRC_MAGIC, 0, 1,
> -						agno, XFS_BTREE_CRC_BLOCKS);
> +						agno, 0);
>  		else
>  			xfs_btree_init_block(mp, bp, XFS_ABTC_MAGIC, 0, 1,
>  						agno, 0);
> @@ -414,7 +414,7 @@
>  			}
>  
>  			xfs_btree_init_block(mp, bp, XFS_RMAP_CRC_MAGIC, 0, 0,
> -						agno, XFS_BTREE_CRC_BLOCKS);
> +						agno, 0);
>  			block = XFS_BUF_TO_BLOCK(bp);
>  
>  
> @@ -490,7 +490,7 @@
>  
>  		if (xfs_sb_version_hascrc(&mp->m_sb))
>  			xfs_btree_init_block(mp, bp, XFS_IBT_CRC_MAGIC, 0, 0,
> -						agno, XFS_BTREE_CRC_BLOCKS);
> +						agno, 0);
>  		else
>  			xfs_btree_init_block(mp, bp, XFS_IBT_MAGIC, 0, 0,
>  						agno, 0);
> @@ -515,8 +515,7 @@
>  
>  			if (xfs_sb_version_hascrc(&mp->m_sb))
>  				xfs_btree_init_block(mp, bp, XFS_FIBT_CRC_MAGIC,
> -						     0, 0, agno,
> -						     XFS_BTREE_CRC_BLOCKS);
> +						     0, 0, agno, 0);
>  			else
>  				xfs_btree_init_block(mp, bp, XFS_FIBT_MAGIC, 0,
>  						     0, agno, 0);
> @@ -541,8 +540,7 @@
>  			}
>  
>  			xfs_btree_init_block(mp, bp, XFS_REFC_CRC_MAGIC,
> -					     0, 0, agno,
> -					     XFS_BTREE_CRC_BLOCKS);
> +					     0, 0, agno, 0);
>  
>  			error = xfs_bwrite(bp);
>  			xfs_buf_relse(bp);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3 V2] xfs: make xfs_btree_magic more generic

[Brian Foster]

On Thu, Dec 22, 2016 at 01:43:55PM -0600, Eric Sandeen wrote:
> Right now the xfs_btree_magic() define takes only a cursor;
> change this to take crc and btnum args to make it more generically
> useful, and move to a function.
> 
> This will allow xfs_btree_init_block_int callers which don't
> have a cursor to make use of the xfs_magics array, which will
> happen in the next patch.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> V2: turn xfs_btree_magic into a function with a built-in ASSERT
> 
> diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> index f49fc2f..81866dd 100644
> --- a/fs/xfs/libxfs/xfs_btree.c
> +++ b/fs/xfs/libxfs/xfs_btree.c
> @@ -50,8 +50,15 @@
>  	  XFS_BMAP_CRC_MAGIC, XFS_IBT_CRC_MAGIC, XFS_FIBT_CRC_MAGIC,
>  	  XFS_REFC_CRC_MAGIC }
>  };
> -#define xfs_btree_magic(cur) \
> -	xfs_magics[!!((cur)->bc_flags & XFS_BTREE_CRC_BLOCKS)][cur->bc_btnum]
> +
> +__uint32_t xfs_btree_magic(int crc, xfs_btnum_t btnum)
> +{
> +	__uint32_t magic = xfs_magics[crc][btnum];
> +
> +	/* Ensure we asked for crc for crc-only magics. */

Purely a nit, but for whatever reason this sentence confused me.
Perhaps just drop it, or say something like "ensure we asked for a valid
magic?"

Either way:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> +	ASSERT(magic != 0);
> +	return magic;
> +}
>  
>  STATIC int				/* error (0 or EFSCORRUPTED) */
>  xfs_btree_check_lblock(
> @@ -62,10 +69,13 @@
>  {
>  	int			lblock_ok = 1; /* block passes checks */
>  	struct xfs_mount	*mp;	/* file system mount point */
> +	xfs_btnum_t		btnum = cur->bc_btnum;
> +	int			crc;
>  
>  	mp = cur->bc_mp;
> +	crc = xfs_sb_version_hascrc(&mp->m_sb);
>  
> -	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> +	if (crc) {
>  		lblock_ok = lblock_ok &&
>  			uuid_equal(&block->bb_u.l.bb_uuid,
>  				   &mp->m_sb.sb_meta_uuid) &&
> @@ -74,7 +84,7 @@
>  	}
>  
>  	lblock_ok = lblock_ok &&
> -		be32_to_cpu(block->bb_magic) == xfs_btree_magic(cur) &&
> +		be32_to_cpu(block->bb_magic) == xfs_btree_magic(crc, btnum) &&
>  		be16_to_cpu(block->bb_level) == level &&
>  		be16_to_cpu(block->bb_numrecs) <=
>  			cur->bc_ops->get_maxrecs(cur, level) &&
> @@ -110,13 +120,16 @@
>  	struct xfs_agf		*agf;	/* ag. freespace structure */
>  	xfs_agblock_t		agflen;	/* native ag. freespace length */
>  	int			sblock_ok = 1; /* block passes checks */
> +	xfs_btnum_t		btnum = cur->bc_btnum;
> +	int			crc;
>  
>  	mp = cur->bc_mp;
> +	crc = xfs_sb_version_hascrc(&mp->m_sb);
>  	agbp = cur->bc_private.a.agbp;
>  	agf = XFS_BUF_TO_AGF(agbp);
>  	agflen = be32_to_cpu(agf->agf_length);
>  
> -	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> +	if (crc) {
>  		sblock_ok = sblock_ok &&
>  			uuid_equal(&block->bb_u.s.bb_uuid,
>  				   &mp->m_sb.sb_meta_uuid) &&
> @@ -125,7 +138,7 @@
>  	}
>  
>  	sblock_ok = sblock_ok &&
> -		be32_to_cpu(block->bb_magic) == xfs_btree_magic(cur) &&
> +		be32_to_cpu(block->bb_magic) == xfs_btree_magic(crc, btnum) &&
>  		be16_to_cpu(block->bb_level) == level &&
>  		be16_to_cpu(block->bb_numrecs) <=
>  			cur->bc_ops->get_maxrecs(cur, level) &&
> @@ -1142,7 +1155,9 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
>  	int			level,
>  	int			numrecs)
>  {
> -	__u64 owner;
> +	__u64			owner;
> +	int			crc = xfs_sb_version_hascrc(&cur->bc_mp->m_sb);
> +	xfs_btnum_t		btnum = cur->bc_btnum;
>  
>  	/*
>  	 * we can pull the owner from the cursor right now as the different
> @@ -1156,7 +1171,7 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
>  		owner = cur->bc_private.a.agno;
>  
>  	xfs_btree_init_block_int(cur->bc_mp, XFS_BUF_TO_BLOCK(bp), bp->b_bn,
> -				 xfs_btree_magic(cur), level, numrecs,
> +				 xfs_btree_magic(crc, btnum), level, numrecs,
>  				 owner, cur->bc_flags);
>  }
>  
> diff --git a/fs/xfs/libxfs/xfs_btree.h b/fs/xfs/libxfs/xfs_btree.h
> index c2b01d1..3aaced3 100644
> --- a/fs/xfs/libxfs/xfs_btree.h
> +++ b/fs/xfs/libxfs/xfs_btree.h
> @@ -76,6 +76,8 @@
>  #define	XFS_BTNUM_RMAP	((xfs_btnum_t)XFS_BTNUM_RMAPi)
>  #define	XFS_BTNUM_REFC	((xfs_btnum_t)XFS_BTNUM_REFCi)
>  
> +__uint32_t xfs_btree_magic(int crc, xfs_btnum_t btnum);
> +
>  /*
>   * For logging record fields.
>   */
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3 V2] xfs: remove boilerplate around xfs_btree_init_block

[Brian Foster]

On Thu, Dec 22, 2016 at 01:44:50PM -0600, Eric Sandeen wrote:
> Now that xfs_btree_init_block_int is able to determine crc
> status from the passed-in mp, we can determine the proper
> magic as well if we are given a btree number, rather than
> an explicit magic value.
> 
> Change xfs_btree_init_block[_int] callers to pass in the
> btree number, and let xfs_btree_init_block_int use the
> xfs_magics array via the xfs_btree_magic macro to determine
> which magic value is needed.  This makes all of the
> if (crc) / else stanzas identical, and the if/else can be
> removed, leading to a single, common init_block call.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

> 
> V2: remove ASSERT after xfs_btree_magic now that it's built
> into that function
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index e645aa8..b30ff0a 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -724,15 +724,9 @@ static inline bool xfs_bmap_wants_extents(struct xfs_inode *ip, int whichfork)
>  	 * Fill in the root.
>  	 */
>  	block = ifp->if_broot;
> -	if (xfs_sb_version_hascrc(&mp->m_sb))
> -		xfs_btree_init_block_int(mp, block, XFS_BUF_DADDR_NULL,
> -				 XFS_BMAP_CRC_MAGIC, 1, 1, ip->i_ino,
> +	xfs_btree_init_block_int(mp, block, XFS_BUF_DADDR_NULL,
> +				 XFS_BTNUM_BMAP, 1, 1, ip->i_ino,
>  				 XFS_BTREE_LONG_PTRS);
> -	else
> -		xfs_btree_init_block_int(mp, block, XFS_BUF_DADDR_NULL,
> -				 XFS_BMAP_MAGIC, 1, 1, ip->i_ino,
> -				 XFS_BTREE_LONG_PTRS);
> -
>  	/*
>  	 * Need a cursor.  Can't allocate until bb_level is filled in.
>  	 */
> @@ -801,13 +795,8 @@ static inline bool xfs_bmap_wants_extents(struct xfs_inode *ip, int whichfork)
>  	 */
>  	abp->b_ops = &xfs_bmbt_buf_ops;
>  	ablock = XFS_BUF_TO_BLOCK(abp);
> -	if (xfs_sb_version_hascrc(&mp->m_sb))
> -		xfs_btree_init_block_int(mp, ablock, abp->b_bn,
> -				XFS_BMAP_CRC_MAGIC, 0, 0, ip->i_ino,
> -				XFS_BTREE_LONG_PTRS);
> -	else
> -		xfs_btree_init_block_int(mp, ablock, abp->b_bn,
> -				XFS_BMAP_MAGIC, 0, 0, ip->i_ino,
> +	xfs_btree_init_block_int(mp, ablock, abp->b_bn,
> +				XFS_BTNUM_BMAP, 0, 0, ip->i_ino,
>  				XFS_BTREE_LONG_PTRS);
>  
>  	arp = XFS_BMBT_REC_ADDR(mp, ablock, 1);
> diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
> index f0293d4..a8867a7 100644
> --- a/fs/xfs/libxfs/xfs_bmap_btree.c
> +++ b/fs/xfs/libxfs/xfs_bmap_btree.c
> @@ -71,15 +71,9 @@
>  	xfs_bmbt_key_t		*tkp;
>  	__be64			*tpp;
>  
> -	if (xfs_sb_version_hascrc(&mp->m_sb))
> -		xfs_btree_init_block_int(mp, rblock, XFS_BUF_DADDR_NULL,
> -				 XFS_BMAP_CRC_MAGIC, 0, 0, ip->i_ino,
> -				 XFS_BTREE_LONG_PTRS);
> -	else
> -		xfs_btree_init_block_int(mp, rblock, XFS_BUF_DADDR_NULL,
> -				 XFS_BMAP_MAGIC, 0, 0, ip->i_ino,
> +	xfs_btree_init_block_int(mp, rblock, XFS_BUF_DADDR_NULL,
> +				 XFS_BTNUM_BMAP, 0, 0, ip->i_ino,
>  				 XFS_BTREE_LONG_PTRS);
> -
>  	rblock->bb_level = dblock->bb_level;
>  	ASSERT(be16_to_cpu(rblock->bb_level) > 0);
>  	rblock->bb_numrecs = dblock->bb_numrecs;
> diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> index 81866dd..0dcc5f2 100644
> --- a/fs/xfs/libxfs/xfs_btree.c
> +++ b/fs/xfs/libxfs/xfs_btree.c
> @@ -1097,13 +1097,14 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
>  	struct xfs_mount	*mp,
>  	struct xfs_btree_block	*buf,
>  	xfs_daddr_t		blkno,
> -	__u32			magic,
> +	xfs_btnum_t		btnum,
>  	__u16			level,
>  	__u16			numrecs,
>  	__u64			owner,
>  	unsigned int		flags)
>  {
>  	int			crc = xfs_sb_version_hascrc(&mp->m_sb);
> +	__u32			magic = xfs_btree_magic(crc, btnum);
>  
>  	buf->bb_magic = cpu_to_be32(magic);
>  	buf->bb_level = cpu_to_be16(level);
> @@ -1138,14 +1139,14 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
>  xfs_btree_init_block(
>  	struct xfs_mount *mp,
>  	struct xfs_buf	*bp,
> -	__u32		magic,
> +	xfs_btnum_t	btnum,
>  	__u16		level,
>  	__u16		numrecs,
>  	__u64		owner,
>  	unsigned int	flags)
>  {
>  	xfs_btree_init_block_int(mp, XFS_BUF_TO_BLOCK(bp), bp->b_bn,
> -				 magic, level, numrecs, owner, flags);
> +				 btnum, level, numrecs, owner, flags);
>  }
>  
>  STATIC void
> @@ -1156,8 +1157,6 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
>  	int			numrecs)
>  {
>  	__u64			owner;
> -	int			crc = xfs_sb_version_hascrc(&cur->bc_mp->m_sb);
> -	xfs_btnum_t		btnum = cur->bc_btnum;
>  
>  	/*
>  	 * we can pull the owner from the cursor right now as the different
> @@ -1171,7 +1170,7 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
>  		owner = cur->bc_private.a.agno;
>  
>  	xfs_btree_init_block_int(cur->bc_mp, XFS_BUF_TO_BLOCK(bp), bp->b_bn,
> -				 xfs_btree_magic(crc, btnum), level, numrecs,
> +				 cur->bc_btnum, level, numrecs,
>  				 owner, cur->bc_flags);
>  }
>  
> diff --git a/fs/xfs/libxfs/xfs_btree.h b/fs/xfs/libxfs/xfs_btree.h
> index 3aaced3..ae10b7b 100644
> --- a/fs/xfs/libxfs/xfs_btree.h
> +++ b/fs/xfs/libxfs/xfs_btree.h
> @@ -415,7 +415,7 @@ struct xfs_buf *				/* buffer for agno/agbno */
>  xfs_btree_init_block(
>  	struct xfs_mount *mp,
>  	struct xfs_buf	*bp,
> -	__u32		magic,
> +	xfs_btnum_t	btnum,
>  	__u16		level,
>  	__u16		numrecs,
>  	__u64		owner,
> @@ -426,7 +426,7 @@ struct xfs_buf *				/* buffer for agno/agbno */
>  	struct xfs_mount	*mp,
>  	struct xfs_btree_block	*buf,
>  	xfs_daddr_t		blkno,
> -	__u32			magic,
> +	xfs_btnum_t		btnum,
>  	__u16			level,
>  	__u16			numrecs,
>  	__u64			owner,
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index bd20051..e702928 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -352,12 +352,7 @@
>  			goto error0;
>  		}
>  
> -		if (xfs_sb_version_hascrc(&mp->m_sb))
> -			xfs_btree_init_block(mp, bp, XFS_ABTB_CRC_MAGIC, 0, 1,
> -						agno, 0);
> -		else
> -			xfs_btree_init_block(mp, bp, XFS_ABTB_MAGIC, 0, 1,
> -						agno, 0);
> +		xfs_btree_init_block(mp, bp, XFS_BTNUM_BNO, 0, 1, agno, 0);
>  
>  		arec = XFS_ALLOC_REC_ADDR(mp, XFS_BUF_TO_BLOCK(bp), 1);
>  		arec->ar_startblock = cpu_to_be32(mp->m_ag_prealloc_blocks);
> @@ -381,12 +376,7 @@
>  			goto error0;
>  		}
>  
> -		if (xfs_sb_version_hascrc(&mp->m_sb))
> -			xfs_btree_init_block(mp, bp, XFS_ABTC_CRC_MAGIC, 0, 1,
> -						agno, 0);
> -		else
> -			xfs_btree_init_block(mp, bp, XFS_ABTC_MAGIC, 0, 1,
> -						agno, 0);
> +		xfs_btree_init_block(mp, bp, XFS_BTNUM_CNT, 0, 1, agno, 0);
>  
>  		arec = XFS_ALLOC_REC_ADDR(mp, XFS_BUF_TO_BLOCK(bp), 1);
>  		arec->ar_startblock = cpu_to_be32(mp->m_ag_prealloc_blocks);
> @@ -413,7 +403,7 @@
>  				goto error0;
>  			}
>  
> -			xfs_btree_init_block(mp, bp, XFS_RMAP_CRC_MAGIC, 0, 0,
> +			xfs_btree_init_block(mp, bp, XFS_BTNUM_RMAP, 0, 0,
>  						agno, 0);
>  			block = XFS_BUF_TO_BLOCK(bp);
>  
> @@ -488,12 +478,7 @@
>  			goto error0;
>  		}
>  
> -		if (xfs_sb_version_hascrc(&mp->m_sb))
> -			xfs_btree_init_block(mp, bp, XFS_IBT_CRC_MAGIC, 0, 0,
> -						agno, 0);
> -		else
> -			xfs_btree_init_block(mp, bp, XFS_IBT_MAGIC, 0, 0,
> -						agno, 0);
> +		xfs_btree_init_block(mp, bp, XFS_BTNUM_INO , 0, 0, agno, 0);
>  
>  		error = xfs_bwrite(bp);
>  		xfs_buf_relse(bp);
> @@ -513,12 +498,8 @@
>  				goto error0;
>  			}
>  
> -			if (xfs_sb_version_hascrc(&mp->m_sb))
> -				xfs_btree_init_block(mp, bp, XFS_FIBT_CRC_MAGIC,
> +			xfs_btree_init_block(mp, bp, XFS_BTNUM_FINO,
>  						     0, 0, agno, 0);
> -			else
> -				xfs_btree_init_block(mp, bp, XFS_FIBT_MAGIC, 0,
> -						     0, agno, 0);
>  
>  			error = xfs_bwrite(bp);
>  			xfs_buf_relse(bp);
> @@ -539,7 +520,7 @@
>  				goto error0;
>  			}
>  
> -			xfs_btree_init_block(mp, bp, XFS_REFC_CRC_MAGIC,
> +			xfs_btree_init_block(mp, bp, XFS_BTNUM_REFC,
>  					     0, 0, agno, 0);
>  
>  			error = xfs_bwrite(bp);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3 V2] xfs: make xfs_btree_magic more generic

[Eric Sandeen]

On 1/3/17 12:28 PM, Brian Foster wrote:
> On Thu, Dec 22, 2016 at 01:43:55PM -0600, Eric Sandeen wrote:
>> Right now the xfs_btree_magic() define takes only a cursor;
>> change this to take crc and btnum args to make it more generically
>> useful, and move to a function.
>>
>> This will allow xfs_btree_init_block_int callers which don't
>> have a cursor to make use of the xfs_magics array, which will
>> happen in the next patch.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> V2: turn xfs_btree_magic into a function with a built-in ASSERT
>>
>> diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
>> index f49fc2f..81866dd 100644
>> --- a/fs/xfs/libxfs/xfs_btree.c
>> +++ b/fs/xfs/libxfs/xfs_btree.c
>> @@ -50,8 +50,15 @@
>>  	  XFS_BMAP_CRC_MAGIC, XFS_IBT_CRC_MAGIC, XFS_FIBT_CRC_MAGIC,
>>  	  XFS_REFC_CRC_MAGIC }
>>  };
>> -#define xfs_btree_magic(cur) \
>> -	xfs_magics[!!((cur)->bc_flags & XFS_BTREE_CRC_BLOCKS)][cur->bc_btnum]
>> +
>> +__uint32_t xfs_btree_magic(int crc, xfs_btnum_t btnum)
>> +{
>> +	__uint32_t magic = xfs_magics[crc][btnum];
>> +
>> +	/* Ensure we asked for crc for crc-only magics. */
> 
> Purely a nit, but for whatever reason this sentence confused me.
> Perhaps just drop it, or say something like "ensure we asked for a valid
> magic?"

Hm, ok, it's supposed to mean "make sure CRC ==1 if we are asking for a
magic number for a structure which exists only on filesystems w/ CRCs"

If not, we'd get 0 for magic, and that's the assert.  But yeah, I can
see how the sentence isn't very clear.  :(

-Eric

> Either way:
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
>> +	ASSERT(magic != 0);
>> +	return magic;
>> +}
>>  
>>  STATIC int				/* error (0 or EFSCORRUPTED) */
>>  xfs_btree_check_lblock(
>> @@ -62,10 +69,13 @@
>>  {
>>  	int			lblock_ok = 1; /* block passes checks */
>>  	struct xfs_mount	*mp;	/* file system mount point */
>> +	xfs_btnum_t		btnum = cur->bc_btnum;
>> +	int			crc;
>>  
>>  	mp = cur->bc_mp;
>> +	crc = xfs_sb_version_hascrc(&mp->m_sb);
>>  
>> -	if (xfs_sb_version_hascrc(&mp->m_sb)) {
>> +	if (crc) {
>>  		lblock_ok = lblock_ok &&
>>  			uuid_equal(&block->bb_u.l.bb_uuid,
>>  				   &mp->m_sb.sb_meta_uuid) &&
>> @@ -74,7 +84,7 @@
>>  	}
>>  
>>  	lblock_ok = lblock_ok &&
>> -		be32_to_cpu(block->bb_magic) == xfs_btree_magic(cur) &&
>> +		be32_to_cpu(block->bb_magic) == xfs_btree_magic(crc, btnum) &&
>>  		be16_to_cpu(block->bb_level) == level &&
>>  		be16_to_cpu(block->bb_numrecs) <=
>>  			cur->bc_ops->get_maxrecs(cur, level) &&
>> @@ -110,13 +120,16 @@
>>  	struct xfs_agf		*agf;	/* ag. freespace structure */
>>  	xfs_agblock_t		agflen;	/* native ag. freespace length */
>>  	int			sblock_ok = 1; /* block passes checks */
>> +	xfs_btnum_t		btnum = cur->bc_btnum;
>> +	int			crc;
>>  
>>  	mp = cur->bc_mp;
>> +	crc = xfs_sb_version_hascrc(&mp->m_sb);
>>  	agbp = cur->bc_private.a.agbp;
>>  	agf = XFS_BUF_TO_AGF(agbp);
>>  	agflen = be32_to_cpu(agf->agf_length);
>>  
>> -	if (xfs_sb_version_hascrc(&mp->m_sb)) {
>> +	if (crc) {
>>  		sblock_ok = sblock_ok &&
>>  			uuid_equal(&block->bb_u.s.bb_uuid,
>>  				   &mp->m_sb.sb_meta_uuid) &&
>> @@ -125,7 +138,7 @@
>>  	}
>>  
>>  	sblock_ok = sblock_ok &&
>> -		be32_to_cpu(block->bb_magic) == xfs_btree_magic(cur) &&
>> +		be32_to_cpu(block->bb_magic) == xfs_btree_magic(crc, btnum) &&
>>  		be16_to_cpu(block->bb_level) == level &&
>>  		be16_to_cpu(block->bb_numrecs) <=
>>  			cur->bc_ops->get_maxrecs(cur, level) &&
>> @@ -1142,7 +1155,9 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
>>  	int			level,
>>  	int			numrecs)
>>  {
>> -	__u64 owner;
>> +	__u64			owner;
>> +	int			crc = xfs_sb_version_hascrc(&cur->bc_mp->m_sb);
>> +	xfs_btnum_t		btnum = cur->bc_btnum;
>>  
>>  	/*
>>  	 * we can pull the owner from the cursor right now as the different
>> @@ -1156,7 +1171,7 @@ static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
>>  		owner = cur->bc_private.a.agno;
>>  
>>  	xfs_btree_init_block_int(cur->bc_mp, XFS_BUF_TO_BLOCK(bp), bp->b_bn,
>> -				 xfs_btree_magic(cur), level, numrecs,
>> +				 xfs_btree_magic(crc, btnum), level, numrecs,
>>  				 owner, cur->bc_flags);
>>  }
>>  
>> diff --git a/fs/xfs/libxfs/xfs_btree.h b/fs/xfs/libxfs/xfs_btree.h
>> index c2b01d1..3aaced3 100644
>> --- a/fs/xfs/libxfs/xfs_btree.h
>> +++ b/fs/xfs/libxfs/xfs_btree.h
>> @@ -76,6 +76,8 @@
>>  #define	XFS_BTNUM_RMAP	((xfs_btnum_t)XFS_BTNUM_RMAPi)
>>  #define	XFS_BTNUM_REFC	((xfs_btnum_t)XFS_BTNUM_REFCi)
>>  
>> +__uint32_t xfs_btree_magic(int crc, xfs_btnum_t btnum);
>> +
>>  /*
>>   * For logging record fields.
>>   */
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 0/3] xfs: reduce boilerplate around xfs_btree_init_block

[Eric Sandeen]

On 12/22/16 11:41 AM, Eric Sandeen wrote:
> We have this pattern all over the kernel & userspace:
> 
>                 if (xfs_sb_version_hascrc(&mp->m_sb))
>                         xfs_btree_init_block(mp, bp, XFS_ABTB_CRC_MAGIC, 0, 1,
>                                                 agno, XFS_BTREE_CRC_BLOCKS);
>                 else
>                         xfs_btree_init_block(mp, bp, XFS_ABTB_MAGIC, 0, 1,
>                                                 agno, 0);
> 
> The last flag (XFS_BTREE_CRC_BLOCKS) can be gleaned from
> the features on the superblock via mp just as the if/else does,
> so no need to pass that in from the caller.  That's patch 1.

Ping on this series?

Thanks,
-Eric

> Then the difference is simply CRC vs not-CRC magic.  This can also
> be determined inside the called function by passing in a btree number,
> and looking up the proper magic in the xfs_magics array.
> 
> patch2 makes xfs_btree_magic() more generic, taking a 
> crc flag & btnum, instead of just a cursor.
> 
> patch3 then makes use of xfs_btree_magic in the block init routine
> to determine the proper magic.
> 
> With those changes, the if/else can go away, and we can simply call:
> 
> xfs_btree_init_block(mp, bp, XFS_BTNUM_BNO, 0, 1, agno, 0);
> 
> and let the lower function sort out crc vs. no crc differences
> by itself.
> 
> It's not a huge reduction in code, but it reduces a lot of the
> boilerplate, particularly in growfs code.  This may also make it
> slightly easier to factor the growfs tree init code into a loop
> (not sure about that, yet) and/or to move mkfs & growfs tree init
> code into a libxfs/ call.
> 
>  libxfs/xfs_bmap.c       |   19 ++++---------------
>  libxfs/xfs_bmap_btree.c |   10 ++--------
>  libxfs/xfs_btree.c      |   34 +++++++++++++++++++++-------------
>  libxfs/xfs_btree.h      |    4 ++--
>  libxfs/xfs_types.h      |    2 ++
>  xfs_fsops.c             |   39 +++++++++------------------------------
>  6 files changed, 40 insertions(+), 68 deletions(-)
> 
> -Eric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 0/3] xfs: reduce boilerplate around xfs_btree_init_block

[Darrick J. Wong]

On Mon, Jan 16, 2017 at 05:13:34PM -0600, Eric Sandeen wrote:
> On 12/22/16 11:41 AM, Eric Sandeen wrote:
> > We have this pattern all over the kernel & userspace:
> > 
> >                 if (xfs_sb_version_hascrc(&mp->m_sb))
> >                         xfs_btree_init_block(mp, bp, XFS_ABTB_CRC_MAGIC, 0, 1,
> >                                                 agno, XFS_BTREE_CRC_BLOCKS);
> >                 else
> >                         xfs_btree_init_block(mp, bp, XFS_ABTB_MAGIC, 0, 1,
> >                                                 agno, 0);
> > 
> > The last flag (XFS_BTREE_CRC_BLOCKS) can be gleaned from
> > the features on the superblock via mp just as the if/else does,
> > so no need to pass that in from the caller.  That's patch 1.
> 
> Ping on this series?

Looks ok to me, it's on my list for 4.11.

(Sorry, I've been a little slow since NYE, dealing with all the ice and
snow eating my house.)

--D

> 
> Thanks,
> -Eric
> 
> > Then the difference is simply CRC vs not-CRC magic.  This can also
> > be determined inside the called function by passing in a btree number,
> > and looking up the proper magic in the xfs_magics array.
> > 
> > patch2 makes xfs_btree_magic() more generic, taking a 
> > crc flag & btnum, instead of just a cursor.
> > 
> > patch3 then makes use of xfs_btree_magic in the block init routine
> > to determine the proper magic.
> > 
> > With those changes, the if/else can go away, and we can simply call:
> > 
> > xfs_btree_init_block(mp, bp, XFS_BTNUM_BNO, 0, 1, agno, 0);
> > 
> > and let the lower function sort out crc vs. no crc differences
> > by itself.
> > 
> > It's not a huge reduction in code, but it reduces a lot of the
> > boilerplate, particularly in growfs code.  This may also make it
> > slightly easier to factor the growfs tree init code into a loop
> > (not sure about that, yet) and/or to move mkfs & growfs tree init
> > code into a libxfs/ call.
> > 
> >  libxfs/xfs_bmap.c       |   19 ++++---------------
> >  libxfs/xfs_bmap_btree.c |   10 ++--------
> >  libxfs/xfs_btree.c      |   34 +++++++++++++++++++++-------------
> >  libxfs/xfs_btree.h      |    4 ++--
> >  libxfs/xfs_types.h      |    2 ++
> >  xfs_fsops.c             |   39 +++++++++------------------------------
> >  6 files changed, 40 insertions(+), 68 deletions(-)
> > 
> > -Eric
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html