[Cluster-devel] [PATCH 0/8] Get rid of gfs2_write_calc_reserv in gfs2_iomap_begin_write

[Cluster-devel] [PATCH 0/8] Get rid of gfs2_write_calc_reserv in gfs2_iomap_begin_write

[Andreas Gruenbacher]

Following several cleanups, this is a first step towards being more
accurate in the number of blocks we reserve in the journal.  I hope this
part of the code is easier to understand now as well.

Thanks,
Andreas

Andreas Gruenbacher (8):
  gfs2: Don't depend on mp_aheight in clone_metapath
  gfs2: Split gfs2_indirect_init
  gfs2: Move empty inode check out of gfs2_unstuff_dinode
  gfs2: Don't create unnecessary indirect blocks
  gfs2: Improve gfs2_alloc_size for stuffed files
  gfs2: Rename size to len in gfs2_alloc_size
  gfs2: Move gfs2_alloc_size out of gfs2_iomap_get
  gfs2: Get rid of gfs2_write_calc_reserv in gfs2_iomap_begin_write

 fs/gfs2/bmap.c | 336 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 205 insertions(+), 131 deletions(-)

-- 
2.17.1

[Cluster-devel] [PATCH 1/8] gfs2: Don't depend on mp_aheight in clone_metapath

[Andreas Gruenbacher]

Change clone_metapath to work the same way as release_metapath.  (This
is merely a clean-up.)

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/bmap.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 03128ed1f34e..a564cf0b7221 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -294,8 +294,11 @@ static void clone_metapath(struct metapath *clone, struct metapath *mp)
 	unsigned int hgt;
 
 	*clone = *mp;
-	for (hgt = 0; hgt < mp->mp_aheight; hgt++)
+	for (hgt = 0; hgt < GFS2_MAX_META_HEIGHT; hgt++) {
+		if (mp->mp_bh[hgt] == NULL)
+			break;
 		get_bh(clone->mp_bh[hgt]);
+	}
 }
 
 static void gfs2_metapath_ra(struct gfs2_glock *gl, __be64 *start, __be64 *end)
-- 
2.17.1

[Cluster-devel] [PATCH 2/8] gfs2: Split gfs2_indirect_init

[Andreas Gruenbacher]

Function gfs2_indirect_init currently initializes a new indirect block
and sets up the pointer to the new block in the parent indirect block.
This is easier to understand when done in two steps.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/bmap.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index a564cf0b7221..caa0b6d71bce 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -571,22 +571,25 @@ static int gfs2_hole_size(struct inode *inode, sector_t lblock, u64 len,
 	return ret;
 }
 
