Re: [PATCH v4 rebase 00/10] Fix cdrom autoclose

[Michal Suchánek]

On Wed, Nov 27, 2019 at 09:11:44AM +0100, Michal Suchánek wrote:
> On Tue, Nov 26, 2019 at 04:13:32PM -0700, Jens Axboe wrote:
> > On 11/26/19 1:21 PM, Michal Suchánek wrote:
> > > On Tue, Nov 26, 2019 at 01:01:42PM -0700, Jens Axboe wrote:
> > >> On 11/26/19 12:54 PM, 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 rebase on top of current master.
> > >>>
> > >>> Also it seems that most people think that this is fix for WMware because
> > >>> there is one patch dealing with WMware.
> > >>
> > >> I think the main complaint with this is that it's kind of a stretch to
> > >> add core functionality for a device type that's barely being
> > >> manufactured anymore and is mostly used in a virtualized fashion. I

That optical drives are hardly manufactured is kind of a stretch. I have
no problem obtaining drives from a few manufacturers in any nearby
computer store. While using DVDs may be slowly getting out of fashion
the same applies to all optical drives, including Blueray. Some of these
will stay for forseeable future.

> > >> think it you could fix this without 10 patches of churn and without
> > >> adding a new ->open() addition to fops, then people would be a lot more
> > >> receptive to the idea of improving cdrom auto-close.
> > > 
> > > I see no way to do that cleanly.
> > > 
> > > There are two open modes for cdrom devices - blocking and
> > > non-blocking.
> > > 
> > > In blocking mode open() should analyze the medium so that it's ready
> > > when it returns. In non-blocking mode it should return immediately so
> > > long as you can talk to the device.
> > > 
> > > When waiting in open() with locks held the processes trying to open
> > > the device are locked out regradless of the mode they use.
> > > 
> > > The only way to solve this is to pretend that the device is open and
> > > do the wait afterwards with the device unlocked.
> > 
> > How is this any different from an open on a file that needs to bring in
> > meta data on a busy rotating device, which can also take seconds?
> 
> First, accessing a file will take seconds only when your system is
> seriously overloaded or misconfigured. The access time for rotational
> storage is tens of milliseconds. With cdrom the access time after
> closing the door is measured in tens of seconds on common hardware. It
> can be shorter but also possibly longer. I am not aware of any limit
> there. It may be reasonable to want to get device status during this
> time.
> 
> Second, fetching the metadata for the file does not block operations that
> don't need the metadata. Here waiting for the drive to get ready blocks
> all access. You could get drive status if you did not try to open it
> but once you do you can no longer talk to it.

So let's look at the alternatives. One proposed alternative was to
change the locking calls to the locks that are held while waiting in
open() to interuptible so that impatient users can at least kill
processes waiting on their CD medium to become ready.

What is held are sr_mutex and bd_mutex.

bd_mutex is per_device so any open() or close() on the same CD-ROM
device is blocked. There are a number of other sites where bd_mutex is
locked and it will be needed to figure out which of these can be called
with a cd-rom device and change them to killable so that processes
waiting on the lock to don't get uninterriptibly stuck. This may be more
code churn than this patchset. I think we can exclude loop.c and
zram-dev.c but ioctl.c, xen-blkfront.c, and block_dev.c apply. Don't
know about dasd.

The bd_mutex is held in iterate_bdevs so all bets are off wrt being able
to operate the system.  Once a process is stuck waiting in blkdev_get()
which calls open() on the cdrom you cannot iterate block devices. With
boot times measured in seconds and medium analysis times measured in
tens of seconds users will not be amused.

autoclose defaults to on, and blkid reads deviced in blocking mode
causing all the fun stuff to trigger (patch pending to change that).
Nonetheless any number of utilities still not aware of this nonblock
quirk out there will try to open the device sooner or later blocking all
operations that require iterating the list of block devices.

Adding tens of seconds to block device opening time (which I assume
might need iterating list of block devices) might even overflow some
systemd timeout and fail boot.  There is timeout for each particular job
but there are also cumulative timeouts for something like 'locate device
with this UUID' which are fixed regardles of the number of layers (LVM,
md, ..) involved.

The other approach is to do like harddisks. With a harddisk a medium is
'fixed' - that is assumed to be present always. Any error accessing the
medium is reported on read() or write() and not necessarily on open().
This would require hooking the autoclose to the operations that require
actual medium - probably something like count_tracks(), and eschew
calling these from open(). That would work but breaks the contract
described in the current API documentation - that is if you don't open
with O_NONBLOCK and there is obvious medium error like no medium at all
or no usable track you get the error on open() rather than on whatever
opration that tries to use the track.

Thanks

Michal