[Cluster-devel] [RHEL7 fs 0/3] gfs2: Fixes to "Implement iomap for block_map"

[Cluster-devel] [RHEL7 fs 0/3] gfs2: Fixes to "Implement iomap for block_map"

[Andreas Gruenbacher]

There are several regressions in commit "[fs] gfs2: Implement iomap for
block_map", which are fixed in mainline commit "Fixes to "Implement
iomap for block_map"".

Because mainline commit "Fixes to "Implement iomap for block_map""
depends on commit "gfs2: Clean up {lookup,fillup}_metapath", backport
that as well.

The regressions trigger a WARN_ON(iomap.length == 0) in iomap_apply in
mainline.  This WARN_ON hasn't been backported to RHEL7 yet, so in order
to have a reasonable reproducer, backport that patch as well.

Andreas Gruenbacher (2):
  gfs2: Clean up {lookup,fillup}_metapath
  gfs2: Fixes to "Implement iomap for block_map"

Darrick J. Wong (1):
  iomap: warn on zero-length mappings

 fs/gfs2/bmap.c | 117 ++++++++++++++++++++++++++-------------------------------
 fs/iomap.c     |   2 +
 2 files changed, 55 insertions(+), 64 deletions(-)

-- 
2.14.3

[Cluster-devel] [RHEL7 fs 1/3] iomap: warn on zero-length mappings

[Andreas Gruenbacher]

From: "Darrick J. Wong" <darrick.wong@oracle.com>

Bugzilla: 1542594
Upstream status: v4.15
Brew: https://brewweb.engineering.redhat.com/brew/taskinfo?taskID=15221703
Tested: locally

commit 0c6dda7a1cbd587e48bcef1999875e29549c2b41
Author: Darrick J. Wong <darrick.wong@oracle.com>
Date:   Fri Jan 26 11:11:20 2018 -0800

    iomap: warn on zero-length mappings

    Don't let the iomap callback get away with feeding us a garbage zero
    length mapping -- there was a bug in xfs that resulted in those leaking
    out to hilarious effect.

    Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
    Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/iomap.c b/fs/iomap.c
