2.6.36: Sound stop working

2.6.36: Sound stop working

[Thomas Meyer]

My sound card stop to work since a few commits. I tried to bisect it and ended up with this:

There are only 'skip'ped commits left to test.
The first bad commit could be any of:
3bcf3860a4ff9bbc522820b4b765e65e4deceb3e
c1e5c954020e123d30b4abf4038ce501861bcf9f
We cannot bisect more!

any ideas what to do now?

Re: 2.6.36: Sound stop working

[Pekka Enberg]

On Thu, Aug 12, 2010 at 11:00 PM, Thomas Meyer <thomas@m3y3r.de> wrote:
> My sound card stop to work since a few commits. I tried to bisect it and ended up with this:
>
> There are only 'skip'ped commits left to test.
> The first bad commit could be any of:
> 3bcf3860a4ff9bbc522820b4b765e65e4deceb3e
> c1e5c954020e123d30b4abf4038ce501861bcf9f
> We cannot bisect more!
>
> any ideas what to do now?

Takashi, maybe this is related to Linus' problem?

Re: 2.6.36: Sound stop working

[Linus Torvalds]

On Thu, Aug 12, 2010 at 1:10 PM, Pekka Enberg <penberg@kernel.org> wrote:
> On Thu, Aug 12, 2010 at 11:00 PM, Thomas Meyer <thomas@m3y3r.de> wrote:
>> My sound card stop to work since a few commits. I tried to bisect it and ended up with this:
>>
>> There are only 'skip'ped commits left to test.
>> The first bad commit could be any of:
>> 3bcf3860a4ff9bbc522820b4b765e65e4deceb3e
>> c1e5c954020e123d30b4abf4038ce501861bcf9f
>> We cannot bisect more!
>>
>> any ideas what to do now?
>
> Takashi, maybe this is related to Linus' problem?

Yes, I'm 99% sure it is. I haven't bisected my problem all the way,
but I've bisected away all the sound changes (and as mentioned, doing
a 'cat' to /dev/audio works). And yes, the fanotify changes are right
in the middle of my bisect.

So I suspect it's not sound that is broken at all, but pulseaudio that
got broken by the fanotify changes.

Eric - over to you.

                               Linus

Re: 2.6.36: Sound stop working

[Takashi Iwai]

At Thu, 12 Aug 2010 23:10:15 +0300,
Pekka Enberg wrote:
> 
> On Thu, Aug 12, 2010 at 11:00 PM, Thomas Meyer <thomas@m3y3r.de> wrote:
> > My sound card stop to work since a few commits. I tried to bisect it and ended up with this:
> >
> > There are only 'skip'ped commits left to test.
> > The first bad commit could be any of:
> > 3bcf3860a4ff9bbc522820b4b765e65e4deceb3e
> > c1e5c954020e123d30b4abf4038ce501861bcf9f
> > We cannot bisect more!
> >
> > any ideas what to do now?
> 
> Takashi, maybe this is related to Linus' problem?

Yeah, I guess so, since I couldn't see any obvious problem in sound/*.

PulseAudio uses inotify in udev-detection module, at least.
It's possible that inotify change may hit the sound in the end.


thanks,

Takashi

Re: 2.6.36: Sound stop working

[Jiri Slaby]

On 08/12/2010 10:26 PM, Takashi Iwai wrote:
> At Thu, 12 Aug 2010 23:10:15 +0300,
> Pekka Enberg wrote:
>>
>> On Thu, Aug 12, 2010 at 11:00 PM, Thomas Meyer <thomas@m3y3r.de> wrote:
>>> My sound card stop to work since a few commits. I tried to bisect it and ended up with this:
>>>
>>> There are only 'skip'ped commits left to test.
>>> The first bad commit could be any of:
>>> 3bcf3860a4ff9bbc522820b4b765e65e4deceb3e
>>> c1e5c954020e123d30b4abf4038ce501861bcf9f
>>> We cannot bisect more!
>>>
>>> any ideas what to do now?
>>
>> Takashi, maybe this is related to Linus' problem?
> 
> Yeah, I guess so, since I couldn't see any obvious problem in sound/*.
> 
> PulseAudio uses inotify in udev-detection module, at least.
> It's possible that inotify change may hit the sound in the end.

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.

