[PATCH 1/2] vfs: make fcheck_files() an exported functions

[PATCH 1/2] vfs: make fcheck_files() an exported functions

[Tejun Heo]

We want to add a trace point to fcheck_files() but macros and inline
functions defined in header files can't have tracing points.  Move
fcheck_files() to fs/file.c and make it a proper function.

A lot of high-frequency fcheck*() users are inside fs/file.c, and, to
reduce the effect of this change, the new exported function is also
declared inline.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
---
These two patches add vfs_fcheck tracepoint.  Making fcheck_files() a
function isn't optimal but given the tracepoint restriction I can't
think of a better way.  The TP is currently in use in google to allow
ioblame to track who's accessing which file which in turn is used to
approximately associate IOs with files.  I'm working to upstream the
rest of ioblame.

Thanks.

 fs/file.c               |   11 +++++++++++
 include/linux/fdtable.h |   11 +----------
 2 files changed, 12 insertions(+), 10 deletions(-)

--- a/fs/file.c
+++ b/fs/file.c
@@ -709,6 +709,17 @@ void do_close_on_exec(struct files_struc
 	spin_unlock(&files->file_lock);
 }
 
+inline struct file *fcheck_files(struct files_struct *files, unsigned int fd)
+{
+	struct file * file = NULL;
+	struct fdtable *fdt = files_fdtable(files);
+
+	if (fd < fdt->max_fds)
+		file = rcu_dereference_check_fdtable(files, fdt->fd[fd]);
+	return file;
+}
+EXPORT_SYMBOL(fcheck_files);
+
 struct file *fget(unsigned int fd)
 {
 	struct file *file;
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -75,16 +75,6 @@ struct dentry;
 
 extern void __init files_defer_init(void);
 
-static inline struct file * fcheck_files(struct files_struct *files, unsigned int fd)
-{
-	struct file * file = NULL;
-	struct fdtable *fdt = files_fdtable(files);
-
-	if (fd < fdt->max_fds)
-		file = rcu_dereference_check_fdtable(files, fdt->fd[fd]);
-	return file;
-}
-
 /*
  * Check whether the specified fd has an open file.
  */
@@ -92,6 +82,7 @@ static inline struct file * fcheck_files
 
 struct task_struct;
 
+struct file *fcheck_files(struct files_struct *files, unsigned int fd);
 struct files_struct *get_files_struct(struct task_struct *);
 void put_files_struct(struct files_struct *fs);
 void reset_files_struct(struct files_struct *);

[PATCH 2/2] vfs: add fcheck tracepoint

[Tejun Heo]

All file accesses from userland go through fcheck to map fd to struct
file, making it a very good location for peeking at what files
userland is accessing.  Add a tracepoint there.

This is part of tracepoint additions to improve visiblity into
dirtying / writeback operations for io tracer and userland.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/file.c                  |    3 +++
 fs/super.c                 |    2 ++
 include/trace/events/vfs.h |   42 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+)

--- a/fs/file.c
+++ b/fs/file.c
@@ -22,6 +22,7 @@
 #include <linux/spinlock.h>
 #include <linux/rcupdate.h>
 #include <linux/workqueue.h>
