Bug 34710 - CD-ROM polling failed due to O_EXCL flag
Summary: CD-ROM polling failed due to O_EXCL flag
Status: RESOLVED FIXED
Alias: None
Product: udisks
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: x86 (IA32) Linux (All)
: medium major
Assignee: Martin Pitt
QA Contact:
URL: http://cgit.freedesktop.org/~martin/u...
Whiteboard:
Keywords:
: 31712 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-02-25 03:52 UTC by pcman.tw
Modified: 2011-06-21 10:05 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Additional info: udisks --dump, udevadm, ...etc. (132.43 KB, text/plain)
2011-02-25 04:03 UTC, pcman.tw
Details
A proposed patch to fix this bug. (2.57 KB, patch)
2011-02-25 05:46 UTC, pcman.tw
Details | Splinter Review
An updated better patch using /proc/self/mountinfo. (2.30 KB, patch)
2011-02-26 00:48 UTC, pcman.tw
Details | Splinter Review
[PATCH 1/4] Use in-kernel media probing when available (4.91 KB, patch)
2011-04-04 03:16 UTC, Martin Pitt
Details | Splinter Review
[PATCH 2/4] Add "unmount" option to DriveEject (11.02 KB, patch)
2011-04-04 03:18 UTC, Martin Pitt
Details | Splinter Review
[PATCH 3/4] Handle the hardware eject button event (7.68 KB, patch)
2011-04-04 03:25 UTC, Martin Pitt
Details | Splinter Review
[PATCH 4/4] Move CD tray unlocking to poller (2.01 KB, patch)
2011-04-04 03:26 UTC, Martin Pitt
Details | Splinter Review
[PATCH 5/5] Force media validation on media change for in-kernel polling mode (3.61 KB, patch)
2011-04-05 12:17 UTC, Martin Pitt
Details | Splinter Review
[PATCH 3/4] Handle the hardware eject button event (7.68 KB, patch)
2011-04-05 12:19 UTC, Martin Pitt
Details | Splinter Review
[Patch 6/6] Limit in-kernel polling to CD devices (1.19 KB, patch)
2011-04-05 14:54 UTC, Martin Pitt
Details | Splinter Review
[PATCH 1/4] Use in-kernel media probing when available (4.91 KB, patch)
2011-05-23 08:36 UTC, Martin Pitt
Details | Splinter Review
[PATCH 2/4] Add "unmount" option to DriveEject (11.02 KB, patch)
2011-05-23 08:38 UTC, Martin Pitt
Details | Splinter Review
[PATCH 3/4] Handle the hardware eject button event (7.69 KB, patch)
2011-05-23 08:38 UTC, Martin Pitt
Details | Splinter Review
[PATCH 4/4] Move CD tray unlocking to poller (2.01 KB, patch)
2011-05-23 08:39 UTC, Martin Pitt
Details | Splinter Review
[PATCH 1/4] Use in-kernel media probing when available (4.17 KB, patch)
2011-06-15 01:41 UTC, Martin Pitt
Details | Splinter Review
[PATCH 3/4] Handle the hardware eject button event (7.99 KB, patch)
2011-06-15 01:42 UTC, Martin Pitt
Details | Splinter Review
[PATCH 4/4] Move CD tray unlocking to poller (2.08 KB, patch)
2011-06-15 01:43 UTC, Martin Pitt
Details | Splinter Review
Disable media polling if the kernel already does it (4.37 KB, patch)
2011-06-19 11:57 UTC, Martin Pitt
Details | Splinter Review

Description pcman.tw 2011-02-25 03:52:56 UTC
Hi list,
UDisks failed to perform ForceUnmount when I press the physical eject button on my CDROM.
So I downloaded udisks source code from git and traced it for a while and then I found the problem.

Here are the related properties of my device.
--------------------------------------------------------------------------------------------------------------
  device-file:                 /dev/sr0
  removable:                   1
  has media:                   1 (detected at Fri Feb 25 19:16:23 2011)
    detects change:            1
    detection by polling:      1  <---  I have no SATA AN, so polling is performed.
    detection inhibitable:     1
    detection inhibited:       0  <-- polling of the device is not inhibited.
  is read only:                0
  is mounted:                  1
  drive:
    vendor:                    HL-DT-ST
    model:                     HL-DT-ST DVDRAM GMA-4082N
    revision:                  CX08
    serial:                    M0573JA3106
    detachable:                0
    ejectable:                 1
    media:                     optical_cd
    interface:                 scsi