(If this is the same issue.)

-- 
js

Re: 2.6.36: Sound stop working

[Linus Torvalds]

On Thu, Aug 12, 2010 at 1:20 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So I suspect it's not sound that is broken at all, but pulseaudio that
> got broken by the fanotify changes.

Confirmed. That broken commit 3bcf3860a4ff9bb doesn't even boot for me
(looks like a stack smash recursion), and the commit to "fix" it
(c1e5c954020e12) is really too ugly to live.

I think we need to totally undo the whole "struct file" thing, and
just admit that it was a mistake. The code really wants a "struct
path", and using a struct file screws up all the refcounting and is
just not right.

The fact that dentry_open() may have some problem needs to be fixed
-there- rather than make the callers do crazy things that they don't
want to do and can't do sanely.

                           Linus

Re: 2.6.36: Sound stop working

[Maxim Levitsky]

On Thu, 2010-08-12 at 23:01 +0200, Jiri Slaby wrote: 
> On 08/12/2010 10:26 PM, Takashi Iwai wrote:
> > At Thu, 12 Aug 2010 23:10:15 +0300,
> > Pekka Enberg wrote:
> >>
> >> On Thu, Aug 12, 2010 at 11:00 PM, Thomas Meyer <thomas@m3y3r.de> wrote:
> >>> My sound card stop to work since a few commits. I tried to bisect it and ended up with this:
> >>>
> >>> There are only 'skip'ped commits left to test.
> >>> The first bad commit could be any of:
> >>> 3bcf3860a4ff9bbc522820b4b765e65e4deceb3e
> >>> c1e5c954020e123d30b4abf4038ce501861bcf9f
> >>> We cannot bisect more!
> >>>
> >>> any ideas what to do now?
> >>
> >> Takashi, maybe this is related to Linus' problem?
> > 
> > Yeah, I guess so, since I couldn't see any obvious problem in sound/*.
> > 
> > PulseAudio uses inotify in udev-detection module, at least.
> > It's possible that inotify change may hit the sound in the end.
> 
> 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.
I confirm that here.

> 
> (If this is the same issue.)
> 
It must be..


Best regards,
Maxim Levitsky

Re: 2.6.36: Sound stop working

[Takashi Iwai]

At Thu, 12 Aug 2010 23:01:04 +0200,
Jiri Slaby wrote:
> 
> On 08/12/2010 10:26 PM, Takashi Iwai wrote:
> > At Thu, 12 Aug 2010 23:10:15 +0300,
> > Pekka Enberg wrote:
> >>
> >> On Thu, Aug 12, 2010 at 11:00 PM, Thomas Meyer <thomas@m3y3r.de> wrote:
> >>> My sound card stop to work since a few commits. I tried to bisect it and ended up with this:
> >>>
> >>> There are only 'skip'ped commits left to test.
> >>> The first bad commit could be any of:
> >>> 3bcf3860a4ff9bbc522820b4b765e65e4deceb3e
> >>> c1e5c954020e123d30b4abf4038ce501861bcf9f
> >>> We cannot bisect more!
> >>>
> >>> any ideas what to do now?
> >>
> >> Takashi, maybe this is related to Linus' problem?
> > 
> > Yeah, I guess so, since I couldn't see any obvious problem in sound/*.
> > 
> > PulseAudio uses inotify in udev-detection module, at least.
> > It's possible that inotify change may hit the sound in the end.
> 
> 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.


thanks,

Takashi

Re: 2.6.36: Sound stop working

[Linus Torvalds]

Eric needs to be cc'd.. Sorry for the extra quoting.

                      Linus
