* [Cluster-devel] [PATCH] GFS2: Hold gl_hash_table bucket locks in glock_hash_walk @ 2015-11-12 17:20 Andrew Price 2015-11-12 20:11 ` Bob Peterson 0 siblings, 1 reply; 8+ messages in thread From: Andrew Price @ 2015-11-12 17:20 UTC (permalink / raw) To: cluster-devel.redhat.com This lockdep splat was being triggered on umount: [55715.973122] =============================== [55715.980169] [ INFO: suspicious RCU usage. ] [55715.981021] 4.3.0-11553-g8d3de01-dirty #15 Tainted: G W [55715.982353] ------------------------------- [55715.983301] fs/gfs2/glock.c:1427 suspicious rcu_dereference_protected() usage! The code it refers to is the rht_for_each_entry_safe usage in glock_hash_walk. The condition that triggers the warning is lockdep_rht_bucket_is_held(tbl, hash) which is checked in the __rcu_dereference_protected macro. lockdep_rht_bucket_is_held returns false when rht_bucket_lock(tbl, hash) is not held, which suggests that glock_hash_walk should be holding the lock for each rhashtable bucket it looks at. Holding those locks clears up the warning. Signed-off-by: Andrew Price <anprice@redhat.com> --- fs/gfs2/glock.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c index 32e7471..7384cbb 100644 --- a/fs/gfs2/glock.c +++ b/fs/gfs2/glock.c @@ -1424,11 +1424,13 @@ static void glock_hash_walk(glock_examiner examiner, const struct gfs2_sbd *sdp) rcu_read_lock(); tbl = rht_dereference_rcu(gl_hash_table.tbl, &gl_hash_table); for (i = 0; i < tbl->size; i++) { + spin_lock(rht_bucket_lock(tbl, i)); rht_for_each_entry_safe(gl, pos, next, tbl, i, gl_node) { if ((gl->gl_name.ln_sbd == sdp) && lockref_get_not_dead(&gl->gl_lockref)) examiner(gl); } + spin_unlock(rht_bucket_lock(tbl, i)); } rcu_read_unlock(); cond_resched(); -- 2.4.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Cluster-devel] [PATCH] GFS2: Hold gl_hash_table bucket locks in glock_hash_walk 2015-11-12 17:20 [Cluster-devel] [PATCH] GFS2: Hold gl_hash_table bucket locks in glock_hash_walk Andrew Price @ 2015-11-12 20:11 ` Bob Peterson 2015-11-12 20:38 ` Steven Whitehouse 0 siblings, 1 reply; 8+ messages in thread From: Bob Peterson @ 2015-11-12 20:11 UTC (permalink / raw) To: cluster-devel.redhat.com ----- Original Message ----- > This lockdep splat was being triggered on umount: > > [55715.973122] =============================== > [55715.980169] [ INFO: suspicious RCU usage. ] > [55715.981021] 4.3.0-11553-g8d3de01-dirty #15 Tainted: G W > [55715.982353] ------------------------------- > [55715.983301] fs/gfs2/glock.c:1427 suspicious rcu_dereference_protected() > usage! > > The code it refers to is the rht_for_each_entry_safe usage in > glock_hash_walk. The condition that triggers the warning is > lockdep_rht_bucket_is_held(tbl, hash) which is checked in the > __rcu_dereference_protected macro. > > lockdep_rht_bucket_is_held returns false when rht_bucket_lock(tbl, hash) > is not held, which suggests that glock_hash_walk should be holding the > lock for each rhashtable bucket it looks at. Holding those locks clears > up the warning. > > Signed-off-by: Andrew Price <anprice@redhat.com> > --- > fs/gfs2/glock.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c > index 32e7471..7384cbb 100644 > --- a/fs/gfs2/glock.c > +++ b/fs/gfs2/glock.c > @@ -1424,11 +1424,13 @@ static void glock_hash_walk(glock_examiner examiner, > const struct gfs2_sbd *sdp) > rcu_read_lock(); > tbl = rht_dereference_rcu(gl_hash_table.tbl, &gl_hash_table); > for (i = 0; i < tbl->size; i++) { > + spin_lock(rht_bucket_lock(tbl, i)); > rht_for_each_entry_safe(gl, pos, next, tbl, i, gl_node) { > if ((gl->gl_name.ln_sbd == sdp) && > lockref_get_not_dead(&gl->gl_lockref)) > examiner(gl); > } > + spin_unlock(rht_bucket_lock(tbl, i)); > } > rcu_read_unlock(); > cond_resched(); > -- > 2.4.3 > > Hi Andy, Thanks. I'll hold onto this until after the merge window is closed. Regards, Bob Peterson Red Hat File Systems ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Cluster-devel] [PATCH] GFS2: Hold gl_hash_table bucket locks in glock_hash_walk 2015-11-12 20:11 ` Bob Peterson @ 2015-11-12 20:38 ` Steven Whitehouse 2015-11-13 11:48 ` Andrew Price 2015-11-13 12:16 ` [Cluster-devel] [PATCH v2] GFS2: Use rht_for_each_entry_rcu " Andrew Price 0 siblings, 2 replies; 8+ messages in thread From: Steven Whitehouse @ 2015-11-12 20:38 UTC (permalink / raw) To: cluster-devel.redhat.com Hi, On 12/11/15 20:11, Bob Peterson wrote: > ----- Original Message ----- >> This lockdep splat was being triggered on umount: >> >> [55715.973122] =============================== >> [55715.980169] [ INFO: suspicious RCU usage. ] >> [55715.981021] 4.3.0-11553-g8d3de01-dirty #15 Tainted: G W >> [55715.982353] ------------------------------- >> [55715.983301] fs/gfs2/glock.c:1427 suspicious rcu_dereference_protected() >> usage! >> >> The code it refers to is the rht_for_each_entry_safe usage in >> glock_hash_walk. The condition that triggers the warning is >> lockdep_rht_bucket_is_held(tbl, hash) which is checked in the >> __rcu_dereference_protected macro. >> >> lockdep_rht_bucket_is_held returns false when rht_bucket_lock(tbl, hash) >> is not held, which suggests that glock_hash_walk should be holding the >> lock for each rhashtable bucket it looks at. Holding those locks clears >> up the warning. >> >> Signed-off-by: Andrew Price <anprice@redhat.com> >> --- >> fs/gfs2/glock.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c >> index 32e7471..7384cbb 100644 >> --- a/fs/gfs2/glock.c >> +++ b/fs/gfs2/glock.c >> @@ -1424,11 +1424,13 @@ static void glock_hash_walk(glock_examiner examiner, >> const struct gfs2_sbd *sdp) >> rcu_read_lock(); >> tbl = rht_dereference_rcu(gl_hash_table.tbl, &gl_hash_table); >> for (i = 0; i < tbl->size; i++) { >> + spin_lock(rht_bucket_lock(tbl, i)); >> rht_for_each_entry_safe(gl, pos, next, tbl, i, gl_node) { >> if ((gl->gl_name.ln_sbd == sdp) && >> lockref_get_not_dead(&gl->gl_lockref)) >> examiner(gl); >> } >> + spin_unlock(rht_bucket_lock(tbl, i)); >> } >> rcu_read_unlock(); >> cond_resched(); >> -- Not sure that this is the correct fix. There is nothing changing in the examiner function that I can see which would require the bucket lock, and we already have this with an rcu_read_lock, which is correct since we are only reading the hash lists, not changing them. So I'm wondering why this triggers the warning, and whether there is some annotation issue perhaps? Steve. >> 2.4.3 >> >> > Hi Andy, > > Thanks. I'll hold onto this until after the merge window is closed. > > Regards, > > Bob Peterson > Red Hat File Systems > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Cluster-devel] [PATCH] GFS2: Hold gl_hash_table bucket locks in glock_hash_walk 2015-11-12 20:38 ` Steven Whitehouse @ 2015-11-13 11:48 ` Andrew Price 2015-11-13 12:16 ` [Cluster-devel] [PATCH v2] GFS2: Use rht_for_each_entry_rcu " Andrew Price 1 sibling, 0 replies; 8+ messages in thread From: Andrew Price @ 2015-11-13 11:48 UTC (permalink / raw) To: cluster-devel.redhat.com On 12/11/15 20:38, Steven Whitehouse wrote: > Hi, > > On 12/11/15 20:11, Bob Peterson wrote: >> ----- Original Message ----- >>> This lockdep splat was being triggered on umount: >>> >>> [55715.973122] =============================== >>> [55715.980169] [ INFO: suspicious RCU usage. ] >>> [55715.981021] 4.3.0-11553-g8d3de01-dirty #15 Tainted: G W >>> [55715.982353] ------------------------------- >>> [55715.983301] fs/gfs2/glock.c:1427 suspicious >>> rcu_dereference_protected() >>> usage! >>> >>> The code it refers to is the rht_for_each_entry_safe usage in >>> glock_hash_walk. The condition that triggers the warning is >>> lockdep_rht_bucket_is_held(tbl, hash) which is checked in the >>> __rcu_dereference_protected macro. >>> >>> lockdep_rht_bucket_is_held returns false when rht_bucket_lock(tbl, hash) >>> is not held, which suggests that glock_hash_walk should be holding the >>> lock for each rhashtable bucket it looks at. Holding those locks clears >>> up the warning. >>> >>> Signed-off-by: Andrew Price <anprice@redhat.com> >>> --- >>> fs/gfs2/glock.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c >>> index 32e7471..7384cbb 100644 >>> --- a/fs/gfs2/glock.c >>> +++ b/fs/gfs2/glock.c >>> @@ -1424,11 +1424,13 @@ static void glock_hash_walk(glock_examiner >>> examiner, >>> const struct gfs2_sbd *sdp) >>> rcu_read_lock(); >>> tbl = rht_dereference_rcu(gl_hash_table.tbl, &gl_hash_table); >>> for (i = 0; i < tbl->size; i++) { >>> + spin_lock(rht_bucket_lock(tbl, i)); >>> rht_for_each_entry_safe(gl, pos, next, tbl, i, gl_node) { >>> if ((gl->gl_name.ln_sbd == sdp) && >>> lockref_get_not_dead(&gl->gl_lockref)) >>> examiner(gl); >>> } >>> + spin_unlock(rht_bucket_lock(tbl, i)); >>> } >>> rcu_read_unlock(); >>> cond_resched(); >>> -- > Not sure that this is the correct fix. There is nothing changing in the > examiner function that I can see which would require the bucket lock, > and we already have this with an rcu_read_lock, which is correct since > we are only reading the hash lists, not changing them. Good point, I wasn't sure whether it was safe to assume the examiner functions weren't changing the buckets and erred on the side of caution. rht_for_each_entry_safe() does an unconditional rcu_dereference_protected(p, lockdep_rht_bucket_is_held(tbl, hash)) so we're likely using the wrong rht_for_each_entry* macro. I'm currently testing a version which uses rht_for_each_entry_rcu() instead. That one uses rcu_dereference_check(p, lockdep_rht_bucket_is_held(tbl, hash)) which is defined as #define rcu_dereference_check(p, c) \ __rcu_dereference_check((p), (c) || rcu_read_lock_held(), __rcu) so that looks like the right one to use. I'll send that patch shortly. Andy ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Cluster-devel] [PATCH v2] GFS2: Use rht_for_each_entry_rcu in glock_hash_walk 2015-11-12 20:38 ` Steven Whitehouse 2015-11-13 11:48 ` Andrew Price @ 2015-11-13 12:16 ` Andrew Price 2015-11-13 12:19 ` Steven Whitehouse ` (2 more replies) 1 sibling, 3 replies; 8+ messages in thread From: Andrew Price @ 2015-11-13 12:16 UTC (permalink / raw) To: cluster-devel.redhat.com This lockdep splat was being triggered on umount: [55715.973122] =============================== [55715.980169] [ INFO: suspicious RCU usage. ] [55715.981021] 4.3.0-11553-g8d3de01-dirty #15 Tainted: G W [55715.982353] ------------------------------- [55715.983301] fs/gfs2/glock.c:1427 suspicious rcu_dereference_protected() usage! The code it refers to is the rht_for_each_entry_safe usage in glock_hash_walk. The condition that triggers the warning is lockdep_rht_bucket_is_held(tbl, hash) which is checked in the __rcu_dereference_protected macro. The rhashtable buckets are not changed in glock_hash_walk so it's safe to rely on the rcu protection. Replace the rht_for_each_entry_safe() usage with rht_for_each_entry_rcu(), which doesn't care whether the bucket lock is held if the rcu read lock is held. Signed-off-by: Andrew Price <anprice@redhat.com> --- fs/gfs2/glock.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c index 32e7471..430326e 100644 --- a/fs/gfs2/glock.c +++ b/fs/gfs2/glock.c @@ -1417,14 +1417,14 @@ static struct shrinker glock_shrinker = { static void glock_hash_walk(glock_examiner examiner, const struct gfs2_sbd *sdp) { struct gfs2_glock *gl; - struct rhash_head *pos, *next; + struct rhash_head *pos; const struct bucket_table *tbl; int i; rcu_read_lock(); tbl = rht_dereference_rcu(gl_hash_table.tbl, &gl_hash_table); for (i = 0; i < tbl->size; i++) { - rht_for_each_entry_safe(gl, pos, next, tbl, i, gl_node) { + rht_for_each_entry_rcu(gl, pos, tbl, i, gl_node) { if ((gl->gl_name.ln_sbd == sdp) && lockref_get_not_dead(&gl->gl_lockref)) examiner(gl); -- 2.4.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Cluster-devel] [PATCH v2] GFS2: Use rht_for_each_entry_rcu in glock_hash_walk 2015-11-13 12:16 ` [Cluster-devel] [PATCH v2] GFS2: Use rht_for_each_entry_rcu " Andrew Price @ 2015-11-13 12:19 ` Steven Whitehouse 2015-11-13 13:36 ` Bob Peterson 2015-11-16 18:13 ` Bob Peterson 2 siblings, 0 replies; 8+ messages in thread From: Steven Whitehouse @ 2015-11-13 12:19 UTC (permalink / raw) To: cluster-devel.redhat.com Hi, Looks good... Acked-by: Steven Whitehouse <swhiteho@redhat.com> Steve. On 13/11/15 12:16, Andrew Price wrote: > This lockdep splat was being triggered on umount: > > [55715.973122] =============================== > [55715.980169] [ INFO: suspicious RCU usage. ] > [55715.981021] 4.3.0-11553-g8d3de01-dirty #15 Tainted: G W > [55715.982353] ------------------------------- > [55715.983301] fs/gfs2/glock.c:1427 suspicious rcu_dereference_protected() usage! > > The code it refers to is the rht_for_each_entry_safe usage in > glock_hash_walk. The condition that triggers the warning is > lockdep_rht_bucket_is_held(tbl, hash) which is checked in the > __rcu_dereference_protected macro. > > The rhashtable buckets are not changed in glock_hash_walk so it's safe > to rely on the rcu protection. Replace the rht_for_each_entry_safe() > usage with rht_for_each_entry_rcu(), which doesn't care whether the > bucket lock is held if the rcu read lock is held. > > Signed-off-by: Andrew Price <anprice@redhat.com> > --- > fs/gfs2/glock.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c > index 32e7471..430326e 100644 > --- a/fs/gfs2/glock.c > +++ b/fs/gfs2/glock.c > @@ -1417,14 +1417,14 @@ static struct shrinker glock_shrinker = { > static void glock_hash_walk(glock_examiner examiner, const struct gfs2_sbd *sdp) > { > struct gfs2_glock *gl; > - struct rhash_head *pos, *next; > + struct rhash_head *pos; > const struct bucket_table *tbl; > int i; > > rcu_read_lock(); > tbl = rht_dereference_rcu(gl_hash_table.tbl, &gl_hash_table); > for (i = 0; i < tbl->size; i++) { > - rht_for_each_entry_safe(gl, pos, next, tbl, i, gl_node) { > + rht_for_each_entry_rcu(gl, pos, tbl, i, gl_node) { > if ((gl->gl_name.ln_sbd == sdp) && > lockref_get_not_dead(&gl->gl_lockref)) > examiner(gl); ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Cluster-devel] [PATCH v2] GFS2: Use rht_for_each_entry_rcu in glock_hash_walk 2015-11-13 12:16 ` [Cluster-devel] [PATCH v2] GFS2: Use rht_for_each_entry_rcu " Andrew Price 2015-11-13 12:19 ` Steven Whitehouse @ 2015-11-13 13:36 ` Bob Peterson 2015-11-16 18:13 ` Bob Peterson 2 siblings, 0 replies; 8+ messages in thread From: Bob Peterson @ 2015-11-13 13:36 UTC (permalink / raw) To: cluster-devel.redhat.com ----- Original Message ----- > This lockdep splat was being triggered on umount: > > [55715.973122] =============================== > [55715.980169] [ INFO: suspicious RCU usage. ] > [55715.981021] 4.3.0-11553-g8d3de01-dirty #15 Tainted: G W > [55715.982353] ------------------------------- > [55715.983301] fs/gfs2/glock.c:1427 suspicious rcu_dereference_protected() > usage! > > The code it refers to is the rht_for_each_entry_safe usage in > glock_hash_walk. The condition that triggers the warning is > lockdep_rht_bucket_is_held(tbl, hash) which is checked in the > __rcu_dereference_protected macro. > > The rhashtable buckets are not changed in glock_hash_walk so it's safe > to rely on the rcu protection. Replace the rht_for_each_entry_safe() > usage with rht_for_each_entry_rcu(), which doesn't care whether the > bucket lock is held if the rcu read lock is held. > > Signed-off-by: Andrew Price <anprice@redhat.com> > --- > fs/gfs2/glock.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c > index 32e7471..430326e 100644 > --- a/fs/gfs2/glock.c > +++ b/fs/gfs2/glock.c > @@ -1417,14 +1417,14 @@ static struct shrinker glock_shrinker = { > static void glock_hash_walk(glock_examiner examiner, const struct gfs2_sbd > *sdp) > { > struct gfs2_glock *gl; > - struct rhash_head *pos, *next; > + struct rhash_head *pos; > const struct bucket_table *tbl; > int i; > > rcu_read_lock(); > tbl = rht_dereference_rcu(gl_hash_table.tbl, &gl_hash_table); > for (i = 0; i < tbl->size; i++) { > - rht_for_each_entry_safe(gl, pos, next, tbl, i, gl_node) { > + rht_for_each_entry_rcu(gl, pos, tbl, i, gl_node) { > if ((gl->gl_name.ln_sbd == sdp) && > lockref_get_not_dead(&gl->gl_lockref)) > examiner(gl); > -- > 2.4.3 > > ACK I'll save it until after this merge window. Bob Peterson Red Hat File Systems ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Cluster-devel] [PATCH v2] GFS2: Use rht_for_each_entry_rcu in glock_hash_walk 2015-11-13 12:16 ` [Cluster-devel] [PATCH v2] GFS2: Use rht_for_each_entry_rcu " Andrew Price 2015-11-13 12:19 ` Steven Whitehouse 2015-11-13 13:36 ` Bob Peterson @ 2015-11-16 18:13 ` Bob Peterson 2 siblings, 0 replies; 8+ messages in thread From: Bob Peterson @ 2015-11-16 18:13 UTC (permalink / raw) To: cluster-devel.redhat.com ----- Original Message ----- > This lockdep splat was being triggered on umount: > > [55715.973122] =============================== > [55715.980169] [ INFO: suspicious RCU usage. ] > [55715.981021] 4.3.0-11553-g8d3de01-dirty #15 Tainted: G W > [55715.982353] ------------------------------- > [55715.983301] fs/gfs2/glock.c:1427 suspicious rcu_dereference_protected() > usage! > > The code it refers to is the rht_for_each_entry_safe usage in > glock_hash_walk. The condition that triggers the warning is > lockdep_rht_bucket_is_held(tbl, hash) which is checked in the > __rcu_dereference_protected macro. > > The rhashtable buckets are not changed in glock_hash_walk so it's safe > to rely on the rcu protection. Replace the rht_for_each_entry_safe() > usage with rht_for_each_entry_rcu(), which doesn't care whether the > bucket lock is held if the rcu read lock is held. > > Signed-off-by: Andrew Price <anprice@redhat.com> > --- > fs/gfs2/glock.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c > index 32e7471..430326e 100644 > --- a/fs/gfs2/glock.c > +++ b/fs/gfs2/glock.c > @@ -1417,14 +1417,14 @@ static struct shrinker glock_shrinker = { > static void glock_hash_walk(glock_examiner examiner, const struct gfs2_sbd > *sdp) > { > struct gfs2_glock *gl; > - struct rhash_head *pos, *next; > + struct rhash_head *pos; > const struct bucket_table *tbl; > int i; > > rcu_read_lock(); > tbl = rht_dereference_rcu(gl_hash_table.tbl, &gl_hash_table); > for (i = 0; i < tbl->size; i++) { > - rht_for_each_entry_safe(gl, pos, next, tbl, i, gl_node) { > + rht_for_each_entry_rcu(gl, pos, tbl, i, gl_node) { > if ((gl->gl_name.ln_sbd == sdp) && > lockref_get_not_dead(&gl->gl_lockref)) > examiner(gl); > -- > 2.4.3 > > 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/fs/gfs2?h=for-next&id=3dd1dd8c696bdb7c8dcc9456cb23558ad1b336b8 Regards, Bob Peterson Red Hat File Systems ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-11-16 18:13 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-12 17:20 [Cluster-devel] [PATCH] GFS2: Hold gl_hash_table bucket locks in glock_hash_walk Andrew Price 2015-11-12 20:11 ` Bob Peterson 2015-11-12 20:38 ` Steven Whitehouse 2015-11-13 11:48 ` Andrew Price 2015-11-13 12:16 ` [Cluster-devel] [PATCH v2] GFS2: Use rht_for_each_entry_rcu " Andrew Price 2015-11-13 12:19 ` Steven Whitehouse 2015-11-13 13:36 ` Bob Peterson 2015-11-16 18:13 ` Bob Peterson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.