[Cluster-devel] [GFS2 PATCH 00/12] gfs2: jdata patch collection revisited

[Cluster-devel] [GFS2 PATCH 00/12] gfs2: jdata patch collection revisited

[Bob Peterson]

On 24 July I posted a set of patches for various problems I found and fixed
while testing jdata with xfstests. The safest of these were recently pushed
upstream in the last merge window. Since then, I've found and fixed more
problems, and cleaned up some of the more risky patches. So this is my
latest collection of patches.

These patches allow xfstests to pass in its entirety using jdata, and
unlike before, 269 can be run multiple times without deadlocking.

Bob Peterson (12):
  gfs2: rename gfs2_write_full_page to gfs2_write_jdata_page, remove
    parm
  gfs2: add missing log_blocks trace points in gfs2_write_revokes
  gfs2: enhance log_blocks trace point to show log blocks free
  gfs2: Wipe jdata and ail1 in gfs2_journal_wipe, formerly
    gfs2_meta_wipe
  gfs2: Calculate number of revokes during evict
  gfs2: Create transaction for inodes with i_nlink != 0
  gfs2: make gfs2_ail1_empty_one return the count of active items
  gfs2: don't lock sd_ail_lock in gfs2_releasepage
  gfs2: Only set PageChecked if we have a transaction
  gfs2: simplify gfs2_block_map
  gfs2: Ignore journal log writes for jdata holes
  gfs2: add some much needed cleanup for log flushes that fail

 fs/gfs2/aops.c       | 51 ++++++++++++++++++++++------
 fs/gfs2/bmap.c       | 14 ++++----
 fs/gfs2/log.c        | 57 +++++++++++++++++++++++++++----
 fs/gfs2/log.h        |  2 +-
 fs/gfs2/meta_io.c    | 81 +++++++++++++++++++++++++++++++++++++++++---
 fs/gfs2/meta_io.h    |  2 +-
 fs/gfs2/rgrp.c       |  6 ++--
 fs/gfs2/super.c      | 28 ++++++++++-----
 fs/gfs2/trace_gfs2.h |  6 ++--
 fs/gfs2/trans.c      |  1 +
 10 files changed, 202 insertions(+), 46 deletions(-)

-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 01/12] gfs2: rename gfs2_write_full_page to gfs2_write_jdata_page, remove parm

[Bob Peterson]

Since the function is only used for writing jdata pages, this patch
simply renames function gfs2_write_full_page to a more appropriate
name: gfs2_write_jdata_page. This makes the code easier to understand.

The function was only called in one place, which passed in a pointer to
function gfs2_get_block_noalloc. The function doesn't need to be
passed in. Therefore, this also eliminates the unnecessary parameter
to increase efficiency.

I also took the liberty of cleaning up the function comments.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/aops.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 68cd700a2719..faf58c40485a 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -115,11 +115,16 @@ static int gfs2_writepage(struct page *page, struct writeback_control *wbc)
 	return 0;
 }
 
-/* This is the same as calling block_write_full_page, but it also
+/**
+ * gfs2_write_jdata_page - gfs2 jdata-specific version of block_write_full_page
+ * @page: The page to write
+ * @wbc: The writeback control
+ *
+ * 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)
+static int gfs2_write_jdata_page(struct page *page,
+				 struct writeback_control *wbc)
 {
 	struct inode * const inode = page->mapping->host;
 	loff_t i_size = i_size_read(inode);
@@ -137,7 +142,7 @@ 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,
+	return __block_write_full_page(inode, page, gfs2_get_block_noalloc, wbc,
 				       end_buffer_async_write);
 }
 
@@ -166,7 +171,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_jdata_page(page, wbc);
 }
 
 /**
-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 02/12] gfs2: add missing log_blocks trace points in gfs2_write_revokes

[Bob Peterson]

Function gfs2_write_revokes was incrementing and decrementing the number
of log blocks free, but there was never a log_blocks trace point for it.
Thus, the free blocks from a log_blocks trace would jump around
mysteriously.

This patch adds the missing trace points so the trace makes more sense.

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

diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
index a58333e3980d..01a645652a7d 100644
--- a/fs/gfs2/log.c
+++ b/fs/gfs2/log.c
@@ -716,16 +716,24 @@ void gfs2_write_revokes(struct gfs2_sbd *sdp)
 		atomic_dec(&sdp->sd_log_blks_free);
 		/* If no blocks have been reserved, we need to also
 		 * reserve a block for the header */
-		if (!sdp->sd_log_blks_reserved)
+		if (!sdp->sd_log_blks_reserved) {
 			atomic_dec(&sdp->sd_log_blks_free);
+			trace_gfs2_log_blocks(sdp, -2);
+		} else {
+			trace_gfs2_log_blocks(sdp, -1);
+		}
 	}
 	gfs2_ail1_empty(sdp, max_revokes);
 	gfs2_log_unlock(sdp);
 
 	if (!sdp->sd_log_num_revoke) {
 		atomic_inc(&sdp->sd_log_blks_free);
-		if (!sdp->sd_log_blks_reserved)
+		if (!sdp->sd_log_blks_reserved) {
 			atomic_inc(&sdp->sd_log_blks_free);
+			trace_gfs2_log_blocks(sdp, 2);
+		} else {
+			trace_gfs2_log_blocks(sdp, 1);
+		}
 	}
 }
 
-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 03/12] gfs2: enhance log_blocks trace point to show log blocks free

[Bob Peterson]

This patch adds some code to enhance the log_blocks trace point. It
reports the number of free log blocks. This makes the trace point much
more useful, especially for debugging performance problems when we can
tell when the journal gets full and needs to wait for flushes, etc.

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

diff --git a/fs/gfs2/trace_gfs2.h b/fs/gfs2/trace_gfs2.h
index e0025258107a..fe140ceef74d 100644
--- a/fs/gfs2/trace_gfs2.h
+++ b/fs/gfs2/trace_gfs2.h
@@ -388,15 +388,17 @@ TRACE_EVENT(gfs2_log_blocks,
 	TP_STRUCT__entry(
 		__field(        dev_t,  dev                     )
 		__field(	int,	blocks			)
+		__field(	int,	blks_free		)
 	),
 
 	TP_fast_assign(
 		__entry->dev		= sdp->sd_vfs->s_dev;
 		__entry->blocks		= blocks;
+		__entry->blks_free	= atomic_read(&sdp->sd_log_blks_free);
 	),
 
-	TP_printk("%u,%u log reserve %d", MAJOR(__entry->dev),
-		  MINOR(__entry->dev), __entry->blocks)
+	TP_printk("%u,%u log reserve %d %d", MAJOR(__entry->dev),
+		  MINOR(__entry->dev), __entry->blocks, __entry->blks_free)
 );
 
 /* Writing back the AIL */
