[Cluster-devel] [PATCH 3/3] gfs2: Don't create unnecessary indirect blocks

[Andreas Gruenbacher]

When gfs2 increases the height of an inode, it always creates an
indirect block for each the new level of indirection, even when the
inode is entirely empty.  For example, these commands:

  $ mkfs.gfs2 -O -b 4096 -p lock_nolock /dev/vdb
  $ mount /dev/vdb /mnt/test/
  $ xfs_io -f -c 'truncate 0' -c 'pwrite 509b 4k' /mnt/test/foo

will create a pointer to an entirely empty indirect block.  This is
unnecessary, so fix the code to avoid that.  While at it, clean things
up and add some more documentation.

Signed-off-by: Andreas Gruenbacher <agruenba at redhat.com>
---
 fs/gfs2/bmap.c | 62 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 45 insertions(+), 17 deletions(-)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index caa0b6d71bce..5d4788b7789e 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -247,13 +247,6 @@ static void find_metapath(const struct gfs2_sbd *sdp, u64 block,
 		mp->mp_list[i] = do_div(block, sdp->sd_inptrs);
 }
 
-static inline unsigned int metapath_branch_start(const struct metapath *mp)
-{
-	if (mp->mp_list[0] == 0)
-		return 2;
-	return 1;
-}
-
 /**
  * metaptr1 - Return the first possible metadata pointer in a metapath buffer
  * @height: The metadata height (0 = dinode)
@@ -592,6 +585,28 @@ static void gfs2_indirect_set(struct metapath *mp, unsigned int i,
 	*ptr = cpu_to_be64(bn);
 }
 
+/*
+ * gfs2_inode_contains_ptrs - check if inode contain pointers
+ *
+ * Return true if the inode contains data or indirect pointers.
+ */
+static bool gfs2_inode_contains_ptrs(struct inode *inode,
+				     struct buffer_head *dibh)
+{
+	struct gfs2_inode *ip = GFS2_I(inode);
+
+	BUG_ON(gfs2_is_stuffed(ip));
+
+	/*
+	 * At the very least, the inode consumes one block plus for the inode
+	 * itself and possibly one block for xattrs.  Any additional blocks are
+	 * used for data or indirect blocks, or for xattr data.
+	 */
+	return (gfs2_get_inode_blocks(inode) > 1 + !!ip->i_eattr) ||
+	       memchr_inv(dibh->b_data + sizeof(struct gfs2_dinode), 0,
+			  dibh->b_size - sizeof(struct gfs2_dinode));
+}
+
 enum alloc_state {
 	ALLOC_DATA = 0,
 	ALLOC_GROW_DEPTH = 1,
@@ -636,7 +651,8 @@ static int gfs2_iomap_alloc(struct inode *inode, struct iomap *iomap,
 	struct gfs2_sbd *sdp = GFS2_SB(inode);
 	struct buffer_head *dibh = mp->mp_bh[0];
 	u64 bn;
-	unsigned n, i, blks, alloced = 0, iblks = 0, branch_start = 0;
+	unsigned n, i, blks, alloced = 0, iblks = 0;
+	bool overlap = false;
 	size_t dblks = iomap->length >> inode->i_blkbits;
 	const unsigned end_of_metadata = mp->mp_fheight - 1;
 	int ret;
@@ -657,16 +673,28 @@ static int gfs2_iomap_alloc(struct inode *inode, struct iomap *iomap,
 		state = ALLOC_DATA;
 	} else {
 		/* Need to allocate indirect blocks */
-		if (mp->mp_fheight == ip->i_height) {
-			/* Writing into existing tree, extend tree down */
-			iblks = mp->mp_fheight - mp->mp_aheight;
+		iblks = mp->mp_fheight - mp->mp_aheight;
+		/*
+		 * If the height doesn't increase or the inode doesn't contain
+		 * any pointers, we can go straight to extending the tree down.
+		 */
+		if (mp->mp_fheight == ip->i_height ||
+		    !gfs2_inode_contains_ptrs(inode, dibh)) {
+			/* Extend tree down */
 			state = ALLOC_GROW_DEPTH;
 		} else {
-			/* Building up tree height */
+			/* Build up tree height */
 			state = ALLOC_GROW_HEIGHT;
-			iblks = mp->mp_fheight - ip->i_height;
-			branch_start = metapath_branch_start(mp);
-			iblks += (mp->mp_fheight - branch_start);
+			iblks += mp->mp_fheight - ip->i_height;
+			if (mp->mp_list[0] == 0) {
+				/*
+				 * The metapath for growing the height and the
+				 * metapath for the new allocation start with
+				 * the same block.
+				 */
+				overlap = true;
+				iblks--;
+			}
 		}
 	}
 
@@ -707,13 +735,13 @@ static int gfs2_iomap_alloc(struct inode *inode, struct iomap *iomap,
 					sizeof(struct gfs2_meta_header));
 				*ptr = zero_bn;
 				state = ALLOC_GROW_DEPTH;
-				for(i = branch_start; i < mp->mp_fheight; i++) {
+				for(i = 1 + overlap; i < mp->mp_fheight; i++) {
 					if (mp->mp_bh[i] == NULL)
 						break;
 					brelse(mp->mp_bh[i]);
 					mp->mp_bh[i] = NULL;
 				}
-				i = branch_start;
+				i = 1 + overlap;
 			}
 			if (n == 0)
 				break;
-- 
2.17.1