+#include <trace/events/vfs.h>
 
 struct fdtable_defer {
 	spinlock_t lock;
@@ -716,6 +717,8 @@ inline struct file *fcheck_files(struct
 
 	if (fd < fdt->max_fds)
 		file = rcu_dereference_check_fdtable(files, fdt->fd[fd]);
+
+	trace_vfs_fcheck(files, fd, file, _THIS_IP_);
 	return file;
 }
 EXPORT_SYMBOL(fcheck_files);
--- a/fs/super.c
+++ b/fs/super.c
@@ -36,6 +36,8 @@
 #include <linux/lockdep.h>
 #include "internal.h"
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/vfs.h>
 
 LIST_HEAD(super_blocks);
 DEFINE_SPINLOCK(sb_lock);
--- /dev/null
+++ b/include/trace/events/vfs.h
@@ -0,0 +1,42 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM vfs
+
+#if !defined(_TRACE_VFS_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_VFS_H
+
+#include <linux/tracepoint.h>
+#include <linux/fs.h>
+
+TRACE_EVENT(vfs_fcheck,
+
+	TP_PROTO(struct files_struct *files, unsigned int fd,
+		 struct file *file, unsigned long ip),
+
+	TP_ARGS(files, fd, file, ip),
+
+	TP_STRUCT__entry(
+		__field(unsigned int,	fd)
+		__field(umode_t,	mode)
+		__field(dev_t,		dev)
+		__field(ino_t,		ino)
+		__field(unsigned long,	ip)
+	),
+
+	TP_fast_assign(
+		__entry->fd = fd;
+		__entry->mode = file ? file->f_path.dentry->d_inode->i_mode : 0;
+		__entry->dev = file ? file->f_path.dentry->d_inode->i_sb->s_dev : 0;
+		__entry->ino = file ? file->f_path.dentry->d_inode->i_ino : 0;
+		__entry->ip = ip;
+	),
+
+	TP_printk("fd %u mode 0x%x dev %d,%d ino %lu caller %pF",
+		  __entry->fd, __entry->mode,
+		  MAJOR(__entry->dev), MINOR(__entry->dev),
+		  (unsigned long)__entry->ino, (void *)__entry->ip)
+);
+
+#endif /* _TRACE_VFS_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>

Re: [PATCH 1/2] vfs: make fcheck_files() an exported functions

[Tejun Heo]

(cc'ing Andrew)

On Wed, Jan 09, 2013 at 09:01:05AM -0800, Tejun Heo wrote:
> We want to add a trace point to fcheck_files() but macros and inline
> functions defined in header files can't have tracing points.  Move
> fcheck_files() to fs/file.c and make it a proper function.
> 
> A lot of high-frequency fcheck*() users are inside fs/file.c, and, to
> reduce the effect of this change, the new exported function is also
> declared inline.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> ---
> These two patches add vfs_fcheck tracepoint.  Making fcheck_files() a
> function isn't optimal but given the tracepoint restriction I can't
> think of a better way.  The TP is currently in use in google to allow
> ioblame to track who's accessing which file which in turn is used to
> approximately associate IOs with files.  I'm working to upstream the
> rest of ioblame.

Andrew, can you please pick up these two patches?  They were posted a
month ago and at least nobody seems violently against them.  The
original patches are,

 http://article.gmane.org/gmane.linux.file-systems/70591/raw
 http://article.gmane.org/gmane.linux.file-systems/70592/raw

Thanks.

-- 
tejun

Re: [PATCH 1/2] vfs: make fcheck_files() an exported functions

[Al Viro]

On Sat, Feb 09, 2013 at 11:24:47AM -0800, Tejun Heo wrote:
> (cc'ing Andrew)
> 
> On Wed, Jan 09, 2013 at 09:01:05AM -0800, Tejun Heo wrote:
> > We want to add a trace point to fcheck_files() but macros and inline
> > functions defined in header files can't have tracing points.  Move
> > fcheck_files() to fs/file.c and make it a proper function.
> > 
> > A lot of high-frequency fcheck*() users are inside fs/file.c, and, to
> > reduce the effect of this change, the new exported function is also
> > declared inline.
> > 
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> > These two patches add vfs_fcheck tracepoint.  Making fcheck_files() a
> > function isn't optimal but given the tracepoint restriction I can't
> > think of a better way.  The TP is currently in use in google to allow
> > ioblame to track who's accessing which file which in turn is used to
> > approximately associate IOs with files.  I'm working to upstream the
> > rest of ioblame.
> 
> Andrew, can you please pick up these two patches?  They were posted a
> month ago and at least nobody seems violently against them.  The
> original patches are,

Consider *any* tracepoints in that area blanketly NAKed.  Sorry, I thought
I made it clear, but just in case:

As far as I'm concerned, *the* *only* interface stability warranties in VFS
are those for syscalls.  Which means that no tracepoints are going to be
acceptable there.  End of story.

Re: [PATCH 1/2] vfs: make fcheck_files() an exported functions

[Tejun Heo]

Hey, Al.

On Sun, Feb 10, 2013 at 12:18:12AM +0000, Al Viro wrote:
> > Andrew, can you please pick up these two patches?  They were posted a
> > month ago and at least nobody seems violently against them.  The
> > original patches are,
> 
> Consider *any* tracepoints in that area blanketly NAKed.  Sorry, I thought
> I made it clear, but just in case:

Heh, this is the first time I hear about it.

> As far as I'm concerned, *the* *only* interface stability warranties in VFS
> are those for syscalls.  Which means that no tracepoints are going to be
> acceptable there.  End of story.

I see, it's about API stability.  This TP is expected to be used by
internal tracer to approxmiately match generated IOs to specific
files, so at least the proposed usage is inside the kernel.  I suppose
there's no way to mark TPs internal, Steven?

Anyways, I haven't looked at the tracer code for a while, and am not
sure whether the information can be acquired differently.  Will take
another look.

Thanks.

-- 
tejun

Re: [PATCH 1/2] vfs: make fcheck_files() an exported functions

[Steven Rostedt]

[ Added Ingo and Linus ]

On Tue, 2013-02-12 at 09:41 -0800, Tejun Heo wrote:
> Hey, Al.
> 
> On Sun, Feb 10, 2013 at 12:18:12AM +0000, Al Viro wrote:
> > > Andrew, can you please pick up these two patches?  They were posted a
> > > month ago and at least nobody seems violently against them.  The
> > > original patches are,
> > 
> > Consider *any* tracepoints in that area blanketly NAKed.  Sorry, I thought
> > I made it clear, but just in case:
> 
> Heh, this is the first time I hear about it.
> 
> > As far as I'm concerned, *the* *only* interface stability warranties in VFS
> > are those for syscalls.  Which means that no tracepoints are going to be
> > acceptable there.  End of story.
> 
> I see, it's about API stability.  This TP is expected to be used by
> internal tracer to approxmiately match generated IOs to specific
> files, so at least the proposed usage is inside the kernel.  I suppose
> there's no way to mark TPs internal, Steven?

We discussed this a couple of years ago at kernel summit, but for
various reasons it never got anywhere.

https://lkml.org/lkml/2010/11/16/606

Perhaps the way to do this is to have certain tracepoints only appear
with a kernel parameter. Something like "enable_debug_tracepoints",
where distros will not have it enabled by default (the word "debug"
should scare them). And thus, tools should never use them. But for those
that need to debug systems, the kernel parameter can be added, and these
tracepoints will "magically" appear.

I mean, do we have to support an ABI that requires a kernel parameter
set in order to use it?  Actually, debug didn't scare them enough for
debugfs, maybe the parameter should be called:
"enable_unstable_tracepoints" That would be even scarier. Or have it be
"enable_tracepoints_but_do_not_expect_them_to_exist", but that wastes up
too much of the kernel parameter text.

Hmm, this should be pretty ease to write up. Think that would work?

-- Steve

> 
> Anyways, I haven't looked at the tracer code for a while, and am not
> sure whether the information can be acquired differently.  Will take
> another look.
> 
> Thanks.
> 

Re: [PATCH 1/2] vfs: make fcheck_files() an exported functions

[Steven Rostedt]

On Tue, 2013-02-12 at 13:21 -0500, Steven Rostedt wrote:

> Perhaps the way to do this is to have certain tracepoints only appear
> with a kernel parameter. Something like "enable_debug_tracepoints",
> where distros will not have it enabled by default (the word "debug"
> should scare them). And thus, tools should never use them. But for those
> that need to debug systems, the kernel parameter can be added, and these
> tracepoints will "magically" appear.
> 
> I mean, do we have to support an ABI that requires a kernel parameter
> set in order to use it?  Actually, debug didn't scare them enough for
> debugfs, maybe the parameter should be called:
> "enable_unstable_tracepoints" That would be even scarier. Or have it be
> "enable_tracepoints_but_do_not_expect_them_to_exist", but that wastes up
> too much of the kernel parameter text.
> 

Thinking about this a little more, we can call it
"enable_unstable_tracepoints" and on boot up have a big banner that
says:


**************************************************************
**************************************************************
****   Unstable tracepoints enabled. Do not rely on them  ****
**************************************************************
**************************************************************

Have the boot up pause for 4 seconds displaying that. That text won't
prevent distros from adding it as default, but the 4 second pause sure
will :-)

With all the work on fast boot up times, if we purposely slow down the
kernel when enabled, no distro will want these enabled by default.

-- Steve

Re: [PATCH 1/2] vfs: make fcheck_files() an exported functions

[Al Viro]

On Tue, Feb 12, 2013 at 03:21:08PM -0500, Steven Rostedt wrote:
> Thinking about this a little more, we can call it
> "enable_unstable_tracepoints" and on boot up have a big banner that
> says:
> 
> 
> **************************************************************
> **************************************************************
> ****   Unstable tracepoints enabled. Do not rely on them  ****
> **************************************************************
> **************************************************************
> 
> Have the boot up pause for 4 seconds displaying that. That text won't
> prevent distros from adding it as default, but the 4 second pause sure
> will :-)
> 
> With all the work on fast boot up times, if we purposely slow down the
> kernel when enabled, no distro will want these enabled by default.

No, they'll just make that delay another kernel option ;-)

Re: [PATCH 1/2] vfs: make fcheck_files() an exported functions

[Steven Rostedt]

On Tue, 2013-02-12 at 20:44 +0000, Al Viro wrote:
> On Tue, Feb 12, 2013 at 03:21:08PM -0500, Steven Rostedt wrote:
> > Thinking about this a little more, we can call it
> > "enable_unstable_tracepoints" and on boot up have a big banner that
> > says:
> > 
> > 
> > **************************************************************
> > **************************************************************
> > ****   Unstable tracepoints enabled. Do not rely on them  ****
> > **************************************************************
> > **************************************************************
> > 
> > Have the boot up pause for 4 seconds displaying that. That text won't
> > prevent distros from adding it as default, but the 4 second pause sure
> > will :-)
> > 
> > With all the work on fast boot up times, if we purposely slow down the
> > kernel when enabled, no distro will want these enabled by default.
> 
> No, they'll just make that delay another kernel option ;-)

