Re: 2.6.36: Sound stop working
From: Linus Torvalds
Date: Thu Aug 12 2010 - 17:42:27 EST
On Thu, Aug 12, 2010 at 2:17 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> At Thu, 12 Aug 2010 23:01:04 +0200,
> Jiri Slaby wrote:
>>
>> Probably I got into this problem yesterday. Found out that PA fails to
>> open /dev/snd/pcmC0D0p _second_ time. It opens it, then closes, then
>> opens it again and gets EBUSY. aplay is OK.
>
> Yes, I can also confirm that it's broken on my machine in the same
> way now :) PA log shows that the succeeding open failed.
>
> PA tries to do quick open/close of the same device to figure out which
> configuration is available at start-up. This implies that the
> fs/notify commits touching the open/close stuff can be the culprit.
Yeah. The f_count stuff is disgusting. This revert patch makes things
work for me again. And I suspect it's the right thing to do
regardless. I reacted to that ugly __fput() hackery when I pulled the
fanotify tree, but I let it slide. I clearly should have let my taste
guide me more.
fsnotify playing games with fput/fget is almost certainly totally wrong.
Al, what was the problem that caused you to think that we want to have
a 'struct file' here? Is it just the fact that some of those fsnotify
things use that 'path' structure that is embedded in the parent? But
isn't the simplest fix for that to just _copy_ the "struct path"
rather than pass the "struct file" around?
Linus
commit 2069601b3f0ea38170d4b509b89f3ca0a373bdc1
Author: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Date: Thu Aug 12 14:23:04 2010 -0700
Revert "fsnotify: store struct file not struct path"
This reverts commit 3bcf3860a4ff9bbc522820b4b765e65e4deceb3e (and the
accompanying commit c1e5c954020e "vfs/fsnotify: fsnotify_close can delay
the final work in fput" that was a horribly ugly hack to make it work at
all).
The 'struct file' approach not only causes that disgusting hack, it
somehow breaks pulseaudio, probably due to some other subtlety with
f_count handling.
Fix up various conflicts due to later fsnotify work.
Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
---
fs/file_table.c | 9 --------
fs/notify/fanotify/fanotify.c | 8 +++---
fs/notify/fanotify/fanotify_user.c | 6 ++--
fs/notify/fsnotify.c | 12 +++++-----
fs/notify/inotify/inotify_fsnotify.c | 12 +++++-----
fs/notify/notification.c | 33 ++++++++++--------------------
include/linux/fsnotify.h | 37 +++++++++++++++++++--------------
include/linux/fsnotify_backend.h | 16 +++++++-------
kernel/audit_watch.c | 4 +-
9 files changed, 61 insertions(+), 76 deletions(-)
diff --git a/fs/file_table.c b/fs/file_table.c
index 2fc3b3c..edecd36 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -230,15 +230,6 @@ static void __fput(struct file *file)
might_sleep();
fsnotify_close(file);
-
- /*
- * fsnotify_create_event may have taken one or more references on this
- * file. If it did so it left one reference for us to drop to make sure
- * its calls to fput could not prematurely destroy the file.
- */
- if (atomic_long_read(&file->f_count))
- return fput(file);
-
/*
* The function eventpoll_release() should be the first called
* in the file cleanup chain.
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index eb8f73c..756566f 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -17,9 +17,9 @@ static bool should_merge(struct fsnotify_event *old, struct fsnotify_event *new)
old->data_type == new->data_type &&
old->tgid == new->tgid) {
switch (old->data_type) {
- case (FSNOTIFY_EVENT_FILE):
- if ((old->file->f_path.mnt == new->file->f_path.mnt) &&
- (old->file->f_path.dentry == new->file->f_path.dentry))
+ case (FSNOTIFY_EVENT_PATH):
+ if ((old->path.mnt == new->path.mnt) &&
+ (old->path.dentry == new->path.dentry))
return true;
case (FSNOTIFY_EVENT_NONE):
return true;
@@ -174,7 +174,7 @@ static bool fanotify_should_send_event(struct fsnotify_group *group,
return false;
/* if we don't have enough info to send an event to userspace say no */
- if (data_type != FSNOTIFY_EVENT_FILE)
+ if (data_type != FSNOTIFY_EVENT_PATH)
return false;
if (inode_mark && vfsmnt_mark) {
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 25a3b4d..032b837 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -65,7 +65,7 @@ static int create_fd(struct fsnotify_group *group, struct fsnotify_event *event)
if (client_fd < 0)
return client_fd;
- if (event->data_type != FSNOTIFY_EVENT_FILE) {
+ if (event->data_type != FSNOTIFY_EVENT_PATH) {
WARN_ON(1);
put_unused_fd(client_fd);
return -EINVAL;
@@ -75,8 +75,8 @@ static int create_fd(struct fsnotify_group *group, struct fsnotify_event *event)
* we need a new file handle for the userspace program so it can read even if it was
* originally opened O_WRONLY.
*/
- dentry = dget(event->file->f_path.dentry);
- mnt = mntget(event->file->f_path.mnt);
+ dentry = dget(event->path.dentry);
+ mnt = mntget(event->path.mnt);
/* it's possible this event was an overflow event. in that case dentry and mnt
* are NULL; That's fine, just don't call dentry open */
if (dentry && mnt)
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 4d2a82c..3970392 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -84,7 +84,7 @@ void __fsnotify_update_child_dentry_flags(struct inode *inode)
}
/* Notify this dentry's parent about a child's events. */
-void __fsnotify_parent(struct file *file, struct dentry *dentry, __u32 mask)
+void __fsnotify_parent(struct path *path, struct dentry *dentry, __u32 mask)
{
struct dentry *parent;
struct inode *p_inode;
@@ -92,7 +92,7 @@ void __fsnotify_parent(struct file *file, struct dentry *dentry, __u32 mask)
bool should_update_children = false;
if (!dentry)
- dentry = file->f_path.dentry;
+ dentry = path->dentry;
if (!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED))
return;
@@ -124,8 +124,8 @@ void __fsnotify_parent(struct file *file, struct dentry *dentry, __u32 mask)
* specifies these are events which came from a child. */
mask |= FS_EVENT_ON_CHILD;
- if (file)
- fsnotify(p_inode, mask, file, FSNOTIFY_EVENT_FILE,
+ if (path)
+ fsnotify(p_inode, mask, path, FSNOTIFY_EVENT_PATH,
dentry->d_name.name, 0);
else
fsnotify(p_inode, mask, dentry->d_inode, FSNOTIFY_EVENT_INODE,
@@ -217,8 +217,8 @@ int fsnotify(struct inode *to_tell, __u32 mask, void *data, int data_is,
/* global tests shouldn't care about events on child only the specific event */
__u32 test_mask = (mask & ~FS_EVENT_ON_CHILD);
- if (data_is == FSNOTIFY_EVENT_FILE)
- mnt = ((struct file *)data)->f_path.mnt;
+ if (data_is == FSNOTIFY_EVENT_PATH)
+ mnt = ((struct path *)data)->mnt;
else
mnt = NULL;
diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
index 5e73eeb..a91b69a 100644
--- a/fs/notify/inotify/inotify_fsnotify.c
+++ b/fs/notify/inotify/inotify_fsnotify.c
@@ -52,9 +52,9 @@ static bool event_compare(struct fsnotify_event *old, struct fsnotify_event *new
!strcmp(old->file_name, new->file_name))
return true;
break;
- case (FSNOTIFY_EVENT_FILE):
- if ((old->file->f_path.mnt == new->file->f_path.mnt) &&
- (old->file->f_path.dentry == new->file->f_path.dentry))
+ case (FSNOTIFY_EVENT_PATH):
+ if ((old->path.mnt == new->path.mnt) &&
+ (old->path.dentry == new->path.dentry))
return true;
break;
case (FSNOTIFY_EVENT_NONE):
@@ -147,10 +147,10 @@ static bool inotify_should_send_event(struct fsnotify_group *group, struct inode
__u32 mask, void *data, int data_type)
{
if ((inode_mark->mask & FS_EXCL_UNLINK) &&
- (data_type == FSNOTIFY_EVENT_FILE)) {
- struct file *file = data;
+ (data_type == FSNOTIFY_EVENT_PATH)) {
+ struct path *path = data;
- if (d_unlinked(file->f_path.dentry))
+ if (d_unlinked(path->dentry))
return false;
}
diff --git a/fs/notify/notification.c b/fs/notify/notification.c
index d6c435a..f39260f 100644
--- a/fs/notify/notification.c
+++ b/fs/notify/notification.c
@@ -31,7 +31,6 @@
* allocated and used.
*/
-#include <linux/file.h>
#include <linux/fs.h>
#include <linux/init.h>
#include <linux/kernel.h>
@@ -90,8 +89,8 @@ void fsnotify_put_event(struct fsnotify_event *event)
if (atomic_dec_and_test(&event->refcnt)) {
pr_debug("%s: event=%p\n", __func__, event);
- if (event->data_type == FSNOTIFY_EVENT_FILE)
- fput(event->file);
+ if (event->data_type == FSNOTIFY_EVENT_PATH)
+ path_put(&event->path);
BUG_ON(!list_empty(&event->private_data_list));
@@ -376,8 +375,8 @@ struct fsnotify_event *fsnotify_clone_event(struct fsnotify_event *old_event)
}
}
event->tgid = get_pid(old_event->tgid);
- if (event->data_type == FSNOTIFY_EVENT_FILE)
- get_file(event->file);
+ if (event->data_type == FSNOTIFY_EVENT_PATH)
+ path_get(&event->path);
return event;
}
@@ -424,22 +423,11 @@ struct fsnotify_event *fsnotify_create_event(struct inode *to_tell, __u32 mask,
event->data_type = data_type;
switch (data_type) {
- case FSNOTIFY_EVENT_FILE: {
- event->file = data;
- /*
- * if this file is about to disappear hold an extra reference
- * until we return to __fput so we don't have to worry about
- * future get/put destroying the file under us or generating
- * additional events. Notice that we change f_mode without
- * holding f_lock. This is safe since this is the only possible
- * reference to this object in the kernel (it was about to be
- * freed, remember?)
- */
- if (!atomic_long_read(&event->file->f_count)) {
- event->file->f_mode |= FMODE_NONOTIFY;
- get_file(event->file);
- }
- get_file(event->file);
+ case FSNOTIFY_EVENT_PATH: {
+ struct path *path = data;
+ event->path.dentry = path->dentry;
+ event->path.mnt = path->mnt;
+ path_get(&event->path);
break;
}
case FSNOTIFY_EVENT_INODE:
@@ -447,7 +435,8 @@ struct fsnotify_event *fsnotify_create_event(struct inode *to_tell, __u32 mask,
break;
case FSNOTIFY_EVENT_NONE:
event->inode = NULL;
- event->file = NULL;
+ event->path.dentry = NULL;
+ event->path.mnt = NULL;
break;
default:
BUG();
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index e4e2204..59d0df4 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -26,18 +26,19 @@ static inline void fsnotify_d_instantiate(struct dentry *dentry,
}
/* Notify this dentry's parent about a child's events. */
-static inline void fsnotify_parent(struct file *file, struct dentry *dentry, __u32 mask)
+static inline void fsnotify_parent(struct path *path, struct dentry *dentry, __u32 mask)
{
if (!dentry)
- dentry = file->f_path.dentry;
+ dentry = path->dentry;
- __fsnotify_parent(file, dentry, mask);
+ __fsnotify_parent(path, dentry, mask);
}
/* simple call site for access decisions */
static inline int fsnotify_perm(struct file *file, int mask)
{
- struct inode *inode = file->f_path.dentry->d_inode;
+ struct path *path = &file->f_path;
+ struct inode *inode = path->dentry->d_inode;
__u32 fsnotify_mask = 0;
if (file->f_mode & FMODE_NONOTIFY)
@@ -51,7 +52,7 @@ static inline int fsnotify_perm(struct file *file, int mask)
else
BUG();
- return fsnotify(inode, fsnotify_mask, file, FSNOTIFY_EVENT_FILE, NULL, 0);
+ return fsnotify(inode, fsnotify_mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
}
/*
@@ -186,15 +187,16 @@ static inline void fsnotify_mkdir(struct inode *inode, struct dentry *dentry)
*/
static inline void fsnotify_access(struct file *file)
{
- struct inode *inode = file->f_path.dentry->d_inode;
+ struct path *path = &file->f_path;
+ struct inode *inode = path->dentry->d_inode;
__u32 mask = FS_ACCESS;
if (S_ISDIR(inode->i_mode))
mask |= FS_IN_ISDIR;
if (!(file->f_mode & FMODE_NONOTIFY)) {
- fsnotify_parent(file, NULL, mask);
- fsnotify(inode, mask, file, FSNOTIFY_EVENT_FILE, NULL, 0);
+ fsnotify_parent(path, NULL, mask);
+ fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
}
}
@@ -203,15 +205,16 @@ static inline void fsnotify_access(struct file *file)
*/
static inline void fsnotify_modify(struct file *file)
{
- struct inode *inode = file->f_path.dentry->d_inode;
+ struct path *path = &file->f_path;
+ struct inode *inode = path->dentry->d_inode;
__u32 mask = FS_MODIFY;
if (S_ISDIR(inode->i_mode))
mask |= FS_IN_ISDIR;
if (!(file->f_mode & FMODE_NONOTIFY)) {
- fsnotify_parent(file, NULL, mask);
- fsnotify(inode, mask, file, FSNOTIFY_EVENT_FILE, NULL, 0);
+ fsnotify_parent(path, NULL, mask);
+ fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
}
}
@@ -220,15 +223,16 @@ static inline void fsnotify_modify(struct file *file)
*/
static inline void fsnotify_open(struct file *file)
{
- struct inode *inode = file->f_path.dentry->d_inode;
+ struct path *path = &file->f_path;
+ struct inode *inode = path->dentry->d_inode;
__u32 mask = FS_OPEN;
if (S_ISDIR(inode->i_mode))
mask |= FS_IN_ISDIR;
if (!(file->f_mode & FMODE_NONOTIFY)) {
- fsnotify_parent(file, NULL, mask);
- fsnotify(inode, mask, file, FSNOTIFY_EVENT_FILE, NULL, 0);
+ fsnotify_parent(path, NULL, mask);
+ fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
}
}
@@ -237,6 +241,7 @@ static inline void fsnotify_open(struct file *file)
*/
static inline void fsnotify_close(struct file *file)
{
+ struct path *path = &file->f_path;
struct inode *inode = file->f_path.dentry->d_inode;
fmode_t mode = file->f_mode;
__u32 mask = (mode & FMODE_WRITE) ? FS_CLOSE_WRITE : FS_CLOSE_NOWRITE;
@@ -245,8 +250,8 @@ static inline void fsnotify_close(struct file *file)
mask |= FS_IN_ISDIR;
if (!(file->f_mode & FMODE_NONOTIFY)) {
- fsnotify_parent(file, NULL, mask);
- fsnotify(inode, mask, file, FSNOTIFY_EVENT_FILE, NULL, 0);
+ fsnotify_parent(path, NULL, mask);
+ fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
}
}
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 9bbfd72..ed36fb5 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -203,20 +203,20 @@ struct fsnotify_event {
/* to_tell may ONLY be dereferenced during handle_event(). */
struct inode *to_tell; /* either the inode the event happened to or its parent */
/*
- * depending on the event type we should have either a file or inode
- * We hold a reference on file, but NOT on inode. Since we have the ref on
- * the file, it may be dereferenced at any point during this object's
+ * depending on the event type we should have either a path or inode
+ * We hold a reference on path, but NOT on inode. Since we have the ref on
+ * the path, it may be dereferenced at any point during this object's
* lifetime. That reference is dropped when this object's refcnt hits
- * 0. If this event contains an inode instead of a file, the inode may
+ * 0. If this event contains an inode instead of a path, the inode may
* ONLY be used during handle_event().
*/
union {
- struct file *file;
+ struct path path;
struct inode *inode;
};
/* when calling fsnotify tell it if the data is a path or inode */
#define FSNOTIFY_EVENT_NONE 0
-#define FSNOTIFY_EVENT_FILE 1
+#define FSNOTIFY_EVENT_PATH 1
#define FSNOTIFY_EVENT_INODE 2
int data_type; /* which of the above union we have */
atomic_t refcnt; /* how many groups still are using/need to send this event */
@@ -293,7 +293,7 @@ struct fsnotify_mark {
/* main fsnotify call to send events */
extern int fsnotify(struct inode *to_tell, __u32 mask, void *data, int data_is,
const unsigned char *name, u32 cookie);
-extern void __fsnotify_parent(struct file *file, struct dentry *dentry, __u32 mask);
+extern void __fsnotify_parent(struct path *path, struct dentry *dentry, __u32 mask);
extern void __fsnotify_inode_delete(struct inode *inode);
extern void __fsnotify_vfsmount_delete(struct vfsmount *mnt);
extern u32 fsnotify_get_cookie(void);
@@ -422,7 +422,7 @@ static inline int fsnotify(struct inode *to_tell, __u32 mask, void *data, int da
return 0;
}
-static inline void __fsnotify_parent(struct file *file, struct dentry *dentry, __u32 mask)
+static inline void __fsnotify_parent(struct path *path, struct dentry *dentry, __u32 mask)
{}
static inline void __fsnotify_inode_delete(struct inode *inode)
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 6bf2306..f0c9b2e 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -526,8 +526,8 @@ static int audit_watch_handle_event(struct fsnotify_group *group,
BUG_ON(group != audit_watch_group);
switch (event->data_type) {
- case (FSNOTIFY_EVENT_FILE):
- inode = event->file->f_path.dentry->d_inode;
+ case (FSNOTIFY_EVENT_PATH):
+ inode = event->path.dentry->d_inode;
break;
case (FSNOTIFY_EVENT_INODE):
inode = event->inode;