[Cluster-devel] [GFS2 PATCH] [v2] GFS2: Fix up some sparse warnings

[Andrew Price <anprice@redhat.com>]

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 <rpeterso@redhat.com>
> ---
> 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"
>