Re: [PATCH 02/16] GFS2: Core locking interface

[Jan Engelhardt <jengelh@linux01.gwdg.de>]

>+void gfs2_lm_others_may_mount(struct gfs2_sbd *sdp)

I think this could be 'const struct gfs2_sbd *sdp'. Also in other places, const
would probably be adequate.

>+void gfs2_lm_unmount(struct gfs2_sbd *sdp)

Like here

>+int gfs2_lm_withdraw(struct gfs2_sbd *sdp, char *fmt, ...)

And here.

>+	/* FIXME: suspend dm device so oustanding bio's complete
>+	   and all further io requests fail */

Would you like to fix them beforehand? :)

>+int gfs2_lm_get_lock(struct gfs2_sbd *sdp, struct lm_lockname *name,
>+		     lm_lock_t **lockp)
>+{
>+	int error;
>+	if (unlikely(test_bit(SDF_SHUTDOWN, &sdp->sd_flags)))
>+		error = -EIO;
>+	else
>+		error = sdp->sd_lockstruct.ls_ops->lm_get_lock(
>+				sdp->sd_lockstruct.ls_lockspace, name, lockp);
>+	return error;
>+}

How about

{
    int err = -EIO;
    if(likely(!test_bit(...)))
        err = sdp->...
    return err;
}?
Same applies for other similar functions, like

>+unsigned int gfs2_lm_lock(struct gfs2_sbd *sdp, lm_lock_t *lock,
>+			  unsigned int cur_state, unsigned int req_state,
>+			  unsigned int flags)
>+unsigned int gfs2_lm_unlock(struct gfs2_sbd *sdp, lm_lock_t *lock,
>+			    unsigned int cur_state)
>+int gfs2_lm_hold_lvb(struct gfs2_sbd *sdp, lm_lock_t *lock, char **lvbp)
and others.


>+#if 0
>+void gfs2_lm_sync_lvb(struct gfs2_sbd *sdp, lm_lock_t *lock, char *lvb)
>+{
>+	if (likely(!test_bit(SDF_SHUTDOWN, &sdp->sd_flags)))
>+		sdp->sd_lockstruct.ls_ops->lm_sync_lvb(lock, lvb);
>+}
>+#endif  /*  0  */

If this is unused, can it be removed?

>+int gfs2_lm_withdraw(struct gfs2_sbd *sdp, char *fmt, ...)
>+__attribute__ ((format(printf, 2, 3)));

Possibly indent the 2nd line a little.

>+static struct list_head lmh_list;
>+static struct mutex lmh_lock;

Is it intended that these do not have an initializer?

>+void __init gfs2_init_lmh(void)
>+{
>+	mutex_init(&lmh_lock);
>+	INIT_LIST_HEAD(&lmh_list);
>+}

I suppose so. If they were initialized statically, this function could possibly
be dropped.

>+typedef void lm_lockspace_t;
>+typedef void lm_lock_t;
>+typedef void lm_fsdata_t;

Try to avoid typedefs for
- simple types like these (int/void/etc.)
- structures

>+typedef void (*lm_callback_t) (lm_fsdata_t *fsdata, unsigned int type,
>+			       void *data);

(Function prototypes are ok for me, because writing it out everytime for
functions with a lot of args is tedious. (For recursive type uses, typedefs are
even necessary.))

>+		if (ret & LM_OUT_CANCELED)
>+			handle_callback(gl, LM_ST_UNLOCKED); /* Lame */

Hm.

>+
>+	} else {
>+		if (gfs2_assert_withdraw(sdp, 0) == -1)
>+			fs_err(sdp, "ret = 0x%.8X\n", ret);
>+	}

If you want, you can drop the {}. In fact, this is just like a "else
if(gfs2_assert..."

>+	if (gl->gl_state == LM_ST_EXCLUSIVE) {
>+		if (glops->go_sync)
>+			glops->go_sync(gl,
>+				       DIO_METADATA | DIO_DATA | DIO_RELEASE);
>+	}

if(gl->gl_state == LM_ST_EXCLUSIVE && glops->go_sync != NULL)
    glops->go_sync(...)

You might want to &&-chain other if()s too, if the if-condition lines do not
get too long. (Note that I am not trying to enforce CodingStyle at all, I am
just giving hints how I would do it.)

>+	while (gl->gl_req_gh != gh &&
>+	       !test_bit(HIF_HOLDER, &gh->gh_iflags) &&
>+	       !list_empty(&gh->gh_list)) {
>+		if (gl->gl_req_bh &&
>+		    !(gl->gl_req_gh &&
>+		      (gl->gl_req_gh->gh_flags & GL_NOCANCEL))) {
>+			spin_unlock(&gl->gl_spin);
>+			gfs2_lm_cancel(gl->gl_sbd, gl->gl_lock);
>+			msleep(100);
>+			spin_lock(&gl->gl_spin);

I'd be a little uncomfortable with the indent scheme used here.

>+	if (gh->gh_flags & (LM_FLAG_TRY | LM_FLAG_TRY_1CB)) {
>+		spin_lock(&gl->gl_spin);
>+		if (gl->gl_req_gh != gh &&
>+		    !test_bit(HIF_HOLDER, &gh->gh_iflags) &&
>+		    !list_empty(&gh->gh_list)) {
>+			list_del_init(&gh->gh_list);

.

>+int gfs2_glock_nq(struct gfs2_holder *gh)
>+{
...
>+	if (!(gh->gh_flags & GL_ASYNC)) {
>+		error = glock_wait_internal(gh);
>+		if (error == GLR_CANCELED) {
>+			msleep(100);

msleep is a busy-waiter IIRC. Really want to do that - what about some
schedulling?

>+			borked = 1;
>+			serious = error;

This got me a laugh :)

>+static inline int gfs2_glock_is_held_excl(struct gfs2_glock *gl)
>+{
>+	return (gl->gl_state == LM_ST_EXCLUSIVE);
>+}

No need for the ().


Quite a lot of code, phew!


Jan Engelhardt
-- 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/