lkml.org 
[lkml]   [2006]   [Sep]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 02/16] GFS2: Core locking interface

>+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/

\
 
 \ /
  Last update: 2006-09-01 09:27    [W:0.293 / U:0.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site