Bug 34710

Created attachment 43791 [details]
Additional info: udisks --dump, udevadm, ...etc.

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);
		}
....

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.

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 on attachment 43798 [details] [review]
A proposed patch to fix this bug.

the patch is out of date

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.

(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.

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.

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?

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

(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....

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

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

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.

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".

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.

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.

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.

*** Bug 31712 has been marked as a duplicate of this bug. ***

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.

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.

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.

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.

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.

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.

Created attachment 47050 [details] [review]
[PATCH 1/4] Use in-kernel media probing when available

Created attachment 47051 [details] [review]
[PATCH 2/4] Add "unmount" option to DriveEject

Created attachment 47052 [details] [review]
[PATCH 3/4] Handle the hardware eject button event

Created attachment 47053 [details] [review]
[PATCH 4/4] Move CD tray unlocking to poller

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

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)).

(In reply to comment #27)
> Created an attachment (id=47051) [details]
> [PATCH 2/4] Add "unmount" option to DriveEject

Looks good to me.

(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...

(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

(In reply to comment #29)
> Created an attachment (id=47053) [details]
> [PATCH 4/4] Move CD tray unlocking to poller

Looks good to me.

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.

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.

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.

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?

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.

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.

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)

(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.

Committed. As the udev parts are also committed, I close this bug report now.

Yay for finally getting this resolved \o/