[PACTH v1] trace: Add trace events for open(), exec() and uselib()

[PACTH v1] trace: Add trace events for open(), exec() and uselib()

[robert.foss]

From: Scott James Remnant <scott@ubuntu.com>

This patch uses TRACE_EVENT to add tracepoints for the open(),
exec() and uselib() syscalls so that ureadahead can cheaply trace
the boot sequence to determine what to read to speed up the next.

Signed-off-by: Scott James Remnant <scott@ubuntu.com>
Tested-by: Robert Foss <robert.foss@collabora.com>
Signed-off-by: Robert Foss <robert.foss@collabora.com>
---
 fs/exec.c                 |  7 ++++++-
 fs/open.c                 |  4 ++++
 include/trace/events/fs.h | 53 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 1 deletion(-)
 create mode 100644 include/trace/events/fs.h

diff --git a/fs/exec.c b/fs/exec.c
index 887c1c9..123b257 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -58,6 +58,8 @@
 #include <linux/compat.h>
 #include <linux/vmalloc.h>
 
+#include <trace/events/fs.h>
+
 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>
 #include <asm/tlb.h>
@@ -797,9 +799,12 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
 	if (err)
 		goto exit;
 
-	if (name->name[0] != '\0')
+	if (name->name[0] != '\0') {
 		fsnotify_open(file);
 
+		trace_open_exec(name->name);
+	}
+
 out:
 	return file;
 
diff --git a/fs/open.c b/fs/open.c
index 93ae3cd..2ec0680 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -34,6 +34,9 @@
 
 #include "internal.h"
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/fs.h>
+
 int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
 	struct file *filp)
 {
@@ -1020,6 +1023,7 @@ long do_sys_open(int dfd, const char __user *filename, int flags, umode_t mode)
 		} else {
 			fsnotify_open(f);
 			fd_install(fd, f);
+			trace_do_sys_open(tmp->name, flags, mode);
 		}
 	}
 	putname(tmp);
diff --git a/include/trace/events/fs.h b/include/trace/events/fs.h
new file mode 100644
index 0000000..fb634b7
--- /dev/null
+++ b/include/trace/events/fs.h
@@ -0,0 +1,53 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM fs
+
+#if !defined(_TRACE_FS_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_FS_H
+
+#include <linux/fs.h>
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(do_sys_open,
+
+	TP_PROTO(const char *filename, int flags, int mode),
+
+	TP_ARGS(filename, flags, mode),
+
+	TP_STRUCT__entry(
+		__string(	filename, filename		)
+		__field(	int, flags			)
+		__field(	int, mode			)
+	),
+
+	TP_fast_assign(
+		__assign_str(filename, filename);
+		__entry->flags = flags;
+		__entry->mode = mode;
+	),
+
+	TP_printk("\"%s\" %x %o",
+		  __get_str(filename), __entry->flags, __entry->mode)
+);
+
+TRACE_EVENT(open_exec,
+
+	TP_PROTO(const char *filename),
+
+	TP_ARGS(filename),
+
+	TP_STRUCT__entry(
+		__string(	filename, filename		)
+	),
+
+	TP_fast_assign(
+		__assign_str(filename, filename);
+	),
+
+	TP_printk("\"%s\"",
+		  __get_str(filename))
+);
+
+#endif /* _TRACE_FS_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
-- 
2.7.4

Re: [PACTH v1] trace: Add trace events for open(), exec() and uselib()

[Steven Rostedt]

On Mon,  1 Aug 2016 13:25:40 -0400
robert.foss@collabora.com wrote:

> From: Scott James Remnant <scott@ubuntu.com>
> 
> This patch uses TRACE_EVENT to add tracepoints for the open(),
> exec() and uselib() syscalls so that ureadahead can cheaply trace
> the boot sequence to determine what to read to speed up the next.
> 

Good luck. AFAIK, Viro refuses to have tracepoints in the vfs subsystem.

