Re: [PATCH 00/16 v3] tracing: Add new file system tracefs

[Steven Rostedt]

On Mon, 26 Jan 2015 19:30:49 +0000
Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:

> You are still fighting an inconvenient API, but now it's not debugfs one -
> it's your copy thereof.  Why not give your instances/ an inode_operations
> of its own?  One with ->mkdir() and ->rmdir(), leaving all other directories
> as-is.  That way you don't need the secondary methods at all.  And sure,
> debugfs_create_dir() grabs ->i_mutex on parent, making you drop that in
> your ->mkdir() if you want to call it.  But now you are not talking to it -
> just to your own code, where you are free to change the calling conventions,
> making it caller's responsibility to get that ->i_mutex.  The same goes for
> the rmdir side...

The vfs layer grabs the i_mutex, which needs to be dropped. I was
hoping to keep the dropping of the i_mutex within the tracefs code and
not have the tracing code worry about it. I still will need to add
i_private to hold this info.

But that said, I could clean it up a little be having a
tracefs_create_instance_dir(), which does all the work within the
tracefs code. The caller would pass in the functions to be called by
that.

Like this patch applied on top of my current series:

-- Steve

diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index c7eb4c58579e..13e17bf677f4 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -50,6 +50,11 @@ static const struct file_operations tracefs_file_operations = {
 	.llseek =	noop_llseek,
 };
 
+struct tracefs_dir_ops {
+	int (*mkdir)(const char *name);
+	int (*rmdir)(const char *name);
+};
+
 static char *get_dname(struct dentry *dentry)
 {
 	const char *dname;
@@ -67,13 +72,10 @@ static char *get_dname(struct dentry *dentry)
 
 static int tracefs_syscall_mkdir(struct inode *inode, struct dentry *dentry, umode_t mode)
 {
-	const struct tracefs_dir_ops *ops = inode ? inode->i_private : NULL;
+	const struct tracefs_dir_ops *ops = inode->i_private;
 	char *name;
 	int ret;
 
-	if (!ops)
-		return -EPERM;
-
 	name = get_dname(dentry);
 	if (!name)
 		return -ENOMEM;
@@ -98,9 +100,6 @@ static int tracefs_syscall_rmdir(struct inode *inode, struct dentry *dentry)
 	char *name;
 	int ret;
 
-	if (!ops)
-		return -EPERM;
-
 	name = get_dname(dentry);
 	if (!name)
 		return -ENOMEM;
@@ -317,16 +316,6 @@ static struct file_system_type trace_fs_type = {
 };
 MODULE_ALIAS_FS("tracefs");
 
-void tracefs_add_dir_ops(struct dentry *dentry, const struct tracefs_dir_ops *ops)
-{
-	struct inode *inode = dentry->d_inode;
-
-	if (!inode)
-		return;
-
-	inode->i_private = (void *)ops;
-}
-
 static struct dentry *start_creating(const char *name, struct dentry *parent)
 {
 	struct dentry *dentry;
@@ -425,6 +414,31 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode,
 	return end_creating(dentry);
 }
 
+static struct dentry *__create_dir(const char *name, struct dentry *parent,
+				   const struct inode_operations *ops)
+{
+	struct dentry *dentry = start_creating(name, parent);
+	struct inode *inode;
+
+	if (IS_ERR(dentry))
+		return NULL;
+
+	inode = tracefs_get_inode(dentry->d_sb);
+	if (unlikely(!inode))
+		return failed_creating(dentry);
+
+	inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
+	inode->i_op = ops;
+	inode->i_fop = &simple_dir_operations;
+
+	/* directory inodes start off with i_nlink == 2 (for "." entry) */
+	inc_nlink(inode);
+	d_instantiate(dentry, inode);
+	inc_nlink(dentry->d_parent->d_inode);
+	fsnotify_mkdir(dentry->d_parent->d_inode, dentry);
+	return end_creating(dentry);
+}
+
 /**
  * tracefs_create_dir - create a directory in the tracefs filesystem
  * @name: a pointer to a string containing the name of the directory to
@@ -444,26 +458,30 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode,
  */
 struct dentry *tracefs_create_dir(const char *name, struct dentry *parent)
 {
-	struct dentry *dentry = start_creating(name, parent);
-	struct inode *inode;
+	return __create_dir(name, parent, &simple_dir_inode_operations);
+}
 
-	if (IS_ERR(dentry))
+struct dentry *tracefs_create_instance_dir(const char *name, struct dentry *parent,
+					  int (*mkdir)(const char *name),
+					  int (*rmdir)(const char *name))
+{
+	struct tracefs_dir_ops *dir_ops;
+	struct dentry *dentry;
+
+	dir_ops = kmalloc(sizeof(*dir_ops), GFP_KERNEL);
+	if (!dir_ops)
 		return NULL;
 
-	inode = tracefs_get_inode(dentry->d_sb);
-	if (unlikely(!inode))
-		return failed_creating(dentry);
+	dir_ops->mkdir = mkdir;
+	dir_ops->rmdir = mkdir;
 
-	inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
-	inode->i_op = &tracefs_dir_inode_operations;
-	inode->i_fop = &simple_dir_operations;
+	dentry = __create_dir(name, parent, &tracefs_dir_inode_operations);
+	if (!dentry)
+		return NULL;
 
-	/* directory inodes start off with i_nlink == 2 (for "." entry) */
-	inc_nlink(inode);
-	d_instantiate(dentry, inode);
-	inc_nlink(dentry->d_parent->d_inode);
-	fsnotify_mkdir(dentry->d_parent->d_inode, dentry);
-	return end_creating(dentry);
+	dentry->d_inode->i_private = (void *)dir_ops;
+
+	return dentry;
 }
 
 static inline int tracefs_positive(struct dentry *dentry)
diff --git a/include/linux/tracefs.h b/include/linux/tracefs.h
index d142b1f9d453..5b727a17beee 100644
--- a/include/linux/tracefs.h
+++ b/include/linux/tracefs.h
@@ -34,12 +34,9 @@ struct dentry *tracefs_create_dir(const char *name, struct dentry *parent);
 void tracefs_remove(struct dentry *dentry);
 void tracefs_remove_recursive(struct dentry *dentry);
 
-struct tracefs_dir_ops {
-	int (*mkdir)(const char *name);
-	int (*rmdir)(const char *name);
-};
-
-void tracefs_add_dir_ops(struct dentry *dentry, const struct tracefs_dir_ops *ops);
+struct dentry *tracefs_create_instance_dir(const char *name, struct dentry *parent,
+					   int (*mkdir)(const char *name),
+					   int (*rmdir)(const char *name));
 
 bool tracefs_initialized(void);
 
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index c555027c8aaf..2047501dcd16 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -6445,18 +6445,13 @@ static int instance_rmdir(const char *name)
 	return ret;
 }
 
-static const struct tracefs_dir_ops instance_dir_ops = {
-	.mkdir		= instance_mkdir,
-	.rmdir		= instance_rmdir,
-};
-
 static __init void create_trace_instances(struct dentry *d_tracer)
 {
-	trace_instance_dir = tracefs_create_dir("instances", d_tracer);
+	trace_instance_dir = tracefs_create_instance_dir("instances", d_tracer,
+							 instance_mkdir,
+							 instance_rmdir);
 	if (WARN_ON(!trace_instance_dir))
 		return;
-
-	tracefs_add_dir_ops(trace_instance_dir, &instance_dir_ops);
 }
 
 static void
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/