Bug 24052 - CDROM eject button is locked while CDROM is mounted
CDROM eject button is locked while CDROM is mounted
Status: RESOLVED FIXED
Product: udisks
Classification: Unclassified
Component: general
unspecified
Other All
: medium normal
Assigned To: Martin Pitt
https://launchpad.net/bugs/397734
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-20 16:49 UTC by maximlevitsky
Modified: 2009-10-09 06:52 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
unlock CD trays after mounting (2.32 KB, patch)
2009-10-06 01:54 UTC, Martin Pitt
Details | Splinter Review
unlock CD trays after mounting (2.33 KB, patch)
2009-10-06 04:13 UTC, Martin Pitt
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description maximlevitsky 2009-09-20 16:49:31 UTC
While this might seem to be not a bug, but it used to work with HAL

It was possible to press the button on the cdrom to trigger the unmount & ejection.

It would even fail if some files were opened.


Best reproduced with this:

unset the /apps/nautilus/preferences/show_desktop gconf key.
(It seem that as a seperate bug, nautilus keeps cdrom opened, thus blocks the ejection), then close all nautilus windows.

Now insert a disk, observe that it can be ejected with button.
Now inset again, open g-d-u, and press 'mount'. Now hardware button is locked.
Now press 'unmount'. observe that now eject button works.

100% reproducible
Comment 1 Martin Pitt 2009-09-29 07:11:06 UTC
For the record, the hal-addon-storage had a poll loop which regularly queried the CD-ROM whether the eject button was pressed, and if so, emit an "EjectPressed" device condition through d-bus. Something in the desktop session (usually nautilus) would pick that up and ask hal to eject the drive.

This mechanism is completely lacking in DK-disks, so it's not a small change. Also, I'm not sure whether we actually want this kind of callback back in DK-disks.

