[PATCH][2.6.3-mm2] unregister path on devfs_remove

[PATCH][2.6.3-mm2] unregister path on devfs_remove

[Andrey Borzenkov]

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

The patch implements cleanup of device path on devfs_remove. It does not
use extra counters as was suggested once - rather it simply removes all
empty directories. Manually created directories (and connecting paths)
are preserved and cleaned up on rmdir/unlink.

The creation/removing of path is not atomic; it will be addressed later.
It apparently requires replacing per-directory locks with global one;
using extra counters does not actually helps here.

This patch will cause devfs_remove on directories emit warning in most
cases. Removing connecting directories makes no sense with this change
so if it is agreed upon I send patch that removes all
devfs_remove("directory") from kernel. It was never done consistently
anyway. I would actually make
devfs_mk_dir noop except for two places - /dev/pts and /dev/shm.

regards

-andrey

[-- Attachment #2: 2.6.3-mm2-devfs_path_removal-1.patch --]
[-- Type: text/plain, Size: 8502 bytes --]

--- ../tmp/linux-2.6.3-mm2/fs/devfs/base.c	2004-02-22 20:25:14.000000000 +0300
+++ linux-2.6.3-mm2/fs/devfs/base.c	2004-02-22 23:44:24.000000000 +0300
@@ -783,7 +783,6 @@ struct devfs_entry
 #ifdef CONFIG_DEVFS_DEBUG
     unsigned int magic_number;
 #endif
-    void *info;
     atomic_t refcount;           /*  When this drops to zero, it's unused    */
     union 
     {
@@ -799,7 +798,7 @@ struct devfs_entry
     struct devfs_inode inode;
     umode_t mode;
     unsigned short namelen;      /*  I think 64k+ filenames are a way off... */
-    unsigned char vfs:1;/*  Whether the VFS may delete the entry   */
+    unsigned char vfs:1;	 /*  Entry was created by VFS                */
     char name[1];                /*  This is just a dummy: the allocated array
 				     is bigger. This is NULL-terminated      */
 };
@@ -904,12 +903,13 @@ static void devfs_put (devfs_handle_t de
 {
     if (!de) return;
     VERIFY_ENTRY (de);
-    if (de->info == POISON_PTR) OOPS ("(%p): poisoned pointer\n", de);
+    if (de->parent == POISON_PTR) OOPS ("(%p): poisoned pointer\n", de);
     if ( !atomic_dec_and_test (&de->refcount) ) return;
     if (de == root_entry) OOPS ("(%p): root entry being freed\n", de);
     DPRINTK (DEBUG_FREE, "(%s): de: %p, parent: %p \"%s\"\n",
 	     de->name, de, de->parent,
 	     de->parent ? de->parent->name : "no parent");
+    devfs_put(de->parent);
     if ( S_ISLNK (de->mode) ) kfree (de->u.symlink.linkname);
     WRITE_ENTRY_MAGIC (de, 0);
 #ifdef CONFIG_DEVFS_DEBUG
@@ -919,7 +919,7 @@ static void devfs_put (devfs_handle_t de
     if ( S_ISLNK (de->mode) ) stat_num_bytes -= de->u.symlink.length + 1;
     spin_unlock (&stat_lock);
 #endif
-    de->info = POISON_PTR;
+    de->parent = POISON_PTR;
     kfree (de);
 }   /*  End Function devfs_put  */
 
@@ -1032,7 +1032,7 @@ static int _devfs_append_entry (devfs_ha
 	else devfs_put (old);
 	if (old == NULL)
 	{
-	    de->parent = dir;
+	    de->parent = devfs_get(dir);
 	    de->prev = dir->u.dir.last;
 	    /*  Append to the directory's list of children  */
 	    if (dir->u.dir.first == NULL) dir->u.dir.first = de;
@@ -1368,8 +1368,8 @@ static int wait_for_devfsd_finished (str
 
 /**
  *	devfsd_notify_de - Notify the devfsd daemon of a change.
- *	@de: The devfs entry that has changed. This and all parent entries will
- *            have their reference counts incremented if the event was queued.
+ *	@de: The devfs entry that has changed. This will have reference
+ *		count incremented if the event was queued.
  *	@type: The type of change.
  *	@mode: The mode of the entry.
  *	@uid: The user ID.
@@ -1384,7 +1384,6 @@ static int devfsd_notify_de (struct devf
 			     uid_t uid, gid_t gid, struct fs_info *fs_info)
 {
     struct devfsd_buf_entry *entry;
-    struct devfs_entry *curr;
 
     if ( !( fs_info->devfsd_event_mask & (1 << type) ) ) return (FALSE);
     if ( ( entry = kmem_cache_alloc (devfsd_buf_cache, SLAB_KERNEL) ) == NULL )
@@ -1392,8 +1391,7 @@ static int devfsd_notify_de (struct devf
 	atomic_inc (&fs_info->devfsd_overrun_count);
 	return (FALSE);
     }
-    for (curr = de; curr != NULL; curr = curr->parent) devfs_get (curr);
-    entry->de = de;
+    entry->de = devfs_get(de);
     entry->type = type;
     entry->mode = mode;
     entry->uid = uid;
@@ -1518,39 +1516,74 @@ static int _devfs_unhook (struct devfs_e
 
 
 /**
- *	_devfs_unregister - Unregister a device entry from its parent.
+ *	_devfs_unregister - Unregister a device path starting from bottom
  *	@dir: The parent directory.
  *	@de: The entry to unregister.
  *
  *	The caller must have a write lock on the parent directory, which is
  *	unlocked by this function.
+ *
+ *	Caller must devfs_get @de to ensure the path won't disappear
+ *	while we traverse it from bottom up
+ *
+ *	If @dir becomes empty we unregister it too recursively unless
+ *	it has been created by user
+ *
+ *	Returns true if entry was really unhooked, false otherwise.
  */
 
-static void _devfs_unregister (struct devfs_entry *dir, struct devfs_entry *de)
+static int _devfs_unregister (struct devfs_entry *dir, struct devfs_entry *de)
 {
-    int unhooked = _devfs_unhook (de);
+	int unhooked;
+	struct devfs_entry *leaf = de;
+	int followup;
 
-    write_unlock (&dir->u.dir.lock);
-    if (!unhooked) return;
-    devfs_get (dir);
-    devfsd_notify (de, DEVFSD_NOTIFY_UNREGISTERED);
-    free_dentry (de);
-    devfs_put (dir);
-    if ( !S_ISDIR (de->mode) ) return;
+repeat:
+	followup = 0;
+	unhooked = _devfs_unhook(de);
+	if (dir->u.dir.first == NULL && dir->u.dir.last == NULL &&
+	    !dir->vfs && dir != root_entry && unhooked) {
+		dir->u.dir.no_more_additions = TRUE;
+		followup = 1;
+	}
+	write_unlock(&dir->u.dir.lock);
+
+	if (!unhooked) {
+		if (de != leaf)
+			goto kill_subtree;
+		else
+			return 0;
+	}
+
+	free_dentry(de);
+	devfs_put(de);
+
+	/* remove parent if it became empty */
+	if (followup) {
+		de = dir;
+		dir = de->parent;
+		write_lock(&dir->u.dir.lock);
+		goto repeat;
+	}
+
+kill_subtree:
+    if ( !S_ISDIR (leaf->mode) ) return 1;
     while (TRUE)  /*  Recursively unregister: this is a stack chomper  */
     {
 	struct devfs_entry *child;
 
-	write_lock (&de->u.dir.lock);
-	de->u.dir.no_more_additions = TRUE;
-	child = de->u.dir.first;
+	write_lock (&leaf->u.dir.lock);
+	leaf->u.dir.no_more_additions = TRUE;
+	child = devfs_get(leaf->u.dir.first);
 	VERIFY_ENTRY (child);
-	_devfs_unregister (de, child);
+	_devfs_unregister (leaf, child);
 	if (!child) break;
 	DPRINTK (DEBUG_UNREGISTER, "(%s): child: %p  refcount: %d\n",
 		 child->name, child, atomic_read (&child->refcount) );
 	devfs_put (child);
     }
+
+    return 1;
 }   /*  End Function _devfs_unregister  */
 
 static int devfs_do_symlink (devfs_handle_t dir, const char *name,
@@ -1584,7 +1617,6 @@ static int devfs_do_symlink (devfs_handl
 	kfree (newlink);
 	return -ENOTDIR;
     }
-    de->info = NULL;
     de->u.symlink.linkname = newlink;
     de->u.symlink.length = linklength;
     if ( ( err = _devfs_append_entry (dir, de, NULL) ) != 0 )
@@ -1699,9 +1731,12 @@ void devfs_remove(const char *fmt, ...)
 			return;
 		}
 
-		write_lock(&de->parent->u.dir.lock);
-		_devfs_unregister(de->parent, de);
-		devfs_put(de);
+		/* no way to return an error; should be EPERM */
+		if (!de->vfs) {
+			write_lock(&de->parent->u.dir.lock);
+			if (_devfs_unregister(de->parent, de))
+				devfsd_notify(de, DEVFSD_NOTIFY_UNREGISTERED);
+		}
 		devfs_put(de);
 	}
 }
@@ -2330,7 +2365,6 @@ out:
 
 static int devfs_unlink (struct inode *dir, struct dentry *dentry)
 {
-    int unhooked;
     struct devfs_entry *de;
     struct inode *inode = dentry->d_inode;
     struct fs_info *fs_info = dir->i_sb->s_fs_info;
@@ -2340,14 +2374,11 @@ static int devfs_unlink (struct inode *d
     if (de == NULL) return -ENOENT;
     if (!de->vfs) return -EPERM;
     write_lock (&de->parent->u.dir.lock);
-    unhooked = _devfs_unhook (de);
-    write_unlock (&de->parent->u.dir.lock);
-    if (!unhooked) return -ENOENT;
+    if (!_devfs_unregister(de->parent, de))
+	return -ENOENT;
     if ( !is_devfsd_or_child (fs_info) )
 	devfsd_notify_de (de, DEVFSD_NOTIFY_DELETE, inode->i_mode,
 			  inode->i_uid, inode->i_gid, fs_info);
-    free_dentry (de);
-    devfs_put (de);
     return 0;
 }   /*  End Function devfs_unlink  */
 
@@ -2434,14 +2465,11 @@ static int devfs_rmdir (struct inode *di
     if (err) return err;
     /*  Now unhook the directory from its parent  */
     write_lock (&de->parent->u.dir.lock);
-    if ( !_devfs_unhook (de) ) err = -ENOENT;
-    write_unlock (&de->parent->u.dir.lock);
-    if (err) return err;
+    if (!_devfs_unregister(de->parent, de))
+	return -ENOENT;
     if ( !is_devfsd_or_child (fs_info) )
 	devfsd_notify_de (de, DEVFSD_NOTIFY_DELETE, inode->i_mode,
 			  inode->i_uid, inode->i_gid, fs_info);
-    free_dentry (de);
-    devfs_put (de);
     return 0;
 }   /*  End Function devfs_rmdir  */
 
@@ -2653,17 +2681,11 @@ static ssize_t devfsd_read (struct file 
     tlen = rpos - *ppos;
     if (done)
     {
-	devfs_handle_t parent;
-
 	spin_lock (&fs_info->devfsd_buffer_lock);
 	fs_info->devfsd_first_event = entry->next;
 	if (entry->next == NULL) fs_info->devfsd_last_event = NULL;
 	spin_unlock (&fs_info->devfsd_buffer_lock);
-	for (; de != NULL; de = parent)
-	{
-	    parent = de->parent;
-	    devfs_put (de);
-	}
+	devfs_put (de);
 	kmem_cache_free (devfsd_buf_cache, entry);
 	if (ival > 0) atomic_sub (ival, &fs_info->devfsd_overrun_count);
 	*ppos = 0;

Re: [PATCH][2.6.3-mm2] unregister path on devfs_remove

[Andrew Morton]

Andrey Borzenkov <arvidjaar@mail.ru> wrote:
>
> The patch implements cleanup of device path on devfs_remove. It does not
> use extra counters as was suggested once - rather it simply removes all
> empty directories. Manually created directories (and connecting paths)
> are preserved and cleaned up on rmdir/unlink.
> 
> The creation/removing of path is not atomic; it will be addressed later.
> It apparently requires replacing per-directory locks with global one;
> using extra counters does not actually helps here.
> 
> This patch will cause devfs_remove on directories emit warning in most
> cases. Removing connecting directories makes no sense with this change
> so if it is agreed upon I send patch that removes all
> devfs_remove("directory") from kernel.

Well could you please either send that patch or remove the warning?  I get
way too much mail when -mm kernels start spitting new warnings.