[v4,00/10] Fix cdrom autoclose

Message

Michal Suchánek Nov. 21, 2019, 5:13 p.m. UTC

Hello,

there is cdrom autoclose feature that is supposed to close the tray,
wait for the disc to become ready, and then open the device.

This used to work in ancient times. Then in old times there was a hack
in util-linux which worked around the breakage which probably resulted
from switching to scsi emulation.

Currently util-linux maintainer refuses to merge another hack on the
basis that kernel still has the feature so it should be fixed there.
The code needs not be replicated in every userspace utility like mount
or dd which has no business knowing which devices are CD-roms and where
the autoclose setting is in the kernel.

v3:
- change the VMware workaround to use blacklist flag
- use exported function instead of ioctl
v4:
- fix crash reported by kernel test robot
- fix the debug message logic while refactoring cdrom_open
- move repeated code out of __blkdev_get

Link: https://lore.kernel.org/lkml/[e-mail]/
Link: https://lore.kernel.org/lkml/[e-mail]/

Michal Suchanek (10):
  cdrom: add poll_event_interruptible
  cdrom: factor out common open_for_* code
  cdrom: wait for the tray to close
  cdrom: export autoclose logic as a separate function
  cdrom: unify log messages.
  bdev: reset first_open when looping in __blkget_dev
  bdev: separate parts of __blkdev_get as helper functions
  bdev: add open_finish
  scsi: blacklist: add VMware ESXi cdrom - broken tray emulation
  scsi: sr: wait for the medium to become ready

 Documentation/filesystems/locking.rst |   2 +
 drivers/cdrom/cdrom.c                 | 471 +++++++++++++-------------
 drivers/scsi/scsi_devinfo.c           |  15 +-
 drivers/scsi/sr.c                     |  60 +++-
 fs/block_dev.c                        |  80 +++--
 include/linux/blkdev.h                |   1 +
 include/linux/cdrom.h                 |   1 +
 include/scsi/scsi_devinfo.h           |   7 +-
 8 files changed, 360 insertions(+), 277 deletions(-)

Comments

Jens Axboe Nov. 21, 2019, 7 p.m. UTC | #1

On 11/21/19 10:13 AM, Michal Suchanek wrote:
> Hello,
> 
> there is cdrom autoclose feature that is supposed to close the tray,
> wait for the disc to become ready, and then open the device.
> 
> This used to work in ancient times. Then in old times there was a hack
> in util-linux which worked around the breakage which probably resulted
> from switching to scsi emulation.
> 
> Currently util-linux maintainer refuses to merge another hack on the
> basis that kernel still has the feature so it should be fixed there.
> The code needs not be replicated in every userspace utility like mount
> or dd which has no business knowing which devices are CD-roms and where
> the autoclose setting is in the kernel.

This is a lot of code/churn (and even an fops addition...) to work around
a broken hw emulation, essentially. Why aren't we just pushing vmware
to fix this?

Michal Suchánek Nov. 21, 2019, 11:43 p.m. UTC | #2

On Thu, Nov 21, 2019 at 12:00:03PM -0700, Jens Axboe wrote:
> On 11/21/19 10:13 AM, Michal Suchanek wrote:
> > Hello,
> > 
> > there is cdrom autoclose feature that is supposed to close the tray,
> > wait for the disc to become ready, and then open the device.
> > 
> > This used to work in ancient times. Then in old times there was a hack
> > in util-linux which worked around the breakage which probably resulted
> > from switching to scsi emulation.
> > 
> > Currently util-linux maintainer refuses to merge another hack on the
> > basis that kernel still has the feature so it should be fixed there.
> > The code needs not be replicated in every userspace utility like mount
> > or dd which has no business knowing which devices are CD-roms and where
> > the autoclose setting is in the kernel.
> 
> This is a lot of code/churn (and even an fops addition...) to work around
> a broken hw emulation, essentially. Why aren't we just pushing vmware
> to fix this?

This is fix for kernel feature: cdrom autoclose.

There is one patch that deals with blacklisting the feature on VMWare
becase their emulation is broken and triggers an issue with the feature
when it actually works.

Thanks

Michal