[Cluster-devel] [PATCH v2 1/4] gfs2: gfs2_glock_get: Wait on freeing glocks

[Andreas Gruenbacher <agruenba@redhat.com>]

On Tue, Jul 4, 2017 at 2:13 PM, Steven Whitehouse <swhiteho@redhat.com> wrote:
> On 03/07/17 15:56, Andreas Gruenbacher wrote:
>>
>> Keep glocks in their hash table until they are freed instead of removing
>> them when their last reference is dropped.  This allows to wait for
>> glocks to go away before recreating them in gfs2_glock_get so that the
>> previous use of the glock won't overlap with the next.
>
> I did a double take when I looked at this and saw it reintroducing gl_rcu
> and I wondered where that had gone in the mean time. Is that a bug fix that
> is included alongside this new feature? At least if it is not required
> anyway, then I can't quite figure out why - maybe something hidden in the
> rhashtable code perhaps?

Hmm, now that you're mentioning it, I can't actually see how the rhashtable
code can be safe unless entries are freed in an RCU-safe way: otherwise,
processes reading the rhashtable can end up accessing random memory while
walking hash chains.  In this case that's rather unlikely, but still possible.

How do you like the following fix?

Thanks,
Andreas

--

gfs2: Fix freeing glock rhashtable entries

The glock rhashtable is accessed using RCU locking, so entries in the
hash table must only be freed in an RCU-safe way.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/glock.c  | 11 +++++++++--
 fs/gfs2/incore.h |  1 +
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 6cd71c5..c38ab6c 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -80,9 +80,9 @@ static struct rhashtable_params ht_parms = {
 
 static struct rhashtable gl_hash_table;
 
-void gfs2_glock_free(struct gfs2_glock *gl)
+static void gfs2_glock_dealloc(struct rcu_head *rcu)
 {
-	struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
+	struct gfs2_glock *gl = container_of(rcu, struct gfs2_glock, gl_rcu);
 
 	if (gl->gl_ops->go_flags & GLOF_ASPACE) {
 		kmem_cache_free(gfs2_glock_aspace_cachep, gl);
@@ -90,6 +90,13 @@ void gfs2_glock_free(struct gfs2_glock *gl)
 		kfree(gl->gl_lksb.sb_lvbptr);
 		kmem_cache_free(gfs2_glock_cachep, gl);
 	}
+}
+
+void gfs2_glock_free(struct gfs2_glock *gl)
+{
+	struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
+
+	call_rcu(&gl->gl_rcu, gfs2_glock_dealloc);
 	if (atomic_dec_and_test(&sdp->sd_glock_disposal))
 		wake_up(&sdp->sd_glock_wait);
 }
diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index de42384..7500566 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -374,6 +374,7 @@ struct gfs2_glock {
 		} gl_vm;
 	};
 	struct rhash_head gl_node;
+	struct rcu_head gl_rcu;
 };
 
 #define GFS2_MIN_LVB_SIZE 32	/* Min size of LVB that gfs2 supports */
-- 
2.7.5