[Cluster-devel] [PATCH] gfs2: Add missing rcu locking for glock lookup

[Cluster-devel] [PATCH] gfs2: Add missing rcu locking for glock lookup

[Andreas Gruenbacher]

From: Andrew Price <anprice@redhat.com>

We must hold the rcu read lock across looking up glocks and trying to
bump their refcount to prevent the glocks from being freed in between.

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

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 94f50ca..1d60f5f 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -658,9 +658,11 @@ int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number,
 	struct kmem_cache *cachep;
 	int ret, tries = 0;
 
+	rcu_read_lock();
 	gl = rhashtable_lookup_fast(&gl_hash_table, &name, ht_parms);
 	if (gl && !lockref_get_not_dead(&gl->gl_lockref))
 		gl = NULL;
+	rcu_read_unlock();
 
 	*glp = gl;
 	if (gl)
@@ -728,15 +730,18 @@ int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number,
 
 	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);
 	}
-- 
2.7.4

[Cluster-devel] [PATCH] gfs2: Add missing rcu locking for glock lookup

[Steven Whitehouse]

Hi,

Looks good, but can we fix that "retry for 100 times" loop too at the 
same time? There does appear to be an rhashtable API function that does 
what we need there,

Steve.

On 22/02/17 15:09, Andreas Gruenbacher wrote:
> From: Andrew Price <anprice@redhat.com>
>
> We must hold the rcu read lock across looking up glocks and trying to
> bump their refcount to prevent the glocks from being freed in between.
>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>   fs/gfs2/glock.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> index 94f50ca..1d60f5f 100644
> --- a/fs/gfs2/glock.c
> +++ b/fs/gfs2/glock.c
> @@ -658,9 +658,11 @@ int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number,
>   	struct kmem_cache *cachep;
>   	int ret, tries = 0;
>   
> +	rcu_read_lock();
>   	gl = rhashtable_lookup_fast(&gl_hash_table, &name, ht_parms);
>   	if (gl && !lockref_get_not_dead(&gl->gl_lockref))
>   		gl = NULL;
> +	rcu_read_unlock();
>   
>   	*glp = gl;
>   	if (gl)
> @@ -728,15 +730,18 @@ int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number,
>   
>   	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);
>   	}

[Cluster-devel] [PATCH] gfs2: Add missing rcu locking for glock lookup

[Andreas Gruenbacher]

On Wed, Feb 22, 2017 at 4:13 PM, Steven Whitehouse <swhiteho@redhat.com> wrote:
> Hi,
>
> Looks good, but can we fix that "retry for 100 times" loop too at the same
> time? There does appear to be an rhashtable API function that does what we
> need there,

The rhashtable cleanup which I've posted separately doesn't actually
affect the weird retry loop. The right thing to do here, so far, would
be to retry forever I believe.

Andreas

[Cluster-devel] [PATCH] gfs2: Add missing rcu locking for glock lookup

[Andreas Gruenbacher]

On Wed, Feb 22, 2017 at 4:26 PM, Andreas Gruenbacher
<agruenba@redhat.com> wrote:
> On Wed, Feb 22, 2017 at 4:13 PM, Steven Whitehouse <swhiteho@redhat.com> wrote:
>> Hi,
>>
>> Looks good, but can we fix that "retry for 100 times" loop too at the same
>> time? There does appear to be an rhashtable API function that does what we
>> need there,
>
> The rhashtable cleanup which I've posted separately doesn't actually
> affect the weird retry loop.

(But it does fix that at the same time.)

Andreas

[Cluster-devel] [PATCH] gfs2: Add missing rcu locking for glock lookup

[Steven Whitehouse]

Hi,


On 22/02/17 15:26, Andreas Gruenbacher wrote:
> On Wed, Feb 22, 2017 at 4:13 PM, Steven Whitehouse <swhiteho@redhat.com> wrote:
>> Hi,
>>
>> Looks good, but can we fix that "retry for 100 times" loop too at the same
>> time? There does appear to be an rhashtable API function that does what we
>> need there,
> The rhashtable cleanup which I've posted separately doesn't actually
> affect the weird retry loop. The right thing to do here, so far, would
> be to retry forever I believe.
>
> Andreas
There should be no need to retry at all. Either the new entry will be 
added to the hash table, or it will find an existing entry in the table. 
Is there really some way this can fail for some reason? We ought not to 
need a loop here,

Steve.

[Cluster-devel] [PATCH] gfs2: Add missing rcu locking for glock lookup

[Andreas Gruenbacher]

On Wed, Feb 22, 2017 at 4:29 PM, Steven Whitehouse <swhiteho@redhat.com> wrote:
> There should be no need to retry at all. Either the new entry will be added
> to the hash table, or it will find an existing entry in the table. Is there
> really some way this can fail for some reason?

Yes, we may find an existing entry but by the time we try to get a
lockref, the entry may be dead. In that case, the right thing to do is
to retry the insert.

Andreas

[Cluster-devel] [PATCH] gfs2: Add missing rcu locking for glock lookup

[Steven Whitehouse]

Hi,


On 22/02/17 15:34, Andreas Gruenbacher wrote:
> On Wed, Feb 22, 2017 at 4:29 PM, Steven Whitehouse <swhiteho@redhat.com> wrote:
>> There should be no need to retry at all. Either the new entry will be added
>> to the hash table, or it will find an existing entry in the table. Is there
>> really some way this can fail for some reason?
> Yes, we may find an existing entry but by the time we try to get a
> lockref, the entry may be dead. In that case, the right thing to do is
> to retry the insert.
>
> Andreas

