[Cluster-devel] [PATCH 1/3] rhashtable: Add rhashtable_lookup_get_insert_fast

[Cluster-devel] [PATCH 1/3] rhashtable: Add rhashtable_lookup_get_insert_fast

[Andreas Gruenbacher]

Add rhashtable_lookup_get_insert_fast for fixed keys, similar to
rhashtable_lookup_get_insert_key for explicit keys.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
---
 include/linux/rhashtable.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h
index 092292b..e507290 100644
--- a/include/linux/rhashtable.h
+++ b/include/linux/rhashtable.h
@@ -916,6 +916,28 @@ static inline int rhashtable_lookup_insert_fast(
 }
 
 /**
+ * rhashtable_lookup_get_insert_fast - lookup and insert object into hash table
+ * @ht:		hash table
+ * @obj:	pointer to hash head inside object
+ * @params:	hash table parameters
+ *
+ * Just like rhashtable_lookup_insert_fast(), but this function returns the
+ * object if it exists, NULL if it did not and the insertion was successful,
+ * and an ERR_PTR otherwise.
+ */
+static inline void *rhashtable_lookup_get_insert_fast(
+	struct rhashtable *ht, struct rhash_head *obj,
+	const struct rhashtable_params params)
+{
+	const char *key = rht_obj(ht, obj);
+
+	BUG_ON(ht->p.obj_hashfn);
+
+	return __rhashtable_insert_fast(ht, key + ht->p.key_offset, obj, params,
+					false);
+}
+
+/**
  * rhashtable_lookup_insert_key - search and insert object to hash table
  *				  with explicit key
  * @ht:		hash table
-- 
2.7.4

[Cluster-devel] [PATCH 2/3] gfs2: Switch to rhashtable_lookup_get_insert_fast

[Andreas Gruenbacher]

Switch from rhashtable_lookup_insert_fast + rhashtable_lookup_fast to
rhashtable_lookup_get_insert_fast, which is cleaner and avoids an extra
rhashtable lookup.

At the same time, turn the retry loop in gfs2_glock_get into an infinite
loop.  The lookup or insert will eventually succeed, usually very fast,
but there is no reason to give up trying at a fixed number of
iterations.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/glock.c | 45 +++++++++++++++++++++------------------------
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 35f3b0a..9e81219 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -655,10 +655,10 @@ int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number,
 	struct lm_lockname name = { .ln_number = number,
 				    .ln_type = glops->go_type,
 				    .ln_sbd = sdp };
-	struct gfs2_glock *gl, *tmp = NULL;
+	struct gfs2_glock *gl, *tmp;
 	struct address_space *mapping;
 	struct kmem_cache *cachep;
-	int ret, tries = 0;
+	int ret = 0;
 
 	rcu_read_lock();
 	gl = rhashtable_lookup_fast(&gl_hash_table, &name, ht_parms);
@@ -723,35 +723,32 @@ int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number,
 	}
 
 again:
-	ret = rhashtable_lookup_insert_fast(&gl_hash_table, &gl->gl_node,
-					    ht_parms);
-	if (ret == 0) {
+	rcu_read_lock();
+	tmp = rhashtable_lookup_get_insert_fast(&gl_hash_table, &gl->gl_node,
+						ht_parms);
+	if (!tmp) {
 		*glp = gl;
-		return 0;
+		goto out;
 	}
-
-	if (ret == -EEXIST) {
-		ret = 0;
-		rcu_read_lock();
-		tmp = rhashtable_lookup_fast(&gl_hash_table, &name, ht_parms);
-		if (tmp == NULL || !lockref_get_not_dead(&tmp->gl_lockref)) {
-			if (++tries < 100) {
-				rcu_read_unlock();
-				cond_resched();
-				goto again;
-			}
-			tmp = NULL;
-			ret = -ENOMEM;
-		}
-		rcu_read_unlock();
-	} else {
-		WARN_ON_ONCE(ret);
+	if (IS_ERR(tmp)) {
+		ret = PTR_ERR(tmp);
+		goto out_free;
+	}
+	if (lockref_get_not_dead(&tmp->gl_lockref)) {
+		*glp = tmp;
+		goto out_free;
 	}
+	rcu_read_unlock();
+	cond_resched();
+	goto again;
+
+out_free:
 	kfree(gl->gl_lksb.sb_lvbptr);
 	kmem_cache_free(cachep, gl);
 	atomic_dec(&sdp->sd_glock_disposal);
-	*glp = tmp;
 
+out:
+	rcu_read_unlock();
 	return ret;
 }
 
-- 
2.7.4

[Cluster-devel] [PATCH 3/3] Revert "GFS2: Wait for iopen glock dequeues"

[Andreas Gruenbacher]

Revert commit 86d067a797d4e8546a7c92b985f31e8cd3ec39ad: it turns out
that waiting for iopen glock dequeues here isn't needed anymore because
the bugs that commit was meant to fix have been fixed otherwise.

In addition, we want to avoid waiting on glocks in gfs2_evict_inode in
shrinker context because the shrinker may be invoked on behalf of DLM,
in which case calling into DLM again would deadlock.  This commit makes
the described scenario less likely without completely avoiding it; it's
still a step in the right direction, though.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/inode.c | 3 +--
 fs/gfs2/super.c | 8 +++-----
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index e279c3c..de919f1 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -202,8 +202,7 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type,
 fail_refresh:
 	ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
 	ip->i_iopen_gh.gh_gl->gl_object = NULL;
-	gfs2_glock_dq_wait(&ip->i_iopen_gh);
-	gfs2_holder_uninit(&ip->i_iopen_gh);
+	gfs2_glock_dq_uninit(&ip->i_iopen_gh);
 fail_put:
 	if (io_gl)
 		gfs2_glock_put(io_gl);
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 2a9a830..29b0473 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -1539,8 +1539,7 @@ static void gfs2_evict_inode(struct inode *inode)
 	error = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, GL_SKIP, &gh);
 	if (unlikely(error)) {
 		ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
-		gfs2_glock_dq_wait(&ip->i_iopen_gh);
-		gfs2_holder_uninit(&ip->i_iopen_gh);
+		gfs2_glock_dq_uninit(&ip->i_iopen_gh);
 		goto out;
 	}
 
@@ -1618,7 +1617,7 @@ static void gfs2_evict_inode(struct inode *inode)
 	if (gfs2_holder_initialized(&ip->i_iopen_gh)) {
 		if (test_bit(HIF_HOLDER, &ip->i_iopen_gh.gh_iflags)) {
 			ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
-			gfs2_glock_dq_wait(&ip->i_iopen_gh);
+			gfs2_glock_dq(&ip->i_iopen_gh);
 		}
 		gfs2_holder_uninit(&ip->i_iopen_gh);
 	}
@@ -1640,8 +1639,7 @@ static void gfs2_evict_inode(struct inode *inode)
 	if (gfs2_holder_initialized(&ip->i_iopen_gh)) {
 		ip->i_iopen_gh.gh_gl->gl_object = NULL;
 		ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
-		gfs2_glock_dq_wait(&ip->i_iopen_gh);
-		gfs2_holder_uninit(&ip->i_iopen_gh);
+		gfs2_glock_dq_uninit(&ip->i_iopen_gh);
 	}
 }
 
-- 
2.7.4

[Cluster-devel] [PATCH 3/3] Revert "GFS2: Wait for iopen glock dequeues"

[Steven Whitehouse]

Hi,

All three look good to me.

Acked-by: Steven Whitehouse <swhiteho@redhat.com>

Steve.


On 17/03/17 14:14, Andreas Gruenbacher wrote:
> Revert commit 86d067a797d4e8546a7c92b985f31e8cd3ec39ad: it turns out
> that waiting for iopen glock dequeues here isn't needed anymore because
> the bugs that commit was meant to fix have been fixed otherwise.
>
> In addition, we want to avoid waiting on glocks in gfs2_evict_inode in
> shrinker context because the shrinker may be invoked on behalf of DLM,
> in which case calling into DLM again would deadlock.  This commit makes
> the described scenario less likely without completely avoiding it; it's
> still a step in the right direction, though.
>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>   fs/gfs2/inode.c | 3 +--
>   fs/gfs2/super.c | 8 +++-----
>   2 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index e279c3c..de919f1 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -202,8 +202,7 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type,
>   fail_refresh:
>   	ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
>   	ip->i_iopen_gh.gh_gl->gl_object = NULL;
> -	gfs2_glock_dq_wait(&ip->i_iopen_gh);
> -	gfs2_holder_uninit(&ip->i_iopen_gh);
> +	gfs2_glock_dq_uninit(&ip->i_iopen_gh);
>   fail_put:
>   	if (io_gl)
>   		gfs2_glock_put(io_gl);
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index 2a9a830..29b0473 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -1539,8 +1539,7 @@ static void gfs2_evict_inode(struct inode *inode)
>   	error = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, GL_SKIP, &gh);
>   	if (unlikely(error)) {
>   		ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
> -		gfs2_glock_dq_wait(&ip->i_iopen_gh);
> -		gfs2_holder_uninit(&ip->i_iopen_gh);
> +		gfs2_glock_dq_uninit(&ip->i_iopen_gh);
>   		goto out;
>   	}
>   
> @@ -1618,7 +1617,7 @@ static void gfs2_evict_inode(struct inode *inode)
>   	if (gfs2_holder_initialized(&ip->i_iopen_gh)) {
>   		if (test_bit(HIF_HOLDER, &ip->i_iopen_gh.gh_iflags)) {
>   			ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
> -			gfs2_glock_dq_wait(&ip->i_iopen_gh);
> +			gfs2_glock_dq(&ip->i_iopen_gh);
>   		}
>   		gfs2_holder_uninit(&ip->i_iopen_gh);
>   	}
> @@ -1640,8 +1639,7 @@ static void gfs2_evict_inode(struct inode *inode)
>   	if (gfs2_holder_initialized(&ip->i_iopen_gh)) {
>   		ip->i_iopen_gh.gh_gl->gl_object = NULL;
>   		ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
> -		gfs2_glock_dq_wait(&ip->i_iopen_gh);
> -		gfs2_holder_uninit(&ip->i_iopen_gh);
> +		gfs2_glock_dq_uninit(&ip->i_iopen_gh);
>   	}
>   }
>   

[Cluster-devel] [PATCH 3/3] Revert "GFS2: Wait for iopen glock dequeues"

[Bob Peterson]

Hi,

Thanks. I merged for-next with David Miller's net-next tree to grab
the first patch, then cherry-picked the other two and pushed it
out to our for-next branch.

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH 1/3] rhashtable: Add rhashtable_lookup_get_insert_fast

[Andreas Gruenbacher]

Dave,

is it a problem if this commit goes in through the gfs2 tree?

  https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git

Thanks,
Andreas

On Fri, Mar 17, 2017 at 3:14 PM, Andreas Gruenbacher
<agruenba@redhat.com> wrote:
> Add rhashtable_lookup_get_insert_fast for fixed keys, similar to
> rhashtable_lookup_get_insert_key for explicit keys.
>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: "David S. Miller" <davem@davemloft.net>
> ---
>  include/linux/rhashtable.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h
> index 092292b..e507290 100644
> --- a/include/linux/rhashtable.h
> +++ b/include/linux/rhashtable.h
> @@ -916,6 +916,28 @@ static inline int rhashtable_lookup_insert_fast(
>  }
>
>  /**
> + * rhashtable_lookup_get_insert_fast - lookup and insert object into hash table
> + * @ht:                hash table
> + * @obj:       pointer to hash head inside object
> + * @params:    hash table parameters
> + *
> + * Just like rhashtable_lookup_insert_fast(), but this function returns the
> + * object if it exists, NULL if it did not and the insertion was successful,
> + * and an ERR_PTR otherwise.
> + */
> +static inline void *rhashtable_lookup_get_insert_fast(
> +       struct rhashtable *ht, struct rhash_head *obj,
> +       const struct rhashtable_params params)
> +{
> +       const char *key = rht_obj(ht, obj);
> +
> +       BUG_ON(ht->p.obj_hashfn);
> +
> +       return __rhashtable_insert_fast(ht, key + ht->p.key_offset, obj, params,
> +                                       false);
> +}
> +
> +/**
>   * rhashtable_lookup_insert_key - search and insert object to hash table
>   *                               with explicit key
>   * @ht:                hash table
> --
> 2.7.4
>

[Cluster-devel] [PATCH 1/3] rhashtable: Add rhashtable_lookup_get_insert_fast

[David Miller]

From: Andreas Gruenbacher <agruenba@redhat.com>
Date: Fri, 17 Mar 2017 15:18:27 +0100

> Dave,
> 
> is it a problem if this commit goes in through the gfs2 tree?
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git

With no review whatseover on netdev or linux-kernel?  I don't think so.
Please repost with appropriate CC:'s added.