From mboxrd@z Thu Jan  1 00:00:00 1970
From: Steven Whitehouse <swhiteho@redhat.com>
Date: Fri, 29 May 2009 20:50:10 +0100
Subject: [Cluster-devel] Re: GFS2: Add tracepoints
In-Reply-To: <20090529192352.GA2706@infradead.org>
References: <1243624260.3431.6.camel@localhost.localdomain>
	<20090529192352.GA2706@infradead.org>
Message-ID: <1243626610.3431.17.camel@localhost.localdomain>
List-Id: <cluster-devel.redhat.com>
To: cluster-devel.redhat.com
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit

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.