From mboxrd@z Thu Jan  1 00:00:00 1970
From: Bob Peterson <rpeterso@redhat.com>
Date: Tue, 22 Aug 2017 15:54:14 -0400 (EDT)
Subject: [Cluster-devel] [GFS2 PATCH] GFS2: Fix up some sparse warnings
In-Reply-To: <20170818155159.GC12296@cicero>
References: <1809292291.1872363.1503065998850.JavaMail.zimbra@redhat.com>
	<359006668.1872591.1503066056464.JavaMail.zimbra@redhat.com>
	<20170818155159.GC12296@cicero>
Message-ID: <58198541.947848.1503431654929.JavaMail.zimbra@redhat.com>
List-Id: <cluster-devel.redhat.com>
To: cluster-devel.redhat.com
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit

Hi Andy,

Comments inline.

----- Original Message -----
| On Fri, Aug 18, 2017 at 10:20:56AM -0400, Bob Peterson wrote:
| > Hi,
| > 
| > 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..db78720f3c89 100644
| > --- a/fs/gfs2/glock.c
| > +++ b/fs/gfs2/glock.c
| > @@ -1550,14 +1550,13 @@ static void glock_hash_walk(glock_examiner
| > examiner, const struct gfs2_sbd *sdp)
| >  
| >  	do {
| >  		gl = ERR_PTR(rhashtable_walk_start(&iter));
| 
| Couldn't the rhashtable_walk_start() call be moved before the outer loop?
| 
| "* Returns -EAGAIN if resize event occured.  Note that the iterator
|  * will rewind back to the beginning and you may use it immediately
|  * by calling rhashtable_walk_next."
| 
| so it looks like -EAGAIN doesn't require a stop and restart of the walk.

Yes, I'm pretty sure you're right. I could (and should) re-factor this
and straighten it out. However, my goal with this set of patches was
to not change the logic at all, and in today's code, it's inside the loop.

| > -		if (gl)
| > -			continue;
| > -
| > -		while ((gl = rhashtable_walk_next(&iter)) && !IS_ERR(gl))
| > -			if ((gl->gl_name.ln_sbd == sdp) &&
| > -			    lockref_get_not_dead(&gl->gl_lockref))
| > -				examiner(gl);
| > -
| > +		if (!gl) {
| 
| This looks like it should be if (!IS_ERR(gl)) { ...

Here again, the original was "if (gl)" and I didn't want to change it,
so it becomes "if (gl) {". Again, I think I need to change it, but
perhaps in another patch that targets this function, especially since
it can't return anything other than 0 or -EAGAIN.
 
| > +			while ((gl = rhashtable_walk_next(&iter)) &&
| > +			       !IS_ERR(gl))
| > +				if ((gl->gl_name.ln_sbd == sdp) &&
| > +				    lockref_get_not_dead(&gl->gl_lockref))
| > +					examiner(gl);
| > +		}
| >  		rhashtable_walk_stop(&iter);

So I propose I push this patch "as is" and write another patch to re-work
function glock_hash_walk so it cleans up the issues you pointed out and
makes more logical sense.

Regards,

Bob Peterson
Red Hat File Systems