[PATCH] filename in tapset nfs.proc.open and nfs.proc.release

[PATCH] filename in tapset nfs.proc.open and nfs.proc.release

[hai wu]

tapset nfs.proc.open and nfs.proc.release in nfs_proc.stp only records
file's basename, which is not helpful when we use it to monitor NFS
file access activities.

The following patch would allow us to be able to see the full path for
NFS file, along with its NFS mount information.

diff --git a/tapset/linux/nfs_proc.stp b/tapset/linux/nfs_proc.stp
index 1339aee..5a804e4 100644
--- a/tapset/linux/nfs_proc.stp
+++ b/tapset/linux/nfs_proc.stp
@@ -1658,7 +1658,7 @@ probe nfs.proc.open = kernel.function("nfs_open") !,
        prot = get_prot_from_client(client)
        version = __nfs_version($inode)

-       filename = __file_filename($filp)
+  filename = task_dentry_path(task_current(), $filp->f_dentry, $filp->f_vfsmnt)
        flag = $filp->f_flags
        mode = $filp->f_mode

@@ -1693,7 +1693,7 @@ probe nfs.proc.release = kernel.function("nfs_release") !,
        prot = get_prot_from_client(client)
        version = __nfs_version($inode)

-       filename = __file_filename($filp)
+  filename = task_dentry_path(task_current(), $filp->f_dentry, $filp->f_vfsmnt)
        flag = $filp->f_flags
        mode = $filp->f_mode

Re: [PATCH] filename in tapset nfs.proc.open and nfs.proc.release

[hai wu]

It turns out the above works fine in RHEL5.9 but not in RHEL6. In
RHEL6, the line needs to be changed to the following instead:

filename = task_dentry_path(task_current(), $filp->f_path->dentry,
$filp->f_path->mnt)

On Sat, Dec 13, 2014 at 10:35 PM, hai wu <haiwu.us@gmail.com> wrote:
> tapset nfs.proc.open and nfs.proc.release in nfs_proc.stp only records
> file's basename, which is not helpful when we use it to monitor NFS
> file access activities.
>
> The following patch would allow us to be able to see the full path for
> NFS file, along with its NFS mount information.
>
> diff --git a/tapset/linux/nfs_proc.stp b/tapset/linux/nfs_proc.stp
> index 1339aee..5a804e4 100644
> --- a/tapset/linux/nfs_proc.stp
> +++ b/tapset/linux/nfs_proc.stp
> @@ -1658,7 +1658,7 @@ probe nfs.proc.open = kernel.function("nfs_open") !,
>         prot = get_prot_from_client(client)
>         version = __nfs_version($inode)
>
> -       filename = __file_filename($filp)
> +  filename = task_dentry_path(task_current(), $filp->f_dentry, $filp->f_vfsmnt)
>         flag = $filp->f_flags
>         mode = $filp->f_mode
>
> @@ -1693,7 +1693,7 @@ probe nfs.proc.release = kernel.function("nfs_release") !,
>         prot = get_prot_from_client(client)
>         version = __nfs_version($inode)
>
> -       filename = __file_filename($filp)
> +  filename = task_dentry_path(task_current(), $filp->f_dentry, $filp->f_vfsmnt)
>         flag = $filp->f_flags
>         mode = $filp->f_mode

Re: [PATCH] filename in tapset nfs.proc.open and nfs.proc.release

[David Smith]

On 12/13/2014 10:35 PM, hai wu wrote:
> tapset nfs.proc.open and nfs.proc.release in nfs_proc.stp only records
> file's basename, which is not helpful when we use it to monitor NFS
> file access activities.
> 
> The following patch would allow us to be able to see the full path for
> NFS file, along with its NFS mount information.
> 
> diff --git a/tapset/linux/nfs_proc.stp b/tapset/linux/nfs_proc.stp
> index 1339aee..5a804e4 100644
> --- a/tapset/linux/nfs_proc.stp
> +++ b/tapset/linux/nfs_proc.stp
> @@ -1658,7 +1658,7 @@ probe nfs.proc.open = kernel.function("nfs_open") !,
>         prot = get_prot_from_client(client)
>         version = __nfs_version($inode)
> 
> -       filename = __file_filename($filp)
> +  filename = task_dentry_path(task_current(), $filp->f_dentry, $filp->f_vfsmnt)