Now this an interesting question. How much userspace must we guarantee,
when the userspace that is used depends on a kernel change by a distro,
and not mainline.

Heck, we could make these tracepoints depend on lockdep. No production
kernel will enable that. But I guess the distro could modify it to not
depend on it, and then base on the tracepoint as is, and also complain.
And the more hacks we do, the more difficult it is for us to use the
tracepoints. :-p

Bah!

-- Steve

Re: [PATCH 1/2] vfs: make fcheck_files() an exported functions

[Al Viro]

On Tue, Feb 12, 2013 at 01:21:48PM -0500, Steven Rostedt wrote:
> [ Added Ingo and Linus ]

> > I see, it's about API stability.  This TP is expected to be used by
> > internal tracer to approxmiately match generated IOs to specific
> > files, so at least the proposed usage is inside the kernel.  I suppose
> > there's no way to mark TPs internal, Steven?
> 
> We discussed this a couple of years ago at kernel summit, but for
> various reasons it never got anywhere.
> 
> https://lkml.org/lkml/2010/11/16/606
> 
> Perhaps the way to do this is to have certain tracepoints only appear
> with a kernel parameter. Something like "enable_debug_tracepoints",
> where distros will not have it enabled by default (the word "debug"
> should scare them). And thus, tools should never use them. But for those
> that need to debug systems, the kernel parameter can be added, and these
> tracepoints will "magically" appear.
> 
> I mean, do we have to support an ABI that requires a kernel parameter
> set in order to use it?  Actually, debug didn't scare them enough for
> debugfs, maybe the parameter should be called:
> "enable_unstable_tracepoints" That would be even scarier. Or have it be
> "enable_tracepoints_but_do_not_expect_them_to_exist", but that wastes up
> too much of the kernel parameter text.
> 
> Hmm, this should be pretty ease to write up. Think that would work?