-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 04/12] gfs2: Wipe jdata and ail1 in gfs2_journal_wipe, formerly gfs2_meta_wipe

[Bob Peterson]

Before this patch, when blocks were freed, it called gfs2_meta_wipe to
take the metadata out of the pending journal blocks. It did this mostly
by calling another function called gfs2_remove_from_journal. This is
shortsighted because it does not do anything with jdata blocks which
may also be in the journal.

This patch expands the function so that it wipes out jdata blocks from
the journal as well, and it wipes it from the ail1 list if it hasn't
been written back yet. Since it now processes jdata blocks as well,
the function has been renamed from gfs2_meta_wipe to gfs2_journal_wipe.

New function gfs2_ail1_wipe wants a static view of the ail list, so it
locks the sd_ail_lock when removing items. To accomplish this, function
gfs2_remove_from_journal no longer locks the sd_ail_lock, and it's now
the caller's responsibility to do so.

I was going to make sd_ail_lock locking conditional, but the practice is
generally frowned upon. For details, see: https://lwn.net/Articles/109066/

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/aops.c    |  5 ++-
 fs/gfs2/log.c     |  2 +-
 fs/gfs2/log.h     |  2 +-
 fs/gfs2/meta_io.c | 81 ++++++++++++++++++++++++++++++++++++++++++++---
 fs/gfs2/meta_io.h |  2 +-
 fs/gfs2/rgrp.c    |  6 ++--
 6 files changed, 86 insertions(+), 12 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index faf58c40485a..f6e36a33b392 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -668,8 +668,11 @@ static void gfs2_discard(struct gfs2_sbd *sdp, struct buffer_head *bh)
 	if (bd) {
 		if (!list_empty(&bd->bd_list) && !buffer_pinned(bh))
 			list_del_init(&bd->bd_list);
-		else
+		else {
+			spin_lock(&sdp->sd_ail_lock);
 			gfs2_remove_from_journal(bh, REMOVE_JDATA);
+			spin_unlock(&sdp->sd_ail_lock);
+		}
 	}
 	bh->b_bdev = NULL;
 	clear_buffer_mapped(bh);
diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
index 01a645652a7d..3e24e8733950 100644
--- a/fs/gfs2/log.c
+++ b/fs/gfs2/log.c
@@ -70,7 +70,7 @@ unsigned int gfs2_struct2blk(struct gfs2_sbd *sdp, unsigned int nstruct)
  *
  */
 
-static void gfs2_remove_from_ail(struct gfs2_bufdata *bd)
+void gfs2_remove_from_ail(struct gfs2_bufdata *bd)
 {
 	bd->bd_tr = NULL;
 	list_del_init(&bd->bd_ail_st_list);
diff --git a/fs/gfs2/log.h b/fs/gfs2/log.h
index 8965c751a303..79f97290146e 100644
--- a/fs/gfs2/log.h
+++ b/fs/gfs2/log.h
@@ -63,7 +63,7 @@ static inline void gfs2_ordered_add_inode(struct gfs2_inode *ip)
 
 extern void gfs2_ordered_del_inode(struct gfs2_inode *ip);
 extern unsigned int gfs2_struct2blk(struct gfs2_sbd *sdp, unsigned int nstruct);
-
+extern void gfs2_remove_from_ail(struct gfs2_bufdata *bd);
 extern void gfs2_log_release(struct gfs2_sbd *sdp, unsigned int blks);
 extern int gfs2_log_reserve(struct gfs2_sbd *sdp, unsigned int blks);
 extern void gfs2_write_log_header(struct gfs2_sbd *sdp, struct gfs2_jdesc *jd,
diff --git a/fs/gfs2/meta_io.c b/fs/gfs2/meta_io.c
index 9856cc2e0795..2db573e31f78 100644
--- a/fs/gfs2/meta_io.c
+++ b/fs/gfs2/meta_io.c
@@ -348,38 +348,109 @@ void gfs2_remove_from_journal(struct buffer_head *bh, int meta)
 		brelse(bh);
 	}
 	if (bd) {
-		spin_lock(&sdp->sd_ail_lock);
 		if (bd->bd_tr) {
 			gfs2_trans_add_revoke(sdp, bd);
 		} else if (was_pinned) {
 			bh->b_private = NULL;
 			kmem_cache_free(gfs2_bufdata_cachep, bd);
+		} else if (!list_empty(&bd->bd_ail_st_list) &&
+					!list_empty(&bd->bd_ail_gl_list)) {
+			gfs2_remove_from_ail(bd);
 		}
-		spin_unlock(&sdp->sd_ail_lock);
 	}
 	clear_buffer_dirty(bh);
 	clear_buffer_uptodate(bh);
 }
 
 /**
- * gfs2_meta_wipe - make inode's buffers so they aren't dirty/pinned anymore
+ * gfs2_ail1_wipe - remove deleted/freed buffers from the ail1 list
+ * @sdp: superblock
+ * @bstart: starting block address of buffers to remove
+ * @blen: length of buffers to be removed
+ *
+ * This function is called from gfs2_journal wipe, whose job is to remove
+ * buffers, corresponding to deleted blocks, from the journal. If we find any
+ * bufdata elements on the system ail1 list, they haven't been written to
+ * the journal yet. So we remove them.
+ */
+static void gfs2_ail1_wipe(struct gfs2_sbd *sdp, u64 bstart, u32 blen)
+{
+	struct gfs2_trans *tr, *s;
+	struct gfs2_bufdata *bd, *bs;
+	struct buffer_head *bh;
+	u64 end = bstart + blen;
+
+	gfs2_log_lock(sdp);
+	spin_lock(&sdp->sd_ail_lock);
+	list_for_each_entry_safe(tr, s, &sdp->sd_ail1_list, tr_list) {
+		list_for_each_entry_safe(bd, bs, &tr->tr_ail1_list,
+					 bd_ail_st_list) {
+			bh = bd->bd_bh;
+			if (bh->b_blocknr < bstart || bh->b_blocknr >= end)
+				continue;
+
+			gfs2_remove_from_journal(bh, REMOVE_JDATA);
+		}
+	}
+	spin_unlock(&sdp->sd_ail_lock);
+	gfs2_log_unlock(sdp);
+}
+
+static struct buffer_head *gfs2_getjdatabuf(struct gfs2_inode *ip, u64 blkno)
+{
+	struct address_space *mapping = ip->i_inode.i_mapping;
+	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
+	struct page *page;
+	struct buffer_head *bh;
+	unsigned int shift = PAGE_SHIFT - sdp->sd_sb.sb_bsize_shift;
+	unsigned long index = blkno >> shift; /* convert block to page */
+	unsigned int bufnum = blkno - (index << shift);
+
+	page = find_get_page_flags(mapping, index, FGP_LOCK|FGP_ACCESSED);
+	if (!page)
+		return NULL;
+	if (!page_has_buffers(page)) {
+		unlock_page(page);
+		put_page(page);
+		return NULL;
+	}
+	/* Locate header for our buffer within our page */
+	for (bh = page_buffers(page); bufnum--; bh = bh->b_this_page)
+		/* Do nothing */;
+	get_bh(bh);
+	unlock_page(page);
+	put_page(page);
+	return bh;
+}
+
+/**
+ * gfs2_journal_wipe - make inode's buffers so they aren't dirty/pinned anymore
  * @ip: the inode who owns the buffers
  * @bstart: the first buffer in the run
  * @blen: the number of buffers in the run
  *
  */
 
