Re: [PATCH] vfs: Add a trace point in the mark_inode_dirty function

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> [ Adding Ingo, Peter and Mathieu ]
> 
> On Mon, 2010-11-29 at 14:15 +0900, KOSAKI Motohiro wrote:
> > > On Mon, 29 Nov 2010 10:41:51 +0900 (JST)
> > > KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:
> > > 
> > > > > Signed-of-by: Arjan van de Ven <arjan@linux.intel.com>
> > > > > ---
> > > > >  fs/fs-writeback.c                |    3 +++
> > > > >  include/linux/fs.h               |   12 ++++++++++++
> > > > >  include/trace/events/writeback.h |   28
> > > > > ++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 0
> > > > > deletions(-)
> > > > > 
> > > > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > > > > index 3d06ccc..62e33cc 100644
> > > > > --- a/fs/fs-writeback.c
> > > > > +++ b/fs/fs-writeback.c
> > > > > @@ -952,6 +952,9 @@ void __mark_inode_dirty(struct inode *inode,
> > > > > int flags) if ((inode->i_state & flags) == flags)
> > > > >  		return;
> > > > >  
> > > > > +	if (flags & (I_DIRTY_SYNC | I_DIRTY_DATASYNC |
> > > > > I_DIRTY_PAGES))
> > > > > +		trace_writeback_inode_dirty(inode, flags);
> > > > > +
> > > > 
> > > > Why can't we move this branch into TP_fast_assign()?
> > > 
> > > not really because then the tracepoint is already in process of being
> > > emitted... no way to retract it anymore.
> > 
> > I'm not tracing expert. but Steven said we can it in past. (cc him)
> > 
> > http://www.spinics.net/lists/linux-ext4/msg20045.html
> > 
> 
> Yes, this came up before. Currently, if you add the if statement in the
> trcacepoint, you just wasted space. But if we can add a discard_entry,
> or better yet, I can add a TRACE_EVENT_CONDITION() that adds an if
> statement to check. Something like:
> 
> 	TRACE_EVENT_CONDITION(name,
> 		[...]
> 	TP_condition(
> 			if (flags & (I_DIRTY_SYNC | I_DIRTY_DATASYNC |
> 			    I_DIRTY_PAGES))
> 				return 1;
> 			else
> 				return 0;
> 		    ),

Yep, something like this would be really useful for us too in user-space. One
example use-case is to let a telecom application filter by "call ID" when
following one call across multiple telecom switches. The kind of condition we
would have is:

 	TRACE_EVENT_CONDITION(name,
 		[...]
 	TP_condition(
 			if (unlikely(call_id == monitored_call_id))
 				return 1;
                        if (unlikely(!call_id_monitoring))
                                return 1;
 			return 0;
 		    ),

So in this case, "call_id" is received as parameter, but "monitored_call_id" and
"call_id_monitoring" are global variables.

One question that strikes me is: would you declare this outside of the
TRACE_EVENT() or inside it ? How would you match the TRACE_EVENT() and the
TRACE_EVENT_CONDITION() if they are separate, by name ?

The dynamic "pre-filters" will also be useful, but I think this kind of
statically defined conditions will answer a large amount of our requirements,
letting these filtering expressions be designed by application developers and
used by operators/engineers.

Thanks,

Mathieu

> 
> Have this run on the parameters and not the entry fields (because the
> entry fields are from the ring buffer, and to use them, we must first
> write to the ring buffer (something we want to avoid).
> 
> We could also make this code work with "pre-filters". That is, filters
> on the tracepoints that access the parameters and not the final entries.
> This would let us circumvent allocating ring buffer space when the
> filter states to skip the entry.
> 
> -- Steve
> 
> 			
> 
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com