Debugfs : inotify, multiple calls to debugfs_create_file, remove

Debugfs : inotify, multiple calls to debugfs_create_file, remove

[Mathieu Desnoyers]

Hi Greg,

I just had to add inotify support to my LTTng consumer so I could inform it
of the presence of new CPUs (for CPU hotplug). I noticed that no
notification event was being sent when a debugfs file is created from within
the kernel through debugfs_create. There are probably other notifications
missing, but here is the patch adding the one I care about. Should it be added
in libfs or in debugfs ?

A second problem I noticed is when a caller calls debugfs_create_file more than
once : the result is that the debugfs_remove will fail. I guess the second call
to debugfs_create_file increments the reference counts (there is not fix for
this issue in my patch).

Third problem : a failing call to debugfs_remove keeps the filesystem pinned.
(fixed by calling simple_release_fs in the error path).

The third problem : When a process is in a directory, the call to simple_rmdir
will fail. Debugfs does not use its return value. I noticed that calling
simple_unlink on a directory when simple_rmdir fails removes the directory that
would otherwise be left there. I am not sure if this approach is correct
through.

This patch is against Linux 2.6.18.

Regards,

Mathieu


diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index e8ae304..d88203f 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -23,6 +23,7 @@ #include <linux/pagemap.h>
 #include <linux/init.h>
 #include <linux/namei.h>
 #include <linux/debugfs.h>
+#include <linux/fsnotify.h>
 
 #define DEBUGFS_MAGIC	0x64626720
 
@@ -156,7 +157,8 @@ static int debugfs_create_by_name(const 
 	} else
 		error = PTR_ERR(dentry);
 	mutex_unlock(&parent->d_inode->i_mutex);
-
+	if (!error)
+		fsnotify_create(parent->d_inode, *dentry);
 	return error;
 }
 
