[Cluster-devel] Re: GFS2: Add tracepoints
Steven Whitehouse
swhiteho at redhat.com
Fri May 29 19:50:10 UTC 2009
Hi,
On Fri, 2009-05-29 at 15:23 -0400, Christoph Hellwig wrote:
> On Fri, May 29, 2009 at 08:11:00PM +0100, Steven Whitehouse wrote:
> > +static inline u8 glock_trace_state(unsigned int state)
> > +{
> > + switch(state) {
> > + case LM_ST_SHARED:
> > + return DLM_LOCK_PR;
> > + case LM_ST_DEFERRED:
> > + return DLM_LOCK_CW;
> > + case LM_ST_EXCLUSIVE:
> > + return DLM_LOCK_EX;
> > + }
> > + return DLM_LOCK_NL;
> > +}
>
> I think this would be better done using __print_symbolic.
>
This is just a conversion of constants, but the others below I'll fix up
in the next version.
> > +static inline const char *glock_trace_name(u8 state)
> > +{
> > + switch(state) {
> > + case DLM_LOCK_PR:
> > + return "PR";
> > + case DLM_LOCK_CW:
> > + return "CW";
> > + case DLM_LOCK_EX:
> > + return "EX";
> > + case DLM_LOCK_NL:
> > + return "NL";
> > + default:
> > + return "--";
> > + }
> > +}
>
> Same here.
>
> > +static inline const char *gfs2_blkst_name(u8 state)
> > +{
> > + switch(state) {
> > + case GFS2_BLKST_FREE:
> > + return "free";
> > + case GFS2_BLKST_USED:
> > + return "used";
> > + case GFS2_BLKST_DINODE:
> > + return "dinode";
> > + case GFS2_BLKST_UNLINKED:
> > + return "unlinked";
> > + }
> > + return "???";
> > +}
>
> Same here.
>
> > + TP_STRUCT__entry(
> > + __array( char, devname, BDEVNAME_SIZE )
>
> This is extremly inefficient. We'd be much better off just storing
> the dev_t and introducing a __trace_bdevname to expand a bdevname
> into the tracer buffer. It's been on my todo list for a while and
> I'll look into it next week unless you beat me to it.
>
Ok. I wasn't sure how efficient bdevname() was vs. copying the name
around, but that sounds like a good plan if its not too expensive an
operation.
> > + __entry->gltype = gl->gl_name.ln_type;
> > + __entry->cur_state = glock_trace_state(gl->gl_state);
> > + __entry->new_state = glock_trace_state(new_state);
> > + __entry->tgt_state = glock_trace_state(gl->gl_target);
> > + __entry->dmt_state = DLM_LOCK_IV;
> > + if (test_bit(GLF_DEMOTE, &gl->gl_flags) ||
> > + test_bit(GLF_PENDING_DEMOTE, &gl->gl_flags))
> > + __entry->dmt_state = glock_trace_state(gl->gl_demote_state);
>
> Wouldn't it be better to just trace gl_flags and gl_demote_state?
>
Yes, that sounds good. That is legacy code from the original patch to
blktrace which was rather more limited in scope and wasn't able to print
the flags.
> > +);
> > +
> > +#endif /* _TRACE_GFS2_H */
> > +
> > +/* This part must be outside protection */
> > +#undef TRACE_INCLUDE_PATH
> > +#define TRACE_INCLUDE_PATH ../../fs/gfs2/
>
> Shouldn't an
>
> #define TRACE_INCLUDE_PATH .
>
> do it? (although that would need -I$(src), not sure how good that is.
Ah, I though this was an alternative after the comments in the earlier
review, but I can look into it if you think thats better,
Steve.
More information about the Cluster-devel
mailing list