I don't know and TBH I'm rather dubious about that.  In the end, it's a
simple question of cost-shifting; people with scripts that use tracepoints
vs. people working with the code those tracepoints are in.  And as far as
I'm concerned, the only acceptable situation is when *all* the price is
paid by the former.  It works for exports and it works for modules; out-of-tree
code is protected only by the amount of work needed to modify the in-tree
code depending on the same things.  And if we decide that changing all
in-tree users of something is not too costly, that's it - out-of-tree folks
might ask for help coping with the resulting change, they might try to
explain why that change is a bad idea if they notice it being discussed before
it goes in, but they have no veto power.

IOW, somebody showing up with "my out-of-tree module doesn't compile anymore,
put those methods back, restore that export and move this mutex from struct
bar back to struct foo" will get told to take a hike.  Somebody showing up
with "my userland tool relied on having event generated every time when
this mutex had been taken on struct foo, with these fields of struct foo
dumped into event" will have much stronger case when they get told that
mutex got moved to struct bar which has no such fields and neither a way to get
to equivalent information nor consistency warranties those pieces of data used
to have under that mutex.

Scary kernel options might suffice, but we had cases of userland Fine
Piece Of Software growing a dependency on "optional" kernel-side FPOS, making
it mandatory and leaving us to deal with the fallout.  Pardon me the bluntness,
but what's to stop the same people from doing the following:
Step 1: randomly chosen unstable tracepoint becomes used by udev, if present
Step 2: Fedora starts putting your scary kernel option into grub configs
Step 3: udev (while swearing support for "legacy" setups lacking that option)
gets more and more dependent on it; at that stage, any kernel-side change
breaking the tracepoint in question would cause serious pain for $BIGNUM
Fedora boxen out there
Step 4: tracepoint in question is moved from unstable class to stable, because
it's not feasible to change anymore without major userland breakage on a lot
of existing systems
Step 5: support of "legacy" setups disappears, at least on the boxen with
sufficiently recent kernel versions.
Lather, rinse, repeat...

