cluster-devel.redhat.com archive mirror
 help / color / mirror / Atom feed
From: Andreas Gruenbacher <agruenba@redhat.com>
To: cluster-devel.redhat.com
Subject: [Cluster-devel] [GFS2 PATCH 2/2] gfs2: Replace gl_revokes with a GLF flag
Date: Fri,  5 Apr 2019 19:54:12 +0200	[thread overview]
Message-ID: <20190405175412.26372-2-agruenba@redhat.com> (raw)
In-Reply-To: <20190405175412.26372-1-agruenba@redhat.com>

From: Bob Peterson <rpeterso@redhat.com>

The gl_revokes value determines how many outstanding revokes a glock has
on the superblock revokes list; this is used to avoid unnecessary log
flushes.  However, gl_revokes is only ever tested for being zero, and it's
only decremented in revoke_lo_after_commit, which removes all revokes
from the list, so we know that the gl_revoke values of all the glocks on
the list will reach zero.  Therefore, we can replace gl_revokes with a
bit flag. This saves an atomic counter in struct gfs2_glock.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/glock.c  |  4 ++--
 fs/gfs2/incore.h |  2 +-
 fs/gfs2/log.c    |  4 +++-
 fs/gfs2/lops.c   | 33 ++++++++++++++++++++++++---------
 fs/gfs2/main.c   |  1 -
 fs/gfs2/super.c  |  2 +-
 6 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 71c28ff98b564..15c605cfcfc85 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -140,7 +140,7 @@ void gfs2_glock_free(struct gfs2_glock *gl)
 {
 	struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
 
-	BUG_ON(atomic_read(&gl->gl_revokes));
+	BUG_ON(test_bit(GLF_REVOKES, &gl->gl_flags));
 	rhashtable_remove_fast(&gl_hash_table, &gl->gl_node, ht_parms);
 	smp_mb();
 	wake_up_glock(gl);
@@ -1801,7 +1801,7 @@ void gfs2_dump_glock(struct seq_file *seq, struct gfs2_glock *gl)
 		  state2str(gl->gl_target),
 		  state2str(gl->gl_demote_state), dtime,
 		  atomic_read(&gl->gl_ail_count),
-		  atomic_read(&gl->gl_revokes),
+		  test_bit(GLF_REVOKES, &gl->gl_flags) ? 1 : 0,
 		  (int)gl->gl_lockref.count, gl->gl_hold_time);
 
 	list_for_each_entry(gh, &gl->gl_holders, gh_list)
diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index 86840a70ee1ae..6a94b094a9043 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -345,6 +345,7 @@ enum {
 	GLF_OBJECT			= 14, /* Used only for tracing */
 	GLF_BLOCKING			= 15,
 	GLF_INODE_CREATING		= 16, /* Inode creation occurring */
+	GLF_REVOKES			= 17, /* Glock has revokes in queue */
 };
 
 struct gfs2_glock {
@@ -374,7 +375,6 @@ struct gfs2_glock {
 	struct list_head gl_lru;
 	struct list_head gl_ail_list;
 	atomic_t gl_ail_count;
-	atomic_t gl_revokes;
 	struct delayed_work gl_work;
 	union {
 		/* For inode and iopen glocks only */
diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
index 7ba94b66c91b2..d55315a46ecee 100644
--- a/fs/gfs2/log.c
+++ b/fs/gfs2/log.c
@@ -606,8 +606,10 @@ void gfs2_add_revoke(struct gfs2_sbd *sdp, struct gfs2_bufdata *bd)
 	gfs2_remove_from_ail(bd); /* drops ref on bh */
 	bd->bd_bh = NULL;
 	sdp->sd_log_num_revoke++;
-	if (atomic_inc_return(&gl->gl_revokes) == 1)
+	if (!test_bit(GLF_REVOKES, &gl->gl_flags)) {
+		set_bit(GLF_REVOKES, &gl->gl_flags);
 		gfs2_glock_hold(gl);
+	}
 	set_bit(GLF_LFLUSH, &gl->gl_flags);
 	list_add(&bd->bd_list, &sdp->sd_log_le_revoke);
 }
diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
index b4f0a6a3ba59d..2fd61853ba639 100644
--- a/fs/gfs2/lops.c
+++ b/fs/gfs2/lops.c
@@ -662,19 +662,34 @@ static void revoke_lo_before_commit(struct gfs2_sbd *sdp, struct gfs2_trans *tr)
 static void revoke_lo_after_commit(struct gfs2_sbd *sdp, struct gfs2_trans *tr)
 {
 	struct list_head *head = &sdp->sd_log_le_revoke;
-	struct gfs2_bufdata *bd;
-	struct gfs2_glock *gl;
+	struct gfs2_bufdata *bd, *tmp;
 
-	while (!list_empty(head)) {
-		bd = list_entry(head->next, struct gfs2_bufdata, bd_list);
-		list_del_init(&bd->bd_list);
-		gl = bd->bd_gl;
-		if (atomic_dec_return(&gl->gl_revokes) == 0) {
-			clear_bit(GLF_LFLUSH, &gl->gl_flags);
-			gfs2_glock_queue_put(gl);
+	/*
+	 * Glocks can be referenced repeatedly on the revoke list, but the list
+	 * only holds one reference.  All glocks on the list will have the
+	 * GLF_REVOKES flag set initially.
+	 */
+
+	list_for_each_entry_safe(bd, tmp, head, bd_list) {
+		struct gfs2_glock *gl = bd->bd_gl;
+
+		if (test_bit(GLF_REVOKES, &gl->gl_flags)) {
+			/* Keep each glock on the list exactly once. */
+			clear_bit(GLF_REVOKES, &gl->gl_flags);
+			continue;
 		}
+		list_del(&bd->bd_list);
+		kmem_cache_free(gfs2_bufdata_cachep, bd);
+	}
+	list_for_each_entry_safe(bd, tmp, head, bd_list) {
+		struct gfs2_glock *gl = bd->bd_gl;
+
+		list_del(&bd->bd_list);
 		kmem_cache_free(gfs2_bufdata_cachep, bd);
+		clear_bit(GLF_LFLUSH, &gl->gl_flags);
+		gfs2_glock_queue_put(gl);
 	}
+	/* the list is empty now */
 }
 
 static void revoke_lo_before_scan(struct gfs2_jdesc *jd,
diff --git a/fs/gfs2/main.c b/fs/gfs2/main.c
index 136484ef35d37..c700738de1f72 100644
--- a/fs/gfs2/main.c
+++ b/fs/gfs2/main.c
@@ -59,7 +59,6 @@ static void gfs2_init_glock_once(void *foo)
 	INIT_LIST_HEAD(&gl->gl_lru);
 	INIT_LIST_HEAD(&gl->gl_ail_list);
 	atomic_set(&gl->gl_ail_count, 0);
-	atomic_set(&gl->gl_revokes, 0);
 }
 
 static void gfs2_init_gl_aspace_once(void *foo)
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index ca71163ff7cfd..e20fa55715e21 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -1474,7 +1474,7 @@ static void gfs2_final_release_pages(struct gfs2_inode *ip)
 	truncate_inode_pages(gfs2_glock2aspace(ip->i_gl), 0);
 	truncate_inode_pages(&inode->i_data, 0);
 
-	if (atomic_read(&gl->gl_revokes) == 0) {
+	if (!test_bit(GLF_REVOKES, &gl->gl_flags)) {
 		clear_bit(GLF_LFLUSH, &gl->gl_flags);
 		clear_bit(GLF_DIRTY, &gl->gl_flags);
 	}
-- 
2.20.1



      reply	other threads:[~2019-04-05 17:54 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-05 17:54 [Cluster-devel] [GFS2 PATCH 1/2] gfs2: Fix occasional glock use-after-free Andreas Gruenbacher
2019-04-05 17:54 ` Andreas Gruenbacher [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190405175412.26372-2-agruenba@redhat.com \
    --to=agruenba@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).