-------------------------------------------------------------------------------------------------------------
I tested udisks on a clean system in a clean xsession + terminal emulator so there shouldn't be other programs interfering with udisks.

In src/poller.c, poller_poll_device() :

      fd = open (device_file, O_RDONLY | O_NONBLOCK | O_EXCL);
      if (fd != -1)
          close (fd);

When I inserted a CD, this call to open() succeeded and returned a valid fd. I enabled POLL_SHOW_DEBUG, and according to the debug message, the device was polled every 2 seconds as expected.

However, after the device is mounted, either by udisks --mount or by calling sudo mount myself, open(device_file) always returns -1 here.
Then, I press the physical eject button on my CDROM to get its tray opened. The polling code should let udisks detect the media removal and then invokes ForceUnmount, but this is not the case. The call to open() kept returning -1 every 2 seconds and udisks didn't detect the media change at all. Checking errno, "Device is busy" is reported.

Then I remove the O_EXCL flag and perform the whole test again. This time, after the CD is removed by pressing physical eject button, the open() call succeeded and returned a valid fd. Then, udisks detects the media change and gracefully perform ForceUnmount for it. Everything works fine as expected.

In addition, when "Device is busy", I tried fuser and lsof, but none of them demonstrates other process using the device. However, if I unmount the device either by calling sudo umount or udisks --unmount, this error is gone and open() can return a valid fd. Then everything works.

Removing O_EXCL from this open() call seems to fix the issue, but this of course is not the correct way to fix it.

Another interesting thing I found is, if I call udisks --poll-for-media change manually after forced removal of CD with physical eject button, then udisks can correctly detect media change. The polling helper process uses the same source code as in poller.c, but poller.c doesn't work and I don't know why.

Please, if any udisks developer is reading this, test this with a CDROM without SATA AN. It's 100% reproducible here. The poller is invoked every 2 seconds correctly, but the polling does not work. One of my friend have similar problems that after physical eject udisks didn't detect the change.
Comment 1 pcman.tw 2011-02-25 04:03:14 UTC
Created attachment 43791 [details]
Additional info: udisks --dump, udevadm, ...etc.
Comment 2 pcman.tw 2011-02-25 04:19:17 UTC
Please see similar source code from the deprecated HAL.:
hal/hald/linux/addons/addon-storage.c:
HAL tries O_EXCL first, and if the device is busy, in some cases it does it without that flag.

