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 <[email protected]> | > --- | > 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