@@ -204,7 +206,10 @@ struct dentry *debugfs_create_file(const
 
 	error = debugfs_create_by_name(name, mode, parent, &dentry);
 	if (error) {
+		/* FIXME : a failing call to debugfs_create_file leaves a
+		 * stalled entry after a remove. */
 		dentry = NULL;
+		simple_release_fs(&debugfs_mount, &debugfs_mount_count);
 		goto exit;
 	}
 
@@ -265,6 +270,7 @@ EXPORT_SYMBOL_GPL(debugfs_create_dir);
 void debugfs_remove(struct dentry *dentry)
 {
 	struct dentry *parent;
+	int ret = 0;
 	
 	if (!dentry)
 		return;
@@ -277,8 +283,12 @@ void debugfs_remove(struct dentry *dentr
 	if (debugfs_positive(dentry)) {
 		if (dentry->d_inode) {
 			if (S_ISDIR(dentry->d_inode->i_mode))
-				simple_rmdir(parent->d_inode, dentry);
+				ret = simple_rmdir(parent->d_inode, dentry);
 			else
+				ret = simple_unlink(parent->d_inode, dentry);
+			/* simple_rmdir failed because the directory
+			 * is used. Force deletion. */
+			if (ret)
 				simple_unlink(parent->d_inode, dentry);
 		dput(dentry);
 		}

OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 

Re: Debugfs : inotify, multiple calls to debugfs_create_file, remove

[Greg KH]

On Mon, Nov 20, 2006 at 01:18:38PM -0500, Mathieu Desnoyers wrote:
> Hi Greg,
> 
> I just had to add inotify support to my LTTng consumer so I could inform it
> of the presence of new CPUs (for CPU hotplug). I noticed that no
> notification event was being sent when a debugfs file is created from within
> the kernel through debugfs_create. There are probably other notifications
> missing, but here is the patch adding the one I care about. Should it be added
> in libfs or in debugfs ?

So does this fix the inotify issue?

> A second problem I noticed is when a caller calls debugfs_create_file more than
> once : the result is that the debugfs_remove will fail. I guess the second call
> to debugfs_create_file increments the reference counts (there is not fix for
> this issue in my patch).
> 
> Third problem : a failing call to debugfs_remove keeps the filesystem pinned.
> (fixed by calling simple_release_fs in the error path).
> 
> The third problem : When a process is in a directory, the call to simple_rmdir
> will fail. Debugfs does not use its return value. I noticed that calling
> simple_unlink on a directory when simple_rmdir fails removes the directory that
> would otherwise be left there. I am not sure if this approach is correct
> through.
> 
> This patch is against Linux 2.6.18.

Care to split this into 4 different patches (you seem to have 4 issues
here), so that it's easier to see them, and it will follow the
1-patch-per-issue rule?

thanks,

greg k-h

Re: Debugfs : inotify, multiple calls to debugfs_create_file, remove

[Mathieu Desnoyers]

Hi Greg,

Sorry for the delay : I reworked my patches so they fully address the issues.
The patches follows.

Mathieu

* Greg KH (greg@kroah.com) wrote:
> On Mon, Nov 20, 2006 at 01:18:38PM -0500, Mathieu Desnoyers wrote:
> > Hi Greg,
> > 
> > I just had to add inotify support to my LTTng consumer so I could inform it
> > of the presence of new CPUs (for CPU hotplug). I noticed that no
> > notification event was being sent when a debugfs file is created from within
> > the kernel through debugfs_create. There are probably other notifications
> > missing, but here is the patch adding the one I care about. Should it be added
> > in libfs or in debugfs ?
> 
> So does this fix the inotify issue?
> 
> > A second problem I noticed is when a caller calls debugfs_create_file more than
> > once : the result is that the debugfs_remove will fail. I guess the second call
> > to debugfs_create_file increments the reference counts (there is not fix for
> > this issue in my patch).
> > 
> > Third problem : a failing call to debugfs_remove keeps the filesystem pinned.
> > (fixed by calling simple_release_fs in the error path).
> > 
> > The third problem : When a process is in a directory, the call to simple_rmdir
> > will fail. Debugfs does not use its return value. I noticed that calling
> > simple_unlink on a directory when simple_rmdir fails removes the directory that
> > would otherwise be left there. I am not sure if this approach is correct
> > through.
> > 
> > This patch is against Linux 2.6.18.
> 
> Care to split this into 4 different patches (you seem to have 4 issues
> here), so that it's easier to see them, and it will follow the
> 1-patch-per-issue rule?
> 
> thanks,
> 
> greg k-h
> 
OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 

[PATCH 1/5] DebugFS : inotify create/mkdir support

[Mathieu Desnoyers]

[-- Attachment #1: Type: text/plain, Size: 268 bytes --]

Add inotify create and mkdir events to DebugFS.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>


OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 

[-- Attachment #2: patch01-debugfs-inotify.diff --]
[-- Type: text/plain, Size: 780 bytes --]

--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -23,6 +23,7 @@
 #include <linux/init.h>
 #include <linux/namei.h>
 #include <linux/debugfs.h>
+#include <linux/fsnotify.h>
 
 #define DEBUGFS_MAGIC	0x64626720
 
@@ -87,15 +88,22 @@
 
 	mode = (mode & (S_IRWXUGO | S_ISVTX)) | S_IFDIR;
 	res = debugfs_mknod(dir, dentry, mode, 0);
-	if (!res)
+	if (!res) {
 		dir->i_nlink++;
+		fsnotify_mkdir(dir, dentry);
+	}
 	return res;
 }
 
 static int debugfs_create(struct inode *dir, struct dentry *dentry, int mode)
 {
+	int res;
+
 	mode = (mode & S_IALLUGO) | S_IFREG;
-	return debugfs_mknod(dir, dentry, mode, 0);
+	res = debugfs_mknod(dir, dentry, mode, 0);
+	if (!res)
+		fsnotify_create(dir, dentry);
+	return res;
 }
 
 static inline int debugfs_positive(struct dentry *dentry)

Re: [PATCH 1/5] DebugFS : inotify create/mkdir support

[Greg KH]

On Thu, Nov 23, 2006 at 02:51:48AM -0500, Mathieu Desnoyers wrote:
> Add inotify create and mkdir events to DebugFS.
> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>

What kernel version are these patches against?  They don't seem to be
against the 2.6.19-rc6 kernel...

thanks,

greg k-h

Re: [PATCH 1/5] DebugFS : inotify create/mkdir support

[Mathieu Desnoyers]

* Greg KH (greg@kroah.com) wrote:
> On Thu, Nov 23, 2006 at 02:51:48AM -0500, Mathieu Desnoyers wrote:
> > Add inotify create and mkdir events to DebugFS.
> > 
> > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
> 
> What kernel version are these patches against?  They don't seem to be
> against the 2.6.19-rc6 kernel...
> 

Sorry about this, I only noticed when sending the 3rd patch : it's against a
2.6.18 kernel.

thanks,

Mathieu

> thanks,
> 
> greg k-h
> 
OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 

[PATCH 1/5] DebugFS : inotify create/mkdir support, 2.6.19-rc6

[Mathieu Desnoyers]

(Same patch, against 2.6.19-rc6)

Add inotify create and mkdir events to DebugFS.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>


--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -23,6 +23,7 @@ #include <linux/pagemap.h>
 #include <linux/init.h>
 #include <linux/namei.h>
 #include <linux/debugfs.h>
+#include <linux/fsnotify.h>
 
 #define DEBUGFS_MAGIC	0x64626720
 
@@ -86,15 +87,22 @@ static int debugfs_mkdir(struct inode *d
 
 	mode = (mode & (S_IRWXUGO | S_ISVTX)) | S_IFDIR;
 	res = debugfs_mknod(dir, dentry, mode, 0);
-	if (!res)
+	if (!res) {
 		inc_nlink(dir);
+		fsnotify_mkdir(dir, dentry);
+	}
 	return res;
 }
 
 static int debugfs_create(struct inode *dir, struct dentry *dentry, int mode)
 {
+	int res;
+
 	mode = (mode & S_IALLUGO) | S_IFREG;
-	return debugfs_mknod(dir, dentry, mode, 0);
+	res = debugfs_mknod(dir, dentry, mode, 0);
+	if (!res)
+		fsnotify_create(dir, dentry);
+	return res;
 }
 
 static inline int debugfs_positive(struct dentry *dentry)


OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 

[PATCH 2/5] DebugFS : coding style fixes

[Mathieu Desnoyers]

[-- Attachment #1: Type: text/plain, Size: 1879 bytes --]

Minor coding style fixes along the way : 80 cols and a white space.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>


* Greg KH (greg@kroah.com) wrote:
> On Mon, Nov 20, 2006 at 01:18:38PM -0500, Mathieu Desnoyers wrote:
> > Hi Greg,
> > 
> > I just had to add inotify support to my LTTng consumer so I could inform it
> > of the presence of new CPUs (for CPU hotplug). I noticed that no
> > notification event was being sent when a debugfs file is created from within
> > the kernel through debugfs_create. There are probably other notifications
> > missing, but here is the patch adding the one I care about. Should it be added
> > in libfs or in debugfs ?
> 
> So does this fix the inotify issue?
> 
> > A second problem I noticed is when a caller calls debugfs_create_file more than
> > once : the result is that the debugfs_remove will fail. I guess the second call
> > to debugfs_create_file increments the reference counts (there is not fix for
> > this issue in my patch).
> > 
> > Third problem : a failing call to debugfs_remove keeps the filesystem pinned.
> > (fixed by calling simple_release_fs in the error path).
> > 
> > The third problem : When a process is in a directory, the call to simple_rmdir
> > will fail. Debugfs does not use its return value. I noticed that calling
> > simple_unlink on a directory when simple_rmdir fails removes the directory that
> > would otherwise be left there. I am not sure if this approach is correct
> > through.
> > 
> > This patch is against Linux 2.6.18.
> 
> Care to split this into 4 different patches (you seem to have 4 issues
> here), so that it's easier to see them, and it will follow the
> 1-patch-per-issue rule?
> 
> thanks,
> 
> greg k-h
> 
OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 

[-- Attachment #2: patch02-debugfs-codingstyle.diff --]
[-- Type: text/plain, Size: 589 bytes --]

--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -54,7 +54,8 @@
 			inode->i_op = &simple_dir_inode_operations;
 			inode->i_fop = &simple_dir_operations;
 
-			/* directory inodes start off with i_nlink == 2 (for "." entry) */
+			/* directory inodes start off with i_nlink == 2
+			 * (for "." entry) */
 			inode->i_nlink++;
 			break;
 		}
@@ -135,7 +136,7 @@
 	 * block. A pointer to that is in the struct vfsmount that we
 	 * have around.
 	 */
-	if (!parent ) {
+	if (!parent) {
 		if (debugfs_mount && debugfs_mount->mnt_sb) {
 			parent = debugfs_mount->mnt_sb->s_root;
 		}

[PATCH 2/5] DebugFS : coding style fixes, 2.6.19-rc6

[Mathieu Desnoyers]

Minor coding style fixes along the way : 80 cols and a white space.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>


--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -54,7 +54,8 @@ static struct inode *debugfs_get_inode(s
 			inode->i_op = &simple_dir_inode_operations;
 			inode->i_fop = &simple_dir_operations;
 
-			/* directory inodes start off with i_nlink == 2 (for "." entry) */
+			/* directory inodes start off with i_nlink == 2
+			 * (for "." entry) */
 			inc_nlink(inode);
 			break;
 		}
@@ -142,7 +143,7 @@ static int debugfs_create_by_name(const 
 	 * block. A pointer to that is in the struct vfsmount that we
 	 * have around.
 	 */
-	if (!parent ) {
+	if (!parent) {
 		if (debugfs_mount && debugfs_mount->mnt_sb) {
 			parent = debugfs_mount->mnt_sb->s_root;
 		}

OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 

[PATCH 3/5] DebugFS : file/directory creation error handling, 2.6.18

[Mathieu Desnoyers]

Fix error handling of file and directory creation in DebugFS.

The error path should release the file system because no _remove will be called
for this erroneous creation.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>


--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -198,13 +198,15 @@
 
 	pr_debug("debugfs: creating file '%s'\n",name);
 
-	error = simple_pin_fs(&debug_fs_type, &debugfs_mount, &debugfs_mount_count);
+	error = simple_pin_fs(&debug_fs_type, &debugfs_mount,
+		&debugfs_mount_count);
 	if (error)
 		goto exit;
 
 	error = debugfs_create_by_name(name, mode, parent, &dentry);
 	if (error) {
 		dentry = NULL;
+		simple_release_fs(&debugfs_mount, &debugfs_mount_count);
 		goto exit;
 	}
 


OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 

[PATCH 3/5] DebugFS : file/directory creation error handling, 2.6.19-rc6

[Mathieu Desnoyers]

Fix error handling of file and directory creation in DebugFS.

The error path should release the file system because no _remove will be called
for this erroneous creation.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>

-- BEGIN --
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -205,13 +205,15 @@ struct dentry *debugfs_create_file(const
 
 	pr_debug("debugfs: creating file '%s'\n",name);
 
-	error = simple_pin_fs(&debug_fs_type, &debugfs_mount, &debugfs_mount_count);
+	error = simple_pin_fs(&debug_fs_type, &debugfs_mount,
+		&debugfs_mount_count);
 	if (error)
 		goto exit;
 
 	error = debugfs_create_by_name(name, mode, parent, &dentry);
 	if (error) {
 		dentry = NULL;
+		simple_release_fs(&debugfs_mount, &debugfs_mount_count);
 		goto exit;
 	}
 
-- END --

OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 

[PATCH 4/5] DebugFS : file/directory creation error handling, 2.6.18

[Mathieu Desnoyers]

Correct dentry count to handle creation errors.

This patch puts a dput at the file creation instead of the file removal :
lookup_one_len already returns a dentry with reference count of 1. Then, 
the dget() in simple_mknod increments it when the dentry is associated with a
file. In a scenario where simple_create or simple_mkdir returns an error, this
would lead to an unwanted increment of the reference counter, therefore making
file removal impossible.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>


--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -153,10 +153,10 @@
 			error = debugfs_mkdir(parent->d_inode, *dentry, mode);
 		else 
 			error = debugfs_create(parent->d_inode, *dentry, mode);
+		dput(*dentry);
 	} else
 		error = PTR_ERR(dentry);
 	mutex_unlock(&parent->d_inode->i_mutex);
-
 	return error;
 }
 
@@ -265,6 +265,7 @@
 void debugfs_remove(struct dentry *dentry)
 {
 	struct dentry *parent;
+	int ret = 0;
 	
 	if (!dentry)
 		return;
@@ -276,11 +277,15 @@
 	mutex_lock(&parent->d_inode->i_mutex);
 	if (debugfs_positive(dentry)) {
 		if (dentry->d_inode) {
-			if (S_ISDIR(dentry->d_inode->i_mode))
-				simple_rmdir(parent->d_inode, dentry);
-			else
+			if (S_ISDIR(dentry->d_inode->i_mode)) {
+				ret = simple_rmdir(parent->d_inode, dentry);
+				if (ret)
+					printk(KERN_ERR
+						"DebugFS rmdir on %s failed : "
+						"directory not empty.\n",
+						dentry->d_name.name);
+			} else
 				simple_unlink(parent->d_inode, dentry);
-		dput(dentry);
 		}
 	}
 	mutex_unlock(&parent->d_inode->i_mutex);

OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 

[PATCH 4/5] DebugFS : file/directory creation error handling, 2.6.19-rc6

[Mathieu Desnoyers]

Correct dentry count to handle creation errors.

This patch puts a dput at the file creation instead of the file removal :
lookup_one_len already returns a dentry with reference count of 1. Then, 
the dget() in simple_mknod increments it when the dentry is associated with a
file. In a scenario where simple_create or simple_mkdir returns an error, this
would lead to an unwanted increment of the reference counter, therefore making
file removal impossible.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>

---BEGIN---
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -161,6 +161,7 @@ static int debugfs_create_by_name(const 
 			error = debugfs_mkdir(parent->d_inode, *dentry, mode);
 		else 
 			error = debugfs_create(parent->d_inode, *dentry, mode);
+		dput(*dentry);
 	} else
 		error = PTR_ERR(*dentry);
 	mutex_unlock(&parent->d_inode->i_mutex);
@@ -272,6 +273,7 @@ EXPORT_SYMBOL_GPL(debugfs_create_dir);
 void debugfs_remove(struct dentry *dentry)
 {
 	struct dentry *parent;
+	int ret = 0;
 	
 	if (!dentry)
 		return;
@@ -283,11 +285,15 @@ void debugfs_remove(struct dentry *dentr
 	mutex_lock(&parent->d_inode->i_mutex);
 	if (debugfs_positive(dentry)) {
 		if (dentry->d_inode) {
-			if (S_ISDIR(dentry->d_inode->i_mode))
-				simple_rmdir(parent->d_inode, dentry);
-			else
+			if (S_ISDIR(dentry->d_inode->i_mode)) {
+				ret = simple_rmdir(parent->d_inode, dentry);
+				if (ret)
+					printk(KERN_ERR
+						"DebugFS rmdir on %s failed : "
+						"directory not empty.\n",
+						dentry->d_name.name);
+			} else
 				simple_unlink(parent->d_inode, dentry);
-		dput(dentry);
 		}
 	}
 	mutex_unlock(&parent->d_inode->i_mutex);
---END---

OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 

[PATCH 5/5] libfs : file/directory removal fix, 2.6.18

[Mathieu Desnoyers]

Fix file and directory removal in libfs. Add inotify support for file removal.

The following scenario :
create dir a
create dir a/b

cd a/b (some process goes in cwd a/b)

rmdir a/b
rmdir a

fails due to the fact that "a" appears to be non empty. It is because the "b"
dentry is not deleted from "a" and still in use. The same problem happens if
"b" is a file. d_delete is nice enough to know when it needs to unhash and free
the dentry if nothing else is using it or, if someone is using it, to remove it
from the hash queues and wait for it to be deleted when it has no users.

The nice side-effect of this fix is that it calls the file removal
notification.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>


--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -276,6 +276,7 @@ int simple_unlink(struct inode *dir, str
 
 	inode->i_ctime = dir->i_ctime = dir->i_mtime = CURRENT_TIME;
 	inode->i_nlink--;
+	d_delete(dentry);
 	dput(dentry);
 	return 0;
 }

OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 

Re: [PATCH 5/5] libfs : file/directory removal fix, 2.6.18

[Al Viro]

On Thu, Nov 23, 2006 at 03:22:44AM -0500, Mathieu Desnoyers wrote:
> Fix file and directory removal in libfs. Add inotify support for file removal.
> 
> The following scenario :
> create dir a
> create dir a/b
> 
> cd a/b (some process goes in cwd a/b)
> 
> rmdir a/b
> rmdir a
>
> fails due to the fact that "a" appears to be non empty.

What?  Caller will do d_delete() itself.  Care to show a version where
that would happen and post an strace of the second rmdir?

> It is because the "b"
> dentry is not deleted from "a" and still in use. The same problem happens if
> "b" is a file. d_delete is nice enough to know when it needs to unhash and free
> the dentry if nothing else is using it or, if someone is using it, to remove it
> from the hash queues and wait for it to be deleted when it has no users.
> 
> The nice side-effect of this fix is that it calls the file removal
> notification.

NAK.  First of all, I won't believe you without actual strace.

What's more, WTF would fs _method_ call idiotify?  Keep that crap
out of filesystems; caller will do it for us just fine.

Re: [PATCH 5/5] libfs : file/directory removal fix, 2.6.18

[Al Viro]

On Thu, Nov 23, 2006 at 08:50:56AM +0000, Al Viro wrote:
> On Thu, Nov 23, 2006 at 03:22:44AM -0500, Mathieu Desnoyers wrote:
> > Fix file and directory removal in libfs. Add inotify support for file removal.
> > 
> > The following scenario :
> > create dir a
> > create dir a/b
> > 
> > cd a/b (some process goes in cwd a/b)
> > 
> > rmdir a/b
> > rmdir a
> >
> > fails due to the fact that "a" appears to be non empty.
> 
> What?  Caller will do d_delete() itself.  Care to show a version where
> that would happen and post an strace of the second rmdir?
> 
> > It is because the "b"
> > dentry is not deleted from "a" and still in use. The same problem happens if
> > "b" is a file. d_delete is nice enough to know when it needs to unhash and free
> > the dentry if nothing else is using it or, if someone is using it, to remove it
> > from the hash queues and wait for it to be deleted when it has no users.
> > 
> > The nice side-effect of this fix is that it calls the file removal
> > notification.
> 
> NAK.  First of all, I won't believe you without actual strace.
> 
> What's more, WTF would fs _method_ call idiotify?  Keep that crap
> out of filesystems; caller will do it for us just fine.

PS: debugfs, sysfs et sodding alia should take care to do things equivalent
to vfs_unlink(), etc.  _That_ is definitive user of fs methods, which, in
turn, sets the rules for library helpers used by such.

Re: [PATCH 5/5] libfs : file/directory removal fix, 2.6.18

[Mathieu Desnoyers]

* Al Viro (viro@ftp.linux.org.uk) wrote:
> On Thu, Nov 23, 2006 at 08:50:56AM +0000, Al Viro wrote:
> > On Thu, Nov 23, 2006 at 03:22:44AM -0500, Mathieu Desnoyers wrote:
> > > Fix file and directory removal in libfs. Add inotify support for file removal.
> > > 
> > > The following scenario :
> > > create dir a
> > > create dir a/b
> > > 
> > > cd a/b (some process goes in cwd a/b)
> > > 
> > > rmdir a/b
> > > rmdir a
> > >
> > > fails due to the fact that "a" appears to be non empty.
> > 
> > What?  Caller will do d_delete() itself.  Care to show a version where
> > that would happen and post an strace of the second rmdir?
> > 
> > > It is because the "b"
> > > dentry is not deleted from "a" and still in use. The same problem happens if
> > > "b" is a file. d_delete is nice enough to know when it needs to unhash and free
> > > the dentry if nothing else is using it or, if someone is using it, to remove it
> > > from the hash queues and wait for it to be deleted when it has no users.
> > > 
> > > The nice side-effect of this fix is that it calls the file removal
> > > notification.
> > 
> > NAK.  First of all, I won't believe you without actual strace.
> > 
> > What's more, WTF would fs _method_ call idiotify?  Keep that crap
> > out of filesystems; caller will do it for us just fine.
> 
> PS: debugfs, sysfs et sodding alia should take care to do things equivalent
> to vfs_unlink(), etc.  _That_ is definitive user of fs methods, which, in
> turn, sets the rules for library helpers used by such.
> 

Ok, I will tweak my modification so it sits in debugfs then.

Thanks,

Mathieu

OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 

Re: [PATCH 5/5] libfs : file/directory removal fix, 2.6.18

[Mathieu Desnoyers]

* Al Viro (viro@ftp.linux.org.uk) wrote:
> On Thu, Nov 23, 2006 at 03:22:44AM -0500, Mathieu Desnoyers wrote:
> > Fix file and directory removal in libfs. Add inotify support for file removal.
> > 
> > The following scenario :
> > create dir a
> > create dir a/b
> > 
> > cd a/b (some process goes in cwd a/b)
> > 
> > rmdir a/b
> > rmdir a
> >
> > fails due to the fact that "a" appears to be non empty.
> 
> What?  Caller will do d_delete() itself.  Care to show a version where
> that would happen and post an strace of the second rmdir?
> 

Hi,

Let me clarify :

The following scenario, in debugfs :
a_dentry = debugfs_create_dir("a", NULL);
b_dentry = debugfs_create_dir("b", a_dentry);


Then, in a shell :
cd /mnt/debugfs/a/b

And, from within the kernel :

debugfs_remove(b_dentry);
debugfs_remove(a_dentry);

If you need an example to see for yourself, here is a _quick and dirty_ one :

-- BEGIN ---
/* test-debugfs.c
 *
 */

#include <linux/marker.h>
#include <linux/module.h>
#include <linux/proc_fs.h>
#include <linux/sched.h>
#include <linux/debugfs.h>
#include <asm/ptrace.h>


static struct dentry *root_dentry;
static struct dentry *a_dentry;

static int my_open(struct inode *inode, struct file *file)
{
	struct dentry *local_dentry;
	printk("create\n");
	local_dentry = debugfs_create_dir("a", root_dentry);
	if (local_dentry == NULL)
		return -EEXIST;
	else
		a_dentry = local_dentry;

	return -EPERM;
}

static int my_open2(struct inode *inode, struct file *file)
{
	printk("remove\n");
	debugfs_remove(a_dentry);
	return -EPERM;
}


static struct file_operations my_operations_a = {
	.open = my_open,
};

static struct file_operations my_operations_b = {
	.open = my_open2,
};


int init_module(void)
{
	struct proc_dir_entry *pentry = NULL;

	root_dentry = debugfs_create_dir("test2", NULL);
	if (root_dentry == NULL)
		return -EEXIST;

	pentry = create_proc_entry("test-create", 0444, NULL);
	if (pentry)
		pentry->proc_fops = &my_operations_a;

	pentry = create_proc_entry("test-remove", 0444, NULL);
	if (pentry)
		pentry->proc_fops = &my_operations_b;

	return 0;
}

void cleanup_module(void)
{
	remove_proc_entry("test-create", NULL);
	remove_proc_entry("test-remove", NULL);
	debugfs_remove(root_dentry);
}

MODULE_LICENSE("GPL");
MODULE_AUTHOR("Mathieu Desnoyers");
MODULE_DESCRIPTION("Marker Test");


-- END ---

Sequence :
insmod test-debugfs.ko
cat /proc/test-create
cd /mnt/debugfs/test2/a
cat /proc/test-remove
cd /
ls /mnt/debugfs : you still see test2.



> > It is because the "b"
> > dentry is not deleted from "a" and still in use. The same problem happens if
> > "b" is a file. d_delete is nice enough to know when it needs to unhash and free
> > the dentry if nothing else is using it or, if someone is using it, to remove it
> > from the hash queues and wait for it to be deleted when it has no users.
> > 
> > The nice side-effect of this fix is that it calls the file removal
> > notification.
> 
> NAK.  First of all, I won't believe you without actual strace.
> 

I am not providing a "strace" because I am talking of simplefs_rmdir,
from libfs, which is meant to be used from within the kernel.

> What's more, WTF would fs _method_ call idiotify?  Keep that crap
> out of filesystems; caller will do it for us just fine.
> 

The 

struct inode_operations simple_dir_inode_operations = {
        .lookup         = simple_lookup,
};

seems to be populated only with the lookup, (not with rmdir, unlink, etc..).
Therefore, as the simple_rmdir and simple_unlink functions are exported with the
primary goal of being called directly (not through the VFS layer). If the goal
is to use them through the VFS, that's fine with me, but I guess their inode
operations should be exported differently, through the
simple_dir_inode_operations.

Mathieu


OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 

[PATCH 5/5] DebugFS : file/directory removal fix, 2.6.18

[Mathieu Desnoyers]

Fix file and directory removal in debugfs. Add inotify support for file removal.

The following scenario :
create dir a
create dir a/b

cd a/b (some process goes in cwd a/b)

rmdir a/b
rmdir a

fails due to the fact that "a" appears to be non empty. It is because the "b"
dentry is not deleted from "a" and still in use. The same problem happens if
"b" is a file. d_delete is nice enough to know when it needs to unhash and free
the dentry if nothing else is using it or, if someone is using it, to remove it
from the hash queues and wait for it to be deleted when it has no users.

The nice side-effect of this fix is that it calls the file removal
notification.

I have been told by Al Viro that this fix belongs to DebugFS, not libfs, so
here it is. Please use instead of "[PATCH 5/5] libfs : file/directory removal
fix, 2.6.18" posted earlier.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>


--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -288,6 +288,7 @@ void debugfs_remove(struct dentry *dentr
 	mutex_lock(&parent->d_inode->i_mutex);
 	if (debugfs_positive(dentry)) {
 		if (dentry->d_inode) {
+			dget(dentry);
 			if (S_ISDIR(dentry->d_inode->i_mode)) {
 				ret = simple_rmdir(parent->d_inode, dentry);
 				if (ret)
@@ -297,6 +298,9 @@ void debugfs_remove(struct dentry *dentr
 						dentry->d_name.name);
 			} else
 				simple_unlink(parent->d_inode, dentry);
+			if (!ret)
+				d_delete(dentry);
+			dput(dentry);
 		}
 	}
 	mutex_unlock(&parent->d_inode->i_mutex);



OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 

[PATCH 5/5] DebugFS : file/directory removal fix, 2.6.19-rc6

[Mathieu Desnoyers]

Fix file and directory removal in debugfs. Add inotify support for file removal.

The following scenario :
create dir a
create dir a/b

cd a/b (some process goes in cwd a/b)

rmdir a/b
rmdir a

fails due to the fact that "a" appears to be non empty. It is because the "b"
dentry is not deleted from "a" and still in use. The same problem happens if
"b" is a file. d_delete is nice enough to know when it needs to unhash and free
the dentry if nothing else is using it or, if someone is using it, to remove it
from the hash queues and wait for it to be deleted when it has no users.

The nice side-effect of this fix is that it calls the file removal
notification.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>

---BEGIN---
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -285,6 +285,7 @@ void debugfs_remove(struct dentry *dentr
 	mutex_lock(&parent->d_inode->i_mutex);
 	if (debugfs_positive(dentry)) {
 		if (dentry->d_inode) {
+			dget(dentry);
 			if (S_ISDIR(dentry->d_inode->i_mode)) {
 				ret = simple_rmdir(parent->d_inode, dentry);
 				if (ret)
@@ -294,6 +295,9 @@ void debugfs_remove(struct dentry *dentr
 						dentry->d_name.name);
 			} else
 				simple_unlink(parent->d_inode, dentry);
+			if (!ret)
+				d_delete(dentry);
+			dput(dentry);
 		}
 	}
 	mutex_unlock(&parent->d_inode->i_mutex);
---END---

OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68