One possible alternative would be to not lock the CD-ROM tray when mounting. This leads to much less clean unmounts, since the device would disappear while it is still mounted (and then the kernel/userspace have to clean up afterwards). For read-only media like CD-ROM or DVDs this does not actually pose a problem, though.
Comment 2 David Zeuthen (not reading bugmail) 2009-09-29 08:19:31 UTC
(In reply to comment #1)
> For the record, the hal-addon-storage had a poll loop which regularly queried
> the CD-ROM whether the eject button was pressed, and if so, emit an
> "EjectPressed" device condition through d-bus. Something in the desktop session
> (usually nautilus) would pick that up and ask hal to eject the drive.
> 
> This mechanism is completely lacking in DK-disks, so it's not a small change.
> Also, I'm not sure whether we actually want this kind of callback back in
> DK-disks.

Right, I don't think we want to reintroduce that behavior. FWIW, newer SATA optical drives has a mechanism so we can avoid polling at all. This should save ~1W or so since the device and SATA link can be powered down all the time.

> One possible alternative would be to not lock the CD-ROM tray when mounting.
> This leads to much less clean unmounts, since the device would disappear while
> it is still mounted (and then the kernel/userspace have to clean up
> afterwards). For read-only media like CD-ROM or DVDs this does not actually
> pose a problem, though.

I think that is the way to go. So we should just tell distros to do

 echo 0 > /proc/sys/dev/cdrom/lock

at boot time.
Comment 3 maximlevitsky 2009-09-29 09:44:01 UTC
No much objection, but what happens if an application has an opened file on the cdrom, or even worse if a file is loop mounted from the cdrom. 

In my opinion this can cause very nasty consequences (lock-ups)

Comment 4 David Zeuthen (not reading bugmail) 2009-09-29 10:10:18 UTC
(In reply to comment #3)
> No much objection, but what happens if an application has an opened file on the
> cdrom, or even worse if a file is loop mounted from the cdrom. 
> 
> In my opinion this can cause very nasty consequences (lock-ups)

It won't cause lock-ups except for the process in question - not for the system anyway. And properly written apps can deal with it, it's just IO errors.

FWIW, what happens today is that if the underlying device goes away, some part of userspace will lazy unmount the device. Ideally the kernel would force unmount filesystems (causing ENXIO (for fds) and SIGBUS (for mapped files) on IO), tear down RAID, LVM, device-mapper etc. but that is not how things currently works. Instead you get stale mount points, broken symlinks in sysfs and so on. My view is that it's a kernel bug, others may agree or disagree. We may end up fixing the kernel, we may not.

BTW, this is not very different from yanking a USB device or pulling out a CF card. And we deal relatively well with this already.
Comment 5 maximlevitsky 2009-09-29 10:17:19 UTC
It makes sense. In fact today USB devices are much more common.

Comment 6 Martin Pitt 2009-09-29 10:56:45 UTC
Thanks David. So should we close this as "wontfix" then?
Comment 7 David Zeuthen (not reading bugmail) 2009-09-29 11:20:05 UTC
(In reply to comment #6)
> Thanks David. So should we close this as "wontfix" then?

Sounds good to me.
Comment 8 David Zeuthen (not reading bugmail) 2009-09-29 11:28:51 UTC
Kay, what do you think about changing the defaults in the kernel so distros won't have to do the thing suggested in comment 2 at boot time?
Comment 9 Kay Sievers 2009-10-05 11:46:14 UTC
(In reply to comment #8)
> Kay, what do you think about changing the defaults in the kernel so distros
> won't have to do the thing suggested in comment 2 at boot time?

I think the kernel defaults should lock all used drives by default, and should not be touched at bootup, and thhings like liveCD rootfs and similar setups should not be unlocked by a global default setting.

I think we might want to unlock individual drives though, if our own services have applied the policy and mounted a media. I expect:
  ioctl(fd, CDROM_LOCKDOOR, 0);
would unlock only the single drive we are currently handling, and would not touch other drives, or drives which have media which is mounted manually or by a system-wide configuration.
Comment 10 David Zeuthen (not reading bugmail) 2009-10-05 11:58:29 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > Kay, what do you think about changing the defaults in the kernel so distros
> > won't have to do the thing suggested in comment 2 at boot time?
> 
> I think the kernel defaults should lock all used drives by default, and should
> not be touched at bootup, and thhings like liveCD rootfs and similar setups
> should not be unlocked by a global default setting.
> 
> I think we might want to unlock individual drives though, if our own services
> have applied the policy and mounted a media. I expect:
>   ioctl(fd, CDROM_LOCKDOOR, 0);
> would unlock only the single drive we are currently handling, and would not
> touch other drives, or drives which have media which is mounted manually or by
> a system-wide configuration.

Sounds good to me.

Comment 11 Martin Pitt 2009-10-06 00:12:48 UTC
Sounds good to me as well.
Comment 12 Martin Pitt 2009-10-06 01:54:45 UTC
Created attachment 30106 [details] [review]
unlock CD trays after mounting

Tested patch. This hooks into the "mount completed" callback, and sends the ioctl for CD drives. If the mount failed, or we didn't mount anything, we should not mess with the drive, and don't need to either (since only the mounting actually locks the drive).

Thanks in advance for reviewing!
Comment 13 Martin Pitt 2009-10-06 01:56:26 UTC
Re-subscribing David, it seems my patch comment wasn't mailed to him.
Comment 14 Martin Pitt 2009-10-06 04:13:56 UTC
Created attachment 30108 [details] [review]
unlock CD trays after mounting

Now with fixed whitespace
Comment 15 David Zeuthen (not reading bugmail) 2009-10-09 06:38:47 UTC
(In reply to comment #14)
> Created an attachment (id=30108) [details]
> unlock CD trays after mounting
> 
> Now with fixed whitespace
> 

Looks good - a couple of notes/questions.

I'm not happy about poking the device directly from the main thread of the daemon to avoid lockups/blocking io. Right now we don't do any IO at all from the daemon - all IO is done from either programs started from udev rules or from separate job processes.

Long term, I want to be able to do IO from separate threads inside the daemon (instead of launching helper processes). But that requires reworking the internals - we should have an abstract Job class with ThreadedJob (to run code in a separate thread) and ProcessJob (to run code in a helper binary - just like most jobs today).

Since this particular case is fairly safe, let's just do it here and then we can move it to a threaded job once the internals has been reworked.

I'm curious what happens when you unmount/eject the disc, turn off automounting, insert a new disc, then mount it manually with mount(8). Is the door the locked?
Comment 16 David Zeuthen (not reading bugmail) 2009-10-09 06:47:30 UTC
(In reply to comment #15)
> I'm curious what happens when you unmount/eject the disc, turn off
> automounting, insert a new disc, then mount it manually with mount(8). Is the
> door the locked?

Just tested this: the door is locked. 

I've committed the patch with a rewritten subject line "Bug 24052 – CDROM eject button is locked while CDROM is mounted". Thanks for fixing this!

(In the future, to make things easier to find, please use the "Bug <Bug Number> – <Bug Title>" format for patches that fixes bugs filed in bz - note the use of the wide hyphen too!)
Comment 17 Martin Pitt 2009-10-09 06:50:57 UTC
(In reply to comment #15)

> I'm curious what happens when you unmount/eject the disc, turn off
> automounting, insert a new disc, then mount it manually with mount(8). Is the
> door the locked?

Right, I forgot to mention this previously, but I tested it when writing the
patch. It seems that mount() re-locks the tray, so unlike
/proc/sys/dev/cdrom/lock
the ioctl isn't "sticky" and needs to be re-issued after mounting.
Comment 18 Martin Pitt 2009-10-09 06:52:57 UTC
> (In the future, to make things easier to find, please use the "Bug <Bug Number>
– <Bug Title>" format for patches that fixes bugs filed in bz - note the use
of the wide hyphen too!)

Ah, will do. Thanks for applying!