Thanks for the patch, but I don't think this is really what we want
here. Here's why:

- The __file_filename() function just does a few casts and copies a
field out of $filp. It is fairly low cost.

- The task_dentry_path() function has to recursively walk the filesystem
building the full path. It can be quite expensive, depending on how deep
the file is in the nfs mount or how far down the the nfs mount is
mounted on the client system.

In general, we try to keep those convenience variables low cost. If your
use case needs a full path, that's fine. We just shouldn't make everyone
pay the higher cost.


Your 2nd email noted that you needed a different line for RHEL6:

filename = task_dentry_path(task_current(), $filp->f_path->dentry,
$filp->f_path->mnt)

Here's a (untested) way to use the same source line everywhere:

  pathname = task_dentry_path(task_current(),
	@choose_defined($filp->f_dentry, $filp->f_path->dentry),
	@choose_defined($filp->f_vfsmnt, $filp->f_path->mnt))

I hope this helps.

-- 
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)

Re: [PATCH] filename in tapset nfs.proc.open and nfs.proc.release

[Frank Ch. Eigler]

David Smith <dsmith@redhat.com> writes:

> [...]  Thanks for the patch, but I don't think this is really what
> we want here. Here's why: [...]

The full pathname is almost certainly more useful in the many probes
where __file_filename() is currently used (I recall receiving some
complaints about that long ago, just not getting to the bottom), and
the cost is not -that- high.  But point taken, some might not want to
pay that.

One way to have it both ways is to extend each these probe aliases
(only some dozen or so) to define a new context variable like
'fullpath' sibling of 'file*name'.  It would fully resolve the path
name.  (I'd certainly switch argstr over to print that instead of the
last component.)

- FChE

Re: [PATCH] filename in tapset nfs.proc.open and nfs.proc.release

[David Smith]

On 12/15/2014 02:34 PM, Frank Ch. Eigler wrote:
> David Smith <dsmith@redhat.com> writes:
> 
>> [...]  Thanks for the patch, but I don't think this is really what
>> we want here. Here's why: [...]
> 
> The full pathname is almost certainly more useful in the many probes
> where __file_filename() is currently used (I recall receiving some
> complaints about that long ago, just not getting to the bottom), and
> the cost is not -that- high.  But point taken, some might not want to
> pay that.
> 
> One way to have it both ways is to extend each these probe aliases
> (only some dozen or so) to define a new context variable like
> 'fullpath' sibling of 'file*name'.  It would fully resolve the path
> name.  (I'd certainly switch argstr over to print that instead of the
> last component.)

I'm OK with providing a 'fullpath' variable (which would get optimized
out if you didn't use it), but I'd need more convincing about the wisdom
of switching argstr over to using it. As you said above "some might not
want to pay that".

-- 
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)

Re: [PATCH] filename in tapset nfs.proc.open and nfs.proc.release

[Lukas Berk]

Hey,

----- Original Message -----
> On 12/15/2014 02:34 PM, Frank Ch. Eigler wrote:
> > David Smith <dsmith@redhat.com> writes:
[...]
> > One way to have it both ways is to extend each these probe aliases
> > (only some dozen or so) to define a new context variable like
> > 'fullpath' sibling of 'file*name'.  It would fully resolve the path
> > name.  (I'd certainly switch argstr over to print that instead of the
> > last component.)
> 
> I'm OK with providing a 'fullpath' variable (which would get optimized
> out if you didn't use it), but I'd need more convincing about the wisdom
> of switching argstr over to using it. As you said above "some might not
> want to pay that".

