* [PATCH]: Fw: [Bugme-new] [Bug 1242] New: devfs oops with SMP kernel (not in UP mode)
2004-01-04 16:32 ` [PATCH]: Fw: [Bugme-new] [Bug 1242] New: devfs oops with SMP kernel (not in UP mode) Andrey Borzenkov
@ 2004-01-04 16:49 ` Andrey Borzenkov
2004-01-04 21:48 ` Andrew Morton
2004-01-13 8:33 ` Pavel Roskin
2 siblings, 0 replies; 4+ messages in thread
From: Andrey Borzenkov @ 2004-01-04 16:49 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, Pavel Roskin, kpfleming
[-- Attachment #1: Type: text/plain, Size: 97 bytes --]
On Sunday 04 January 2004 19:32, Andrey Borzenkov wrote:
>
> The attached patch
really attached
[-- Attachment #2: 2.6.0-devfs-d_revalidate.patch --]
[-- Type: text/x-diff, Size: 7635 bytes --]
--- 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
*/
^ permalink raw reply [flat|nested] 4+ messages in thread