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

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

[Andreas Gruenbacher]

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/3] 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 separate calls.

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 2/3] gfs2: Split gfs2_indirect_init

[Bob Peterson]

----- Original Message -----
> 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 separate calls.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
Hi,

Reviewed-by: Bob Peterson <rpeterso@redhat.com>

This looks right. It took me a while to convince myself though. :)

Regards,

Bob Peterson

[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@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

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

[Bob Peterson]

Hi,

----- Original Message -----
> 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@redhat.com>
> ---
(snip)
> +	bool overlap = false;

Can we change overlap to be unsigned or something? Although technically
there's nothing wrong with doing so, arithmetic with a bool just rubs
me the wrong way.

(snip)
> +				i = 1 + overlap;

This patch is a little more cryptic. I'll have to look at it again
when I'm fresh.

Regards,

Bob Peterson

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

[Andreas Gruenbacher]

This changes clone_metapath to work the same way as release_metapath.
This is merely a clean-up, not a fix, and nothing else depends on it.

On 30 July 2018 at 14:33, Andreas Gruenbacher <agruenba@redhat.com> wrote:
> 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 1/3] gfs2: Don't depend on mp_aheight in clone_metapath

[Bob Peterson]

----- Original Message -----
> 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
> 
> 
Hi,

Reviewed-by: Bob Peterson <rpeterso@redhat.com>

Bob Peterson