---
On Thu, Aug 12, 2010 at 2:01 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
> On 08/12/2010 10:26 PM, Takashi Iwai wrote:
>> At Thu, 12 Aug 2010 23:10:15 +0300,
>> Pekka Enberg wrote:
>>>
>>> On Thu, Aug 12, 2010 at 11:00 PM, Thomas Meyer <thomas@m3y3r.de> wrote:
>>>> My sound card stop to work since a few commits. I tried to bisect it and ended up with this:
>>>>
>>>> There are only 'skip'ped commits left to test.
>>>> The first bad commit could be any of:
>>>> 3bcf3860a4ff9bbc522820b4b765e65e4deceb3e
>>>> c1e5c954020e123d30b4abf4038ce501861bcf9f
>>>> We cannot bisect more!
>>>>
>>>> any ideas what to do now?
>>>
>>> Takashi, maybe this is related to Linus' problem?
>>
>> Yeah, I guess so, since I couldn't see any obvious problem in sound/*.
>>
>> PulseAudio uses inotify in udev-detection module, at least.
>> It's possible that inotify change may hit the sound in the end.
>
> 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.
>
> (If this is the same issue.)
>
> --
> js
>

Re: 2.6.36: Sound stop working

[Jiri Slaby]

On 08/12/2010 11:18 PM, Linus Torvalds wrote:
> On Thu, Aug 12, 2010 at 2:01 PM, Jiri Slaby <jirislaby@gmail.com> 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.

Perfectly reproducible in qemu-kvm with ac97 soundhw, i.e. intel8x0
driver. Just in case you want to debug that easily.

-- 
js

Re: 2.6.36: Sound stop working

[Linus Torvalds]

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

On Thu, Aug 12, 2010 at 2:17 PM, Takashi Iwai <tiwai@suse.de> 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