-void gfs2_meta_wipe(struct gfs2_inode *ip, u64 bstart, u32 blen)
+void gfs2_journal_wipe(struct gfs2_inode *ip, u64 bstart, u32 blen)
 {
 	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
 	struct buffer_head *bh;
+	int ty;
 
+	gfs2_ail1_wipe(sdp, bstart, blen);
 	while (blen) {
+		ty = REMOVE_META;
 		bh = gfs2_getbuf(ip->i_gl, bstart, NO_CREATE);
+		if (!bh && gfs2_is_jdata(ip)) {
+			bh = gfs2_getjdatabuf(ip, bstart);
+			ty = REMOVE_JDATA;
+		}
 		if (bh) {
 			lock_buffer(bh);
 			gfs2_log_lock(sdp);
-			gfs2_remove_from_journal(bh, REMOVE_META);
+			spin_lock(&sdp->sd_ail_lock);
+			gfs2_remove_from_journal(bh, ty);
+			spin_unlock(&sdp->sd_ail_lock);
 			gfs2_log_unlock(sdp);
 			unlock_buffer(bh);
 			brelse(bh);
diff --git a/fs/gfs2/meta_io.h b/fs/gfs2/meta_io.h
index eafb74e861c6..4a8c01929b79 100644
--- a/fs/gfs2/meta_io.h
+++ b/fs/gfs2/meta_io.h
@@ -60,7 +60,7 @@ enum {
 };
 
 extern void gfs2_remove_from_journal(struct buffer_head *bh, int meta);
-extern void gfs2_meta_wipe(struct gfs2_inode *ip, u64 bstart, u32 blen);
+extern void gfs2_journal_wipe(struct gfs2_inode *ip, u64 bstart, u32 blen);
 extern int gfs2_meta_indirect_buffer(struct gfs2_inode *ip, int height, u64 num,
 				     struct buffer_head **bhp);
 
diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index 074f228ea839..bade48f2522f 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -2445,8 +2445,8 @@ void __gfs2_free_blocks(struct gfs2_inode *ip, struct gfs2_rgrpd *rgd,
 	gfs2_rgrp_out(rgd, rgd->rd_bits[0].bi_bh->b_data);
 
 	/* Directories keep their data in the metadata address space */
-	if (meta || ip->i_depth)
-		gfs2_meta_wipe(ip, bstart, blen);
+	if (meta || ip->i_depth || gfs2_is_jdata(ip))
+		gfs2_journal_wipe(ip, bstart, blen);
 }
 
 /**
@@ -2502,7 +2502,7 @@ void gfs2_free_di(struct gfs2_rgrpd *rgd, struct gfs2_inode *ip)
 	gfs2_statfs_change(sdp, 0, +1, -1);
 	trace_gfs2_block_alloc(ip, rgd, ip->i_no_addr, 1, GFS2_BLKST_FREE);
 	gfs2_quota_change(ip, -1, ip->i_inode.i_uid, ip->i_inode.i_gid);
-	gfs2_meta_wipe(ip, ip->i_no_addr, 1);
+	gfs2_journal_wipe(ip, ip->i_no_addr, 1);
 }
 
 /**
-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 05/12] gfs2: Calculate number of revokes during evict

[Bob Peterson]

Before this patch, function gfs2_evict_inode would start a transaction
in order to write some unknown number of revokes. Instead of calculating
the value, it used sdp->sd_jdesc->jd_blocks, the number of blocks in the
entire journal. You can fit a lot of revokes in a block (in a 4K block
you can fit 503 of them). For a 512MB journal has 131072 blocks, it would
ask for 261 free blocks.

This patch changes it to calculate the number of revokes based on the
nrpages values of the address space plus glock address space. Note that
since it does this after clearing out the ail list, we don't need to
use the number of (old) revokes in the calculation. We calculate it from
the number of standing new items that need revoking.

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

diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 9f4d9e7be839..80ac446f0110 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -1336,6 +1336,7 @@ static void gfs2_evict_inode(struct inode *inode)
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_holder gh;
 	struct address_space *metamapping;
+	int nr_revokes;
 	int error;
 
 	if (test_bit(GIF_FREE_VFS_INODE, &ip->i_flags)) {
@@ -1434,7 +1435,11 @@ static void gfs2_evict_inode(struct inode *inode)
 	write_inode_now(inode, 1);
 	gfs2_ail_flush(ip->i_gl, 0);
 
-	error = gfs2_trans_begin(sdp, 0, sdp->sd_jdesc->jd_blocks);
+	nr_revokes = inode->i_mapping->nrpages + metamapping->nrpages;
+	if (!nr_revokes)
+		goto out_unlock;
+
+	error = gfs2_trans_begin(sdp, 0, nr_revokes);
 	if (error)
 		goto out_unlock;
 	/* Needs to be done before glock release & also in a transaction */
-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 06/12] gfs2: Create transaction for inodes with i_nlink != 0

[Bob Peterson]

Before this patch, function gfs2_evict_inode would check if i_nlink
was non-zero, and if so, go to label out. The problem is, the evicted
file may still have outstanding pages that need invalidating, but
the call to truncate_inode_pages_final at label out doesn't start a
transaction. It needs a transaction in order to write revokes for any
pages it has to invalidate.

This patch removes the early check for i_nlink in gfs2_evict_inode.
Not much further down in the code, there's another check for i_nlink
that skips to out_truncate. That one is proper because the calls
to truncate_inode_pages after out_truncate use a proper transaction,
so the page invalidates and subsequent revokes may be done properly.

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

diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 80ac446f0110..1f3dee740431 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -1344,7 +1344,7 @@ static void gfs2_evict_inode(struct inode *inode)
 		return;
 	}
 
-	if (inode->i_nlink || sb_rdonly(sb))
+	if (sb_rdonly(sb))
 		goto out;
 
 	if (test_bit(GIF_ALLOC_FAILED, &ip->i_flags)) {
@@ -1370,15 +1370,19 @@ static void gfs2_evict_inode(struct inode *inode)
 	}
 
 	if (gfs2_inode_already_deleted(ip->i_gl, ip->i_no_formal_ino))
-		goto out_truncate;
+		goto out_flush;
 	error = gfs2_check_blk_type(sdp, ip->i_no_addr, GFS2_BLKST_UNLINKED);
-	if (error)
-		goto out_truncate;
+	if (error) {
+		error = 0;
+		goto out_flush;
+	}
 
 	if (test_bit(GIF_INVALID, &ip->i_flags)) {
 		error = gfs2_inode_refresh(ip);
-		if (error)
-			goto out_truncate;
+		if (error) {
+			error = 0;
+			goto out_flush;
+		}
 	}
 
 	/*
@@ -1392,7 +1396,7 @@ static void gfs2_evict_inode(struct inode *inode)
 	    test_bit(HIF_HOLDER, &ip->i_iopen_gh.gh_iflags)) {
 		if (!gfs2_upgrade_iopen_glock(inode)) {
 			gfs2_holder_uninit(&ip->i_iopen_gh);
-			goto out_truncate;
+			goto out_flush;
 		}
 	}
 
@@ -1424,7 +1428,7 @@ static void gfs2_evict_inode(struct inode *inode)
 	gfs2_inode_remember_delete(ip->i_gl, ip->i_no_formal_ino);
 	goto out_unlock;
 
-out_truncate:
+out_flush:
 	gfs2_log_flush(sdp, ip->i_gl, GFS2_LOG_HEAD_FLUSH_NORMAL |
 		       GFS2_LFC_EVICT_INODE);
 	metamapping = gfs2_glock2aspace(ip->i_gl);
@@ -1435,6 +1439,7 @@ static void gfs2_evict_inode(struct inode *inode)
 	write_inode_now(inode, 1);
 	gfs2_ail_flush(ip->i_gl, 0);
 
+out_truncate:
 	nr_revokes = inode->i_mapping->nrpages + metamapping->nrpages;
 	if (!nr_revokes)
 		goto out_unlock;
-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 07/12] gfs2: make gfs2_ail1_empty_one return the count of active items

[Bob Peterson]

This patch is one baby step toward simplifying the journal management.
It simply changes function gfs2_ail1_empty_one from a void to an int and
makes it return a count of active items. This allows the caller to check
the return code rather than list_empty on the tr_ail1_list. This way
we can, in a later patch, combine transaction ail1 and ail2 lists.

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

diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
index 3e24e8733950..4fb1a96b8124 100644
--- a/fs/gfs2/log.c
+++ b/fs/gfs2/log.c
@@ -244,13 +244,15 @@ static void gfs2_ail1_start(struct gfs2_sbd *sdp)
  * @tr: the transaction
  * @max_revokes: If nonzero, issue revokes for the bd items for written buffers
  *
+ * returns: the transaction's count of remaining active items
  */
 
-static void gfs2_ail1_empty_one(struct gfs2_sbd *sdp, struct gfs2_trans *tr,
+static int gfs2_ail1_empty_one(struct gfs2_sbd *sdp, struct gfs2_trans *tr,
 				int *max_revokes)
 {
 	struct gfs2_bufdata *bd, *s;
 	struct buffer_head *bh;
+	int active_count = 0;
 
 	list_for_each_entry_safe_reverse(bd, s, &tr->tr_ail1_list,
 					 bd_ail_st_list) {
@@ -265,8 +267,10 @@ static void gfs2_ail1_empty_one(struct gfs2_sbd *sdp, struct gfs2_trans *tr,
 		 * If the ail buffer is not busy and caught an error, flag it
 		 * for others.
 		 */
-		if (!sdp->sd_log_error && buffer_busy(bh))
+		if (!sdp->sd_log_error && buffer_busy(bh)) {
+			active_count++;
 			continue;
+		}
 		if (!buffer_uptodate(bh) &&
 		    !cmpxchg(&sdp->sd_log_error, 0, -EIO)) {
 			gfs2_io_error_bh(sdp, bh);
@@ -285,6 +289,7 @@ static void gfs2_ail1_empty_one(struct gfs2_sbd *sdp, struct gfs2_trans *tr,
 		}
 		list_move(&bd->bd_ail_st_list, &tr->tr_ail2_list);
 	}
+	return active_count;
 }
 
 /**
@@ -303,8 +308,7 @@ static int gfs2_ail1_empty(struct gfs2_sbd *sdp, int max_revokes)
 
 	spin_lock(&sdp->sd_ail_lock);
 	list_for_each_entry_safe_reverse(tr, s, &sdp->sd_ail1_list, tr_list) {
-		gfs2_ail1_empty_one(sdp, tr, &max_revokes);
-		if (list_empty(&tr->tr_ail1_list) && oldest_tr)
+		if (!gfs2_ail1_empty_one(sdp, tr, &max_revokes) && oldest_tr)
 			list_move(&tr->tr_list, &sdp->sd_ail2_list);
 		else
 			oldest_tr = 0;
-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 08/12] gfs2: don't lock sd_ail_lock in gfs2_releasepage

[Bob Peterson]

Patch 380f7c65a7eb3288e4b6812acf3474a1de230707 changed gfs2_releasepage
so that it held the sd_ail_lock spin_lock for most of its processing.
It did this for some mysterious undocumented bug somewhere in the
evict code path. But in the nine years since, evict has been reworked
and fixed many times, and so have the transactions and ail list.
I can't see a reason to hold the sd_ail_lock unless it's protecting
the actual ail lists hung off the transactions. Therefore, this patch
removes the locking to increase speed and efficiency, and to further help
us rework the log flush code to be more concurrent with transactions.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/aops.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index f6e36a33b392..5c1914c8968f 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -744,7 +744,6 @@ int gfs2_releasepage(struct page *page, gfp_t gfp_mask)
 	 */
 
 	gfs2_log_lock(sdp);
-	spin_lock(&sdp->sd_ail_lock);
 	head = bh = page_buffers(page);
 	do {
 		if (atomic_read(&bh->b_count))
@@ -756,7 +755,6 @@ int gfs2_releasepage(struct page *page, gfp_t gfp_mask)
 			goto cannot_release;
 		bh = bh->b_this_page;
 	} while(bh != head);
-	spin_unlock(&sdp->sd_ail_lock);
 
 	head = bh = page_buffers(page);
 	do {
@@ -782,7 +780,6 @@ int gfs2_releasepage(struct page *page, gfp_t gfp_mask)
 	return try_to_free_buffers(page);
 
 cannot_release:
-	spin_unlock(&sdp->sd_ail_lock);
 	gfs2_log_unlock(sdp);
 	return 0;
 }
-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 09/12] gfs2: Only set PageChecked if we have a transaction

[Bob Peterson]

With jdata writes, we frequently got into situations where gfs2 deadlocked
because of this calling sequence:

gfs2_ail1_start
   gfs2_ail1_flush - for every tr on the sd_ail1_list:
      gfs2_ail1_start_one - for every bd on the tr's tr_ail1_list:
         generic_writepages
	    write_cache_pages passing __writepage()
	       calls clear_page_dirty_for_io which calls set_page_dirty:
	          which calls jdata_set_page_dirty which sets PageChecked.
	       __writepage() calls
	          mapping->a_ops->writepage AKA gfs2_jdata_writepage

However, gfs2_jdata_writepage checks if PageChecked is set, and if so, it
ignores the write and redirties the page. The problem is that write_cache_pages
calls clear_page_dirty_for_io, which often calls set_page_dirty(). See comments
in page-writeback.c starting with "Yes, Virginia". If it's jdata,
set_page_dirty will call jdata_set_page_dirty which will set PageChecked.
That causes a conflict because it makes it look like the page has been
redirtied by another writer, in which case we need to skip writing it and
redirty the page. That ends up in a deadlock because it isn't a "real" writer
and nothing will ever clear PageChecked.

If we do have a real writer, it will have started a transaction. So this
patch checks if a transaction is in use, and if not, it skips setting
PageChecked. That way, the page will be dirtied, cleaned, and written
appropriately.

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

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 5c1914c8968f..9b2e927e6535 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -626,7 +626,8 @@ void adjust_fs_space(struct inode *inode)
  
 static int jdata_set_page_dirty(struct page *page)
 {
-	SetPageChecked(page);
+	if (current->journal_info)
+		SetPageChecked(page);
 	return __set_page_dirty_buffers(page);
 }
 
-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 10/12] gfs2: simplify gfs2_block_map

[Bob Peterson]

Function gfs2_block_map had a lot of redundancy between its create and
no_create paths. This patch simplifies the code to eliminate the redundancy.

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

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 6d2ea788d0a1..67aff327bf38 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -1291,6 +1291,7 @@ int gfs2_block_map(struct inode *inode, sector_t lblock,
 	loff_t length = bh_map->b_size;
 	struct metapath mp = { .mp_aheight = 1, };
 	struct iomap iomap = { };
+	int flags = create ? IOMAP_WRITE : 0;
 	int ret;
 
 	clear_buffer_mapped(bh_map);
@@ -1298,15 +1299,10 @@ int gfs2_block_map(struct inode *inode, sector_t lblock,
 	clear_buffer_boundary(bh_map);
 	trace_gfs2_bmap(ip, bh_map, lblock, create, 1);
 
-	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, &mp);
-		release_metapath(&mp);
-	} else {
-		ret = gfs2_iomap_get(inode, pos, length, 0, &iomap, &mp);
-		release_metapath(&mp);
-	}
+	ret = gfs2_iomap_get(inode, pos, length, flags, &iomap, &mp);
+	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 11/12] gfs2: Ignore journal log writes for jdata holes

[Bob Peterson]

Before this patch, when flushing out its ail1 list, gfs2_write_jdata_page
called __block_write_full_page passing in function gfs2_get_block_noalloc.
But there was a problem when a process wrote to a jdata file, then
truncated it or punched a hole, leaving references to the blocks within
the new hole in its ail list, which are to be written to the journal log.

In writing them to the journal, after calling gfs2_block_map, function
gfs2_get_block_noalloc, determined that the (hole-punched) block was not
mapped, so it returned -EIO to generic_writepages, which passed it back
to gfs2_ail1_start_one. This, in turn, performed a withdraw, assuming
there was a real IO error writing to the journal.

This might be a valid error when writing metadata to the journal, but for
journaled data writes, it does not warrant with withdraw.

This patch makes a separate function gfs2_get_jblock_noalloc which checks
for this situation and makes an exception for journaled data writes that
correspond to jdata holes. Other errors are returned as before.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/aops.c | 27 +++++++++++++++++++++++++--
 fs/gfs2/bmap.c |  8 ++++++--
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 9b2e927e6535..b2ab0077e150 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -81,6 +81,29 @@ static int gfs2_get_block_noalloc(struct inode *inode, sector_t lblock,
 	return 0;
 }
 
+/**
+ * gfs2_get_jblock_noalloc - Fills in a buffer head with details about a block
+ * @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
+ *
+ * Returns: errno
+ */
+
+static int gfs2_get_jblock_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 -EIO;
+	return 0;
+}
+
 /**
  * gfs2_writepage - Write page for writeback mappings
  * @page: The page
@@ -142,8 +165,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, gfs2_get_jblock_noalloc,
+				      wbc, end_buffer_async_write);
 }
 
 /**
diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 67aff327bf38..2e3b121fb78b 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -1300,8 +1300,12 @@ 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 (create && !ret && iomap.type == IOMAP_HOLE)
-		ret = gfs2_iomap_alloc(inode, &iomap, &mp);
+	if (!ret && iomap.type == IOMAP_HOLE) {
+		if (create)
+			ret = gfs2_iomap_alloc(inode, &iomap, &mp);
+		else
+			ret = -ENODATA;
+	}
 	release_metapath(&mp);
 	if (ret)
 		goto out;
-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 12/12] gfs2: add some much needed cleanup for log flushes that fail

[Bob Peterson]

When a log flush fails due to io errors, it signals the failure but does
not clean up after itself very well. This is because buffers are added to
the transaction tr_buf and tr_databuf queue, but the io error causes
gfs2_log_flush to bypass the "after_commit" functions responsible for
dequeueing the bd elements. If the bd elements are added to the ail list
before the error, function ail_drain takes care of dequeueing them.
But if they haven't gotten that far, the elements are forgotten and
make the transactions unable to be freed.

This patch introduces new function trans_drain which drains the bd
elements from the transaction so they can be freed properly.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/log.c   | 31 +++++++++++++++++++++++++++++++
 fs/gfs2/trans.c |  1 +
 2 files changed, 32 insertions(+)

diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
index 4fb1a96b8124..2e2625ee59e3 100644
--- a/fs/gfs2/log.c
+++ b/fs/gfs2/log.c
@@ -913,6 +913,36 @@ static void empty_ail1_list(struct gfs2_sbd *sdp)
 	}
 }
 
+/**
+ * drain_bd - drain the buf and databuf queue for a failed transaction
+ * @tr: the transaction to drain
+ *
+ * When this is called, we're taking an error exit for a log write that failed
+ * but since we bypassed the after_commit functions, we need to remove the
+ * items from the buf and databuf queue.
+ */
+static void trans_drain(struct gfs2_trans *tr)
+{
+	struct gfs2_bufdata *bd;
+	struct list_head *head;
+
+	if (!tr)
+		return;
+
+	head = &tr->tr_buf;
+	while (!list_empty(head)) {
+		bd = list_first_entry(head, struct gfs2_bufdata, bd_list);
+		list_del_init(&bd->bd_list);
+		kmem_cache_free(gfs2_bufdata_cachep, bd);
+	}
+	head = &tr->tr_databuf;
+	while (!list_empty(head)) {
+		bd = list_first_entry(head, struct gfs2_bufdata, bd_list);
+		list_del_init(&bd->bd_list);
+		kmem_cache_free(gfs2_bufdata_cachep, bd);
+	}
+}
+
 /**
  * gfs2_log_flush - flush incore transaction(s)
  * @sdp: the filesystem
@@ -1017,6 +1047,7 @@ void gfs2_log_flush(struct gfs2_sbd *sdp, struct gfs2_glock *gl, u32 flags)
 
 out:
 	if (gfs2_withdrawn(sdp)) {
+		trans_drain(tr);
 		/**
 		 * If the tr_list is empty, we're withdrawing during a log
 		 * flush that targets a transaction, but the transaction was
diff --git a/fs/gfs2/trans.c b/fs/gfs2/trans.c
index e1c7eb6eb00a..6d4bf7ea7b3b 100644
--- a/fs/gfs2/trans.c
+++ b/fs/gfs2/trans.c
@@ -67,6 +67,7 @@ int gfs2_trans_begin(struct gfs2_sbd *sdp, unsigned int blocks,
 		tr->tr_reserved += gfs2_struct2blk(sdp, revokes);
 	INIT_LIST_HEAD(&tr->tr_databuf);
 	INIT_LIST_HEAD(&tr->tr_buf);
+	INIT_LIST_HEAD(&tr->tr_list);
 	INIT_LIST_HEAD(&tr->tr_ail1_list);
 	INIT_LIST_HEAD(&tr->tr_ail2_list);
 
-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 06/12] gfs2: Create transaction for inodes with i_nlink != 0

[Andreas Gruenbacher]

On Fri, Aug 21, 2020 at 7:33 PM Bob Peterson <rpeterso@redhat.com> wrote:
> Before this patch, function gfs2_evict_inode would check if i_nlink
> was non-zero, and if so, go to label out. The problem is, the evicted
> file may still have outstanding pages that need invalidating, but
> the call to truncate_inode_pages_final at label out doesn't start a
> transaction. It needs a transaction in order to write revokes for any
> pages it has to invalidate.

This is only true for jdata inodes though, right? If so, I'd rather
just create transactions in the jdata case.

> This patch removes the early check for i_nlink in gfs2_evict_inode.
> Not much further down in the code, there's another check for i_nlink
> that skips to out_truncate. That one is proper because the calls
> to truncate_inode_pages after out_truncate use a proper transaction,
> so the page invalidates and subsequent revokes may be done properly.
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>  fs/gfs2/super.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index 80ac446f0110..1f3dee740431 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -1344,7 +1344,7 @@ static void gfs2_evict_inode(struct inode *inode)
>                 return;
>         }
>
> -       if (inode->i_nlink || sb_rdonly(sb))
> +       if (sb_rdonly(sb))
>                 goto out;
>         if (test_bit(GIF_ALLOC_FAILED, &ip->i_flags)) {
> @@ -1370,15 +1370,19 @@ static void gfs2_evict_inode(struct inode *inode)
>         }
>
>         if (gfs2_inode_already_deleted(ip->i_gl, ip->i_no_formal_ino))
> -               goto out_truncate;
> +               goto out_flush;
>         error = gfs2_check_blk_type(sdp, ip->i_no_addr, GFS2_BLKST_UNLINKED);
> -       if (error)
> -               goto out_truncate;
> +       if (error) {
> +               error = 0;
> +               goto out_flush;
> +       }
>
>         if (test_bit(GIF_INVALID, &ip->i_flags)) {
>                 error = gfs2_inode_refresh(ip);
> -               if (error)
> -                       goto out_truncate;
> +               if (error) {
> +                       error = 0;
> +                       goto out_flush;
> +               }
>         }
>
>         /*
> @@ -1392,7 +1396,7 @@ static void gfs2_evict_inode(struct inode *inode)
>             test_bit(HIF_HOLDER, &ip->i_iopen_gh.gh_iflags)) {
>                 if (!gfs2_upgrade_iopen_glock(inode)) {
>                         gfs2_holder_uninit(&ip->i_iopen_gh);
> -                       goto out_truncate;
> +                       goto out_flush;
>                 }
>         }
>
> @@ -1424,7 +1428,7 @@ static void gfs2_evict_inode(struct inode *inode)
>         gfs2_inode_remember_delete(ip->i_gl, ip->i_no_formal_ino);
>         goto out_unlock;
>
> -out_truncate:
> +out_flush:
>         gfs2_log_flush(sdp, ip->i_gl, GFS2_LOG_HEAD_FLUSH_NORMAL |
>                        GFS2_LFC_EVICT_INODE);
>         metamapping = gfs2_glock2aspace(ip->i_gl);
> @@ -1435,6 +1439,7 @@ static void gfs2_evict_inode(struct inode *inode)
>         write_inode_now(inode, 1);
>         gfs2_ail_flush(ip->i_gl, 0);
>
> +out_truncate:
>         nr_revokes = inode->i_mapping->nrpages + metamapping->nrpages;
>         if (!nr_revokes)
>                 goto out_unlock;
> --
> 2.26.2
>

Thanks,
Andreas

[Cluster-devel] [GFS2 PATCH 05/12] gfs2: Calculate number of revokes during evict

[Andreas Gruenbacher]

On Fri, Aug 21, 2020 at 7:33 PM Bob Peterson <rpeterso@redhat.com> wrote:
> Before this patch, function gfs2_evict_inode would start a transaction
> in order to write some unknown number of revokes. Instead of calculating
> the value, it used sdp->sd_jdesc->jd_blocks, the number of blocks in the
> entire journal. You can fit a lot of revokes in a block (in a 4K block
> you can fit 503 of them). For a 512MB journal has 131072 blocks, it would
> ask for 261 free blocks.
>
> This patch changes it to calculate the number of revokes based on the
> nrpages values of the address space plus glock address space. Note that
> since it does this after clearing out the ail list, we don't need to
> use the number of (old) revokes in the calculation. We calculate it from
> the number of standing new items that need revoking.
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>  fs/gfs2/super.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index 9f4d9e7be839..80ac446f0110 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -1336,6 +1336,7 @@ static void gfs2_evict_inode(struct inode *inode)
>         struct gfs2_inode *ip = GFS2_I(inode);
>         struct gfs2_holder gh;
>         struct address_space *metamapping;
> +       int nr_revokes;
>         int error;
>
>         if (test_bit(GIF_FREE_VFS_INODE, &ip->i_flags)) {
> @@ -1434,7 +1435,11 @@ static void gfs2_evict_inode(struct inode *inode)
>         write_inode_now(inode, 1);
>         gfs2_ail_flush(ip->i_gl, 0);
>
> -       error = gfs2_trans_begin(sdp, 0, sdp->sd_jdesc->jd_blocks);
> +       nr_revokes = inode->i_mapping->nrpages + metamapping->nrpages;

We could surely use a more realistic number of revokes here, but this
approach really doesn't make any sense, sorry.

> +       if (!nr_revokes)
> +               goto out_unlock;
> +
> +       error = gfs2_trans_begin(sdp, 0, nr_revokes);
>         if (error)
>                 goto out_unlock;
>         /* Needs to be done before glock release & also in a transaction */
> --
> 2.26.2

