[PATCH]: Fw: [Bugme-new] [Bug 1242] New: devfs oops with SMP kernel (not in UP mode)

[Andrey Borzenkov]

On Sunday 04 January 2004 19:32, Andrey Borzenkov wrote:
>
> The attached patch

really attached
--- linux-2.6.0/fs/devfs/base.c.devfs_revalidate	2003-12-18 05:58:16.000000000 +0300
+++ linux-2.6.0/fs/devfs/base.c	2004-01-04 14:04:22.000000000 +0300
@@ -2185,17 +2185,9 @@ static void devfs_d_iput (struct dentry 
 }   /*  End Function devfs_d_iput  */
 
 static int devfs_d_delete (struct dentry *dentry);
-
-static struct dentry_operations devfs_dops =
-{
-    .d_delete     = devfs_d_delete,
-    .d_release    = devfs_d_release,
-    .d_iput       = devfs_d_iput,
-};
-
 static int devfs_d_revalidate_wait (struct dentry *dentry, struct nameidata *);
 
-static struct dentry_operations devfs_wait_dops =
+static struct dentry_operations devfs_dops =
 {
     .d_delete     = devfs_d_delete,
     .d_release    = devfs_d_release,
@@ -2212,8 +2204,8 @@ static int devfs_d_delete (struct dentry
 {
     struct inode *inode = dentry->d_inode;
 
-    if (dentry->d_op == &devfs_wait_dops) dentry->d_op = &devfs_dops;
     /*  Unhash dentry if negative (has no inode)  */
+    /* FIXME should we check for d_fsdata? */
     if (inode == NULL)
     {
 	DPRINTK (DEBUG_D_DELETE, "(%p): dropping negative dentry\n", dentry);
@@ -2230,6 +2222,11 @@ struct devfs_lookup_struct
 
 /* XXX: this doesn't handle the case where we got a negative dentry
         but a devfs entry has been registered in the meanwhile */
+/*
+ * This relies on the fact that d_revalidate is called under parent i_sem
+ * providing mutual exclusion with devfs_lookup and protection for
+ * dentry->d_fsdata
+ */
 static int devfs_d_revalidate_wait (struct dentry *dentry, struct nameidata *nd)
 {
     struct inode *dir = dentry->d_parent->d_inode;
@@ -2238,6 +2235,10 @@ static int devfs_d_revalidate_wait (stru
     struct devfs_lookup_struct *lookup_info = dentry->d_fsdata;
     DECLARE_WAITQUEUE (wait, current);
 
+    /* Ordinary case - nothing to revalidate */
+    if (lookup_info == NULL) return 1;  /*  Early termination  */
+
+    /* Called from devfsd action - cannot block. */
     if ( is_devfsd_or_child (fs_info) )
     {
 	devfs_handle_t de = lookup_info->de;
@@ -2266,25 +2267,22 @@ static int devfs_d_revalidate_wait (stru
 	d_instantiate (dentry, inode);
 	return 1;
     }
-    if (lookup_info == NULL) return 1;  /*  Early termination  */
-    read_lock (&parent->u.dir.lock);
-    if (dentry->d_fsdata)
-    {
-	set_current_state (TASK_UNINTERRUPTIBLE);
-	add_wait_queue (&lookup_info->wait_queue, &wait);
-	read_unlock (&parent->u.dir.lock);
-	schedule ();
-	/*
-	 * This does not need nor should remove wait from wait_queue.
-	 * Wait queue head is never reused - nothing is ever added to it
-	 * after all waiters have been waked up and head itself disappears
-	 * very soon after it. Moreover it is local variable on stack that
-	 * is likely to have already disappeared so any reference to it
-	 * at this point is buggy.
-	 */
 
-    }
-    else read_unlock (&parent->u.dir.lock);
+    /* Not devfsd - must wait for devfsd to return */
+    set_current_state (TASK_UNINTERRUPTIBLE);
+    add_wait_queue (&lookup_info->wait_queue, &wait);
+    up(&dir->i_sem);
+    schedule ();
+    down(&dir->i_sem);
+    /*
+     * This does not need nor should remove wait from wait_queue.
+     * Wait queue head is never reused - nothing is ever added to it
+     * after all waiters have been waked up and head itself disappears
+     * very soon after it. Moreover it is local variable on stack that
+     * is likely to have already disappeared so any reference to it
+     * at this point is buggy.
+     */
+
     return 1;
 }   /*  End Function devfs_d_revalidate_wait  */
 
@@ -2323,17 +2321,18 @@ static struct dentry *devfs_lookup (stru
 	if (try_modload (parent, fs_info,
 			 dentry->d_name.name, dentry->d_name.len, &tmp) < 0)
 	{   /*  Lookup event was not queued to devfsd  */
+	    dentry->d_fsdata = NULL;
 	    d_add (dentry, NULL);
 	    return NULL;
 	}
     }
-    dentry->d_op = &devfs_wait_dops;
     d_add (dentry, NULL);  /*  Open the floodgates  */
     /*  Unlock directory semaphore, which will release any waiters. They
 	will get the hashed dentry, and may be forced to wait for
 	revalidation  */
     up (&dir->i_sem);
     wait_for_devfsd_finished (fs_info);  /*  If I'm not devfsd, must wait  */
+    down (&dir->i_sem);      /*  Grab it again because them's the rules  */
     de = lookup_info.de;
     /*  If someone else has been so kind as to make the inode, we go home
 	early  */
@@ -2358,12 +2357,8 @@ static struct dentry *devfs_lookup (stru
 	     de->name, de->inode.ino, inode, de, current->comm);
     d_instantiate (dentry, inode);
 out:
-    write_lock (&parent->u.dir.lock);
-    dentry->d_op = &devfs_dops;
     dentry->d_fsdata = NULL;
     wake_up (&lookup_info.wait_queue);
-    write_unlock (&parent->u.dir.lock);
-    down (&dir->i_sem);      /*  Grab it again because them's the rules  */
     devfs_put (de);
     return retval;
 }   /*  End Function devfs_lookup  */
@@ -2606,6 +2601,7 @@ static struct file_system_type devfs_fs_
     .name	= DEVFS_NAME,
     .get_sb	= devfs_get_sb,
     .kill_sb	= kill_anon_super,
+    .fs_flags	= FS_ODD_REVALIDATE,
 };
 
 /*  File operations for devfsd follow  */
--- linux-2.6.0/fs/namei.c.devfs_revalidate	2003-12-18 05:58:40.000000000 +0300
+++ linux-2.6.0/fs/namei.c	2004-01-04 14:06:15.000000000 +0300
@@ -274,6 +274,9 @@ void path_release(struct nameidata *nd)
  * Internal lookup() using the new generic dcache.
  * SMP-safe
  */
+/*
+ * This seems to always be called under parent->i_sem locked
+ */
 static struct dentry * cached_lookup(struct dentry * parent, struct qstr * name, struct nameidata *nd)
 {
 	struct dentry * dentry = __d_lookup(parent, name);
@@ -342,6 +345,7 @@ static struct dentry * real_lookup(struc
 {
 	struct dentry * result;
 	struct inode *dir = parent->d_inode;
+	int needlock = dir->i_sb->s_type->fs_flags & FS_ODD_REVALIDATE;
 
 	down(&dir->i_sem);
 	/*
@@ -377,13 +381,16 @@ static struct dentry * real_lookup(struc
 	 * Uhhuh! Nasty case: the cache was re-populated while
 	 * we waited on the semaphore. Need to revalidate.
 	 */
-	up(&dir->i_sem);
+	if (!needlock)
+		up(&dir->i_sem);
 	if (result->d_op && result->d_op->d_revalidate) {
 		if (!result->d_op->d_revalidate(result, nd) && !d_invalidate(result)) {
 			dput(result);
 			result = ERR_PTR(-ENOENT);
 		}
 	}
+	if (needlock)
+		up(&dir->i_sem);
 	return result;
 }
 
@@ -528,11 +535,17 @@ static int do_lookup(struct nameidata *n
 {
 	struct vfsmount *mnt = nd->mnt;
 	struct dentry *dentry = __d_lookup(nd->dentry, name);
+	int needlock = mnt->mnt_sb->s_type->fs_flags & FS_ODD_REVALIDATE;
 
 	if (!dentry)
 		goto need_lookup;
+	if (needlock)
+		down(&nd->dentry->d_inode->i_sem);
 	if (dentry->d_op && dentry->d_op->d_revalidate)
 		goto need_revalidate;
+unlock:
+	if (needlock)
+		up(&nd->dentry->d_inode->i_sem);
 done:
 	path->mnt = mnt;
 	path->dentry = dentry;
@@ -546,9 +559,11 @@ need_lookup:
 
 need_revalidate:
 	if (dentry->d_op->d_revalidate(dentry, nd))
-		goto done;
+		goto unlock;
 	if (d_invalidate(dentry))
-		goto done;
+		goto unlock;
+	if (needlock)
+		up(&nd->dentry->d_inode->i_sem);
 	dput(dentry);
 	goto need_lookup;
 
--- linux-2.6.0/include/linux/fs.h.devfs_revalidate	2003-12-28 15:57:45.000000000 +0300
+++ linux-2.6.0/include/linux/fs.h	2004-01-04 18:47:11.000000000 +0300
@@ -94,6 +94,7 @@ extern int leases_enable, dir_notify_ena
 #define FS_ODD_RENAME	32768	/* Temporary stuff; will go away as soon
 				  * as nfs_rename() will be cleaned up
 				  */
+#define FS_ODD_REVALIDATE	(1<<16) /* d_revalidate needs lock on i_sem */
 /*
  * These are the fs-independent mount-flags: up to 32 flags are supported
  */