[-- Attachment #2: diff.txt --]
[-- Type: text/plain, Size: 15771 bytes --]

commit 2069601b3f0ea38170d4b509b89f3ca0a373bdc1
Author: Linus Torvalds <torvalds@linux-foundation.org>
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@linux-foundation.org>
---
 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;

Re: 2.6.36: Sound stop working

[Takashi Iwai]

At Thu, 12 Aug 2010 23:24:43 +0200,
Jiri Slaby wrote:
> 
> On 08/12/2010 11:18 PM, Linus Torvalds wrote:
> > On Thu, Aug 12, 2010 at 2:01 PM, Jiri Slaby <jirislaby@gmail.com> 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.
> 
> Perfectly reproducible in qemu-kvm with ac97 soundhw, i.e. intel8x0
> driver. Just in case you want to debug that easily.

And the below is a minimal test case to simulate the situation
PulseAudio does.


Takashi

===

#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/inotify.h>
#include <unistd.h>

int main()
{
	int fd;

	inotify_add_watch(inotify_init(), "/dev/snd", IN_CLOSE_WRITE);

	fd = open("/dev/snd/pcmC0D0p", O_RDWR | O_NONBLOCK);
	if (fd < 0)
		perror("open1");
	else
		close(fd);
	fd = open("/dev/snd/pcmC0D0p", O_RDWR | O_NONBLOCK);
	if (fd < 0)
		perror("open2");
	else
		close(fd);
	return 0;
}

Re: 2.6.36: Sound stop working

[Linus Torvalds]

On Thu, Aug 12, 2010 at 2:41 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> 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?

Btw, that's exactly what the fsnotify code does seem to do, in
fsnotify_create_event(). So as far as I can tell, it's all ok - we
only use that file->f_path pointer while we hold the file open, and
then we copy it to event->path and increment the counts properly.

Of course, maybe I'm missing some other case where we _don't_ copy the
data, and pass a pointer to a file->f_path around that could get
stale. Or maybe I'm missing some other worry entirely.

But those games with f_count are just disgusting. The path-based thing
seems to be much more straightforward.

                              Linus

Re: 2.6.36: Sound stop working

[Al Viro]

On Thu, Aug 12, 2010 at 02:41:51PM -0700, Linus Torvalds wrote:

> 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?

I agree that what this crap is doing to f_count is blatantly wrong,
of course - no arguments here.  I *do* have a reason to want struct
file, but not that way, TYVM.

Basically, dentry_open() is not even promised to work on arbitrary
dentry.  Thanks to !@#!@#!@#!@# intents crap we are not promised
that the damn thing won't need setup by ->lookup/->d_revalidate.
We _are_ more or less promised that it'll work while the file is
opened (provided that /proc/*/fd/* is openable), but that's it.

It's not an API that can be made to work on an arbitrary dentries.  If
caller knows what it's dealing with, it's OK, but not in general.  And
no, I'm not fond of that situation, to put it mildly.

I'll see what can be done to fix that mess more or less right way...

Re: 2.6.36: Sound stop working

[Takashi Iwai]

At Thu, 12 Aug 2010 14:41:51 -0700,
Linus Torvalds wrote:
> 
> On Thu, Aug 12, 2010 at 2:17 PM, Takashi Iwai <tiwai@suse.de> 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.

It makes working for me, too.


thanks,

Takashi

Re: 2.6.36: Sound stop working

[Linus Torvalds]

On Thu, Aug 12, 2010 at 2:52 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Aug 12, 2010 at 02:41:51PM -0700, Linus Torvalds wrote:
>
>> 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?
>
> I agree that what this crap is doing to f_count is blatantly wrong,
> of course - no arguments here.  I *do* have a reason to want struct
> file, but not that way, TYVM.
>
> Basically, dentry_open() is not even promised to work on arbitrary
> dentry.  Thanks to !@#!@#!@#!@# intents crap we are not promised
> that the damn thing won't need setup by ->lookup/->d_revalidate.
> We _are_ more or less promised that it'll work while the file is
> opened (provided that /proc/*/fd/* is openable), but that's it.

Hmm. We do basically dentry_open() these days with the whole
path_init() of a dfd. Yes, we have the 'struct file', but the only
thing we take from there is the dentry.

So why not just say that 'dentry_open()' is exactly that path_init()
setup, and then look up an empty path. Allow a non-directory, and
leave it to the filesystems to then say "I can't do that at all" if
they really want to. fanotify seems to be perfectly happy with the
lookup failing.

So I'm not convinced this is really a fanotify problem, as much as a
VFS layer thing.

> It's not an API that can be made to work on an arbitrary dentries.  If
> caller knows what it's dealing with, it's OK, but not in general.  And
> no, I'm not fond of that situation, to put it mildly.

So I think it's probably doable for arbitrary dentries too, although
maybe we need to have a special "reopen()" function. I suspect few
filesystems will care, and if you can fail it, the ones that do care
deeply are home free too. No?

> I'll see what can be done to fix that mess more or less right way...

.. but I assume you don't wan tto keep those "struct file" games in
fanotify regardless, right? So we can fix the sound issue by the
revert I sent out, no?

                    Linus

Re: 2.6.36: Sound stop working

[Eric Paris]

On Thu, 2010-08-12 at 15:19 -0700, Linus Torvalds wrote:
> On Thu, Aug 12, 2010 at 2:52 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Thu, Aug 12, 2010 at 02:41:51PM -0700, Linus Torvalds wrote:

> > I'll see what can be done to fix that mess more or less right way...
> 
> .. but I assume you don't wan tto keep those "struct file" games in
> fanotify regardless, right? So we can fix the sound issue by the
> revert I sent out, no?

Sorry I'm just coming into this now that it is 'solved.'  I just got
back to my hotel from Linuxcon and haven't been checking e-mail.  I
guess noone (including me) is testing sound in linux-next   :(.  I
wonder what kind of tom foolery it must be doing with f_count (like I
am) to have hit problems.

I certainly agree the revert patch you sent should be applied if the
worst artifact for calling dentry_open() with an 'arbitrary dentry' is
that it fails.

An easier long term fix might be a 'dentry_open_as' which I can call in
the context of the original opening process but which will be done (for
security/accounting/fd table/etc purposes) as the process which will
ultimately consume the new struct file.  I don't know which would be
better/easier for the VFS, Al?

-Eric

Re: 2.6.36: Sound stop working

[Jiri Slaby]

On 08/13/2010 03:53 AM, Eric Paris wrote:
> I guess noone (including me) is testing sound in linux-next   :(.

Yeah, there are some people like Valdis and me who run -next based
kernels (-mm). But it got upstream too fast to react earlier. I updated
to the latest -next few days ago, Valdis with the latest mmotm release 2
days ago. So I think these things need to sit in -next a bit longer than
they usually do. The reason is also that I need to solve 2-3 issues with
every -next update and it takes time too.

regards,
-- 
js