[Cluster-devel] [GFS2 PATCH V2] gfs2: Implement special writepage for ail start operations

[Bob Peterson <rpeterso@redhat.com>]

Hi,

Here's a rework of my previously posted patch based on Steve
Whitehouse's suggestions. This one's quite a bit simpler.
Also note that since gfs2_write_full_page was only ever called
with gfs2_get_block_noalloc, it's not necessary to pass in the
get_block function, so that parameter is now removed.

This version has now passed more than 425 iterations of the
failing test on rhel7, 350 iterations of the failing test on
rhel8, and dozens of iterations on an upstream kernel.

Regards,

Bob Peterson
---
gfs2: Implement special writepage for ail start operations

This patch fixes the hangs that result from doing certain jdata I/O,
primarily xfstests/269.

It implements a special new version of start_writepage that can
distinguish between writing pages from an ail sync operation, as
opposed to writes done on behalf of an inode write operation.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/aops.c  | 11 +++++------
 fs/gfs2/aops.h  |  4 ++++
 fs/gfs2/log.c   | 40 +++++++++++++++++++++++++++++++++-------
 fs/gfs2/log.h   |  3 ++-
 fs/gfs2/super.c |  2 +-
 5 files changed, 45 insertions(+), 15 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 05dd78f4b2b3..c5fb08eb9bb8 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -141,8 +141,7 @@ static int gfs2_writepage(struct page *page, struct writeback_control *wbc)
 /* This is the same as calling block_write_full_page, but it also
  * writes pages outside of i_size
  */
