[Cluster-devel] Re: GFS2: Add tracepoints

[Christoph Hellwig <hch@infradead.org>]

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.