[Cluster-devel] [GFS2 PATCH 0/2] Fix regression in "Ignore journal log writes for jdata holes"

[Cluster-devel] [GFS2 PATCH 0/2] Fix regression in "Ignore journal log writes for jdata holes"

[Bob Peterson]

Patch b2a846dbef4e ("gfs2: Ignore journal log writes for jdata holes"
caused a regression. It fixed one specific problem while breaking others.
The problem was that it changed the behavior of function gfs2_block_map
which is used by multiple callers so it had unintended consequences for
other callers.

This patch set reverts the patch and replaces it with a more targeted
solution that fixes just the one case it needs to.

Bob Peterson (2):
  Revert "gfs2: Ignore journal log writes for jdata holes"
  gfs2: Make special version of gfs2_get_block_noalloc for jdata

 fs/gfs2/aops.c | 30 ++++++++++++++++++++++++++++--
 fs/gfs2/bmap.c |  8 ++------
 fs/gfs2/log.c  |  2 ++
 3 files changed, 32 insertions(+), 8 deletions(-)

-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 1/2] Revert "gfs2: Ignore journal log writes for jdata holes"

[Bob Peterson]

This reverts commit b2a846dbef4ef54ef032f0f5ee188c609a0278a7.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/bmap.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 8dff9cbd0a87..62d9081d1e26 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -1301,12 +1301,8 @@ int gfs2_block_map(struct inode *inode, sector_t lblock,
 	trace_gfs2_bmap(ip, bh_map, lblock, create, 1);
 
 	ret = gfs2_iomap_get(inode, pos, length, flags, &iomap, &mp);
-	if (!ret && iomap.type == IOMAP_HOLE) {
-		if (create)
-			ret = gfs2_iomap_alloc(inode, &iomap, &mp);
-		else
-			ret = -ENODATA;
-	}
+	if (create && !ret && iomap.type == IOMAP_HOLE)
+		ret = gfs2_iomap_alloc(inode, &iomap, &mp);
 	release_metapath(&mp);
 	if (ret)
 		goto out;
-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 2/2] gfs2: Make special version of gfs2_get_block_noalloc for jdata

[Bob Peterson]

Patch b2a846dbef4ef54ef032f0f5ee188c609a0278a7 fixed cases in which
writes were attempted to blocks that had been freed by punch_hole in a jdata
file, but the blocks were still represented on the ail1 list.

It had an unfortunate side-effect of returning -ENODATA in cases that
were not jdata because it affected function gfs2_block_map which is used by
many other callers. This resulted in several xfstests test failures. For
example, generic/029 would fail with this error:

    +hexdump: /mnt/scratch/testfile: Input/output error
     00000000  58 58 58 58 58 58 58 58  58 58 58 58 58 58 58 58  |XXXXXXXXXXXXXXXX|

This patch creates a special version of gfs2_get_block_noalloc for
jdata which returns -ENODATA instead of -EIO for unmapped blocks.
This is a more targeted approach that doesn't break non-jdata cases.

Fixes: b2a846dbef4e ("gfs2: Ignore journal log writes for jdata holes")
Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/aops.c | 30 ++++++++++++++++++++++++++++--
 fs/gfs2/log.c  |  2 ++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 9cd2ecad07db..835c50e6a871 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -80,6 +80,32 @@ static int gfs2_get_block_noalloc(struct inode *inode, sector_t lblock,
 		return -EIO;
 	return 0;
 }
+/**
+ * get_jdata_block_noalloc - jdata version of gfs2_get_block_noalloc
+ * @inode: The inode
+ * @lblock: The block number to look up
+ * @bh_result: The buffer head to return the result in
+ * @create: Non-zero if we may add block to the file
+ *
+ * The reason we have a separate function for jdata is that jdata go through
+ * the journal, and we don't want jdata holes (left by punch_hole) to appear
+ * as IO errors. We can safely ignore them.
+ *
+ * Returns: errno
+ */
+static int get_jdata_block_noalloc(struct inode *inode, sector_t lblock,
+				      struct buffer_head *bh_result,
+				      int create)
+{
+	int error;
+
+	error = gfs2_block_map(inode, lblock, bh_result, 0);
+	if (error)
+		return error;
+	if (!buffer_mapped(bh_result))
+		return -ENODATA;
+	return 0;
+}
 
 /**
  * gfs2_writepage - Write page for writeback mappings
@@ -133,8 +159,8 @@ static int gfs2_write_jdata_page(struct page *page,
 	if (page->index == end_index && offset)
 		zero_user_segment(page, offset, PAGE_SIZE);
 
-	return __block_write_full_page(inode, page, gfs2_get_block_noalloc, wbc,
-				       end_buffer_async_write);
+	return __block_write_full_page(inode, page, get_jdata_block_noalloc,
+				       wbc, end_buffer_async_write);
 }
 
 /**
diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
index 9133b3178677..2e9314091c81 100644
--- a/fs/gfs2/log.c
+++ b/fs/gfs2/log.c
@@ -132,6 +132,8 @@ __acquires(&sdp->sd_ail_lock)
 		spin_unlock(&sdp->sd_ail_lock);
 		ret = generic_writepages(mapping, wbc);
 		spin_lock(&sdp->sd_ail_lock);
+		if (ret == -ENODATA) /* if a jdata write into a new hole */
+			ret = 0; /* ignore it */
 		if (ret || wbc->nr_to_write <= 0)
 			break;
 		return -EBUSY;
-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 0/2] Fix regression in "Ignore journal log writes for jdata holes"

[Bob Peterson]

----- Original Message -----
> Patch b2a846dbef4e ("gfs2: Ignore journal log writes for jdata holes"
> caused a regression. It fixed one specific problem while breaking others.
> The problem was that it changed the behavior of function gfs2_block_map
> which is used by multiple callers so it had unintended consequences for
> other callers.
> 
> This patch set reverts the patch and replaces it with a more targeted
> solution that fixes just the one case it needs to.
> 
> Bob Peterson (2):
>   Revert "gfs2: Ignore journal log writes for jdata holes"
>   gfs2: Make special version of gfs2_get_block_noalloc for jdata
> 
>  fs/gfs2/aops.c | 30 ++++++++++++++++++++++++++++--
>  fs/gfs2/bmap.c |  8 ++------
>  fs/gfs2/log.c  |  2 ++
>  3 files changed, 32 insertions(+), 8 deletions(-)
> 
> --
> 2.26.2

NACK. Ignore this for now. Something's wrong and I'm investigating.

Regards,

Bob Peterson