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.

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

> +             __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?

> +);
> +
> +#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.

Reply via email to