Re: [PATCH 1/2] vfs: make fcheck_files() an exported functions

[Steven Rostedt]

On Tue, 2013-02-12 at 20:43 +0000, Al Viro wrote:

> I don't know and TBH I'm rather dubious about that.  In the end, it's a
> simple question of cost-shifting; people with scripts that use tracepoints
> vs. people working with the code those tracepoints are in.  And as far as
> I'm concerned, the only acceptable situation is when *all* the price is
> paid by the former.  It works for exports and it works for modules; out-of-tree
> code is protected only by the amount of work needed to modify the in-tree
> code depending on the same things.  And if we decide that changing all
> in-tree users of something is not too costly, that's it - out-of-tree folks
> might ask for help coping with the resulting change, they might try to
> explain why that change is a bad idea if they notice it being discussed before
> it goes in, but they have no veto power.

There's your answer!

I can create tracepoints that will not show up to userspace.

Then I can write an out of tree module, that will find these "internal"
tracepoints and this module will make them visible to usespace. Anyone
that wants these tracepoints will have to compile and load my module.

The trick is, I will never submit this module to be included into
mainline.

Sure, a distro could load this by default, but it would be no different
than any other out of tree module. They would have no veto power over
changes to these tracepoints.

I mean come on. If we go so far as to have kernel developers use an out
of tree / never to be included module to have access to these
tracepoints. We can not be held liable for changes to them anymore than
we make other module ABI incompatible changes.

-- Steve