udev git head: cdrom_id regression

udev git head: cdrom_id regression

[Martin Pitt]

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

Hello,

We got a report in Ubuntu [1] that CDs recently stopped being
recognized after insertion. I did some tests, and can reproduce this
perfectly on my wife's computer (which is of slightly older vintage).
It doesn't happen on my notebook with an USB CD drive, though.

The culprit was found quickly, udev does not get any ID_CDROM_*
properties after insertion (the change event triggered by udisks'
poller or the eject button). This bug was introduced with the recent
cdrom_id change to open the drive with O_EXCL [2]. Several people
(including me) confirmed that things work perfectly again after
reverting this.

The drive seems to be busy quite often, when I strace the udisks
poller process, I regularly get

poll([{fd=5, events=POLLIN}, {fd=3, events=POLLIN}], 2, 1997) = 0 (Timeout)
open("/dev/sr0", O_RDONLY|O_EXCL|O_NONBLOCK) = -1 EBUSY (Device or resource busy)

and I suppose the same fate also happens to cdrom_id when being
triggered by udev rules.

gvfs' cdda backend seems to keep an open fd, which explains the -EBUSY:

$ sudo fuser /dev/sr0
/dev/sr0:            14821
$ ps aux|grep 14821
martin   14821  0.0  0.1  60956  2784 ?        S    14:15   0:00 /usr/lib/gvfs/gvfsd-cdda --spawner :1.7 /org/gtk/gvfs/exec_spaw/7

So this looks like a race condition between gvfs and udev? But then
again, calling it manually has always worked for me:

$ sudo strace -e open /lib/udev/cdrom_id /dev/sr0
[...]
open("/dev/sr0", O_RDONLY|O_EXCL|O_NONBLOCK) = 3

Kay, do you know why [2] was done in the first place? Do you happen to
have a pointer to a bug etc. which describes the problems with CD
burning sessions?

Thanks,

Martin

[1] https://launchpad.net/bugs/554433
[2] http://git.kernel.org/?p=linux/hotplug/udev.git;a=commitdiff;h=38a3cde11bc77af49a96245b8a8a0f2b583a344c
-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: udev git head: cdrom_id regression

[Kay Sievers]

On Tue, Apr 6, 2010 at 14:19, Martin Pitt <martin.pitt@ubuntu.com> wrote:
> Kay, do you know why [2] was done in the first place? Do you happen to
> have a pointer to a bug etc. which describes the problems with CD
> burning sessions?

Harald said, he has several bugs with cd burning caused by cdrom_id
and blkid opening the device when a uevent is handled during cd
burning. I did not receive any such bug.

Kay

Re: udev git head: cdrom_id regression

[Harald Hoyer]

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

Am 06.04.2010 14:26, schrieb Kay Sievers:
> On Tue, Apr 6, 2010 at 14:19, Martin Pitt<martin.pitt@ubuntu.com>  wrote:
>> Kay, do you know why [2] was done in the first place? Do you happen to
>> have a pointer to a bug etc. which describes the problems with CD
>> burning sessions?
>
> Harald said, he has several bugs with cd burning caused by cdrom_id
> and blkid opening the device when a uevent is handled during cd
> burning. I did not receive any such bug.
>
> Kay


https://bugzilla.redhat.com/show_bug.cgi?id=481346
https://bugzilla.redhat.com/show_bug.cgi?id=566535

We might want to apply the attached patches.

[-- Attachment #2: 0001-cdrom_id-remove-debugging-code.patch --]
[-- Type: text/plain, Size: 746 bytes --]

From 194c5c10b65499e52864132ed542845b4c2e757c Mon Sep 17 00:00:00 2001
From: Harald Hoyer <harald@redhat.com>
Date: Tue, 6 Apr 2010 14:34:32 +0200
Subject: [PATCH 1/2] cdrom_id: remove debugging code

---
 extras/cdrom_id/cdrom_id.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/extras/cdrom_id/cdrom_id.c b/extras/cdrom_id/cdrom_id.c
index e485768..036ef28 100644
--- a/extras/cdrom_id/cdrom_id.c
+++ b/extras/cdrom_id/cdrom_id.c
@@ -126,7 +126,6 @@ static int is_mounted(const char *device)
 	if (fp == NULL)
 		return -ENOSYS;
 	while (fscanf(fp, "%*s %*s %i:%i %*[^\n]", &maj, &min) == 2) {
-		printf("got %u %u\n", maj, min);
 		if (makedev(maj, min) == statbuf.st_rdev) {
 			mounted = 1;
 			break;
-- 
1.6.6.1


[-- Attachment #3: 0002-cdrom_id-retry-to-open-the-device-if-EBUSY.patch --]
[-- Type: text/plain, Size: 1530 bytes --]

From cb3d97513fec69c4f75cf24651a16404dd576da4 Mon Sep 17 00:00:00 2001
From: Harald Hoyer <harald@redhat.com>
Date: Tue, 6 Apr 2010 14:37:21 +0200
Subject: [PATCH 2/2] cdrom_id: retry to open the device, if EBUSY

---
 extras/cdrom_id/cdrom_id.c |   29 +++++++++++++++++++++++------
 1 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/extras/cdrom_id/cdrom_id.c b/extras/cdrom_id/cdrom_id.c
index 036ef28..5e0ddba 100644
--- a/extras/cdrom_id/cdrom_id.c
+++ b/extras/cdrom_id/cdrom_id.c
@@ -591,12 +591,29 @@ int main(int argc, char *argv[])
 		goto exit;
 	}
 
-	fd = open(node, O_RDONLY|O_NONBLOCK|(is_mounted(node) ? 0 : O_EXCL));
-	if (fd < 0) {
-		info(udev, "unable to open '%s'\n", node);
-		fprintf(stderr, "unable to open '%s'\n", node);
-		rc = 1;
-		goto exit;
+
+	{
+		int errno_open=0;
+		int i = 0;
+
+		do {
+			int excl = is_mounted(node) ? 0 : O_EXCL;
+			fd = open(node, O_RDONLY|O_NONBLOCK|excl);
+			errno_open = errno;
+			if (fd < 0) {
+				if (errno_open != EBUSY || i >= 100) {
+					char *strerr = strerror(errno_open);
+					info(udev, "unable to open '%s': %s\n", node, strerr);
+					fprintf(stderr, "unable to open '%s': %s\n", node, strerr);
+					rc = 1;
+					goto exit;
+				} else {
+					fprintf(stderr, "unable to open '%s' exclusively: retrying (%d/100)\n", node, i);
+					usleep((useconds_t)(400000.0 + 400000.0 * rand() * 1.0 / (RAND_MAX+1.0)));
+					i++; 
+				}
+			}
+		} while ((fd < 0) && (errno_open == EBUSY));
 	}
 	info(udev, "probing: '%s'\n", node);
 
-- 
1.6.6.1

Re: udev git head: cdrom_id regression

[Marco d'Itri]

On Apr 06, Kay Sievers <kay.sievers@vrfy.org> wrote:

> Harald said, he has several bugs with cd burning caused by cdrom_id
> and blkid opening the device when a uevent is handled during cd
> burning. I did not receive any such bug.
I think Debian did.

-- 
ciao,
Marco

Re: udev git head: cdrom_id regression

[Martin Pitt]

Hello Harald,

Harald Hoyer [2010-04-06 14:43 +0200]:
> Subject: [PATCH 2/2] cdrom_id: retry to open the device, if EBUSY

Thanks! I confirmed that this fixes the problem.

I see that Kay already committed patches for this to trunk.

Martin

-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)