-static inline __be64 *gfs2_indirect_init(struct metapath *mp,
-					 struct gfs2_glock *gl, unsigned int i,
-					 unsigned offset, u64 bn)
+static void gfs2_indirect_init(struct metapath *mp, struct gfs2_glock *gl,
+			       unsigned int i, u64 bn)
 {
-	__be64 *ptr = (__be64 *)(mp->mp_bh[i - 1]->b_data +
-		       ((i > 1) ? sizeof(struct gfs2_meta_header) :
-				 sizeof(struct gfs2_dinode)));
-	BUG_ON(i < 1);
 	BUG_ON(mp->mp_bh[i] != NULL);
 	mp->mp_bh[i] = gfs2_meta_new(gl, bn);
 	gfs2_trans_add_meta(gl, mp->mp_bh[i]);
 	gfs2_metatype_set(mp->mp_bh[i], GFS2_METATYPE_IN, GFS2_FORMAT_IN);
 	gfs2_buffer_clear_tail(mp->mp_bh[i], sizeof(struct gfs2_meta_header));
+}
+
+static void gfs2_indirect_set(struct metapath *mp, unsigned int i,
+			      unsigned offset, u64 bn)
+{
+	__be64 *ptr = (__be64 *)(mp->mp_bh[i]->b_data +
+		       (i ? sizeof(struct gfs2_meta_header) :
+			    sizeof(struct gfs2_dinode)));
+
 	ptr += offset;
 	*ptr = cpu_to_be64(bn);
-	return ptr;
 }
 
 enum alloc_state {
@@ -688,8 +691,10 @@ static int gfs2_iomap_alloc(struct inode *inode, struct iomap *iomap,
 				zero_bn = *ptr;
 			}
 			for (; i - 1 < mp->mp_fheight - ip->i_height && n > 0;
-			     i++, n--)
-				gfs2_indirect_init(mp, ip->i_gl, i, 0, bn++);
+			     i++, n--, bn++) {
+				gfs2_indirect_init(mp, ip->i_gl, i, bn);
+				gfs2_indirect_set(mp, i - 1, 0, bn);
+			}
 			if (i - 1 == mp->mp_fheight - ip->i_height) {
 				i--;
 				gfs2_buffer_copy_tail(mp->mp_bh[i],
@@ -716,9 +721,10 @@ static int gfs2_iomap_alloc(struct inode *inode, struct iomap *iomap,
 		case ALLOC_GROW_DEPTH:
 			if (i > 1 && i < mp->mp_fheight)
 				gfs2_trans_add_meta(ip->i_gl, mp->mp_bh[i-1]);
-			for (; i < mp->mp_fheight && n > 0; i++, n--)
-				gfs2_indirect_init(mp, ip->i_gl, i,
-						   mp->mp_list[i-1], bn++);
+			for (; i < mp->mp_fheight && n > 0; i++, n--, bn++) {
+				gfs2_indirect_init(mp, ip->i_gl, i, bn);
+				gfs2_indirect_set(mp, i - 1, mp->mp_list[i - 1], bn);
+			}
 			if (i == mp->mp_fheight)
 				state = ALLOC_DATA;
 			if (n == 0)
-- 
2.17.1

[Cluster-devel] [PATCH 3/8] gfs2: Move empty inode check out of gfs2_unstuff_dinode

[Andreas Gruenbacher]

Split up gfs2_unstuff_dinode so that the empty inode check it contains
can be moved out of there and into a new gfs2_inode_contains_data
helper.  We want to use the same check in gfs2_unstuff_dinode that we
use for determining how many blocks need to be allocated and reserved.
The gfs2_inode_contains_data helper currently still only treats stuffed,
zero-length inodes as empty, but other criteria like does the inode
contain and direct or indirect pointers could be used.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/bmap.c | 90 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 58 insertions(+), 32 deletions(-)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index caa0b6d71bce..1963c730e0be 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -44,6 +44,18 @@ struct metapath {
 
 static int punch_hole(struct gfs2_inode *ip, u64 offset, u64 length);
 
+/*
+ * gfs2_inode_contains_data - check if inode contains any data
+ *
+ * Return true if the inode contains (or may contain) data or indirect blocks.
+ */
+static bool gfs2_inode_contains_data(struct inode *inode)
+{
+	struct gfs2_inode *ip = GFS2_I(inode);
+
+	return !gfs2_is_stuffed(ip) || i_size_read(inode) > 0;
+}
+
 /**
  * gfs2_unstuffer_page - unstuff a stuffed inode into a block cached by a page
  * @ip: the inode
@@ -107,20 +119,10 @@ static int gfs2_unstuffer_page(struct gfs2_inode *ip, struct buffer_head *dibh,
 	return 0;
 }
 
-/**
- * gfs2_unstuff_dinode - Unstuff a dinode when the data has grown too big
- * @ip: The GFS2 inode to unstuff
- * @page: The (optional) page. This is looked up if the @page is NULL
- *
- * This routine unstuffs a dinode and returns it to a "normal" state such
- * that the height can be grown in the traditional way.
- *
- * Returns: errno
- */
-
-int gfs2_unstuff_dinode(struct gfs2_inode *ip, struct page *page)
+static int __gfs2_unstuff_dinode(struct gfs2_inode *ip, struct page *page,
+				 struct buffer_head *dibh, bool contains_data)
 {
-	struct buffer_head *bh, *dibh;
+	struct buffer_head *bh;
 	struct gfs2_dinode *di;
 	u64 block = 0;
 	int isdir = gfs2_is_dir(ip);
@@ -128,30 +130,26 @@ int gfs2_unstuff_dinode(struct gfs2_inode *ip, struct page *page)
 
 	down_write(&ip->i_rw_mutex);
 
-	error = gfs2_meta_inode_buffer(ip, &dibh);
-	if (error)
-		goto out;
-
-	if (i_size_read(&ip->i_inode)) {
+	if (contains_data) {
 		/* Get a free block, fill it with the stuffed data,
 		   and write it out to disk */
 
 		unsigned int n = 1;
 		error = gfs2_alloc_blocks(ip, &block, &n, 0, NULL);
 		if (error)
-			goto out_brelse;
+			goto out;
 		if (isdir) {
 			gfs2_trans_add_unrevoke(GFS2_SB(&ip->i_inode), block, 1);
 			error = gfs2_dir_get_new_buffer(ip, block, &bh);
 			if (error)
-				goto out_brelse;
+				goto out;
 			gfs2_buffer_copy_tail(bh, sizeof(struct gfs2_meta_header),
 					      dibh, sizeof(struct gfs2_dinode));
 			brelse(bh);
 		} else {
 			error = gfs2_unstuffer_page(ip, dibh, block, page);
 			if (error)
-				goto out_brelse;
+				goto out;
 		}
 	}
 
@@ -161,7 +159,7 @@ int gfs2_unstuff_dinode(struct gfs2_inode *ip, struct page *page)
 	di = (struct gfs2_dinode *)dibh->b_data;
 	gfs2_buffer_clear_tail(dibh, sizeof(struct gfs2_dinode));
 
-	if (i_size_read(&ip->i_inode)) {
+	if (contains_data) {
 		*(__be64 *)(di + 1) = cpu_to_be64(block);
 		gfs2_add_inode_blocks(&ip->i_inode, 1);
 		di->di_blocks = cpu_to_be64(gfs2_get_inode_blocks(&ip->i_inode));
@@ -170,13 +168,35 @@ int gfs2_unstuff_dinode(struct gfs2_inode *ip, struct page *page)
 	ip->i_height = 1;
 	di->di_height = cpu_to_be16(1);
 
-out_brelse:
-	brelse(dibh);
 out:
 	up_write(&ip->i_rw_mutex);
 	return error;
 }
 
+/**
+ * gfs2_unstuff_dinode - Unstuff a dinode when the data has grown too big
+ * @ip: The GFS2 inode to unstuff
+ * @page: The (optional) page. This is looked up if the @page is NULL
+ *
+ * This routine unstuffs a dinode and returns it to a "normal" state such
+ * that the height can be grown in the traditional way.
+ *
+ * Returns: errno
+ */
+
+int gfs2_unstuff_dinode(struct gfs2_inode *ip, struct page *page)
+{
+	bool contains_data = gfs2_inode_contains_data(&ip->i_inode);
+	struct buffer_head *dibh;
+	int error;
+
+	error = gfs2_meta_inode_buffer(ip, &dibh);
+	if (error)
+		return error;
+	error = __gfs2_unstuff_dinode(ip, page, dibh, contains_data);
+	brelse(dibh);
+	return error;
+}
 
 /**
  * find_metapath - Find path through the metadata tree
@@ -1044,7 +1064,10 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
 		goto out_trans_fail;
 
 	if (unstuff) {
-		ret = gfs2_unstuff_dinode(ip, NULL);
+		bool contains_data = gfs2_inode_contains_data(inode);
+		struct buffer_head *dibh = mp.mp_bh[0];
+
+		ret = __gfs2_unstuff_dinode(ip, NULL, dibh, contains_data);
 		if (ret)
 			goto out_trans_end;
 		release_metapath(&mp);
@@ -2076,7 +2099,7 @@ static int do_grow(struct inode *inode, u64 size)
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_sbd *sdp = GFS2_SB(inode);
 	struct gfs2_alloc_parms ap = { .target = 1, };
-	struct buffer_head *dibh;
+	struct buffer_head *dibh = NULL;
 	int error;
 	int unstuff = 0;
 
@@ -2097,23 +2120,26 @@ static int do_grow(struct inode *inode, u64 size)
 	if (error)
 		goto do_grow_release;
 
+	error = gfs2_meta_inode_buffer(ip, &dibh);
+	if (error)
+		goto do_end_trans;
+
 	if (unstuff) {
-		error = gfs2_unstuff_dinode(ip, NULL);
+		bool contains_data = gfs2_inode_contains_data(inode);
+
+		error = __gfs2_unstuff_dinode(ip, NULL, dibh, contains_data);
 		if (error)
 			goto do_end_trans;
 	}
 
-	error = gfs2_meta_inode_buffer(ip, &dibh);
-	if (error)
-		goto do_end_trans;
-
 	i_size_write(inode, size);
 	ip->i_inode.i_mtime = ip->i_inode.i_ctime = current_time(&ip->i_inode);
 	gfs2_trans_add_meta(ip->i_gl, dibh);
 	gfs2_dinode_out(ip, dibh->b_data);
-	brelse(dibh);
 
 do_end_trans:
+	if (dibh)
+		brelse(dibh);
 	gfs2_trans_end(sdp);
 do_grow_release:
 	if (unstuff) {
-- 
2.17.1

[Cluster-devel] [PATCH 4/8] 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 'pwrite 509b 1b' /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@redhat.com>
---
 fs/gfs2/bmap.c | 61 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 21 deletions(-)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 1963c730e0be..b0cdd606be13 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -267,13 +267,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)
@@ -650,13 +643,14 @@ enum alloc_state {
  */
 
 static int gfs2_iomap_alloc(struct inode *inode, struct iomap *iomap,
-			    unsigned flags, struct metapath *mp)
+			    unsigned flags, struct metapath *mp,
+			    bool contains_data)
 {
 	struct gfs2_inode *ip = GFS2_I(inode);
 	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;
 	size_t dblks = iomap->length >> inode->i_blkbits;
 	const unsigned end_of_metadata = mp->mp_fheight - 1;
 	int ret;
@@ -677,16 +671,26 @@ 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 || !contains_data) {
+			/* 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; only allocate it once.
+				 */
+				iblks--;
+			}
 		}
 	}
 
@@ -716,6 +720,8 @@ static int gfs2_iomap_alloc(struct inode *inode, struct iomap *iomap,
 				gfs2_indirect_set(mp, i - 1, 0, bn);
 			}
 			if (i - 1 == mp->mp_fheight - ip->i_height) {
+				unsigned int branch_start;
+
 				i--;
 				gfs2_buffer_copy_tail(mp->mp_bh[i],
 						sizeof(struct gfs2_meta_header),
@@ -727,6 +733,15 @@ static int gfs2_iomap_alloc(struct inode *inode, struct iomap *iomap,
 					sizeof(struct gfs2_meta_header));
 				*ptr = zero_bn;
 				state = ALLOC_GROW_DEPTH;
+				/*
+				 * If the metapath for growing the height and
+				 * the metapath for the new allocation start
+				 * with the same block (mp->mp_list[0] == 0),
+				 * set things up so that ALLOC_GROW_DEPTH will
+				 * skip the level that was already allocated
+				 * here.
+				 */
+				branch_start = 1 + (mp->mp_list[0] == 0);
 				for(i = branch_start; i < mp->mp_fheight; i++) {
 					if (mp->mp_bh[i] == NULL)
 						break;
@@ -803,6 +818,7 @@ static u64 gfs2_alloc_size(struct inode *inode, struct metapath *mp, u64 size)
 	if (gfs2_is_stuffed(ip) || mp->mp_fheight != mp->mp_aheight) {
 		unsigned int maxsize = mp->mp_fheight > 1 ?
 			sdp->sd_inptrs : sdp->sd_diptrs;
+
 		maxsize -= mp->mp_list[mp->mp_fheight - 1];
 		if (size > maxsize)
 			size = maxsize;
@@ -1015,7 +1031,7 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_sbd *sdp = GFS2_SB(inode);
 	unsigned int data_blocks = 0, ind_blocks = 0, rblocks;
-	bool unstuff, alloc_required;
+	bool contains_data, unstuff, alloc_required;
 	int ret;
 
 	ret = gfs2_write_lock(inode);
@@ -1025,6 +1041,8 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
 	unstuff = gfs2_is_stuffed(ip) &&
 		  pos + length > gfs2_max_stuffed_size(ip);
 
+	contains_data = gfs2_inode_contains_data(inode);
+
 	ret = gfs2_iomap_get(inode, pos, length, flags, iomap, &mp);
 	if (ret)
 		goto out_release;
@@ -1064,7 +1082,6 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
 		goto out_trans_fail;
 
 	if (unstuff) {
-		bool contains_data = gfs2_inode_contains_data(inode);
 		struct buffer_head *dibh = mp.mp_bh[0];
 
 		ret = __gfs2_unstuff_dinode(ip, NULL, dibh, contains_data);
@@ -1080,7 +1097,7 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
 	}
 
 	if (iomap->type == IOMAP_HOLE) {
-		ret = gfs2_iomap_alloc(inode, iomap, flags, &mp);
+		ret = gfs2_iomap_alloc(inode, iomap, flags, &mp, contains_data);
 		if (ret) {
 			gfs2_trans_end(sdp);
 			gfs2_inplace_release(ip);
@@ -1230,7 +1247,8 @@ int gfs2_block_map(struct inode *inode, sector_t lblock,
 	if (create) {
 		ret = gfs2_iomap_get(inode, pos, length, IOMAP_WRITE, &iomap, &mp);
 		if (!ret && iomap.type == IOMAP_HOLE)
-			ret = gfs2_iomap_alloc(inode, &iomap, IOMAP_WRITE, &mp);
+			ret = gfs2_iomap_alloc(inode, &iomap, IOMAP_WRITE, &mp,
+					       gfs2_inode_contains_data(inode));
 		release_metapath(&mp);
 	} else {
 		ret = gfs2_iomap_get(inode, pos, length, 0, &iomap, &mp);
@@ -1460,7 +1478,8 @@ int gfs2_iomap_get_alloc(struct inode *inode, loff_t pos, loff_t length,
 
 	ret = gfs2_iomap_get(inode, pos, length, IOMAP_WRITE, iomap, &mp);
 	if (!ret && iomap->type == IOMAP_HOLE)
-		ret = gfs2_iomap_alloc(inode, iomap, IOMAP_WRITE, &mp);
+		ret = gfs2_iomap_alloc(inode, iomap, IOMAP_WRITE, &mp,
+				       gfs2_inode_contains_data(inode));
 	release_metapath(&mp);
 	return ret;
 }
-- 
2.17.1

[Cluster-devel] [PATCH 5/8] gfs2: Improve gfs2_alloc_size for stuffed files

[Andreas Gruenbacher]

In gfs2_alloc_size, compute exactly which effect unstuffing will have on
a write: if unstuffing will allocate block 0 and the write starts at
block 0, only that block can be written in the first iomap operation; if
the write starts above block 0, it won't be affected by the unstuffing.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/bmap.c | 49 ++++++++++++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index b0cdd606be13..7d5fa46f648b 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -795,45 +795,44 @@ static int gfs2_iomap_alloc(struct inode *inode, struct iomap *iomap,
  * gfs2_alloc_size - Compute the maximum allocation size
  * @inode: The inode
  * @mp: The metapath
- * @size: Requested size in blocks
+ * @iomap: The iomap
  *
  * Compute the maximum size of the next allocation at @mp.
  *
  * Returns: size in blocks
  */
-static u64 gfs2_alloc_size(struct inode *inode, struct metapath *mp, u64 size)
+static u64 gfs2_alloc_size(struct inode *inode, struct metapath *mp,
+			   struct iomap *iomap)
 {
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_sbd *sdp = GFS2_SB(inode);
-	const __be64 *first, *ptr, *end;
-
-	/*
-	 * For writes to stuffed files, this function is called twice via
-	 * gfs2_iomap_get, before and after unstuffing. The size we return the
-	 * first time needs to be large enough to get the reservation and
-	 * allocation sizes right.  The size we return the second time must
-	 * be exact or else gfs2_iomap_alloc won't do the right thing.
-	 */
+	u64 size = iomap->length >> inode->i_blkbits;
 
 	if (gfs2_is_stuffed(ip) || mp->mp_fheight != mp->mp_aheight) {
-		unsigned int maxsize = mp->mp_fheight > 1 ?
-			sdp->sd_inptrs : sdp->sd_diptrs;
+		unsigned int maxsize;
 
+		maxsize = mp->mp_fheight > 1 ? sdp->sd_inptrs : sdp->sd_diptrs;
 		maxsize -= mp->mp_list[mp->mp_fheight - 1];
+		if (gfs2_inode_contains_data(inode)) {
+			if (iomap->offset == 0)
+				maxsize = 1;
+		}
 		if (size > maxsize)
 			size = maxsize;
-		return size;
-	}
-
-	first = metapointer(ip->i_height - 1, mp);
-	end = metaend(ip->i_height - 1, mp);
-	if (end - first > size)
-		end = first + size;
-	for (ptr = first; ptr < end; ptr++) {
-		if (*ptr)
-			break;
+	} else {
+		const __be64 *first, *ptr, *end;
+
+		first = metapointer(ip->i_height - 1, mp);
+		end = metaend(ip->i_height - 1, mp);
+		if (end - first > size)
+			end = first + size;
+		for (ptr = first; ptr < end; ptr++) {
+			if (*ptr)
+				break;
+		}
+		size = ptr - first;
 	}
-	return ptr - first;
+	return size;
 }
 
 /**
@@ -963,7 +962,7 @@ static int gfs2_iomap_get(struct inode *inode, loff_t pos, loff_t length,
 		if (flags & IOMAP_DIRECT)
 			goto out;  /* (see gfs2_file_direct_write) */
 
-		len = gfs2_alloc_size(inode, mp, len);
+		len = gfs2_alloc_size(inode, mp, iomap);
 		alloc_size = len << inode->i_blkbits;
 		if (alloc_size < iomap->length)
 			iomap->length = alloc_size;
-- 
2.17.1

[Cluster-devel] [PATCH 6/8] gfs2: Rename size to len in gfs2_alloc_size

[Andreas Gruenbacher]

A simple variable rename.

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

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 7d5fa46f648b..5168e5c39d49 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -806,33 +806,33 @@ static u64 gfs2_alloc_size(struct inode *inode, struct metapath *mp,
 {
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_sbd *sdp = GFS2_SB(inode);
-	u64 size = iomap->length >> inode->i_blkbits;
+	u64 len = iomap->length >> inode->i_blkbits;
 
 	if (gfs2_is_stuffed(ip) || mp->mp_fheight != mp->mp_aheight) {
-		unsigned int maxsize;
+		unsigned int maxlen;
 
-		maxsize = mp->mp_fheight > 1 ? sdp->sd_inptrs : sdp->sd_diptrs;
-		maxsize -= mp->mp_list[mp->mp_fheight - 1];
+		maxlen = mp->mp_fheight > 1 ? sdp->sd_inptrs : sdp->sd_diptrs;
+		maxlen -= mp->mp_list[mp->mp_fheight - 1];
 		if (gfs2_inode_contains_data(inode)) {
 			if (iomap->offset == 0)
-				maxsize = 1;
+				maxlen = 1;
 		}
-		if (size > maxsize)
-			size = maxsize;
+		if (len > maxlen)
+			len = maxlen;
 	} else {
 		const __be64 *first, *ptr, *end;
 
 		first = metapointer(ip->i_height - 1, mp);
 		end = metaend(ip->i_height - 1, mp);
-		if (end - first > size)
-			end = first + size;
+		if (end - first > len)
+			end = first + len;
 		for (ptr = first; ptr < end; ptr++) {
 			if (*ptr)
 				break;
 		}
-		size = ptr - first;
+		len = ptr - first;
 	}
-	return size;
+	return len;
 }
 
 /**
-- 
2.17.1

[Cluster-devel] [PATCH 7/8] gfs2: Move gfs2_alloc_size out of gfs2_iomap_get

[Andreas Gruenbacher]

Move gfs2_alloc_size out of gfs2_iomap_get and rename it to
gfs2_compute_alloc_size.  This simplifies gfs2_iomap_get, and will allow
to pass additional parameters to gfs2_alloc_size that are not relevant
to gfs2_iomap_get later.  In addition, gfs2_alloc_size is now no longer
called again unnecessarily after unstuffing in gfs2_iomap_begin_write.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/bmap.c | 36 ++++++++++++++----------------------
 1 file changed, 14 insertions(+), 22 deletions(-)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 5168e5c39d49..66ecc4439e77 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -624,9 +624,6 @@ enum alloc_state {
  *  ii) Indirect blocks to fill in lower part of the metadata tree
  * iii) Data blocks
  *
- * This function is called after gfs2_iomap_get, which works out the
- * total number of blocks which we need via gfs2_alloc_size.
- *
  * We then do the actual allocation asking for an extent at a time (if
  * enough contiguous free blocks are available, there will only be one
  * allocation request per call) and uses the state machine to initialise
@@ -792,17 +789,15 @@ static int gfs2_iomap_alloc(struct inode *inode, struct iomap *iomap,
 #define IOMAP_F_GFS2_BOUNDARY IOMAP_F_PRIVATE
 
 /**
- * gfs2_alloc_size - Compute the maximum allocation size
+ * gfs2_compute_alloc_size - Compute the maximum allocation size
  * @inode: The inode
  * @mp: The metapath
  * @iomap: The iomap
  *
  * Compute the maximum size of the next allocation at @mp.
- *
- * Returns: size in blocks
  */
-static u64 gfs2_alloc_size(struct inode *inode, struct metapath *mp,
-			   struct iomap *iomap)
+static void gfs2_compute_alloc_size(struct inode *inode, struct metapath *mp,
+				    struct iomap *iomap)
 {
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_sbd *sdp = GFS2_SB(inode);
@@ -832,7 +827,7 @@ static u64 gfs2_alloc_size(struct inode *inode, struct metapath *mp,
 		}
 		len = ptr - first;
 	}
-	return len;
+	iomap->length = len << inode->i_blkbits;
 }
 
 /**
@@ -956,17 +951,7 @@ static int gfs2_iomap_get(struct inode *inode, loff_t pos, loff_t length,
 			ret = gfs2_hole_size(inode, lblock, len, mp, iomap);
 		else
 			iomap->length = size - pos;
-	} else if (flags & IOMAP_WRITE) {
-		u64 alloc_size;
-
-		if (flags & IOMAP_DIRECT)
-			goto out;  /* (see gfs2_file_direct_write) */
-
-		len = gfs2_alloc_size(inode, mp, iomap);
-		alloc_size = len << inode->i_blkbits;
-		if (alloc_size < iomap->length)
-			iomap->length = alloc_size;
-	} else {
+	} else if (!(flags & IOMAP_WRITE)) {
 		if (pos < size && height == ip->i_height)
 			ret = gfs2_hole_size(inode, lblock, len, mp, iomap);
 	}
@@ -1048,6 +1033,9 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
 
 	alloc_required = unstuff || iomap->type == IOMAP_HOLE;
 
+	if (iomap->type == IOMAP_HOLE)
+		gfs2_compute_alloc_size(inode, &mp, iomap);
+
 	if (alloc_required || gfs2_is_jdata(ip))
 		gfs2_write_calc_reserv(ip, iomap->length, &data_blocks,
 				       &ind_blocks);
@@ -1245,9 +1233,11 @@ int gfs2_block_map(struct inode *inode, sector_t lblock,
 
 	if (create) {
 		ret = gfs2_iomap_get(inode, pos, length, IOMAP_WRITE, &iomap, &mp);
-		if (!ret && iomap.type == IOMAP_HOLE)
+		if (!ret && iomap.type == IOMAP_HOLE) {
+			gfs2_compute_alloc_size(inode, &mp, &iomap);
 			ret = gfs2_iomap_alloc(inode, &iomap, IOMAP_WRITE, &mp,
 					       gfs2_inode_contains_data(inode));
+		}
 		release_metapath(&mp);
 	} else {
 		ret = gfs2_iomap_get(inode, pos, length, 0, &iomap, &mp);
@@ -1476,9 +1466,11 @@ int gfs2_iomap_get_alloc(struct inode *inode, loff_t pos, loff_t length,
 	int ret;
 
 	ret = gfs2_iomap_get(inode, pos, length, IOMAP_WRITE, iomap, &mp);
-	if (!ret && iomap->type == IOMAP_HOLE)
+	if (!ret && iomap->type == IOMAP_HOLE) {
+		gfs2_compute_alloc_size(inode, &mp, iomap);
 		ret = gfs2_iomap_alloc(inode, iomap, IOMAP_WRITE, &mp,
 				       gfs2_inode_contains_data(inode));
+	}
 	release_metapath(&mp);
 	return ret;
 }
-- 
2.17.1

[Cluster-devel] [PATCH 8/8] gfs2: Get rid of gfs2_write_calc_reserv in gfs2_iomap_begin_write

[Andreas Gruenbacher]

Compute the number of blocks to allocate in gfs2_compute_alloc_size.
This allows to get rid of gfs2_write_calc_reserv in
gfs2_iomap_begin_write.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/bmap.c | 113 +++++++++++++++++++++++++++++++------------------
 1 file changed, 71 insertions(+), 42 deletions(-)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 66ecc4439e77..0ccabf925afd 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -32,14 +32,15 @@
 #include "trace_gfs2.h"
 
 /* This doesn't need to be that large as max 64 bit pointers in a 4k
- * block is 512, so __u16 is fine for that. It saves stack space to
+ * block is 512, so u16 is fine for that. It saves stack space to
  * keep it small.
  */
 struct metapath {
 	struct buffer_head *mp_bh[GFS2_MAX_META_HEIGHT];
-	__u16 mp_list[GFS2_MAX_META_HEIGHT];
-	int mp_fheight; /* find_metapath height */
-	int mp_aheight; /* actual height (lookup height) */
+	u16 mp_list[GFS2_MAX_META_HEIGHT];
+	u16 mp_data_blocks, mp_ind_blocks;
+	u8 mp_fheight; /* find_metapath height */
+	u8 mp_aheight; /* actual height (lookup height) */
 };
 
 static int punch_hole(struct gfs2_inode *ip, u64 offset, u64 length);
@@ -647,8 +648,7 @@ 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;
-	size_t dblks = iomap->length >> inode->i_blkbits;
+	unsigned n, i, blks, alloced = 0;
 	const unsigned end_of_metadata = mp->mp_fheight - 1;
 	int ret;
 	enum alloc_state state;
@@ -657,7 +657,7 @@ static int gfs2_iomap_alloc(struct inode *inode, struct iomap *iomap,
 
 	BUG_ON(mp->mp_aheight < 1);
 	BUG_ON(dibh == NULL);
-	BUG_ON(dblks < 1);
+	BUG_ON(mp->mp_data_blocks < 1);
 
 	gfs2_trans_add_meta(ip->i_gl, dibh);
 
@@ -668,32 +668,18 @@ static int gfs2_iomap_alloc(struct inode *inode, struct iomap *iomap,
 		state = ALLOC_DATA;
 	} else {
 		/* Need to allocate indirect blocks */
-		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 || !contains_data) {
 			/* Extend tree down */
 			state = ALLOC_GROW_DEPTH;
 		} else {
 			/* Build up tree height */
 			state = ALLOC_GROW_HEIGHT;
-			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; only allocate it once.
-				 */
-				iblks--;
-			}
 		}
 	}
 
 	/* start of the second part of the function (state machine) */
 
-	blks = dblks + iblks;
+	blks = mp->mp_data_blocks + mp->mp_ind_blocks;
 	i = mp->mp_aheight;
 	do {
 		n = blks - alloced;
@@ -763,12 +749,13 @@ static int gfs2_iomap_alloc(struct inode *inode, struct iomap *iomap,
 				break;
 		/* Tree complete, adding data blocks */
 		case ALLOC_DATA:
-			BUG_ON(n > dblks);
+			BUG_ON(n > mp->mp_data_blocks);
 			BUG_ON(mp->mp_bh[end_of_metadata] == NULL);
 			gfs2_trans_add_meta(ip->i_gl, mp->mp_bh[end_of_metadata]);
-			dblks = n;
 			ptr = metapointer(end_of_metadata, mp);
 			iomap->addr = bn << inode->i_blkbits;
+			iomap->length = (u64)n << inode->i_blkbits;
+			iomap->type = IOMAP_MAPPED;
 			iomap->flags |= IOMAP_F_MERGED | IOMAP_F_NEW;
 			while (n-- > 0)
 				*ptr++ = cpu_to_be64(bn++);
@@ -776,8 +763,6 @@ static int gfs2_iomap_alloc(struct inode *inode, struct iomap *iomap,
 		}
 	} while (iomap->addr == IOMAP_NULL_ADDR);
 
-	iomap->type = IOMAP_MAPPED;
-	iomap->length = (u64)dblks << inode->i_blkbits;
 	ip->i_height = mp->mp_fheight;
 	gfs2_add_inode_blocks(&ip->i_inode, alloced);
 	gfs2_dinode_out(ip, dibh->b_data);
@@ -793,16 +778,19 @@ static int gfs2_iomap_alloc(struct inode *inode, struct iomap *iomap,
  * @inode: The inode
  * @mp: The metapath
  * @iomap: The iomap
+ * @contains_data: False if the inode is known empty
  *
  * Compute the maximum size of the next allocation at @mp.
  */
 static void gfs2_compute_alloc_size(struct inode *inode, struct metapath *mp,
-				    struct iomap *iomap)
+				    struct iomap *iomap, bool contains_data)
 {
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_sbd *sdp = GFS2_SB(inode);
 	u64 len = iomap->length >> inode->i_blkbits;
 
+	mp->mp_data_blocks = 0;
+	mp->mp_ind_blocks = 0;
 	if (gfs2_is_stuffed(ip) || mp->mp_fheight != mp->mp_aheight) {
 		unsigned int maxlen;
 
@@ -811,9 +799,43 @@ static void gfs2_compute_alloc_size(struct inode *inode, struct metapath *mp,
 		if (gfs2_inode_contains_data(inode)) {
 			if (iomap->offset == 0)
 				maxlen = 1;
+			else if (gfs2_is_stuffed(ip))
+				mp->mp_data_blocks++;
 		}
 		if (len > maxlen)
 			len = maxlen;
+
+		if (mp->mp_fheight != mp->mp_aheight) {
+			unsigned int h;
+
+			/* Fill out the metadata tree to its full height. */
+			mp->mp_ind_blocks += mp->mp_fheight - mp->mp_aheight;
+
+			/*
+			 * When growing the height, going from height 0 to 1
+			 * requires no indirect blocks.  Going any further
+			 * requires one indirect block for each level.
+			 */
+			h = max_t(unsigned int, ip->i_height, 1);
+			if (mp->mp_fheight > h && contains_data) {
+				mp->mp_ind_blocks += mp->mp_fheight - h;
+				/*
+				 * If the metapath for growing the height and
+				 * the metapath for the new allocation start
+				 * with the same block, we only need to account
+				 * for that block once.
+				 *
+				 * (This kind of overlap is possible because
+				 * indirect blocks contain more pointers than
+				 * the inode.  When we grow the height, a write
+				 * that would have gone beyond the last pointer
+				 * in the inode may still go into the first
+				 * indirect block.)
+				 */
+				if (mp->mp_list[0] == 0)
+					mp->mp_ind_blocks--;
+			}
+		}
 	} else {
 		const __be64 *first, *ptr, *end;
 
@@ -827,6 +849,7 @@ static void gfs2_compute_alloc_size(struct inode *inode, struct metapath *mp,
 		}
 		len = ptr - first;
 	}
+	mp->mp_data_blocks += len;
 	iomap->length = len << inode->i_blkbits;
 }
 
@@ -1014,7 +1037,7 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
 	struct metapath mp = { .mp_aheight = 1, };
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_sbd *sdp = GFS2_SB(inode);
-	unsigned int data_blocks = 0, ind_blocks = 0, rblocks;
+	unsigned int rblocks;
 	bool contains_data, unstuff, alloc_required;
 	int ret;
 
@@ -1034,15 +1057,11 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
 	alloc_required = unstuff || iomap->type == IOMAP_HOLE;
 
 	if (iomap->type == IOMAP_HOLE)
-		gfs2_compute_alloc_size(inode, &mp, iomap);
-
-	if (alloc_required || gfs2_is_jdata(ip))
-		gfs2_write_calc_reserv(ip, iomap->length, &data_blocks,
-				       &ind_blocks);
+		gfs2_compute_alloc_size(inode, &mp, iomap, contains_data);
 
 	if (alloc_required) {
 		struct gfs2_alloc_parms ap = {
-			.target = data_blocks + ind_blocks
+			.target = mp.mp_data_blocks + mp.mp_ind_blocks
 		};
 
 		ret = gfs2_quota_lock_check(ip, &ap);
@@ -1054,15 +1073,16 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
 			goto out_qunlock;
 	}
 
-	rblocks = RES_DINODE + ind_blocks;
+	rblocks = RES_DINODE + mp.mp_ind_blocks;
 	if (gfs2_is_jdata(ip))
-		rblocks += data_blocks;
-	if (ind_blocks || data_blocks)
+		rblocks += mp.mp_data_blocks;
+	if (mp.mp_ind_blocks || mp.mp_data_blocks)
 		rblocks += RES_STATFS + RES_QUOTA;
 	if (inode == sdp->sd_rindex)
 		rblocks += 2 * RES_STATFS;
 	if (alloc_required)
-		rblocks += gfs2_rg_blocks(ip, data_blocks + ind_blocks);
+		rblocks += gfs2_rg_blocks(ip, mp.mp_data_blocks +
+					      mp.mp_ind_blocks);
 
 	ret = gfs2_trans_begin(sdp, rblocks, iomap->length >> inode->i_blkbits);
 	if (ret)
@@ -1074,6 +1094,10 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
 		ret = __gfs2_unstuff_dinode(ip, NULL, dibh, contains_data);
 		if (ret)
 			goto out_trans_end;
+		if (contains_data) {
+			BUG_ON(mp.mp_data_blocks < 1);
+			mp.mp_data_blocks--;
+		}
 		release_metapath(&mp);
 		brelse(iomap->private);
 		iomap->private = NULL;
@@ -1234,9 +1258,12 @@ int gfs2_block_map(struct inode *inode, sector_t lblock,
 	if (create) {
 		ret = gfs2_iomap_get(inode, pos, length, IOMAP_WRITE, &iomap, &mp);
 		if (!ret && iomap.type == IOMAP_HOLE) {
-			gfs2_compute_alloc_size(inode, &mp, &iomap);
+			bool contains_data = gfs2_inode_contains_data(inode);
+
+			gfs2_compute_alloc_size(inode, &mp, &iomap,
+						contains_data);
 			ret = gfs2_iomap_alloc(inode, &iomap, IOMAP_WRITE, &mp,
-					       gfs2_inode_contains_data(inode));
+					       contains_data);
 		}
 		release_metapath(&mp);
 	} else {
@@ -1467,9 +1494,11 @@ int gfs2_iomap_get_alloc(struct inode *inode, loff_t pos, loff_t length,
 
 	ret = gfs2_iomap_get(inode, pos, length, IOMAP_WRITE, iomap, &mp);
 	if (!ret && iomap->type == IOMAP_HOLE) {
-		gfs2_compute_alloc_size(inode, &mp, iomap);
+		bool contains_data = gfs2_inode_contains_data(inode);
+
+		gfs2_compute_alloc_size(inode, &mp, iomap, contains_data);
 		ret = gfs2_iomap_alloc(inode, iomap, IOMAP_WRITE, &mp,
-				       gfs2_inode_contains_data(inode));
+				       contains_data);
 	}
 	release_metapath(&mp);
 	return ret;
-- 
2.17.1

[Cluster-devel] [PATCH 0/8] Get rid of gfs2_write_calc_reserv in gfs2_iomap_begin_write

[Steven Whitehouse]

Hi,


On 28/08/18 22:47, Andreas Gruenbacher wrote:
> Following several cleanups, this is a first step towards being more
> accurate in the number of blocks we reserve in the journal.  I hope this
> part of the code is easier to understand now as well.
>
> Thanks,
> Andreas
Looks good to me. Lets make sure it gets lots of testing,

Steve.

> Andreas Gruenbacher (8):
>    gfs2: Don't depend on mp_aheight in clone_metapath
>    gfs2: Split gfs2_indirect_init
>    gfs2: Move empty inode check out of gfs2_unstuff_dinode
>    gfs2: Don't create unnecessary indirect blocks
>    gfs2: Improve gfs2_alloc_size for stuffed files
>    gfs2: Rename size to len in gfs2_alloc_size
>    gfs2: Move gfs2_alloc_size out of gfs2_iomap_get
>    gfs2: Get rid of gfs2_write_calc_reserv in gfs2_iomap_begin_write
>
>   fs/gfs2/bmap.c | 336 ++++++++++++++++++++++++++++++-------------------
>   1 file changed, 205 insertions(+), 131 deletions(-)
>