index ca77b55ca134..4b8dc781bbd3 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -63,6 +63,8 @@ iomap_apply(struct inode *inode, loff_t pos, loff_t length, unsigned flags,
 		return ret;
 	if (WARN_ON(iomap.offset > pos))
 		return -EIO;
+	if (WARN_ON(iomap.length == 0))
+		return -EIO;
 
 	/*
 	 * Cut down the length to the one actually provided by the filesystem,
-- 
2.14.3

[Cluster-devel] [RHEL7 fs 2/3] gfs2: Clean up {lookup, fillup}_metapath

[Andreas Gruenbacher]

Bugzilla: 1542594
Upstream status: v4.15
Brew: https://brewweb.engineering.redhat.com/brew/taskinfo?taskID=15221703
Tested: locally

commit e8b43fe0c1e035a135be7ca3791d465fcb1b501e
Author: Andreas Gruenbacher <agruenba@redhat.com>
Date:   Fri Dec 8 17:01:57 2017 +0100

    gfs2: Clean up {lookup,fillup}_metapath

    Split out the entire lookup loop from lookup_metapath and
    fillup_metapath.  Make both functions return the actual height in
    mp->mp_aheight, and return 0 on success.  Handle lookup errors properly
    in trunc_dealloc.

    Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
    Signed-off-by: Bob Peterson <rpeterso@redhat.com>

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

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 8569bf37ad7f..55fc3c05103c 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -303,21 +303,22 @@ static void gfs2_metapath_ra(struct gfs2_glock *gl,
 	}
 }
 
-/**
- * lookup_mp_height - helper function for lookup_metapath
- * @ip: the inode
- * @mp: the metapath
- * @h: the height which needs looking up
- */
-static int lookup_mp_height(struct gfs2_inode *ip, struct metapath *mp, int h)
+static int __fillup_metapath(struct gfs2_inode *ip, struct metapath *mp,
+			     unsigned int x, unsigned int h)
 {
-	__be64 *ptr = metapointer(h, mp);
-	u64 dblock = be64_to_cpu(*ptr);
+	for (; x < h; x++) {
+		__be64 *ptr = metapointer(x, mp);
+		u64 dblock = be64_to_cpu(*ptr);
+		int ret;
 
-	if (!dblock)
-		return h + 1;
-
-	return gfs2_meta_indirect_buffer(ip, h + 1, dblock, &mp->mp_bh[h + 1]);
+		if (!dblock)
+			break;
+		ret = gfs2_meta_indirect_buffer(ip, x + 1, dblock, &mp->mp_bh[x + 1]);
+		if (ret)
+			return ret;
+	}
+	mp->mp_aheight = x + 1;
+	return 0;
 }
 
 /**
@@ -334,25 +335,12 @@ static int lookup_mp_height(struct gfs2_inode *ip, struct metapath *mp, int h)
  * at which it found the unallocated block. Blocks which are found are
  * added to the mp->mp_bh[] list.
  *
- * Returns: error or height of metadata tree
+ * Returns: error
  */
 
 static int lookup_metapath(struct gfs2_inode *ip, struct metapath *mp)
 {
-	unsigned int end_of_metadata = ip->i_height - 1;
-	unsigned int x;
-	int ret;
-
-	for (x = 0; x < end_of_metadata; x++) {
-		ret = lookup_mp_height(ip, mp, x);
-		if (ret)
-			goto out;
-	}
-
-	ret = ip->i_height;
-out:
-	mp->mp_aheight = ret;
-	return ret;
+	return __fillup_metapath(ip, mp, 0, ip->i_height - 1);
 }
 
 /**
@@ -363,25 +351,21 @@ out:
  *
  * Similar to lookup_metapath, but does lookups for a range of heights
  *
- * Returns: error or height of metadata tree
+ * Returns: error
  */
 
 static int fillup_metapath(struct gfs2_inode *ip, struct metapath *mp, int h)
 {
-	unsigned int start_h = h - 1;
-	int ret;
+	unsigned int x = 0;
 
 	if (h) {
 		/* find the first buffer we need to look up. */
-		while (start_h > 0 && mp->mp_bh[start_h] == NULL)
-			start_h--;
-		for (; start_h < h; start_h++) {
-			ret = lookup_mp_height(ip, mp, start_h);
-			if (ret)
-				return ret;
+		for (x = h - 1; x > 0; x--) {
+			if (mp->mp_bh[x])
+				break;
 		}
 	}
-	return ip->i_height;
+	return __fillup_metapath(ip, mp, x, h);
 }
 
 static inline void release_metapath(struct metapath *mp)
@@ -786,7 +770,7 @@ int gfs2_iomap_begin(struct inode *inode, loff_t pos, loff_t length,
 		goto do_alloc;
 
 	ret = lookup_metapath(ip, &mp);
-	if (ret < 0)
+	if (ret)
 		goto out_release;
 
 	if (mp.mp_aheight != ip->i_height)
@@ -1334,7 +1318,9 @@ static int trunc_dealloc(struct gfs2_inode *ip, u64 newsize)
 
 	mp.mp_bh[0] = dibh;
 	ret = lookup_metapath(ip, &mp);
-	if (ret == ip->i_height)
+	if (ret)
+		goto out_metapath;
+	if (mp.mp_aheight == ip->i_height)
 		state = DEALLOC_MP_FULL; /* We have a complete metapath */
 	else
 		state = DEALLOC_FILL_MP; /* deal with partial metapath */
@@ -1430,16 +1416,16 @@ static int trunc_dealloc(struct gfs2_inode *ip, u64 newsize)
 		case DEALLOC_FILL_MP:
 			/* Fill the buffers out to the current height. */
 			ret = fillup_metapath(ip, &mp, mp_h);
-			if (ret < 0)
+			if (ret)
 				goto out;
 
 			/* If buffers found for the entire strip height */
-			if ((ret == ip->i_height) && (mp_h == strip_h)) {
+			if (mp.mp_aheight - 1 == strip_h) {
 				state = DEALLOC_MP_FULL;
 				break;
 			}
-			if (ret < ip->i_height) /* We have a partial height */
-				mp_h = ret - 1;
+			if (mp.mp_aheight < ip->i_height) /* We have a partial height */
+				mp_h = mp.mp_aheight - 1;
 
 			/* If we find a non-null block pointer, crawl a bit
 			   higher up in the metapath and try again, otherwise
-- 
2.14.3

[Cluster-devel] [RHEL7 fs 3/3] gfs2: Fixes to "Implement iomap for block_map"

[Andreas Gruenbacher]

Bugzilla: 1542594
Upstream status: https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/log/?h=for-linus
Brew: https://brewweb.engineering.redhat.com/brew/taskinfo?taskID=15221703
Tested: locally

commit 6644a0e9aaa430e40ce175f079751b2694adbe26
Author: Andreas Gruenbacher <agruenba@redhat.com>
Date:   Tue Feb 6 07:20:55 2018 -0700

    gfs2: Fixes to "Implement iomap for block_map"

    It turns out that commit 3974320ca6 "Implement iomap for block_map"
    introduced a few bugs that trigger occasional failures with xfstest
    generic/476:

    In gfs2_iomap_begin, we jump to do_alloc when we determine that we are
    beyond the end of the allocated metadata (height > ip->i_height).
    There, we can end up calling hole_size with a metapath that doesn't
    match the current metadata tree, which deosn't make sense.  After
    untangling the code at do_alloc, fix this by checking if the block we
    are looking for is within the range of allocated metadata.

    In addition, add a BUG() in case gfs2_iomap_begin is accidentally called
    for reading stuffed files: this is handled separately.  Make sure we
    don't truncate iomap->length for reads beyond the end of the file; in
    that case, the entire range counts as a hole.

    Finally, revert to taking a bitmap write lock when doing allocations.
    It's unclear why that change didn't lead to any failures during testing.

    Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
    Signed-off-by: Bob Peterson <rpeterso@redhat.com>

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

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 55fc3c05103c..199d3f9bd668 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -718,7 +718,7 @@ int gfs2_iomap_begin(struct inode *inode, loff_t pos, loff_t length,
 	__be64 *ptr;
 	sector_t lblock;
 	sector_t lend;
-	int ret;
+	int ret = 0;
 	int eob;
 	unsigned int len;
 	struct buffer_head *bh;
@@ -730,12 +730,14 @@ int gfs2_iomap_begin(struct inode *inode, loff_t pos, loff_t length,
 		goto out;
 	}
 
-	if ((flags & IOMAP_REPORT) && gfs2_is_stuffed(ip)) {
-		gfs2_stuffed_iomap(inode, iomap);
-		if (pos >= iomap->length)
-			return -ENOENT;
-		ret = 0;
-		goto out;
+	if (gfs2_is_stuffed(ip)) {
+		if (flags & IOMAP_REPORT) {
+			gfs2_stuffed_iomap(inode, iomap);
+			if (pos >= iomap->length)
+				ret = -ENOENT;
+			goto out;
+		}
+		BUG_ON(!(flags & IOMAP_WRITE));
 	}
 
 	lblock = pos >> inode->i_blkbits;
@@ -746,7 +748,7 @@ int gfs2_iomap_begin(struct inode *inode, loff_t pos, loff_t length,
 	iomap->type = IOMAP_HOLE;
 	iomap->length = (u64)(lend - lblock) << inode->i_blkbits;
 	iomap->flags = IOMAP_F_MERGED;
-	bmap_lock(ip, 0);
+	bmap_lock(ip, flags & IOMAP_WRITE);
 
 	/*
 	 * Directory data blocks have a struct gfs2_meta_header header, so the
@@ -789,27 +791,28 @@ int gfs2_iomap_begin(struct inode *inode, loff_t pos, loff_t length,
 		iomap->flags |= IOMAP_F_BOUNDARY;
 	iomap->length = (u64)len << inode->i_blkbits;
 
-	ret = 0;
-
 out_release:
 	release_metapath(&mp);
-	bmap_unlock(ip, 0);
+	bmap_unlock(ip, flags & IOMAP_WRITE);
 out:
 	trace_gfs2_iomap_end(ip, iomap, ret);
 	return ret;
 
 do_alloc:
-	if (!(flags & IOMAP_WRITE)) {
-		if (pos >= i_size_read(inode)) {
+	if (flags & IOMAP_WRITE) {
+		ret = gfs2_iomap_alloc(inode, iomap, flags, &mp);
+	} else if (flags & IOMAP_REPORT) {
+		loff_t size = i_size_read(inode);
+		if (pos >= size)
 			ret = -ENOENT;
-			goto out_release;
-		}
-		ret = 0;
-		iomap->length = hole_size(inode, lblock, &mp);
-		goto out_release;
+		else if (height <= ip->i_height)
+			iomap->length = hole_size(inode, lblock, &mp);
+		else
+			iomap->length = size - pos;
+	} else {
+		if (height <= ip->i_height)
+			iomap->length = hole_size(inode, lblock, &mp);
 	}
-
-	ret = gfs2_iomap_alloc(inode, iomap, flags, &mp);
 	goto out_release;
 }
 
-- 
2.14.3

[Cluster-devel] [RHEL7 fs 0/3] gfs2: Fixes to "Implement iomap for block_map"

[Andreas Gruenbacher]

On 6 February 2018 at 19:31, Andreas Gruenbacher <agruenba@redhat.com> wrote:
> There are several regressions in commit "[fs] gfs2: Implement iomap for
> block_map", which are fixed in mainline commit "Fixes to "Implement
> iomap for block_map"".
>
> Because mainline commit "Fixes to "Implement iomap for block_map""
> depends on commit "gfs2: Clean up {lookup,fillup}_metapath", backport
> that as well.
>
> The regressions trigger a WARN_ON(iomap.length == 0) in iomap_apply in
> mainline.  This WARN_ON hasn't been backported to RHEL7 yet, so in order
> to have a reasonable reproducer, backport that patch as well.

Please disregard this thread, this was meant to be sent to another mailing list.

Thanks,
Andreas