-- Steve

> Signed-off-by: Scott James Remnant <scott@ubuntu.com>
> Tested-by: Robert Foss <robert.foss@collabora.com>
> Signed-off-by: Robert Foss <robert.foss@collabora.com>
> ---
>  fs/exec.c                 |  7 ++++++-
>  fs/open.c                 |  4 ++++
>  include/trace/events/fs.h | 53 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+), 1 deletion(-)
>  create mode 100644 include/trace/events/fs.h
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 887c1c9..123b257 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -58,6 +58,8 @@
>  #include <linux/compat.h>
>  #include <linux/vmalloc.h>
>  
> +#include <trace/events/fs.h>
> +
>  #include <asm/uaccess.h>
>  #include <asm/mmu_context.h>
>  #include <asm/tlb.h>
> @@ -797,9 +799,12 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
>  	if (err)
>  		goto exit;
>  
> -	if (name->name[0] != '\0')
> +	if (name->name[0] != '\0') {
>  		fsnotify_open(file);
>  
> +		trace_open_exec(name->name);
> +	}
> +
>  out:
>  	return file;
>  
> diff --git a/fs/open.c b/fs/open.c
> index 93ae3cd..2ec0680 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -34,6 +34,9 @@
>  
>  #include "internal.h"
>  
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/fs.h>
> +
>  int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
>  	struct file *filp)
>  {
> @@ -1020,6 +1023,7 @@ long do_sys_open(int dfd, const char __user *filename, int flags, umode_t mode)
>  		} else {
>  			fsnotify_open(f);
>  			fd_install(fd, f);
> +			trace_do_sys_open(tmp->name, flags, mode);
>  		}
>  	}
>  	putname(tmp);
> diff --git a/include/trace/events/fs.h b/include/trace/events/fs.h
> new file mode 100644
> index 0000000..fb634b7
> --- /dev/null
> +++ b/include/trace/events/fs.h
> @@ -0,0 +1,53 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM fs
> +
> +#if !defined(_TRACE_FS_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_FS_H
> +
> +#include <linux/fs.h>
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(do_sys_open,
> +
> +	TP_PROTO(const char *filename, int flags, int mode),
> +
> +	TP_ARGS(filename, flags, mode),
> +
> +	TP_STRUCT__entry(
> +		__string(	filename, filename		)
> +		__field(	int, flags			)
> +		__field(	int, mode			)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(filename, filename);
> +		__entry->flags = flags;
> +		__entry->mode = mode;
> +	),
> +
> +	TP_printk("\"%s\" %x %o",
> +		  __get_str(filename), __entry->flags, __entry->mode)
> +);
> +
> +TRACE_EVENT(open_exec,
> +
> +	TP_PROTO(const char *filename),
> +
> +	TP_ARGS(filename),
> +
> +	TP_STRUCT__entry(
> +		__string(	filename, filename		)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(filename, filename);
> +	),
> +
> +	TP_printk("\"%s\"",
> +		  __get_str(filename))
> +);
> +
> +#endif /* _TRACE_FS_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>

Re: [PACTH v1] trace: Add trace events for open(), exec() and uselib()

[Al Viro]

On Mon, Aug 01, 2016 at 01:44:53PM -0400, Steven Rostedt wrote:
> On Mon,  1 Aug 2016 13:25:40 -0400
> robert.foss@collabora.com wrote:
> 
> > From: Scott James Remnant <scott@ubuntu.com>
> > 
> > This patch uses TRACE_EVENT to add tracepoints for the open(),
> > exec() and uselib() syscalls so that ureadahead can cheaply trace
> > the boot sequence to determine what to read to speed up the next.
> > 
> 
> Good luck. AFAIK, Viro refuses to have tracepoints in the vfs subsystem.

Damn right.  Somebody wants to use those as private debugging patches -
sure, that's what the mechanism is good for.  Just keep them in your
test builds, where *you* will be responsible for keeping them working, etc.