Thanks,
Andreas

[Cluster-devel] [GFS2 PATCH 07/12] gfs2: make gfs2_ail1_empty_one return the count of active items

[Andreas Gruenbacher]

Hi Bob,

On Fri, Aug 21, 2020 at 7:33 PM Bob Peterson <rpeterso@redhat.com> wrote:
> This patch is one baby step toward simplifying the journal management.
> It simply changes function gfs2_ail1_empty_one from a void to an int and
> makes it return a count of active items. This allows the caller to check
> the return code rather than list_empty on the tr_ail1_list. This way
> we can, in a later patch, combine transaction ail1 and ail2 lists.

what does "we can combine transaction ail1 and ail2 lists" mean?

> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>  fs/gfs2/log.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
> index 3e24e8733950..4fb1a96b8124 100644
> --- a/fs/gfs2/log.c
> +++ b/fs/gfs2/log.c
> @@ -244,13 +244,15 @@ static void gfs2_ail1_start(struct gfs2_sbd *sdp)
>   * @tr: the transaction
>   * @max_revokes: If nonzero, issue revokes for the bd items for written buffers
>   *
> + * returns: the transaction's count of remaining active items
>   */
>
> -static void gfs2_ail1_empty_one(struct gfs2_sbd *sdp, struct gfs2_trans *tr,
> +static int gfs2_ail1_empty_one(struct gfs2_sbd *sdp, struct gfs2_trans *tr,
>                                 int *max_revokes)
>  {
>         struct gfs2_bufdata *bd, *s;
>         struct buffer_head *bh;
> +       int active_count = 0;
>
>         list_for_each_entry_safe_reverse(bd, s, &tr->tr_ail1_list,
>                                          bd_ail_st_list) {
> @@ -265,8 +267,10 @@ static void gfs2_ail1_empty_one(struct gfs2_sbd *sdp, struct gfs2_trans *tr,
>                  * If the ail buffer is not busy and caught an error, flag it
>                  * for others.
>                  */
> -               if (!sdp->sd_log_error && buffer_busy(bh))
> +               if (!sdp->sd_log_error && buffer_busy(bh)) {
> +                       active_count++;
>                         continue;
> +               }
>                 if (!buffer_uptodate(bh) &&
>                     !cmpxchg(&sdp->sd_log_error, 0, -EIO)) {
>                         gfs2_io_error_bh(sdp, bh);
> @@ -285,6 +289,7 @@ static void gfs2_ail1_empty_one(struct gfs2_sbd *sdp, struct gfs2_trans *tr,
>                 }
>                 list_move(&bd->bd_ail_st_list, &tr->tr_ail2_list);
>         }
> +       return active_count;
>  }
>
>  /**
> @@ -303,8 +308,7 @@ static int gfs2_ail1_empty(struct gfs2_sbd *sdp, int max_revokes)
>
>         spin_lock(&sdp->sd_ail_lock);
>         list_for_each_entry_safe_reverse(tr, s, &sdp->sd_ail1_list, tr_list) {
> -               gfs2_ail1_empty_one(sdp, tr, &max_revokes);
> -               if (list_empty(&tr->tr_ail1_list) && oldest_tr)
> +               if (!gfs2_ail1_empty_one(sdp, tr, &max_revokes) && oldest_tr)
>                         list_move(&tr->tr_list, &sdp->sd_ail2_list);
>                 else
>                         oldest_tr = 0;
> --
> 2.26.2
>

Thanks,
Andreas

[Cluster-devel] [GFS2 PATCH 06/12] gfs2: Create transaction for inodes with i_nlink != 0

[Steven Whitehouse]

Hi,

On 27/08/2020 07:00, Andreas Gruenbacher wrote:
> On Fri, Aug 21, 2020 at 7:33 PM Bob Peterson <rpeterso@redhat.com> wrote:
>> Before this patch, function gfs2_evict_inode would check if i_nlink
>> was non-zero, and if so, go to label out. The problem is, the evicted
>> file may still have outstanding pages that need invalidating, but
>> the call to truncate_inode_pages_final at label out doesn't start a
>> transaction. It needs a transaction in order to write revokes for any
>> pages it has to invalidate.
> This is only true for jdata inodes though, right? If so, I'd rather
> just create transactions in the jdata case.

Yes, and also if the inode is being deallocated, then we might be able 
to skip that step. We'll no doubt have to retain it in case this is just 
an unlink and there are still openers somewhere,

Steve.


>> This patch removes the early check for i_nlink in gfs2_evict_inode.
>> Not much further down in the code, there's another check for i_nlink
>> that skips to out_truncate. That one is proper because the calls
>> to truncate_inode_pages after out_truncate use a proper transaction,
>> so the page invalidates and subsequent revokes may be done properly.
>>
>> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
>> ---
>>   fs/gfs2/super.c | 21 +++++++++++++--------
>>   1 file changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
>> index 80ac446f0110..1f3dee740431 100644
>> --- a/fs/gfs2/super.c
>> +++ b/fs/gfs2/super.c
>> @@ -1344,7 +1344,7 @@ static void gfs2_evict_inode(struct inode *inode)
>>                  return;
>>          }
>>
>> -       if (inode->i_nlink || sb_rdonly(sb))
>> +       if (sb_rdonly(sb))
>>                  goto out;
>>          if (test_bit(GIF_ALLOC_FAILED, &ip->i_flags)) {
>> @@ -1370,15 +1370,19 @@ static void gfs2_evict_inode(struct inode *inode)
>>          }
>>
>>          if (gfs2_inode_already_deleted(ip->i_gl, ip->i_no_formal_ino))
>> -               goto out_truncate;
>> +               goto out_flush;
>>          error = gfs2_check_blk_type(sdp, ip->i_no_addr, GFS2_BLKST_UNLINKED);
>> -       if (error)
>> -               goto out_truncate;
>> +       if (error) {
>> +               error = 0;
>> +               goto out_flush;
>> +       }
>>
>>          if (test_bit(GIF_INVALID, &ip->i_flags)) {
>>                  error = gfs2_inode_refresh(ip);
>> -               if (error)
>> -                       goto out_truncate;
>> +               if (error) {
>> +                       error = 0;
>> +                       goto out_flush;
>> +               }
>>          }
>>
>>          /*
>> @@ -1392,7 +1396,7 @@ static void gfs2_evict_inode(struct inode *inode)
>>              test_bit(HIF_HOLDER, &ip->i_iopen_gh.gh_iflags)) {
>>                  if (!gfs2_upgrade_iopen_glock(inode)) {
>>                          gfs2_holder_uninit(&ip->i_iopen_gh);
>> -                       goto out_truncate;
>> +                       goto out_flush;
>>                  }
>>          }
>>
>> @@ -1424,7 +1428,7 @@ static void gfs2_evict_inode(struct inode *inode)
>>          gfs2_inode_remember_delete(ip->i_gl, ip->i_no_formal_ino);
>>          goto out_unlock;
>>
>> -out_truncate:
>> +out_flush:
>>          gfs2_log_flush(sdp, ip->i_gl, GFS2_LOG_HEAD_FLUSH_NORMAL |
>>                         GFS2_LFC_EVICT_INODE);
>>          metamapping = gfs2_glock2aspace(ip->i_gl);
>> @@ -1435,6 +1439,7 @@ static void gfs2_evict_inode(struct inode *inode)
>>          write_inode_now(inode, 1);
>>          gfs2_ail_flush(ip->i_gl, 0);
>>
>> +out_truncate:
>>          nr_revokes = inode->i_mapping->nrpages + metamapping->nrpages;
>>          if (!nr_revokes)
>>                  goto out_unlock;
>> --
>> 2.26.2
>>
> Thanks,
> Andreas
>

[Cluster-devel] [GFS2 PATCH 06/12] gfs2: Create transaction for inodes with i_nlink != 0

[Bob Peterson]

----- Original Message -----
> On Fri, Aug 21, 2020 at 7:33 PM Bob Peterson <rpeterso@redhat.com> wrote:
> > Before this patch, function gfs2_evict_inode would check if i_nlink
> > was non-zero, and if so, go to label out. The problem is, the evicted
> > file may still have outstanding pages that need invalidating, but
> > the call to truncate_inode_pages_final at label out doesn't start a
> > transaction. It needs a transaction in order to write revokes for any
> > pages it has to invalidate.
> 
> This is only true for jdata inodes though, right? If so, I'd rather
> just create transactions in the jdata case.

The truncate_inode_pages_final() for i_data is only for jdata, which
includes directories for their hash tables. However, for regular files,
evict's call to gfs2_glock_put_eventually() has the potential to be the
last put for the inode's glock (in a race), which might still have pages
attached (metamapping). I firmly believe this is our "nrpages" bug I've been
chasing, but I haven't proven it yet because it's very hard to recreate.

Afaik, some of these unresolved metadata pages may still need revokes, and
we still need a transaction to do that, even if the dinode still has links.

The "nrpages" problem always seems to involve the system quotas file,
probably because it's jdata, but imagine a directory with a large hash
table, which is modified, then is quickly evicted (without being deleted).

It wasn't that long ago I was working on a patch to take glock reference
even sooner than we did for f4e2f5e1a527ce58fc9f85145b03704779a3123e.
I titled the patch "grab glock reference as early as possible in transactions
but it was never pushed anywhere because it added a new atomic to the
glock. It may be an alternative solution to the problem. My comments on
that patch were:

   Before this patch, an additional glock reference was taken when
   the bufdata element, bd, was revoked. That's not early enough
   because the caller who created the bd (via trans_add_meta) may
   have already come and gone with the bd still not revoked (but
   in the ail).
   
   This patch takes the glock reference earlier in the process, when
   the first bd element is allocated for a glock. It queues the glock
   reference to be put when the last bd element for the glock is freed.
   
   To this end, a new atomic glock field, gl_bd_count, keeps count.

Regards,

Bob Peterson

[Cluster-devel] [GFS2 PATCH 07/12] gfs2: make gfs2_ail1_empty_one return the count of active items

[Bob Peterson]

----- Original Message -----
> Hi Bob,
> 
> On Fri, Aug 21, 2020 at 7:33 PM Bob Peterson <rpeterso@redhat.com> wrote:
> > This patch is one baby step toward simplifying the journal management.
> > It simply changes function gfs2_ail1_empty_one from a void to an int and
> > makes it return a count of active items. This allows the caller to check
> > the return code rather than list_empty on the tr_ail1_list. This way
> > we can, in a later patch, combine transaction ail1 and ail2 lists.
> 
> what does "we can combine transaction ail1 and ail2 lists" mean?

Hi Andreas,

Well, I've been looking into ways we can simplify the journal logging
code to give us more throughput. One way is to reduce the number of
calls to lock the ail lists. Right now we lock the sd_ail_lock spin_lock
when we add items to the ail1, move them from ail1 to ail2, and for
processing the items on ail2. If we had one combined "ail_list" rather
than two (ail1 and ail2) we wouldn't need to move items from list to list.
We could instead just flag the bufdata elements (somehow--like set_bit
and clear_bit) which could allow us to grab that spin_lock less often,
leading to better throughput. This in turn might lead us to other
simplifications.

Bob Peterson