-static int gfs2_write_full_page(struct page *page, get_block_t *get_block,
-				struct writeback_control *wbc)
+int gfs2_write_full_page(struct page *page, struct writeback_control *wbc)
 {
 	struct inode * const inode = page->mapping->host;
 	loff_t i_size = i_size_read(inode);
@@ -160,8 +159,8 @@ static int gfs2_write_full_page(struct page *page, get_block_t *get_block,
 	if (page->index == end_index && offset)
 		zero_user_segment(page, offset, PAGE_SIZE);
 
-	return __block_write_full_page(inode, page, get_block, wbc,
-				       end_buffer_async_write);
+	return __block_write_full_page(inode, page, gfs2_get_block_noalloc,
+				       wbc, end_buffer_async_write);
 }
 
 /**
@@ -189,7 +188,7 @@ static int __gfs2_jdata_writepage(struct page *page, struct writeback_control *w
 		}
 		gfs2_page_add_databufs(ip, page, 0, sdp->sd_vfs->s_blocksize);
 	}
-	return gfs2_write_full_page(page, gfs2_get_block_noalloc, wbc);
+	return gfs2_write_full_page(page, wbc);
 }
 
 /**
@@ -201,7 +200,7 @@ static int __gfs2_jdata_writepage(struct page *page, struct writeback_control *w
  *
  */
 
-static int gfs2_jdata_writepage(struct page *page, struct writeback_control *wbc)
+int gfs2_jdata_writepage(struct page *page, struct writeback_control *wbc)
 {
 	struct inode *inode = page->mapping->host;
 	struct gfs2_inode *ip = GFS2_I(inode);
diff --git a/fs/gfs2/aops.h b/fs/gfs2/aops.h
index fa8e5d0144dd..2b9f27b6c729 100644
--- a/fs/gfs2/aops.h
+++ b/fs/gfs2/aops.h
@@ -15,5 +15,9 @@ extern int gfs2_stuffed_write_end(struct inode *inode, struct buffer_head *dibh,
 extern void adjust_fs_space(struct inode *inode);
 extern void gfs2_page_add_databufs(struct gfs2_inode *ip, struct page *page,
 				   unsigned int from, unsigned int len);
+extern int gfs2_jdata_writepage(struct page *page,
+				struct writeback_control *wbc);
+extern int gfs2_write_full_page(struct page *page,
+				struct writeback_control *wbc);
 
 #endif /* __AOPS_DOT_H__ */
diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
index 5bfaf381921a..787dba1db9a8 100644
--- a/fs/gfs2/log.c
+++ b/fs/gfs2/log.c
@@ -23,6 +23,7 @@
 #include <linux/writeback.h>
 #include <linux/list_sort.h>
 
+#include "aops.h"
 #include "gfs2.h"
 #include "incore.h"
 #include "bmap.h"
@@ -82,18 +83,40 @@ static void gfs2_remove_from_ail(struct gfs2_bufdata *bd)
 	brelse(bd->bd_bh);
 }
 
+/*
+ * Function used by generic_writepages to call the real writepage
+ * function and set the mapping flags on error
+ */
+static int start_writepage(struct page *page, struct writeback_control *wbc,
+			   void *data)
+{
+	struct address_space *mapping = page->mapping;
+	int ail_start = *(int *)data;
+	int ret;
+
+	if (ail_start)
+		ret = gfs2_write_full_page(page, wbc);
+	else
+		ret = mapping->a_ops->writepage(page, wbc);
+
+	mapping_set_error(mapping, ret);
+	return ret;
+}
+
 /**
  * gfs2_ail1_start_one - Start I/O on a part of the AIL
  * @sdp: the filesystem
  * @wbc: The writeback control structure
- * @ai: The ail structure
+ * @tr: the transaction
+ * @withdraw: this is set if we need to withdraw due to io errors
+ * @start: True if called from gfs2_ail1_start
  *
  */
 
 static int gfs2_ail1_start_one(struct gfs2_sbd *sdp,
 			       struct writeback_control *wbc,
-			       struct gfs2_trans *tr,
-			       bool *withdraw)
+			       struct gfs2_trans *tr, bool *withdraw,
+			       int ail_start)
 __releases(&sdp->sd_ail_lock)
 __acquires(&sdp->sd_ail_lock)
 {
@@ -128,7 +151,8 @@ __acquires(&sdp->sd_ail_lock)
 		if (!mapping)
 			continue;
 		spin_unlock(&sdp->sd_ail_lock);
-		generic_writepages(mapping, wbc);
+		write_cache_pages(mapping, wbc, start_writepage,
+				  &ail_start);
 		spin_lock(&sdp->sd_ail_lock);
 		if (wbc->nr_to_write <= 0)
 			break;
@@ -143,12 +167,14 @@ __acquires(&sdp->sd_ail_lock)
  * gfs2_ail1_flush - start writeback of some ail1 entries 
  * @sdp: The super block
  * @wbc: The writeback control structure
+ * @start: true if this was called from an ail1_start
  *
  * Writes back some ail1 entries, according to the limits in the
  * writeback control structure
  */
 
-void gfs2_ail1_flush(struct gfs2_sbd *sdp, struct writeback_control *wbc)
+void gfs2_ail1_flush(struct gfs2_sbd *sdp, struct writeback_control *wbc,
+		     int start)
 {
 	struct list_head *head = &sdp->sd_ail1_list;
 	struct gfs2_trans *tr;
@@ -162,7 +188,7 @@ void gfs2_ail1_flush(struct gfs2_sbd *sdp, struct writeback_control *wbc)
 	list_for_each_entry_reverse(tr, head, tr_list) {
 		if (wbc->nr_to_write <= 0)
 			break;
-		if (gfs2_ail1_start_one(sdp, wbc, tr, &withdraw))
+		if (gfs2_ail1_start_one(sdp, wbc, tr, &withdraw, start))
 			goto restart;
 	}
 	spin_unlock(&sdp->sd_ail_lock);
@@ -186,7 +212,7 @@ static void gfs2_ail1_start(struct gfs2_sbd *sdp)
 		.range_end = LLONG_MAX,
 	};
 
-	return gfs2_ail1_flush(sdp, &wbc);
+	return gfs2_ail1_flush(sdp, &wbc, 1);
 }
 
 /**
diff --git a/fs/gfs2/log.h b/fs/gfs2/log.h
index 1bc9bd444b28..3ebad7e505e4 100644
--- a/fs/gfs2/log.h
+++ b/fs/gfs2/log.h
@@ -74,7 +74,8 @@ extern void gfs2_write_log_header(struct gfs2_sbd *sdp, struct gfs2_jdesc *jd,
 extern void gfs2_log_flush(struct gfs2_sbd *sdp, struct gfs2_glock *gl,
 			   u32 type);
 extern void gfs2_log_commit(struct gfs2_sbd *sdp, struct gfs2_trans *trans);
-extern void gfs2_ail1_flush(struct gfs2_sbd *sdp, struct writeback_control *wbc);
+extern void gfs2_ail1_flush(struct gfs2_sbd *sdp, struct writeback_control *wbc,
+			    int start);
 
 extern void gfs2_log_shutdown(struct gfs2_sbd *sdp);
 extern int gfs2_logd(void *data);
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index d4b11c903971..31e7948c3c0c 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -762,7 +762,7 @@ static int gfs2_write_inode(struct inode *inode, struct writeback_control *wbc)
 			       GFS2_LOG_HEAD_FLUSH_NORMAL |
 			       GFS2_LFC_WRITE_INODE);
 	if (bdi->wb.dirty_exceeded)
-		gfs2_ail1_flush(sdp, wbc);
+		gfs2_ail1_flush(sdp, wbc, 0);
 	else
 		filemap_fdatawrite(metamapping);
 	if (flush_all)