On Wed, Aug 23, 2017 at 11:27:22AM -0400, Bob Peterson wrote: > Hi, > > I've been doing some investigation with regard to Andy Price's > questions about the glock / rhashtable portion of this patch. > > It turns out that the rhashtable code in function glock_hash_walk > was patterned after the code in function tipc_sk_reinit() in > net/tips/socket.c. The loop in Andy's question was added recently > by Herbert Xu in commit 40f9f439706073b4b0a654b3b99e18296b7990b3 > and comments in that patch explain why the loop was added. > This differs, however, from net/netlink/diag.c which does not have > the loop. > > As far as I can tell, the difference between having and not having > the loop comes down to what happens when an rhashtable resize > event occurs: Would you rather have it (a) process some > unpredictable number of glocks twice, or (b) skip some altogether? > > Since the callers of function glock_hash_walk are gfs2_glock_thaw > and gfs2_gl_hash_clear, it makes more sense to me to process them > twice than not at all. > > In doing this investigation, I found a bug in the original tipc > code, and submitted a patch for it this morning.
Nice! > That bug was > one I has proposed fixing with version 1 of this patch. > > I've therefore revised this patch to (a) use IS_ERR(gl) rather > than just (gl) as Andy suggested. (b) Added a goto instead of > using braces, so that it more closely matches the patched tipc > code, and (c) got rid of some parenthesis to appease Andreas. Looks good to me. Cheers, Andy > Bob Peterson > > Patch description: > ------------------ > This patch cleans up various pieces of GFS2 to avoid sparse errors. > This doesn't fix them all, but it fixes several. The first error, > in function glock_hash_walk was a genuine bug where the rhashtable > could be started and not stopped. > > Signed-off-by: Bob Peterson <[email protected]> > --- > diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c > index ffca19598525..4178417249c3 100644 > --- a/fs/gfs2/glock.c > +++ b/fs/gfs2/glock.c > @@ -1550,14 +1550,15 @@ static void glock_hash_walk(glock_examiner examiner, > const struct gfs2_sbd *sdp) > > do { > gl = ERR_PTR(rhashtable_walk_start(&iter)); > - if (gl) > - continue; > + if (IS_ERR(gl)) > + goto walk_stop; > > while ((gl = rhashtable_walk_next(&iter)) && !IS_ERR(gl)) > - if ((gl->gl_name.ln_sbd == sdp) && > + if (gl->gl_name.ln_sbd == sdp && > lockref_get_not_dead(&gl->gl_lockref)) > examiner(gl); > > +walk_stop: > rhashtable_walk_stop(&iter); > } while (cond_resched(), gl == ERR_PTR(-EAGAIN)); > > @@ -1940,6 +1941,7 @@ static void gfs2_glock_iter_next(struct gfs2_glock_iter > *gi) > } > > static void *gfs2_glock_seq_start(struct seq_file *seq, loff_t *pos) > + __acquires(RCU) > { > struct gfs2_glock_iter *gi = seq->private; > loff_t n = *pos; > @@ -1972,6 +1974,7 @@ static void *gfs2_glock_seq_next(struct seq_file *seq, > void *iter_ptr, > } > > static void gfs2_glock_seq_stop(struct seq_file *seq, void *iter_ptr) > + __releases(RCU) > { > struct gfs2_glock_iter *gi = seq->private; > > diff --git a/fs/gfs2/lock_dlm.c b/fs/gfs2/lock_dlm.c > index 1d98b8a36eb3..65f33a0ac190 100644 > --- a/fs/gfs2/lock_dlm.c > +++ b/fs/gfs2/lock_dlm.c > @@ -23,8 +23,6 @@ > #include "sys.h" > #include "trace_gfs2.h" > > -extern struct workqueue_struct *gfs2_control_wq; > - > /** > * gfs2_update_stats - Update time based stats > * @mv: Pointer to mean/variance structure to update > @@ -1176,7 +1174,7 @@ static void gdlm_recovery_result(struct gfs2_sbd *sdp, > unsigned int jid, > spin_unlock(&ls->ls_recover_spin); > } > > -const struct dlm_lockspace_ops gdlm_lockspace_ops = { > +static const struct dlm_lockspace_ops gdlm_lockspace_ops = { > .recover_prep = gdlm_recover_prep, > .recover_slot = gdlm_recover_slot, > .recover_done = gdlm_recover_done, > diff --git a/fs/gfs2/main.c b/fs/gfs2/main.c > index 0a89e6f7a314..899329cfd733 100644 > --- a/fs/gfs2/main.c > +++ b/fs/gfs2/main.c > @@ -32,8 +32,6 @@ > #include "dir.h" > #include "glops.h" > > -struct workqueue_struct *gfs2_control_wq; > - > static void gfs2_init_inode_once(void *foo) > { > struct gfs2_inode *ip = foo; > diff --git a/fs/gfs2/util.h b/fs/gfs2/util.h > index c81295f407f6..3926f95a6eb7 100644 > --- a/fs/gfs2/util.h > +++ b/fs/gfs2/util.h > @@ -151,6 +151,7 @@ extern struct kmem_cache *gfs2_rgrpd_cachep; > extern struct kmem_cache *gfs2_quotad_cachep; > extern struct kmem_cache *gfs2_qadata_cachep; > extern mempool_t *gfs2_page_pool; > +extern struct workqueue_struct *gfs2_control_wq; > > static inline unsigned int gfs2_tune_get_i(struct gfs2_tune *gt, > unsigned int *p) > diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c > index 54179554c7d2..cf694de4991a 100644 > --- a/fs/gfs2/xattr.c > +++ b/fs/gfs2/xattr.c > @@ -25,6 +25,7 @@ > #include "meta_io.h" > #include "quota.h" > #include "rgrp.h" > +#include "super.h" > #include "trans.h" > #include "util.h" >
