Messages in this thread |  | | Subject | Re: [PATCH 02/16] GFS2: Core locking interface | | From | Steven Whitehouse <> | | Date | Mon, 04 Sep 2006 10:01:37 +0100 |
| |
Hi,
On Fri, 2006-09-01 at 09:19 +0200, Jan Engelhardt wrote: > >+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. > Unfortunately thats not possible as the struct gfs2_sbd is actually changed lower down the call chain, but only in the lock_dlm module.
> >+ /* FIXME: suspend dm device so oustanding bio's complete > >+ and all further io requests fail */ > > Would you like to fix them beforehand? :) > Well we have a patch that we've been discussing recently. The question has been how to avoid making GFS2 depend on dm. This function is not required for using GFS2 as a local filesystem at all, its only required for the cluster case and it allows for withdrawing a node gracefully without fencing it. Its not an essential feature, but a "nice to have" and it will probably be fixed up fairly shortly.
> >+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 > Ok, all changed now.
> >+#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? Yes, now done.
> > >+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. > Ok. Now done.
> >+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 > I'll talk to Dave Teigland and we'll try and fix this shortly.
> >+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. > I'm not sure what the comment is referring to - the code looks fine to me, so I've removed the comment since its confusing.
[various other style comments snipped]
The other changes you suggest have all be done in the latest patch:
http://www.kernel.org/git/?p=linux/kernel/git/steve/gfs2-2.6.git;a=commitdiff;h=5029996547a9f3988459e11955c13259495308ef
> > >+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? > This very rarely happens, its on the failure path rather than something that you'd see during normal operation. As Ingo says, its a scheduling based wait. You could argue that we ought to use an exponential back-off rather than a fixed delay, but it seems to work fine in practice and has the advantage of being simple.
> >+ borked = 1; > >+ serious = error; > > This got me a laugh :) > Well I think thats a Ken Preslan original variable name - I can't take credit for that one :-)
> >+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
The glock code is the largest single file in GFS2. I'm looking at ways of reducing that a bit over the medium term as I think that ought to be possible. The main thing which that requires is looking at how we deal with cached locks. I'd also like to use RCU for the glock hash table eventually as well,
Steve.
-- VGER BF report: U 0.5 - 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/
|  |