Re: [patch, rfc] autofs4: add trace points to the mount and unmount paths

[Ian Kent <raven@themaw.net>]

Steve Dickson wrote:
> Hey,
> 
> Comments in-line...
> 
> On 06/30/2009 01:08 PM, Jeff Moyer wrote:
>> Hi, Ian,
>>
>> I got a fairly simply request from a customer.  They wanted to use
>> systemtap to track automounter mount and unmount activity.
>> Unfortunately, the compiler inlined the expire functions I wanted to
>> instrument, so I needed to add a tracepoint to get at useful
>> information.
>>
>> This patch, then, adds a few trace points so that we don't have to worry
>> about compiler inlining.  I have an example script using these trace
>> points that I'll be sending along to the systemtap mailing list
>> shortly.  Let me know what you think.  If you're interested in trying it
>> out, I've made my systemtap patch available at:
>>   http://people.redhat.com/jmoyer/systemtap
>>
>> You can try running testsuite/systemtap.examples/general/autofs.stp.  It
>> should run with or without the below patch applied, though the results
>> are not as accurate (can't get full paths for unmounts) without the
>> tracepoint in autofs4_do_expire_multi.
>>
>> Cheers,
>> Jeff
>>
>> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
>>
>> diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c
>> index aa39ae8..6c86df9 100644
>> --- a/fs/autofs4/expire.c
>> +++ b/fs/autofs4/expire.c
>> @@ -13,6 +13,7 @@
>>   * ------------------------------------------------------------------------- */
>>  
>>  #include "autofs_i.h"
>> +#include <trace/events/autofs4.h>
>>  
>>  static unsigned long now;
>>  
>> @@ -496,6 +497,7 @@ int autofs4_do_expire_multi(struct super_block *sb, struct vfsmount *mnt,
>>  		/* This is synchronous because it makes the daemon a
>>                     little easier */
>>  		ret = autofs4_wait(sbi, dentry, NFY_EXPIRE);
>> +		trace_autofs4_do_expire_multi(mnt, dentry, ret);

Probably should be before the wait because if it expires away we
probably won't be able to retrieve the path.

>>  
>>  		spin_lock(&sbi->fs_lock);
>>  		if (ino->flags & AUTOFS_INF_MOUNTPOINT) {
>> diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
>> index b96a3c5..cbb17b7 100644
>> --- a/fs/autofs4/root.c
>> +++ b/fs/autofs4/root.c
>> @@ -19,6 +19,9 @@
>>  #include <linux/time.h>
>>  #include "autofs_i.h"
>>  
>> +#define CREATE_TRACE_POINTS
>> +#include <trace/events/autofs4.h>
>> +
>>  static int autofs4_dir_symlink(struct inode *,struct dentry *,const char *);
>>  static int autofs4_dir_unlink(struct inode *,struct dentry *);
>>  static int autofs4_dir_rmdir(struct inode *,struct dentry *);
>> @@ -216,6 +219,7 @@ static void *autofs4_follow_link(struct dentry *dentry, struct nameidata *nd)
>>  	    (!d_mountpoint(dentry) && __simple_empty(dentry))) {
>>  		spin_unlock(&dcache_lock);
>>  
>> +		trace_autofs4_follow_link(nd, dentry);
>>  		status = try_to_fill_dentry(dentry, 0);
> Would it make sense to put the trace point after the 
> try_to_fill_dentry() so the status could be recorded in the 
> trace point? Similar to the trace point above?

I suspect that's likely not going to be the most interesting case for
us. People are likely to want to know when mounting and expiring is
happening but and know when a fail return occurrs, of course, that still
leaves the issue of being swamped with output.

> 
>>  		if (status)
>>  			goto out_error;
>> @@ -540,6 +544,8 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
>>  			dput(expiring);
>>  		}
>>  
>> +		trace_autofs4_lookup(nd, dentry);
> Same kinda idea here... would it make sense to move this
> trace point some where so the status can be recorded?
> 
> The main point with these two question is I have found it 
> very handy to write system tap scripts that only fire 
> when there is a non-zero status.... Which means instead 
> of getting pages and pages of info scrolling off the 
> screen, I get one (or few) lines of error conditions that
> generally have a bit more meaning... 
> 
> Just a suggestion...  
> 
> steved.