It may be the case where, those looking to use the argstr convenience variable
simply want to print out all the information being passed, regardless of
the cost, already.  In that case, finding they need to craft another, very
similar script to get a piece of related information (without knowing why it
wasn't passed in the first place) could be frustrating.  I've always looked
at argstr (rightly or wrongly) as 'just tell me everything being passed',
knowing I could write a more tailored script later if needed.

Either way, I'd recommend noting the choice we've made, and the alternate
variable in a manpage.

Cheers,

Lukas

Re: [PATCH] filename in tapset nfs.proc.open and nfs.proc.release

[David Smith]

On 12/17/2014 11:36 AM, Lukas Berk wrote:

> It may be the case where, those looking to use the argstr convenience variable
> simply want to print out all the information being passed, regardless of
> the cost, already.  In that case, finding they need to craft another, very
> similar script to get a piece of related information (without knowing why it
> wasn't passed in the first place) could be frustrating.  I've always looked
> at argstr (rightly or wrongly) as 'just tell me everything being passed',
> knowing I could write a more tailored script later if needed.

In this case, we're not providing "everything being passed". In
nfs.proc.open, the probe provides:

- server_ip: IP address of server
- prot: transfer protocol
- version: NFS version (the function is used for all NFS version)
- filename: file name
- flag: file flag
- mode: file mode

Only filename, flag, and mode end up in argstr.

(Why was the probe written this way? I don't know, the tapset has been
around a long time.)

If our definition of argstr is "everything (of importance) being
*passed*", then we'd want the filename (which is passed), not the full
path (which isn't passed). The kernel rarely passes around full paths
internally, just a file pointer which contains the file name and a
pointer to the dentry (which points us to the parent directory). Also
note that task_dentry_path() can fail for various reasons, while getting
the file name from the file pointer should succeed most (if not all) of
the time.

-- 
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)

Re: [PATCH] filename in tapset nfs.proc.open and nfs.proc.release

[Lukas Berk]

Hey,

David Smith <dsmith@redhat.com> writes:
> On 12/17/2014 11:36 AM, Lukas Berk wrote:
[...]
>> at argstr (rightly or wrongly) as 'just tell me everything being passed',
>> knowing I could write a more tailored script later if needed.
>
> In this case, we're not providing "everything being passed". In
> nfs.proc.open, the probe provides:

Ok, so wrongly percieved by me :)  Simply trying to point out that users
of such convience variables may be more interested having variables
easily reported to them.

> - server_ip: IP address of server
> - prot: transfer protocol
> - version: NFS version (the function is used for all NFS version)
> - filename: file name
> - flag: file flag
> - mode: file mode
>
> Only filename, flag, and mode end up in argstr.
>
> (Why was the probe written this way? I don't know, the tapset has been
> around a long time.)

Right, though I think it pertinent to echo that, we (at some point) have
decided to summarize/select variables of importance for the user, and
that it's not a direct mapping to the call itself.

> If our definition of argstr is "everything (of importance) being
> *passed*", then we'd want the filename (which is passed), not the full
> path (which isn't passed).

I'm not sure I follow why the path isn't important, given that the patch
originated due to needing the full path.  AIUI (and as mentioned below),
the parent directory *is* passed as a member variable to the file struct
($filp).  

> The kernel rarely passes around full paths internally, just a file
> pointer which contains the file name and a pointer to the dentry
> (which points us to the parent directory).

Understood, though we've recognized that the convenience variable we
provide don't always map directly to what is passed internally.  From
the perspective of the user it may not always be relevant how the kernel
passes the information.

> Also note that task_dentry_path() can fail for various reasons, while
> getting the file name from the file pointer should succeed most (if
> not all) of the time.

Again, understood, though perhaps we could fallback on just the file
pointer if task_dentry_path() fails automatically for the user?  Aim for
the best case and provide them with the information if available?

In the context of the original discussion to include the full path in
argstr; I think that providing more relevant information to the user
than less is the right way to go, especially when the full path name can
eliminate some uncertainty for the user.  Whichever way you decide to
go, I'd be happy to contribute some documentation in the synopsis
portion of the manpage.

Cheers,

Lukas

Re: [PATCH] filename in tapset nfs.proc.open and nfs.proc.release

[Frank Ch. Eigler]

Hi -

IMHO, a reasonable compromise is to have an extra full-path variable
in the tapset rather than change the existing one, -and- include the
full-path rather than basename in argstr.

- FChE