static gboolean
poll_for_media_force (void)
{
        int fd;
        int got_media;
        int old_media_status;

	got_media = FALSE;

        old_media_status = media_status;
	if (is_cdrom) {
		int drive;
		
		fd = open (device_file, O_RDONLY | O_NONBLOCK | O_EXCL);
		
		if (fd < 0 && errno == EBUSY) {
			/* this means the disc is mounted or some other app,
			 * like a cd burner, has already opened O_EXCL */
			
			/* HOWEVER, when starting hald, a disc may be
			 * mounted; so check /etc/mtab to see if it
			 * actually is mounted. If it is we retry to open
			 * without O_EXCL
			 */
			if (!is_mounted (device_file)) {
                                if (!is_locked_via_o_excl) {
                                        is_locked_via_o_excl = TRUE;
                                        update_proc_title ();
                                } else {
                                        is_locked_via_o_excl = TRUE;
                                }
				goto skip_check;
                        }
			
			fd = open (device_file, O_RDONLY | O_NONBLOCK);
		}
....
Comment 3 pcman.tw 2011-02-25 05:46:07 UTC
Created attachment 43798 [details] [review]
A proposed patch to fix this bug.

I made a small patch against the latest udisks in git after hacking HAL daemon and it works. Please review the patch. Thanks a lot.
Comment 4 pcman.tw 2011-02-26 00:48:42 UTC
Created attachment 43846 [details] [review]
An updated better patch using /proc/self/mountinfo.

I made a new patch according to the suggestions from Kay Sievers.
Comment 5 pcman.tw 2011-02-26 00:50:17 UTC
Comment on attachment 43798 [details] [review]
A proposed patch to fix this bug.

the patch is out of date
Comment 6 David Zeuthen (not reading bugmail) 2011-02-26 05:16:12 UTC
I think the fix here is to move to a setup where in-kernel polling is available (this recenty landed in Linux, see https://lkml.org/lkml/2010/12/8/299 ). That way we can bypass the whole O_EXCL mess altogether.
Comment 7 Kay Sievers 2011-02-26 05:22:17 UTC
(In reply to comment #6)
> I think the fix here is to move to a setup where in-kernel polling is available

Yeah, that makes the most sense and should finally solve the remaining problems with cd burning, which can not be made entirely safe with userspace polling.

Here are a few details about the in-kernel stuff:

Global polling interval:
 $ cat /sys/module/block/parameters/events_dfl_poll_msecs
 0

Device supported events, and device polling interval:
 $ grep . /sys/block/*/events*
 /sys/block/sdb/events:media_change
 /sys/block/sdb/events_poll_msecs:-1
 /sys/block/sr0/events:media_change eject_request
 /sys/block/sr0/events_poll_msecs:-1

Kernel event, when media has changed. Sent out regardless of in-kernel
polling ins enabled or not. It replaces the udev hack that forwarded
the SCSI event to the blockdev:
 $ udevadm monitor -k -e
 KERNEL[1293540783.293562] change
 /devices/pci0000:00/0000:00:1f.2/host1/target1:0:0/1:0:0:0/block/sr0 (block)
 ACTION=change
 SUBSYSTEM=block
 DISK_MEDIA_CHANGE=1
 DEVTYPE=disk

We got the "eject button pressed" for the optical drives back. We can
add support for that in udisks again. :)
 $ udevadm monitor -k -e
 KERNEL[1293541100.234493] change
 /devices/pci0000:00/0000:00:1f.2/host1/target1:0:0/1:0:0:0/block/sr0 (block)
 ACTION=change
 SUBSYSTEM=block
 DISK_EJECT_REQUEST=1
 DEVTYPE=disk

The SCSI change events and the udev event forward will be history. We
will have all local to the block device.

Note, that all events are only sent out once. We have to clear the
device flag with open(), otherwise no further same event will be sent
out.

I guess all we need is udisks enabling the device-specific polling interval:
 echo 2000 > /sys/block/sr0/events_poll_msecs
not starting the udisks userspace poller.

We could also support the new:
 DISK_EJECT_REQUEST=1
to make the eject button unmount a cdrom and eject the drive, which
currently only works from the desktop UI.
Comment 8 pcman.tw 2011-02-26 06:39:49 UTC
Thanks for the info about in-kernel disk event handling. This one looks promising and might be the right solution. However, some distros are still using older kernels for stability reason and I guess you don't have time to rewrite this part with the new kernel feature at the moment. So before the redesign of this part, is it possible to at least include this fix as a temporary workaround? The eject button issue has been reported for several times in different distros and it breaks desktop applications such as file managers.

Thanks.
Comment 9 Martin Pitt 2011-04-01 04:28:22 UTC
I seem to have some difficulty with this.

$ echo 2000 | sudo tee /sys/block/sr0/events_poll_msecs
$ grep . /sys/block/*/events*
/sys/block/sr0/events:media_change eject_request
/sys/block/sr0/events_poll_msecs:2000

With that, I indeed get a change event on sr0, with DISK_MEDIA_CHANGE=1 when I insert a medium.

However, I never managed to get any event when I press the eject button. I did open the device with "head /dev/sr0" in between, but that didn't help. Do I need to open(/dev/sr0) with some particular flags to make this work?
Comment 10 David Zeuthen (not reading bugmail) 2011-04-01 06:31:50 UTC
Updating the bug with some relevant information from my experiment on using the in-kernel poller:

<davidz> hi pitti
 kernel polling .. the short story is that it's hard to enable by default
 a couple of weeks ago I spent a few hours looking into it with help from tejun
 the problem is that the kernel just gives us whatever event the hardware gives it
 so there's a lot of stuff and assumptions that user space will have to do
 suggest to start by reading that backlog
<kay> i did not really understand the problem that time
<davidz> the relevant chat started at Mar 15, 1:42pm EST
 the problem , in a nutshell is, that the kernel will only tell you when there is _new_ media
 in particular it won't tell you if the media is removed
<kay> <davidz> tj1: ok, on this blu-ray drive I'm only seeing events when the disc is inserted - never when it is removed ...
 <davidz> tj1: would you like me to open a bug on https://bugzilla.kernel.org/ ?
<kay> <tj1> well, that's the hardware limitation.
 <tj1> optical drives only report media changed event when a new media becomes available
 <tj1> it does report eject request tho.
 <tj1> so, the combination of those two events should give enough clue about what's going on.
<kay> <davidz> ah
<davidz> so the suggestion is... the suggestion is to always lock the door.. and listen for the Eject button.. and upon receving the eject button event... eject the media and then revalidate (thus clearing the media)
 right now the kernel does not do that - it expects user space to do that
 and I think that's really broken
 in a way, it's the kernel just shrugging and saying "meh, not our problem" ...
 I mean, we could do this in userspace
 but I don't think that's a good idea
 the kernel should really provide a complete abstraction
 not something that requires user space to have a detailed understanding and send very hardware-specific commands
 anyway, I haven't communicated this to Tejun since I didn't have time to think about it just when we chatted
Comment 11 David Zeuthen (not reading bugmail) 2011-04-01 06:32:29 UTC
(In reply to comment #9)
> I seem to have some difficulty with this.
> 
> $ echo 2000 | sudo tee /sys/block/sr0/events_poll_msecs
> $ grep . /sys/block/*/events*
> /sys/block/sr0/events:media_change eject_request
> /sys/block/sr0/events_poll_msecs:2000
> 
> With that, I indeed get a change event on sr0, with DISK_MEDIA_CHANGE=1 when I
> insert a medium.
> 
> However, I never managed to get any event when I press the eject button. I did
> open the device with "head /dev/sr0" in between, but that didn't help. Do I
> need to open(/dev/sr0) with some particular flags to make this work?

Right. My experiments indicated you only get the eject-button event when the door is locked....
Comment 12 Martin Pitt 2011-04-01 07:19:09 UTC
Current plan:

- if the events attribute exists and contains eject_request, keep the door locked, set the events_poll_msecs=2000, and listen for the eject request uevent (works for 2.6.38+)

- otherwise (older kernels), fall back to starting the udisks poller on the device and unlock the door after mount
Comment 13 Martin Pitt 2011-04-01 08:37:59 UTC
kay | pitti: if you lock the door from udisk, please disable any kernel lock/unlock the door smartness with: ioctl(CDROM_CLEAR_OPTIONS, CDO_LOCK)

kay | pitti: otherwise a open()/close() of the mon-mounted device might clear your lock
Comment 14 Martin Pitt 2011-04-04 03:16:56 UTC
Created attachment 45212 [details] [review]
[PATCH 1/4] Use in-kernel media probing when available

I now have this working quite nicely. I committed some small and rather obvious preparatory patches to git head already (af80cd56 and 378a755), but I attach the main patches here for review first.

The first patch changes poller_set_devices() to check the drive sysfs attributes whether it supports the "media_change" and "eject_request" events. If so, we are running a newer Linux which supports in-kernel media polling, which then gets enabled (poll every 2 s, as we have now). In that case the drive will not be added to the poller's list of polled devices.

This also introduces two (private) Device properties, "supports_media_change_event" and "supports_eject_request_event". The former is used for caching the result, the latter is not used by this patch, but by the followup ones to support the Eject button.
Comment 15 Martin Pitt 2011-04-04 03:18:14 UTC
Created attachment 45214 [details] [review]
[PATCH 2/4] Add "unmount" option to DriveEject

DriveEject currently fails if there are mounted file systems on the drive. Add
an option "unmount" which will unmount any mounted file system on the drive to
be ejected first. The "eject" command conveniently already does this, so the
only thing we need to do here is to skip the "is mounted" check in
device_local_is_busy().

This is a preparation for this bug: When pressing the Eject button, we do not
want to (and neither need to) run FilesystemUnmount() first, wait for that to
complete, and then run "eject".
Comment 16 Martin Pitt 2011-04-04 03:25:19 UTC
Created attachment 45215 [details] [review]
[PATCH 3/4] Handle the hardware eject button event

This is now the gist of the change, as discussed previously. If we get a change event with DISK_EJECT_REQUEST, call device_drive_eject() on the device with the "unmount" option (which will DTRT with the previous patch).

This now uses the private supports_eject_request_event propert (see patch 1/4) to check whether it should lock the drive after media becomes available, or unlock the drive after mount (previous behaviour).

As discussed the actual ioctls are sent in the polling process. This adds a new poller_lock_cdtray() function which sends the new "lock-tray: <value> <device>" command to the poller, which will call poller_do_lock_cdtray() on the drive. Doing this in the poller avoids potential long blocking in the main process if there's something wrong with the drive.

The hunk which adds handling of "lock-tray:" to poller_have_data() looks a little confusing, thanks to patch formatting the diff so badly. It moves the "set-poll:" handling code into the "if (g_str_has_prefix (line, "set-poll:"))" block (which generates most of the noise), and then adds a new "else if (g_str_has_prefix (line, "lock-tray:"))" block.

I mark PCMan's original patch as obsoleted by this, as David says that he doesn't want to apply it upstream.
Comment 17 Martin Pitt 2011-04-04 03:26:42 UTC
Created attachment 45216 [details] [review]
[PATCH 4/4] Move CD tray unlocking to poller

And finally, this now removes the ioctl handling for unlocking the drive after mounting (i. e. the "legacy" behaviour) from device.c and uses the new poller command instead. This removes the potentially problematic ioctl from device.c again.
Comment 18 Martin Pitt 2011-04-04 03:30:09 UTC
While testing this I noticed that pressing the eject button causes the drive to spin up before it gets ejected. This is due to the eject button press being sent as a change event on the block device (if only we had something in the kernel which would send events for input devices like keyboards, mouses, headphone jacks, or power buttons...), and thus the udev rules call cdrom_id and blkid on the device.

As a DISK_EJECT_REQUEST==1 change event does not actually change anything in the media state, I eliminated the blkid/cdrom_id calls for this case:

http://git.kernel.org/?p=linux/hotplug/udev.git;a=commitdiff;h=55eb500cc5465babf1fd773dbcceeb90eeefd997

This is independent from udisks actually handling the event, so I committed this to udev already.
Comment 19 Martin Pitt 2011-04-04 03:40:52 UTC
*** Bug 31712 has been marked as a duplicate of this bug. ***
Comment 20 Martin Pitt 2011-04-05 08:50:29 UTC
Kay noticed that this behaves wrongly with USB SD-card readers. I debugged this and eventually found that this was already reported as https://bugzilla.kernel.org/show_bug.cgi?id=13029 . I added my findings to it and will come up with an updated patch which adds the workaround for the in-kernerl polling as well.
Comment 21 Martin Pitt 2011-04-05 12:17:14 UTC
Created attachment 45307 [details] [review]
[PATCH 5/5] Force media validation on media change for in-kernel polling mode

With this additional patch SD cards now work as before as well. This is basically the same "open with O_NONBLOCK" workaround that the continuous poller already uses, just applied to the new in-kernel polling mode.
Comment 22 Martin Pitt 2011-04-05 12:19:27 UTC
Created attachment 45308 [details] [review]
[PATCH 3/4] Handle the hardware eject button event

This fixes a typo (forgotten "not") in a debugging message, thanks to Kay for spotting.
Comment 23 Martin Pitt 2011-04-05 14:54:52 UTC
Created attachment 45319 [details] [review]
[Patch 6/6] Limit in-kernel polling to CD devices

Kay and I tested this some more, and found that we can't use in-kernel polling with non-CD devices such as SD card readers yet, as the kernel won't poll mounted media and thus we won't get an uevent for ripped out SD cards when they are mounted.

This patch limits in-kernel polling to ID_CDROM devices.

Kay says that this should be fixed in the kernel (i. e. polling non-CD devices when they are mounted), so this can hopefully be reverted at some point, just like the 5/5 patch.
Comment 24 Martin Pitt 2011-04-05 14:57:53 UTC
As these patches pile up, I pushed them to http://cgit.freedesktop.org/~martin/udisks/ (also added to the URL field above). Probably easier to test/review them there.
Comment 25 Martin Pitt 2011-05-23 08:34:58 UTC
I rebased the patches to current head, and also removed the workarounds, now that the fixes landed in 2.6.38 and later kernels. As the "events" attribute was only added in .38, we won't need the workarounds any more.

I'll just invalidate all my older patches wholesale and attach the current ones, as per David's request.
Comment 26 Martin Pitt 2011-05-23 08:36:47 UTC
Created attachment 47050 [details] [review]
[PATCH 1/4] Use in-kernel media probing when available
Comment 27 Martin Pitt 2011-05-23 08:38:24 UTC
Created attachment 47051 [details] [review]
[PATCH 2/4] Add "unmount" option to DriveEject
Comment 28 Martin Pitt 2011-05-23 08:38:49 UTC
Created attachment 47052 [details] [review]
[PATCH 3/4] Handle the hardware eject button event
Comment 29 Martin Pitt 2011-05-23 08:39:25 UTC
Created attachment 47053 [details] [review]
[PATCH 4/4] Move CD tray unlocking to poller
Comment 30 Martin Pitt 2011-05-23 08:41:19 UTC
Comment on attachment 45307 [details] [review]
[PATCH 5/5] Force media validation on media change for in-kernel polling mode

obsoleted by kernel fix:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=bf2253a6f00e8fea5b026e471e9f0d0a1b3621f2
Comment 31 Martin Pitt 2011-05-23 08:42:35 UTC
Comment on attachment 45319 [details] [review]
[Patch 6/6] Limit in-kernel polling to CD devices

Should be fixed in kernel by http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=02e352287a40bd456eb78df705bf888bc3161d3f

so workaround no longer necessary.
Comment 32 David Zeuthen (not reading bugmail) 2011-06-14 12:04:59 UTC
Sorry for being so slow.

(In reply to comment #26)
> Created an attachment (id=47050) [details]
> [PATCH 1/4] Use in-kernel media probing when available

+      emit_changed (device, "supports_media_change_event");

+      emit_changed (device, "supports_eject_request_event");

This suggests that these are D-Bus properties but you didn't add them in the D-Bus XML nor Device's class_init() so they are currently not.

I don't think we want them to be D-Bus properties (apps don't need to know this) so it's unnecessary with setters (e.g. device_set_supports_media_change_event() and device_set_supports_eject_request_event()) - suggest to just use direct struct access.

+static gboolean
+check_in_kernel_polling (Device* d)
+{

I would call this check_and_enable_in_kernel_polling() as it enables kernel polling if available.

+  /* only check once */
+  if (d->priv->supports_media_change_event)
+    return TRUE;

But if d->priv->supports_media_change_event ends up being FALSE we keep checking over and over again. I'd add another boolean (suggested name: have_checked_in_kernel_polling) to the struct for whether we have checked or not.


+	  char *attr = g_build_filename (d->priv->native_path, "events_poll_msecs", NULL);
+	  const char* poll_time = "2000";
+	  size_t poll_time_size = strlen (poll_time);

Please use gchar* since memory must be freed with g_free() (the other part of the rule: use char* if memory must be freed with free(3)).
Comment 33 David Zeuthen (not reading bugmail) 2011-06-14 12:13:06 UTC
(In reply to comment #27)
> Created an attachment (id=47051) [details]
> [PATCH 2/4] Add "unmount" option to DriveEject

Looks good to me.
Comment 34 David Zeuthen (not reading bugmail) 2011-06-14 12:28:05 UTC
(In reply to comment #26)
> Created an attachment (id=47050) [details]
> [PATCH 1/4] Use in-kernel media probing when available

Additionally I would like the first patch to set a new bool

 device->priv->using_kernel_polling 

and the code to check a udev variable UDISKS_DISABLE_KERNEL_POLLING on a per-device basis. Just so it's easy to turn on and off if/when we run into bugs...
Comment 35 David Zeuthen (not reading bugmail) 2011-06-14 12:30:35 UTC
(In reply to comment #28)
> Created an attachment (id=47052) [details]
> [PATCH 3/4] Handle the hardware eject button event

+  /* On newer kernels which support the "eject_request" uevent we need to
+   * lock the tray as soon as we find a medium, as the uevent is only being
+   * sent while the tray is locked. */

How about replacing it with

   /* When using in-kernel polling on a drive which supports the
    * "eject_request" uevent we MUST lock the tray as soon as we find
    * a medium, otherwise we don't get ANY uevent not even for media
    * changes. This is because of hardware limitations.
    *
    * See https://bugs.freedesktop.org/show_bug.cgi?id=34710 for more
    * details.
    */

to reflect the finding from comment 10 (which is the whole reason we are handling the eject button in user space at all).

+  if (device->priv->supports_eject_request_event && device->priv->device_is_media_available)
+      poller_lock_cdtray (device, TRUE);

Should also check device->priv->using_kernel_polling

+      if (!device->priv->supports_eject_request_event)
+	  unlock_cd_tray (device);

Should also check device->priv->using_kernel_polling
Comment 36 David Zeuthen (not reading bugmail) 2011-06-14 12:31:43 UTC
(In reply to comment #29)
> Created an attachment (id=47053) [details]
> [PATCH 4/4] Move CD tray unlocking to poller

Looks good to me.
Comment 37 Martin Pitt 2011-06-15 01:41:38 UTC
Created attachment 47983 [details] [review]
[PATCH 1/4] Use in-kernel media probing when available

Thanks for the review!

This updates the first patch according to your comment 32 and comment 34.
Comment 38 Martin Pitt 2011-06-15 01:42:48 UTC
Created attachment 47984 [details] [review]
[PATCH 3/4] Handle the hardware eject button event

This updates the third patch for the recent changes in the first one (in particular, new using_in_kernel_polling flag), and also updates the comment that you suggested. I. e. this addresses comment 35.
Comment 39 Martin Pitt 2011-06-15 01:43:23 UTC
Created attachment 47985 [details] [review]
[PATCH 4/4] Move CD tray unlocking to poller

Updated fourth patch, as the original one now had some fuzz due to the previous changed patches.
Comment 40 David Zeuthen (not reading bugmail) 2011-06-15 07:33:05 UTC
I still feel very uneasy about locking the door and handling eject button requests from udisks. If we do this, I fear that we are going to see a bunch of confused users filing bug reports against the wrong components. And each bug report is going to be hard to deal with because there are so many components involved. Btw, have you considered how this is going to work when landing udisks v2? In that case we are going to have

 /usr/libexec/udisks-daemon

and (something like)

 /usr/lib64/udisks2/udisksd

running in the worst case (e.g. when the user has apps running that wants to talk to both org.fd.UDisks and org.fd.UDisks2). Both are probably going to want to turn on kernel polling. And both are going to run eject(1) when the eject button is pressed.

So I would like to consider the possibility of doing "all this" in udev. E.g.

 - turn on kernel polling (and set a property so e.g. udisksd knows not
   to poll it from userspace)

 - lock/unlock the door (if needed)

 - handle eject button requests

I think it probably could be as simple as just a few udev rules and a simple C program. Thoughts?
Comment 41 Martin Pitt 2011-06-17 09:38:54 UTC
This was discussed in #udev, but for the record:

 * tray locking and kernel poll enabling will go to udev

 * udisks then just needs to check if global or per-device kernel polling is enabled, and if so, don't poll by itself

I'll provide a patch for the second next week.
Comment 42 Martin Pitt 2011-06-19 11:11:22 UTC
The 'Add "unmount" option to DriveEject' patch seems useful in its own right, and got reviewed by David already, so I pushed this to trunk.
Comment 43 Martin Pitt 2011-06-19 11:57:54 UTC
Created attachment 48169 [details] [review]
Disable media polling if the kernel already does it

The udev support for enabling kernel polling and tray locking on eject button landed in trunk. Before it can be released, udisks should be taught to cope with this by disabling its own polling when the kernel polling is enabled.

The attached patch provides this. I tested it with these scenarios: 
 - no kernel polling -> udisks keeps polling
 - /sys/module/block/parameters/events_dfl_poll_msecs == 2000 and /sys/block/sr0/events_poll_msecs == -1 -> udisks does not poll
 - /sys/module/block/parameters/events_dfl_poll_msecs == 0 and sys/block/sr0/events_poll_msecs == 2000 -> udisks does not poll
 - /sys/module/block/parameters/events_dfl_poll_msecs == 2000 and /sys/block/sr0/events_poll_msecs == 0 -> udisks polls (per-device disabling of kernel polling)
Comment 44 David Zeuthen (not reading bugmail) 2011-06-21 09:55:03 UTC
(In reply to comment #43)
> Created an attachment (id=48169) [details]
> Disable media polling if the kernel already does it
> 
> The udev support for enabling kernel polling and tray locking on eject button
> landed in trunk. Before it can be released, udisks should be taught to cope
> with this by disabling its own polling when the kernel polling is enabled.

Sounds good to me. Thanks.
Comment 45 Martin Pitt 2011-06-21 10:05:55 UTC
Committed. As the udev parts are also committed, I close this bug report now.

Yay for finally getting this resolved \o/


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.