Re: [PACTH v1] trace: Add trace events for open(), exec() and uselib()

[Al Viro]

On Mon, Aug 01, 2016 at 01:25:40PM -0400, robert.foss@collabora.com wrote:
> From: Scott James Remnant <scott@ubuntu.com>
> 
> This patch uses TRACE_EVENT to add tracepoints for the open(),
> exec() and uselib() syscalls so that ureadahead can cheaply trace
> the boot sequence to determine what to read to speed up the next.

NAK.  No Tracepoints In VFS.  Not going to happen - any tracepoint can all too
easily become a cast-in-stone userland ABI.

Re: [PACTH v1] trace: Add trace events for open(), exec() and uselib()

[Robert Foss]

On 2016-08-01 02:10 PM, Al Viro wrote:
> On Mon, Aug 01, 2016 at 01:25:40PM -0400, robert.foss@collabora.com wrote:
>> From: Scott James Remnant <scott@ubuntu.com>
>>
>> This patch uses TRACE_EVENT to add tracepoints for the open(),
>> exec() and uselib() syscalls so that ureadahead can cheaply trace
>> the boot sequence to determine what to read to speed up the next.
>
> NAK.  No Tracepoints In VFS.  Not going to happen - any tracepoint can all too
> easily become a cast-in-stone userland ABI.
>

Hey Al,

I'm slightly unfamiliar with this territory, so please forgive my lack 
of knowledge of this topic.

What is the negative side of having tracepoint be a permanent fixture in 
the VFS ABI?
And how is VFS different from other subsystems in that regard?


Rob.

Re: [PACTH v1] trace: Add trace events for open(), exec() and uselib()

[Steven Rostedt]

On Mon, 1 Aug 2016 15:51:54 -0400
Robert Foss <robert.foss@collabora.com> wrote:

> On 2016-08-01 02:10 PM, Al Viro wrote:
> > On Mon, Aug 01, 2016 at 01:25:40PM -0400, robert.foss@collabora.com wrote:  
> >> From: Scott James Remnant <scott@ubuntu.com>
> >>
> >> This patch uses TRACE_EVENT to add tracepoints for the open(),
> >> exec() and uselib() syscalls so that ureadahead can cheaply trace
> >> the boot sequence to determine what to read to speed up the next.  
> >
> > NAK.  No Tracepoints In VFS.  Not going to happen - any tracepoint can all too
> > easily become a cast-in-stone userland ABI.
> >  
> 
> Hey Al,
> 
> I'm slightly unfamiliar with this territory, so please forgive my lack 
> of knowledge of this topic.
> 
> What is the negative side of having tracepoint be a permanent fixture in 
> the VFS ABI?

Well, tracepoints are not vetted like syscalls are, but since they are
an interface for userspace tools, they can become just as permanent
of a fixture without the forethought of it being something that must be
maintained forever. This has already bitten us a couple of times.

> And how is VFS different from other subsystems in that regard?

The main difference is that Al is the maintainer of VFS and he's the
one that has to deal with maintaining a permanent tracepoint. He could
give less about other subsystems because it ain't his problem ;-)

-- Steve

Re: [PACTH v1] trace: Add trace events for open(), exec() and uselib()

[Ingo Molnar]

* Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Mon, Aug 01, 2016 at 01:25:40PM -0400, robert.foss@collabora.com wrote:
> > From: Scott James Remnant <scott@ubuntu.com>
> > 
> > This patch uses TRACE_EVENT to add tracepoints for the open(),
> > exec() and uselib() syscalls so that ureadahead can cheaply trace
> > the boot sequence to determine what to read to speed up the next.
> 
> NAK.  No Tracepoints In VFS.  Not going to happen - any tracepoint can all too
> easily become a cast-in-stone userland ABI.

And what's the problem with that? ABIs increase the utility of the kernel.

Thanks,

	Ingo