Hmm. If it is dead but not yet removed from the list, then we will spin 
on this. The original code prior to the rhashtable conversion would have 
just looked at the next entry on the list, if it found a dead entry matching
the key.

In gfs2_glock_put() we do remove the glock from the hash table, but not 
until after removing it from the lru and dropping the gl_lockref.lock, 
so there is some potential for this to happen. Whether it will be a 
problem in reality I'm not sure, but it is not ideal - I wonder how 
other rhashtable users deal with this issue... or maybe we are the only 
ones using lockref with rhashtable.

This is certainly a big improvement on what we had before, but at the 
same time, we should make a note to fix this properly in due course,

Steve.

[Cluster-devel] [PATCH] gfs2: Add missing rcu locking for glock lookup

[Andrew Price]

On 22/02/17 15:09, Andreas Gruenbacher wrote:
> From: Andrew Price <anprice@redhat.com>
>
> We must hold the rcu read lock across looking up glocks and trying to
> bump their refcount to prevent the glocks from being freed in between.
>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>

Signed-off-by: Andrew Price <anprice@redhat.com>

Thanks,
Andy

> ---
>  fs/gfs2/glock.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> index 94f50ca..1d60f5f 100644
> --- a/fs/gfs2/glock.c
> +++ b/fs/gfs2/glock.c
> @@ -658,9 +658,11 @@ int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number,
>  	struct kmem_cache *cachep;
>  	int ret, tries = 0;
>
> +	rcu_read_lock();
>  	gl = rhashtable_lookup_fast(&gl_hash_table, &name, ht_parms);
>  	if (gl && !lockref_get_not_dead(&gl->gl_lockref))
>  		gl = NULL;
> +	rcu_read_unlock();
>
>  	*glp = gl;
>  	if (gl)
> @@ -728,15 +730,18 @@ int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number,
>
>  	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);
>  	}
>

[Cluster-devel] [PATCH] gfs2: Add missing rcu locking for glock lookup

[Bob Peterson]

----- Original Message -----
| From: Andrew Price <anprice@redhat.com>
| 
| We must hold the rcu read lock across looking up glocks and trying to
| bump their refcount to prevent the glocks from being freed in between.
| 
| Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
| ---
Hi,

Thanks. This is now applied to the for-next branch of the linux-gfs2 tree:
https://git.kernel.org/cgit/linux/kernel/git/gfs2/linux-gfs2.git/commit/?h=for-next&id=329414f6696541d1680f5867ce747050b8054e66

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH] gfs2: Add missing rcu locking for glock lookup

[Andrew Price]

On 22/02/17 19:19, Bob Peterson wrote:
> ----- Original Message -----
> | From: Andrew Price <anprice@redhat.com>
> |
> | We must hold the rcu read lock across looking up glocks and trying to
> | bump their refcount to prevent the glocks from being freed in between.
> |
> | Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> | ---
> Hi,
>
> Thanks. This is now applied to the for-next branch of the linux-gfs2 tree:
> https://git.kernel.org/cgit/linux/kernel/git/gfs2/linux-gfs2.git/commit/?h=for-next&id=329414f6696541d1680f5867ce747050b8054e66

I think Andreas intended for me to be the "author" of this commit, which 
is what git am should have done with the "From:" line. Not that I'm 
overly bothered about it :)

Andy

[Cluster-devel] [PATCH] gfs2: Add missing rcu locking for glock lookup

[Bob Peterson]

----- Original Message -----
| On 22/02/17 19:19, Bob Peterson wrote:
| > ----- Original Message -----
| > | From: Andrew Price <anprice@redhat.com>
| > |
| > | We must hold the rcu read lock across looking up glocks and trying to
| > | bump their refcount to prevent the glocks from being freed in between.
| > |
| > | Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
| > | ---
| > Hi,
| >
| > Thanks. This is now applied to the for-next branch of the linux-gfs2 tree:
| > https://git.kernel.org/cgit/linux/kernel/git/gfs2/linux-gfs2.git/commit/?h=for-next&id=329414f6696541d1680f5867ce747050b8054e66
| 
| I think Andreas intended for me to be the "author" of this commit, which
| is what git am should have done with the "From:" line. Not that I'm
| overly bothered about it :)
| 
| Andy
| 
Sorry about that. I fixed it now.

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH] gfs2: Add missing rcu locking for glock lookup

[Andrew Price]

On 23/02/17 13:08, Bob Peterson wrote:
> ----- Original Message -----
> | On 22/02/17 19:19, Bob Peterson wrote:
> | > ----- Original Message -----
> | > | From: Andrew Price <anprice@redhat.com>
> | > |
> | > | We must hold the rcu read lock across looking up glocks and trying to
> | > | bump their refcount to prevent the glocks from being freed in between.
> | > |
> | > | Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> | > | ---
> | > Hi,
> | >
> | > Thanks. This is now applied to the for-next branch of the linux-gfs2 tree:
> | > https://git.kernel.org/cgit/linux/kernel/git/gfs2/linux-gfs2.git/commit/?h=for-next&id=329414f6696541d1680f5867ce747050b8054e66
> |
> | I think Andreas intended for me to be the "author" of this commit, which
> | is what git am should have done with the "From:" line. Not that I'm
> | overly bothered about it :)
> |
> | Andy
> |
> Sorry about that. I fixed it now.

Great, thanks :)

Andy