Link: https://lore.kernel.org/lkml/[email protected]/
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.
Michal Suchanek (8):
cdrom: add poll_event_interruptible
cdrom: factor out common open_for_* code
cdrom: wait for the tray to close
cdrom: separate autoclose into an IOCTL
docs: cdrom: Add autoclose IOCTL
bdev: add open_finish.
scsi: sr: workaround VMware ESXi cdrom emulation bug
scsi: sr: wait for the medium to become ready
Documentation/filesystems/locking.rst | 2 +
Documentation/ioctl/cdrom.rst | 25 ++++
drivers/cdrom/cdrom.c | 188 ++++++++++++++------------
drivers/scsi/sr.c | 60 ++++++--
fs/block_dev.c | 21 ++-
include/linux/blkdev.h | 1 +
include/uapi/linux/cdrom.h | 1 +
7 files changed, 198 insertions(+), 100 deletions(-)
--
2.23.0
The open_for_audio and open_for_data copies are bitrotten in different
ways already and will need to update the autoclose logic in both.
Signed-off-by: Michal Suchanek <[email protected]>
---
drivers/cdrom/cdrom.c | 96 +++++++++++++++----------------------------
1 file changed, 34 insertions(+), 62 deletions(-)
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 2ad6b73482fe..f504ca70f93f 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -1046,12 +1046,12 @@ static void cdrom_count_tracks(struct cdrom_device_info *cdi, tracktype *tracks)
}
static
-int open_for_data(struct cdrom_device_info *cdi)
+int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
{
int ret;
const struct cdrom_device_ops *cdo = cdi->ops;
- tracktype tracks;
- cd_dbg(CD_OPEN, "entering open_for_data\n");
+
+ cd_dbg(CD_OPEN, "entering open_for_common\n");
/* Check if the driver can report drive status. If it can, we
can do clever things. If it can't, well, we at least tried! */
if (cdo->drive_status != NULL) {
@@ -1071,37 +1071,45 @@ int open_for_data(struct cdrom_device_info *cdi)
couldn't close the tray. We only care
that there is no disc in the drive,
since that is the _REAL_ problem here.*/
- ret=-ENOMEDIUM;
- goto clean_up_and_return;
+ return -ENOMEDIUM;
}
} else {
cd_dbg(CD_OPEN, "bummer. this drive can't close the tray.\n");
- ret=-ENOMEDIUM;
- goto clean_up_and_return;
+ return -ENOMEDIUM;
}
/* Ok, the door should be closed now.. Check again */
ret = cdo->drive_status(cdi, CDSL_CURRENT);
if ((ret == CDS_NO_DISC) || (ret==CDS_TRAY_OPEN)) {
cd_dbg(CD_OPEN, "bummer. the tray is still not closed.\n");
cd_dbg(CD_OPEN, "tray might not contain a medium\n");
- ret=-ENOMEDIUM;
- goto clean_up_and_return;
+ return -ENOMEDIUM;
}
cd_dbg(CD_OPEN, "the tray is now closed\n");
}
- /* the door should be closed now, check for the disc */
- ret = cdo->drive_status(cdi, CDSL_CURRENT);
- if (ret!=CDS_DISC_OK) {
- ret = -ENOMEDIUM;
- goto clean_up_and_return;
- }
+ if (ret != CDS_DISC_OK)
+ return -ENOMEDIUM;
}
- cdrom_count_tracks(cdi, &tracks);
- if (tracks.error == CDS_NO_DISC) {
+ cdrom_count_tracks(cdi, tracks);
+ if (tracks->error == CDS_NO_DISC) {
cd_dbg(CD_OPEN, "bummer. no disc.\n");
- ret=-ENOMEDIUM;
- goto clean_up_and_return;
+ return -ENOMEDIUM;
}
+
+ return 0;
+}
+
+static
+int open_for_data(struct cdrom_device_info *cdi)
+{
+ int ret;
+ const struct cdrom_device_ops *cdo = cdi->ops;
+ tracktype tracks;
+
+ cd_dbg(CD_OPEN, "entering open_for_data\n");
+ ret = open_for_common(cdi, &tracks);
+ if (ret)
+ goto clean_up_and_return;
+
/* CD-Players which don't use O_NONBLOCK, workman
* for example, need bit CDO_CHECK_TYPE cleared! */
if (tracks.data==0) {
@@ -1208,53 +1216,17 @@ int cdrom_open(struct cdrom_device_info *cdi, struct block_device *bdev,
/* This code is similar to that in open_for_data. The routine is called
whenever an audio play operation is requested.
*/
-static int check_for_audio_disc(struct cdrom_device_info *cdi,
- const struct cdrom_device_ops *cdo)
+static int check_for_audio_disc(struct cdrom_device_info *cdi)
{
int ret;
tracktype tracks;
cd_dbg(CD_OPEN, "entering check_for_audio_disc\n");
if (!(cdi->options & CDO_CHECK_TYPE))
return 0;
- if (cdo->drive_status != NULL) {
- ret = cdo->drive_status(cdi, CDSL_CURRENT);
- cd_dbg(CD_OPEN, "drive_status=%d\n", ret);
- if (ret == CDS_TRAY_OPEN) {
- cd_dbg(CD_OPEN, "the tray is open...\n");
- /* can/may i close it? */
- if (CDROM_CAN(CDC_CLOSE_TRAY) &&
- cdi->options & CDO_AUTO_CLOSE) {
- cd_dbg(CD_OPEN, "trying to close the tray\n");
- ret=cdo->tray_move(cdi,0);
- if (ret) {
- cd_dbg(CD_OPEN, "bummer. tried to close tray but failed.\n");
- /* Ignore the error from the low
- level driver. We don't care why it
- couldn't close the tray. We only care
- that there is no disc in the drive,
- since that is the _REAL_ problem here.*/
- return -ENOMEDIUM;
- }
- } else {
- cd_dbg(CD_OPEN, "bummer. this driver can't close the tray.\n");
- return -ENOMEDIUM;
- }
- /* Ok, the door should be closed now.. Check again */
- ret = cdo->drive_status(cdi, CDSL_CURRENT);
- if ((ret == CDS_NO_DISC) || (ret==CDS_TRAY_OPEN)) {
- cd_dbg(CD_OPEN, "bummer. the tray is still not closed.\n");
- return -ENOMEDIUM;
- }
- if (ret!=CDS_DISC_OK) {
- cd_dbg(CD_OPEN, "bummer. disc isn't ready.\n");
- return -EIO;
- }
- cd_dbg(CD_OPEN, "the tray is now closed\n");
- }
- }
- cdrom_count_tracks(cdi, &tracks);
- if (tracks.error)
- return(tracks.error);
+
+ ret = open_for_common(cdi, &tracks);
+ if (ret)
+ return ret;
if (tracks.audio==0)
return -EMEDIUMTYPE;
@@ -2725,7 +2697,7 @@ static int cdrom_ioctl_play_trkind(struct cdrom_device_info *cdi,
if (copy_from_user(&ti, argp, sizeof(ti)))
return -EFAULT;
- ret = check_for_audio_disc(cdi, cdi->ops);
+ ret = check_for_audio_disc(cdi);
if (ret)
return ret;
return cdi->ops->audio_ioctl(cdi, CDROMPLAYTRKIND, &ti);
@@ -2773,7 +2745,7 @@ static int cdrom_ioctl_audioctl(struct cdrom_device_info *cdi,
if (!CDROM_CAN(CDC_PLAY_AUDIO))
return -ENOSYS;
- ret = check_for_audio_disc(cdi, cdi->ops);
+ ret = check_for_audio_disc(cdi);
if (ret)
return ret;
return cdi->ops->audio_ioctl(cdi, cmd, NULL);
--
2.23.0
The scsi command to close the tray only starts the motor and does not
wait for the tray to close. Wait until the state chages from TRAY_OPEN
so users do not race with the tray closing.
This looks like inifinte wait but unless the drive is broken it either
closes the tray shortly or reports an error when it detects the tray is
blocked. At worst the wait can be interrupted by the user.
Also wait for the drive to become ready once the tray closes.
Signed-off-by: Michal Suchanek <[email protected]>
---
v2:
- check drive_status exists before using it
- rename tray_close -> cdrom_tray_close
- also wait for drive to become ready after tray closes
- do not wait in cdrom_ioctl_closetray
---
drivers/cdrom/cdrom.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index f504ca70f93f..c0fc9a02b70c 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -1045,6 +1045,18 @@ static void cdrom_count_tracks(struct cdrom_device_info *cdi, tracktype *tracks)
tracks->cdi, tracks->xa);
}
+static int cdrom_tray_close(struct cdrom_device_info *cdi)
+{
+ int ret;
+
+ ret = cdi->ops->tray_move(cdi, 0);
+ if (ret || !cdi->ops->drive_status)
+ return ret;
+
+ return poll_event_interruptible(CDS_TRAY_OPEN !=
+ cdi->ops->drive_status(cdi, CDSL_CURRENT), 500);
+}
+
static
int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
{
@@ -1063,7 +1075,9 @@ int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
if (CDROM_CAN(CDC_CLOSE_TRAY) &&
cdi->options & CDO_AUTO_CLOSE) {
cd_dbg(CD_OPEN, "trying to close the tray\n");
- ret=cdo->tray_move(cdi,0);
+ ret = cdrom_tray_close(cdi);
+ if (ret == -ERESTARTSYS)
+ return ret;
if (ret) {
cd_dbg(CD_OPEN, "bummer. tried to close the tray but failed.\n");
/* Ignore the error from the low
@@ -1086,6 +1100,15 @@ int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
}
cd_dbg(CD_OPEN, "the tray is now closed\n");
}
+ /* the door should be closed now, check for the disc */
+ if (ret == CDS_DRIVE_NOT_READY) {
+ int poll_res = poll_event_interruptible(
+ CDS_DRIVE_NOT_READY !=
+ (ret = cdo->drive_status(cdi, CDSL_CURRENT)),
+ 500);
+ if (poll_res == -ERESTARTSYS)
+ return poll_res;
+ }
if (ret != CDS_DISC_OK)
return -ENOMEDIUM;
}
--
2.23.0
Add convenience macro for polling an event that does not have a
waitqueue.
Signed-off-by: Michal Suchanek <[email protected]>
---
drivers/cdrom/cdrom.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index ac42ae4651ce..2ad6b73482fe 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -282,10 +282,24 @@
#include <linux/fcntl.h>
#include <linux/blkdev.h>
#include <linux/times.h>
+#include <linux/delay.h>
#include <linux/uaccess.h>
+#include <linux/sched/signal.h>
#include <scsi/scsi_common.h>
#include <scsi/scsi_request.h>
+#define poll_event_interruptible(event, interval) ({ \
+ int ret = 0; \
+ while (!(event)) { \
+ if (signal_pending(current)) { \
+ ret = -ERESTARTSYS; \
+ break; \
+ } \
+ msleep_interruptible(interval); \
+ } \
+ ret; \
+})
+
/* used to tell the module to turn on full debugging messages */
static bool debug;
/* default compatibility mode */
--
2.23.0
This allows the sr driver to call it separately without blocking other
processes accessing the device. This solves a issue with process waiting
in open() on broken drive to close the tray blocking out all access to
the device, including nonblocking access to determine drive status.
Signed-off-by: Michal Suchanek <[email protected]>
---
drivers/cdrom/cdrom.c | 83 +++++++++++++++++++++-----------------
include/uapi/linux/cdrom.h | 1 +
2 files changed, 48 insertions(+), 36 deletions(-)
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index c0fc9a02b70c..bd8813b5afdb 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -1071,46 +1071,16 @@ int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
cd_dbg(CD_OPEN, "drive_status=%d\n", ret);
if (ret == CDS_TRAY_OPEN) {
cd_dbg(CD_OPEN, "the tray is open...\n");
- /* can/may i close it? */
- if (CDROM_CAN(CDC_CLOSE_TRAY) &&
- cdi->options & CDO_AUTO_CLOSE) {
- cd_dbg(CD_OPEN, "trying to close the tray\n");
- ret = cdrom_tray_close(cdi);
- if (ret == -ERESTARTSYS)
- return ret;
- if (ret) {
- cd_dbg(CD_OPEN, "bummer. tried to close the tray but failed.\n");
- /* Ignore the error from the low
- level driver. We don't care why it
- couldn't close the tray. We only care
- that there is no disc in the drive,
- since that is the _REAL_ problem here.*/
- return -ENOMEDIUM;
- }
- } else {
- cd_dbg(CD_OPEN, "bummer. this drive can't close the tray.\n");
- return -ENOMEDIUM;
- }
- /* Ok, the door should be closed now.. Check again */
- ret = cdo->drive_status(cdi, CDSL_CURRENT);
- if ((ret == CDS_NO_DISC) || (ret==CDS_TRAY_OPEN)) {
- cd_dbg(CD_OPEN, "bummer. the tray is still not closed.\n");
- cd_dbg(CD_OPEN, "tray might not contain a medium\n");
- return -ENOMEDIUM;
- }
- cd_dbg(CD_OPEN, "the tray is now closed\n");
+ return -ENOMEDIUM;
}
- /* the door should be closed now, check for the disc */
if (ret == CDS_DRIVE_NOT_READY) {
- int poll_res = poll_event_interruptible(
- CDS_DRIVE_NOT_READY !=
- (ret = cdo->drive_status(cdi, CDSL_CURRENT)),
- 500);
- if (poll_res == -ERESTARTSYS)
- return poll_res;
+ cd_dbg(CD_OPEN, "the drive is not ready...\n");
+ return -ENOMEDIUM;
}
- if (ret != CDS_DISC_OK)
+ if (ret != CDS_DISC_OK) {
+ cd_dbg(CD_OPEN, "drive returned status %i...\n", ret);
return -ENOMEDIUM;
+ }
}
cdrom_count_tracks(cdi, tracks);
if (tracks->error == CDS_NO_DISC) {
@@ -2353,6 +2323,45 @@ static int cdrom_ioctl_closetray(struct cdrom_device_info *cdi)
return cdi->ops->tray_move(cdi, 0);
}
+static int cdrom_ioctl_autoclose(struct cdrom_device_info *cdi)
+{
+ const struct cdrom_device_ops *cdo = cdi->ops;
+ int ret;
+
+ if (!cdo->drive_status)
+ return -ENXIO;
+
+ ret = cdo->drive_status(cdi, CDSL_CURRENT);
+
+ if ((ret == CDS_TRAY_OPEN) && CDROM_CAN(CDC_CLOSE_TRAY) &&
+ (cdi->options & CDO_AUTO_CLOSE)) {
+ cd_dbg(CD_DO_IOCTL, "trying to close the tray...\n");
+ ret = cdrom_tray_close(cdi);
+ if (ret == -ERESTARTSYS)
+ return ret;
+ if (ret) {
+ cd_dbg(CD_DO_IOCTL, "bummer. tried to close the tray but failed.\n");
+ return -ENOMEDIUM;
+ ret = cdo->drive_status(cdi, CDSL_CURRENT);
+ }
+ ret = cdo->drive_status(cdi, CDSL_CURRENT);
+ }
+
+ if (ret == CDS_DRIVE_NOT_READY) {
+ int poll_res;
+
+ cd_dbg(CD_DO_IOCTL, "waiting for drive to become ready...\n");
+ poll_res = poll_event_interruptible(CDS_DRIVE_NOT_READY !=
+ (ret = cdo->drive_status(cdi, CDSL_CURRENT)), 50);
+ if (poll_res == -ERESTARTSYS)
+ return poll_res;
+ }
+ if (ret != CDS_DISC_OK)
+ return -ENOMEDIUM;
+
+ return 0;
+}
+
static int cdrom_ioctl_eject_sw(struct cdrom_device_info *cdi,
unsigned long arg)
{
@@ -3365,6 +3374,8 @@ int cdrom_ioctl(struct cdrom_device_info *cdi, struct block_device *bdev,
return cdrom_ioctl_debug(cdi, arg);
case CDROM_GET_CAPABILITY:
return cdrom_ioctl_get_capability(cdi);
+ case CDROM_AUTOCLOSE:
+ return cdrom_ioctl_autoclose(cdi);
case CDROM_GET_MCN:
return cdrom_ioctl_get_mcn(cdi, argp);
case CDROM_DRIVE_STATUS:
diff --git a/include/uapi/linux/cdrom.h b/include/uapi/linux/cdrom.h
index 2817230148fd..6493d8c593ee 100644
--- a/include/uapi/linux/cdrom.h
+++ b/include/uapi/linux/cdrom.h
@@ -129,6 +129,7 @@
#define CDROM_LOCKDOOR 0x5329 /* lock or unlock door */
#define CDROM_DEBUG 0x5330 /* Turn debug messages on/off */
#define CDROM_GET_CAPABILITY 0x5331 /* get capabilities */
+#define CDROM_AUTOCLOSE 0x5332 /* If autoclose enabled close tray */
/* Note that scsi/scsi_ioctl.h also uses 0x5382 - 0x5386.
* Future CDROM ioctls should be kept below 0x537F
--
2.23.0
This IOCTL will be used internally by the sr driver but there is no
reason to not document it for userspace.
Signed-off-by: Michal Suchanek <[email protected]>
---
Documentation/ioctl/cdrom.rst | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/Documentation/ioctl/cdrom.rst b/Documentation/ioctl/cdrom.rst
index 3b4c0506de46..9372ff1b2b47 100644
--- a/Documentation/ioctl/cdrom.rst
+++ b/Documentation/ioctl/cdrom.rst
@@ -60,6 +60,7 @@ are as follows:
CDROM_LOCKDOOR lock or unlock door
CDROM_DEBUG Turn debug messages on/off
CDROM_GET_CAPABILITY get capabilities
+ CDROM_AUTOCLOSE If autoclose enabled close the tray
CDROMAUDIOBUFSIZ set the audio buffer size
DVD_READ_STRUCT Read structure
DVD_WRITE_STRUCT Write structure
@@ -1056,6 +1057,30 @@ CDROM_GET_CAPABILITY
+CDROM_AUTOCLOSE
+ Close the tray if the device has one, and autoclose is enabled.
+ Wait for drive to become ready.
+
+
+ usage::
+
+ ioctl(fd, CDROM_AUTOCLOSE, 0);
+
+
+ inputs:
+ none
+
+
+ outputs:
+ The ioctl return value is 0 or an error code.
+
+
+ error return:
+ - ENOMEDIUM No medium found or drive error.
+ - ERESTARTSYS Received a signal while waiting for drive to become ready.
+
+
+
CDROMAUDIOBUFSIZ
set the audio buffer size
--
2.23.0
The WMware ESXi cdrom identifies itself as:
sr 0:0:0:0: [sr0] scsi3-mmc drive: vendor: "NECVMWarVMware SATA CD001.00"
model: "VMware SATA CD001.00"
with the following get_capabilities print in sr.c:
sr_printk(KERN_INFO, cd,
"scsi3-mmc drive: vendor: \"%s\" model: \"%s\"\n",
cd->device->vendor, cd->device->model);
So the model looks like reliable identification while vendor does not.
The drive claims to have a tray and claims to be able to close it.
However, the UI has no notion of a tray - when medium is ejected it is
dropped in the floor and the user must select a medium again before the
drive can be re-loaded. On the kernel side the tray_move call to close
the tray succeeds but the drive state does not change as a result of the
call.
The drive does not in fact emulate the tray state. There are two ways to
get the medium state. One is the SCSI status:
Physical drive:
Fixed format, current; Sense key: Not Ready
Additional sense: Medium not present - tray open
Raw sense data (in hex):
70 00 02 00 00 00 00 0a 00 00 00 00 3a 02 00 00
00 00
Fixed format, current; Sense key: Not Ready
Additional sense: Medium not present - tray closed
Raw sense data (in hex):
70 00 02 00 00 00 00 0a 00 00 00 00 3a 01 00 00
00 00
VMware ESXi:
Fixed format, current; Sense key: Not Ready
Additional sense: Medium not present
Info fld=0x0 [0]
Raw sense data (in hex):
f0 00 02 00 00 00 00 0a 00 00 00 00 3a 00 00 00
00 00
So the tray state is not reported here. Other is medium status which the
kernel prefers if available. Adding a print here gives:
cdrom: get_media_event success: code = 0, door_open = 1, medium_present = 0
door_open is interpreted as open tray. This is fine so long as tray_move
would close the tray when requested or report an error which never
happens on VMware ESXi servers (5.5 and 6.5 tested).
This is a popular virtualization platform so a workaround is worthwhile.
Signed-off-by: Michal Suchanek <[email protected]>
---
drivers/scsi/sr.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 4664fdf75c0f..8090c5bdec09 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -867,6 +867,7 @@ static void get_capabilities(struct scsi_cd *cd)
unsigned int ms_len = 128;
int rc, n;
+ static const char *model_vmware = "VMware";
static const char *loadmech[] =
{
"caddy",
@@ -922,6 +923,11 @@ static void get_capabilities(struct scsi_cd *cd)
buffer[n + 4] & 0x20 ? "xa/form2 " : "", /* can read xa/from2 */
buffer[n + 5] & 0x01 ? "cdda " : "", /* can read audio data */
loadmech[buffer[n + 6] >> 5]);
+ if (!strncmp(cd->device->model, model_vmware, strlen(model_vmware))) {
+ buffer[n + 6] &= ~(0xff << 5);
+ sr_printk(KERN_INFO, cd,
+ "VMware ESXi bug workaround: tray -> caddy\n");
+ }
if ((buffer[n + 6] >> 5) == 0)
/* caddy drives can't close tray... */
cd->cdi.mask |= CDC_CLOSE_TRAY;
--
2.23.0
Opening a block device may require a long operation such as waiting for
the cdrom tray to close. Performing this operation with locks held locks
out other attempts to open the device. These processes waiting to open
the device are not killable.
To avoid this issue and still be able to perform time-consuming checks
at open() time the block device driver can provide open_finish(). If it
does opening the device proceeds even when an error is returned from
open(), bd_mutex is released and open_finish() is called. If
open_finish() succeeds the device is now open, if it fails release() is
called.
When -ERESTARTSYS is returned from open() blkdev_get may loop without
calling open_finish(). On -ERESTARTSYS open_finish() is not called.
Move a ret = 0 assignment up in the if/else branching to avoid returning
-ENXIO. Previously the return value was ignored on the unhandled branch.
Signed-off-by: Michal Suchanek <[email protected]>
---
Documentation/filesystems/locking.rst | 2 ++
fs/block_dev.c | 21 +++++++++++++++++----
include/linux/blkdev.h | 1 +
3 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
index fc3a0704553c..2471ced5a8cf 100644
--- a/Documentation/filesystems/locking.rst
+++ b/Documentation/filesystems/locking.rst
@@ -456,6 +456,7 @@ block_device_operations
prototypes::
int (*open) (struct block_device *, fmode_t);
+ int (*open_finish) (struct block_device *, fmode_t, int);
int (*release) (struct gendisk *, fmode_t);
int (*ioctl) (struct block_device *, fmode_t, unsigned, unsigned long);
int (*compat_ioctl) (struct block_device *, fmode_t, unsigned, unsigned long);
@@ -473,6 +474,7 @@ locking rules:
ops bd_mutex
======================= ===================
open: yes
+open_finish: no
release: yes
ioctl: no
compat_ioctl: no
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 9c073dbdc1b0..009b5dedb1f7 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1526,6 +1526,7 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
int partno;
int perm = 0;
bool first_open = false;
+ bool need_finish = false;
if (mode & FMODE_READ)
perm |= MAY_READ;
@@ -1581,6 +1582,8 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
put_disk_and_module(disk);
goto restart;
}
+ if (bdev->bd_disk->fops->open_finish)
+ need_finish = true;
}
if (!ret) {
@@ -1601,7 +1604,7 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
invalidate_partitions(disk, bdev);
}
- if (ret)
+ if (ret && !need_finish)
goto out_clear;
} else {
struct block_device *whole;
@@ -1627,10 +1630,14 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
if (bdev->bd_bdi == &noop_backing_dev_info)
bdev->bd_bdi = bdi_get(disk->queue->backing_dev_info);
} else {
+ ret = 0;
if (bdev->bd_contains == bdev) {
- ret = 0;
- if (bdev->bd_disk->fops->open)
+ if (bdev->bd_disk->fops->open) {
ret = bdev->bd_disk->fops->open(bdev, mode);
+ if ((ret != -ERESTARTSYS) &&
+ bdev->bd_disk->fops->open_finish)
+ need_finish = true;
+ }
/* the same as first opener case, read comment there */
if (bdev->bd_invalidated) {
if (!ret)
@@ -1638,7 +1645,7 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
else if (ret == -ENOMEDIUM)
invalidate_partitions(bdev->bd_disk, bdev);
}
- if (ret)
+ if (ret && !need_finish)
goto out_unlock_bdev;
}
}
@@ -1650,6 +1657,12 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
/* only one opener holds refs to the module and disk */
if (!first_open)
put_disk_and_module(disk);
+ if (ret && need_finish)
+ ret = bdev->bd_disk->fops->open_finish(bdev, mode, ret);
+ if (ret) {
+ __blkdev_put(bdev, mode, for_part);
+ return ret;
+ }
return 0;
out_clear:
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index f3ea78b0c91c..b67e93c6afb7 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1695,6 +1695,7 @@ static inline struct bio_vec *rq_integrity_vec(struct request *rq)
struct block_device_operations {
int (*open) (struct block_device *, fmode_t);
+ int (*open_finish)(struct block_device *bdev, fmode_t mode, int ret);
void (*release) (struct gendisk *, fmode_t);
int (*rw_page)(struct block_device *, sector_t, struct page *, unsigned int);
int (*ioctl) (struct block_device *, fmode_t, unsigned, unsigned long);
--
2.23.0
Use the autoclose IOCLT provided by cdrom driver to wait for drive to
close in open_finish, and attempt to open once more after the door
closes.
Signed-off-by: Michal Suchanek <[email protected]>
---
drivers/scsi/sr.c | 54 ++++++++++++++++++++++++++++++++++++-----------
1 file changed, 42 insertions(+), 12 deletions(-)
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 8090c5bdec09..34d9a818b9e0 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -521,29 +521,58 @@ static blk_status_t sr_init_command(struct scsi_cmnd *SCpnt)
return ret;
}
-static int sr_block_open(struct block_device *bdev, fmode_t mode)
+static int __sr_block_open(struct block_device *bdev, fmode_t mode)
{
- struct scsi_cd *cd;
- struct scsi_device *sdev;
- int ret = -ENXIO;
-
- cd = scsi_cd_get(bdev->bd_disk);
- if (!cd)
- goto out;
+ struct scsi_cd *cd = scsi_cd(bdev->bd_disk);
+ int ret;
- sdev = cd->device;
- scsi_autopm_get_device(sdev);
check_disk_change(bdev);
mutex_lock(&sr_mutex);
ret = cdrom_open(&cd->cdi, bdev, mode);
mutex_unlock(&sr_mutex);
+ return ret;
+}
+
+static int sr_block_open(struct block_device *bdev, fmode_t mode)
+{
+ struct scsi_cd *cd = scsi_cd_get(bdev->bd_disk);
+ struct scsi_device *sdev;
+ int ret;
+
+ if (!cd)
+ return -ENXIO;
+
+ sdev = cd->device;
+ scsi_autopm_get_device(sdev);
+ ret = __sr_block_open(bdev, mode);
scsi_autopm_put_device(sdev);
- if (ret)
+
+ if (ret == -ERESTARTSYS)
scsi_cd_put(cd);
-out:
+ return ret;
+}
+
+static int sr_block_open_finish(struct block_device *bdev, fmode_t mode,
+ int ret)
+{
+ struct scsi_cd *cd = scsi_cd(bdev->bd_disk);
+
+ /* wait for drive to get ready */
+ if ((ret == -ENOMEDIUM) && !(mode & FMODE_NDELAY)) {
+ struct scsi_device *sdev = cd->device;
+ /*
+ * Cannot use sr_block_ioctl because it locks sr_mutex blocking
+ * out any processes trying to access the drive
+ */
+ scsi_autopm_get_device(sdev);
+ cdrom_ioctl(&cd->cdi, bdev, mode, CDROM_AUTOCLOSE, 0);
+ ret = __sr_block_open(bdev, mode);
+ scsi_autopm_put_device(sdev);
+ }
+
return ret;
}
@@ -639,6 +668,7 @@ static const struct block_device_operations sr_bdops =
{
.owner = THIS_MODULE,
.open = sr_block_open,
+ .open_finish = sr_block_open_finish,
.release = sr_block_release,
.ioctl = sr_block_ioctl,
.check_events = sr_block_check_events,
--
2.23.0
On 10/23/19 2:52 PM, Michal Suchanek wrote:
> The WMware ESXi cdrom identifies itself as:
> sr 0:0:0:0: [sr0] scsi3-mmc drive: vendor: "NECVMWarVMware SATA CD001.00"
> model: "VMware SATA CD001.00"
> with the following get_capabilities print in sr.c:
> sr_printk(KERN_INFO, cd,
> "scsi3-mmc drive: vendor: \"%s\" model: \"%s\"\n",
> cd->device->vendor, cd->device->model);
>
> So the model looks like reliable identification while vendor does not.
>
> The drive claims to have a tray and claims to be able to close it.
> However, the UI has no notion of a tray - when medium is ejected it is
> dropped in the floor and the user must select a medium again before the
> drive can be re-loaded. On the kernel side the tray_move call to close
> the tray succeeds but the drive state does not change as a result of the
> call.
>
> The drive does not in fact emulate the tray state. There are two ways to
> get the medium state. One is the SCSI status:
>
> Physical drive:
>
> Fixed format, current; Sense key: Not Ready
> Additional sense: Medium not present - tray open
> Raw sense data (in hex):
> 70 00 02 00 00 00 00 0a 00 00 00 00 3a 02 00 00
> 00 00
>
> Fixed format, current; Sense key: Not Ready
> Additional sense: Medium not present - tray closed
> Raw sense data (in hex):
> 70 00 02 00 00 00 00 0a 00 00 00 00 3a 01 00 00
> 00 00
>
> VMware ESXi:
>
> Fixed format, current; Sense key: Not Ready
> Additional sense: Medium not present
> Info fld=0x0 [0]
> Raw sense data (in hex):
> f0 00 02 00 00 00 00 0a 00 00 00 00 3a 00 00 00
> 00 00
>
> So the tray state is not reported here. Other is medium status which the
> kernel prefers if available. Adding a print here gives:
>
> cdrom: get_media_event success: code = 0, door_open = 1, medium_present = 0
>
> door_open is interpreted as open tray. This is fine so long as tray_move
> would close the tray when requested or report an error which never
> happens on VMware ESXi servers (5.5 and 6.5 tested).
>
> This is a popular virtualization platform so a workaround is worthwhile.
>
> Signed-off-by: Michal Suchanek <[email protected]>
> ---
> drivers/scsi/sr.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 4664fdf75c0f..8090c5bdec09 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -867,6 +867,7 @@ static void get_capabilities(struct scsi_cd *cd)
> unsigned int ms_len = 128;
> int rc, n;
>
> + static const char *model_vmware = "VMware";
> static const char *loadmech[] =
> {
> "caddy",
> @@ -922,6 +923,11 @@ static void get_capabilities(struct scsi_cd *cd)
> buffer[n + 4] & 0x20 ? "xa/form2 " : "", /* can read xa/from2 */
> buffer[n + 5] & 0x01 ? "cdda " : "", /* can read audio data */
> loadmech[buffer[n + 6] >> 5]);
> + if (!strncmp(cd->device->model, model_vmware, strlen(model_vmware))) {
> + buffer[n + 6] &= ~(0xff << 5);
> + sr_printk(KERN_INFO, cd,
> + "VMware ESXi bug workaround: tray -> caddy\n");
> + }
> if ((buffer[n + 6] >> 5) == 0)
> /* caddy drives can't close tray... */
> cd->cdi.mask |= CDC_CLOSE_TRAY;
>
This looks something which should be handled via a blacklist flag, not
some inline hack which everyone forgets about it...
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer
> static
> -int open_for_data(struct cdrom_device_info *cdi)
> +int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
Please fix the coding style. static never should be on a line of its
own..
> } else {
> cd_dbg(CD_OPEN, "bummer. this drive can't close the tray.\n");
> - ret=-ENOMEDIUM;
> - goto clean_up_and_return;
> + return -ENOMEDIUM;
Can you revert the polarity of the if opening the block before and
return early for the -ENOMEDIUM case to save on leven of indentation?
> if ((ret == CDS_NO_DISC) || (ret==CDS_TRAY_OPEN)) {
If you touch the whole area please remove the inner braces and add the
proper spaces around the second ==.
> +static
> +int open_for_data(struct cdrom_device_info *cdi)
Same issue with the static here.
On Wed, Oct 23, 2019 at 04:13:15PM +0200, Hannes Reinecke wrote:
> On 10/23/19 2:52 PM, Michal Suchanek wrote:
> > The WMware ESXi cdrom identifies itself as:
> > sr 0:0:0:0: [sr0] scsi3-mmc drive: vendor: "NECVMWarVMware SATA CD001.00"
> > model: "VMware SATA CD001.00"
> > with the following get_capabilities print in sr.c:
> > sr_printk(KERN_INFO, cd,
> > "scsi3-mmc drive: vendor: \"%s\" model: \"%s\"\n",
> > cd->device->vendor, cd->device->model);
> >
> > So the model looks like reliable identification while vendor does not.
> >
> > The drive claims to have a tray and claims to be able to close it.
> > However, the UI has no notion of a tray - when medium is ejected it is
> > dropped in the floor and the user must select a medium again before the
> > drive can be re-loaded. On the kernel side the tray_move call to close
> > the tray succeeds but the drive state does not change as a result of the
> > call.
> >
> > The drive does not in fact emulate the tray state. There are two ways to
> > get the medium state. One is the SCSI status:
> >
> > Physical drive:
> >
> > Fixed format, current; Sense key: Not Ready
> > Additional sense: Medium not present - tray open
> > Raw sense data (in hex):
> > 70 00 02 00 00 00 00 0a 00 00 00 00 3a 02 00 00
> > 00 00
> >
> > Fixed format, current; Sense key: Not Ready
> > Additional sense: Medium not present - tray closed
> > Raw sense data (in hex):
> > 70 00 02 00 00 00 00 0a 00 00 00 00 3a 01 00 00
> > 00 00
> >
> > VMware ESXi:
> >
> > Fixed format, current; Sense key: Not Ready
> > Additional sense: Medium not present
> > Info fld=0x0 [0]
> > Raw sense data (in hex):
> > f0 00 02 00 00 00 00 0a 00 00 00 00 3a 00 00 00
> > 00 00
> >
> > So the tray state is not reported here. Other is medium status which the
> > kernel prefers if available. Adding a print here gives:
> >
> > cdrom: get_media_event success: code = 0, door_open = 1, medium_present = 0
> >
> > door_open is interpreted as open tray. This is fine so long as tray_move
> > would close the tray when requested or report an error which never
> > happens on VMware ESXi servers (5.5 and 6.5 tested).
> >
> > This is a popular virtualization platform so a workaround is worthwhile.
> >
> > Signed-off-by: Michal Suchanek <[email protected]>
> > ---
> > drivers/scsi/sr.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> > index 4664fdf75c0f..8090c5bdec09 100644
> > --- a/drivers/scsi/sr.c
> > +++ b/drivers/scsi/sr.c
> > @@ -867,6 +867,7 @@ static void get_capabilities(struct scsi_cd *cd)
> > unsigned int ms_len = 128;
> > int rc, n;
> >
> > + static const char *model_vmware = "VMware";
> > static const char *loadmech[] =
> > {
> > "caddy",
> > @@ -922,6 +923,11 @@ static void get_capabilities(struct scsi_cd *cd)
> > buffer[n + 4] & 0x20 ? "xa/form2 " : "", /* can read xa/from2 */
> > buffer[n + 5] & 0x01 ? "cdda " : "", /* can read audio data */
> > loadmech[buffer[n + 6] >> 5]);
> > + if (!strncmp(cd->device->model, model_vmware, strlen(model_vmware))) {
> > + buffer[n + 6] &= ~(0xff << 5);
> > + sr_printk(KERN_INFO, cd,
> > + "VMware ESXi bug workaround: tray -> caddy\n");
> > + }
> > if ((buffer[n + 6] >> 5) == 0)
> > /* caddy drives can't close tray... */
> > cd->cdi.mask |= CDC_CLOSE_TRAY;
> >
> This looks something which should be handled via a blacklist flag, not
> some inline hack which everyone forgets about it...
AFAIK we used to have a blacklist but don't have anymore. So either it
has to be resurrected for this one flag or an inline hack should be good
enough.
Thanks
Michal
On Wed, 2019-10-23 at 18:23 +0200, Michal Suchánek wrote:
> On Wed, Oct 23, 2019 at 04:13:15PM +0200, Hannes Reinecke wrote:
> > On 10/23/19 2:52 PM, Michal Suchanek wrote:
> > > The WMware ESXi cdrom identifies itself as:
> > > sr 0:0:0:0: [sr0] scsi3-mmc drive: vendor: "NECVMWarVMware SATA CD001.00"
> > > model: "VMware SATA CD001.00"
> > > with the following get_capabilities print in sr.c:
> > > sr_printk(KERN_INFO, cd,
> > > "scsi3-mmc drive: vendor: \"%s\" model: \"%s\"\n",
> > > cd->device->vendor, cd->device->model);
> > >
> > > So the model looks like reliable identification while vendor does not.
> > >
> > > The drive claims to have a tray and claims to be able to close it.
> > > However, the UI has no notion of a tray - when medium is ejected it is
> > > dropped in the floor and the user must select a medium again before the
> > > drive can be re-loaded. On the kernel side the tray_move call to close
> > > the tray succeeds but the drive state does not change as a result of the
> > > call.
> > >
> > > The drive does not in fact emulate the tray state. There are two ways to
> > > get the medium state. One is the SCSI status:
> > >
> > > Physical drive:
> > >
> > > Fixed format, current; Sense key: Not Ready
> > > Additional sense: Medium not present - tray open
> > > Raw sense data (in hex):
> > > 70 00 02 00 00 00 00 0a 00 00 00 00 3a 02 00 00
> > > 00 00
> > >
> > > Fixed format, current; Sense key: Not Ready
> > > Additional sense: Medium not present - tray closed
> > > Raw sense data (in hex):
> > > 70 00 02 00 00 00 00 0a 00 00 00 00 3a 01 00 00
> > > 00 00
> > >
> > > VMware ESXi:
> > >
> > > Fixed format, current; Sense key: Not Ready
> > > Additional sense: Medium not present
> > > Info fld=0x0 [0]
> > > Raw sense data (in hex):
> > > f0 00 02 00 00 00 00 0a 00 00 00 00 3a 00 00 00
> > > 00 00
> > >
> > > So the tray state is not reported here. Other is medium status which the
> > > kernel prefers if available. Adding a print here gives:
> > >
> > > cdrom: get_media_event success: code = 0, door_open = 1, medium_present = 0
> > >
> > > door_open is interpreted as open tray. This is fine so long as tray_move
> > > would close the tray when requested or report an error which never
> > > happens on VMware ESXi servers (5.5 and 6.5 tested).
> > >
> > > This is a popular virtualization platform so a workaround is worthwhile.
> > >
> > > Signed-off-by: Michal Suchanek <[email protected]>
> > > ---
> > > drivers/scsi/sr.c | 6 ++++++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> > > index 4664fdf75c0f..8090c5bdec09 100644
> > > --- a/drivers/scsi/sr.c
> > > +++ b/drivers/scsi/sr.c
> > > @@ -867,6 +867,7 @@ static void get_capabilities(struct scsi_cd *cd)
> > > unsigned int ms_len = 128;
> > > int rc, n;
> > >
> > > + static const char *model_vmware = "VMware";
> > > static const char *loadmech[] =
> > > {
> > > "caddy",
> > > @@ -922,6 +923,11 @@ static void get_capabilities(struct scsi_cd *cd)
> > > buffer[n + 4] & 0x20 ? "xa/form2 " : "", /* can read xa/from2 */
> > > buffer[n + 5] & 0x01 ? "cdda " : "", /* can read audio data */
> > > loadmech[buffer[n + 6] >> 5]);
> > > + if (!strncmp(cd->device->model, model_vmware, strlen(model_vmware))) {
> > > + buffer[n + 6] &= ~(0xff << 5);
> > > + sr_printk(KERN_INFO, cd,
> > > + "VMware ESXi bug workaround: tray -> caddy\n");
> > > + }
> > > if ((buffer[n + 6] >> 5) == 0)
> > > /* caddy drives can't close tray... */
> > > cd->cdi.mask |= CDC_CLOSE_TRAY;
> > >
> >
> > This looks something which should be handled via a blacklist flag, not
> > some inline hack which everyone forgets about it...
>
> AFAIK we used to have a blacklist but don't have anymore. So either it
> has to be resurrected for this one flag or an inline hack should be good
> enough.
>
I agree with Hannes. We should have a blacklist flag for this.
Putting inline code in the sr driver that special cases on a particular
device model string is not clean. The "VMware ESXi bug workaround" message
is not particularly descriptive either.
-Ewan
On Wed, Oct 23, 2019 at 02:52:45PM +0200, Michal Suchanek wrote:
> Opening a block device may require a long operation such as waiting for
> the cdrom tray to close. Performing this operation with locks held locks
> out other attempts to open the device. These processes waiting to open
> the device are not killable.
>
> To avoid this issue and still be able to perform time-consuming checks
> at open() time the block device driver can provide open_finish(). If it
> does opening the device proceeds even when an error is returned from
> open(), bd_mutex is released and open_finish() is called. If
> open_finish() succeeds the device is now open, if it fails release() is
> called.
>
> When -ERESTARTSYS is returned from open() blkdev_get may loop without
> calling open_finish(). On -ERESTARTSYS open_finish() is not called.
>
> Move a ret = 0 assignment up in the if/else branching to avoid returning
> -ENXIO. Previously the return value was ignored on the unhandled branch.
This just doesn't make much sense. There is no point in messing up
the block API for ugly workarounds like that.
On Wed, Oct 23, 2019 at 02:52:46PM +0200, Michal Suchanek wrote:
>
> The drive claims to have a tray and claims to be able to close it.
> However, the UI has no notion of a tray - when medium is ejected it is
> dropped in the floor and the user must select a medium again before the
> drive can be re-loaded. On the kernel side the tray_move call to close
> the tray succeeds but the drive state does not change as a result of the
> call.
>
> The drive does not in fact emulate the tray state. There are two ways to
> get the medium state. One is the SCSI status:
Given that this is a buggy software emulation we should not add more
than 100 lines of kernel code to work around it. Ask VMware to fix
their mess instead.
On Wed, Oct 23, 2019 at 02:52:47PM +0200, Michal Suchanek wrote:
> +static int sr_block_open_finish(struct block_device *bdev, fmode_t mode,
> + int ret)
> +{
> + struct scsi_cd *cd = scsi_cd(bdev->bd_disk);
> +
> + /* wait for drive to get ready */
> + if ((ret == -ENOMEDIUM) && !(mode & FMODE_NDELAY)) {
> + struct scsi_device *sdev = cd->device;
> + /*
> + * Cannot use sr_block_ioctl because it locks sr_mutex blocking
> + * out any processes trying to access the drive
> + */
> + scsi_autopm_get_device(sdev);
> + cdrom_ioctl(&cd->cdi, bdev, mode, CDROM_AUTOCLOSE, 0);
> + ret = __sr_block_open(bdev, mode);
> + scsi_autopm_put_device(sdev);
Ioctls should never be used from kernel space. We have a few leftovers,
but we need to get rid of that and not add more.
On 10/23/19 6:23 PM, Michal Suchánek wrote:
> On Wed, Oct 23, 2019 at 04:13:15PM +0200, Hannes Reinecke wrote:
>> On 10/23/19 2:52 PM, Michal Suchanek wrote:
>>> The WMware ESXi cdrom identifies itself as:
>>> sr 0:0:0:0: [sr0] scsi3-mmc drive: vendor: "NECVMWarVMware SATA CD001.00"
>>> model: "VMware SATA CD001.00"
>>> with the following get_capabilities print in sr.c:
>>> sr_printk(KERN_INFO, cd,
>>> "scsi3-mmc drive: vendor: \"%s\" model: \"%s\"\n",
>>> cd->device->vendor, cd->device->model);
>>>
>>> So the model looks like reliable identification while vendor does not.
>>>
>>> The drive claims to have a tray and claims to be able to close it.
>>> However, the UI has no notion of a tray - when medium is ejected it is
>>> dropped in the floor and the user must select a medium again before the
>>> drive can be re-loaded. On the kernel side the tray_move call to close
>>> the tray succeeds but the drive state does not change as a result of the
>>> call.
>>>
>>> The drive does not in fact emulate the tray state. There are two ways to
>>> get the medium state. One is the SCSI status:
>>>
>>> Physical drive:
>>>
>>> Fixed format, current; Sense key: Not Ready
>>> Additional sense: Medium not present - tray open
>>> Raw sense data (in hex):
>>> 70 00 02 00 00 00 00 0a 00 00 00 00 3a 02 00 00
>>> 00 00
>>>
>>> Fixed format, current; Sense key: Not Ready
>>> Additional sense: Medium not present - tray closed
>>> Raw sense data (in hex):
>>> 70 00 02 00 00 00 00 0a 00 00 00 00 3a 01 00 00
>>> 00 00
>>>
>>> VMware ESXi:
>>>
>>> Fixed format, current; Sense key: Not Ready
>>> Additional sense: Medium not present
>>> Info fld=0x0 [0]
>>> Raw sense data (in hex):
>>> f0 00 02 00 00 00 00 0a 00 00 00 00 3a 00 00 00
>>> 00 00
>>>
>>> So the tray state is not reported here. Other is medium status which the
>>> kernel prefers if available. Adding a print here gives:
>>>
>>> cdrom: get_media_event success: code = 0, door_open = 1, medium_present = 0
>>>
>>> door_open is interpreted as open tray. This is fine so long as tray_move
>>> would close the tray when requested or report an error which never
>>> happens on VMware ESXi servers (5.5 and 6.5 tested).
>>>
>>> This is a popular virtualization platform so a workaround is worthwhile.
>>>
>>> Signed-off-by: Michal Suchanek <[email protected]>
>>> ---
>>> drivers/scsi/sr.c | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
>>> index 4664fdf75c0f..8090c5bdec09 100644
>>> --- a/drivers/scsi/sr.c
>>> +++ b/drivers/scsi/sr.c
>>> @@ -867,6 +867,7 @@ static void get_capabilities(struct scsi_cd *cd)
>>> unsigned int ms_len = 128;
>>> int rc, n;
>>>
>>> + static const char *model_vmware = "VMware";
>>> static const char *loadmech[] =
>>> {
>>> "caddy",
>>> @@ -922,6 +923,11 @@ static void get_capabilities(struct scsi_cd *cd)
>>> buffer[n + 4] & 0x20 ? "xa/form2 " : "", /* can read xa/from2 */
>>> buffer[n + 5] & 0x01 ? "cdda " : "", /* can read audio data */
>>> loadmech[buffer[n + 6] >> 5]);
>>> + if (!strncmp(cd->device->model, model_vmware, strlen(model_vmware))) {
>>> + buffer[n + 6] &= ~(0xff << 5);
>>> + sr_printk(KERN_INFO, cd,
>>> + "VMware ESXi bug workaround: tray -> caddy\n");
>>> + }
>>> if ((buffer[n + 6] >> 5) == 0)
>>> /* caddy drives can't close tray... */
>>> cd->cdi.mask |= CDC_CLOSE_TRAY;
>>>
>> This looks something which should be handled via a blacklist flag, not
>> some inline hack which everyone forgets about it...
>
> AFAIK we used to have a blacklist but don't have anymore. So either it
> has to be resurrected for this one flag or an inline hack should be good
> enough.
>
But we do have one for generic scsi; cf drivers/scsi/scsi_devinfo.c.
And this pretty much falls into the category of SCSI quirks, so I'd
prefer have it hooked into that.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer
On Wed, Oct 23, 2019 at 07:19:58PM -0700, Christoph Hellwig wrote:
> > static
> > -int open_for_data(struct cdrom_device_info *cdi)
> > +int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
>
> Please fix the coding style. static never should be on a line of its
> own..
That's fine.
>
> > } else {
> > cd_dbg(CD_OPEN, "bummer. this drive can't close the tray.\n");
> > - ret=-ENOMEDIUM;
> > - goto clean_up_and_return;
> > + return -ENOMEDIUM;
>
> Can you revert the polarity of the if opening the block before and
> return early for the -ENOMEDIUM case to save on leven of indentation?
Then I will get complaints I do unrelated changes and it's hard to
review. The code gets removed later anyway.
Thanks
Michal
On Wed, Oct 23, 2019 at 07:23:07PM -0700, Christoph Hellwig wrote:
> On Wed, Oct 23, 2019 at 02:52:46PM +0200, Michal Suchanek wrote:
> >
> > The drive claims to have a tray and claims to be able to close it.
> > However, the UI has no notion of a tray - when medium is ejected it is
> > dropped in the floor and the user must select a medium again before the
> > drive can be re-loaded. On the kernel side the tray_move call to close
> > the tray succeeds but the drive state does not change as a result of the
> > call.
> >
> > The drive does not in fact emulate the tray state. There are two ways to
> > get the medium state. One is the SCSI status:
>
> Given that this is a buggy software emulation we should not add more
> than 100 lines of kernel code to work around it. Ask VMware to fix
> their mess instead.
And never hear back from them. Not to mention the installed base of
already buggy servers.
Thanks
Michal
On Wed, Oct 23, 2019 at 07:22:32PM -0700, Christoph Hellwig wrote:
> On Wed, Oct 23, 2019 at 02:52:45PM +0200, Michal Suchanek wrote:
> > Opening a block device may require a long operation such as waiting for
> > the cdrom tray to close. Performing this operation with locks held locks
> > out other attempts to open the device. These processes waiting to open
> > the device are not killable.
> >
> > To avoid this issue and still be able to perform time-consuming checks
> > at open() time the block device driver can provide open_finish(). If it
> > does opening the device proceeds even when an error is returned from
> > open(), bd_mutex is released and open_finish() is called. If
> > open_finish() succeeds the device is now open, if it fails release() is
> > called.
> >
> > When -ERESTARTSYS is returned from open() blkdev_get may loop without
> > calling open_finish(). On -ERESTARTSYS open_finish() is not called.
> >
> > Move a ret = 0 assignment up in the if/else branching to avoid returning
> > -ENXIO. Previously the return value was ignored on the unhandled branch.
>
> This just doesn't make much sense. There is no point in messing up
> the block API for ugly workarounds like that.
If you have ideas how to do this better then share them.
Thanks
Michal
On Wed, Oct 23, 2019 at 07:24:06PM -0700, Christoph Hellwig wrote:
> On Wed, Oct 23, 2019 at 02:52:47PM +0200, Michal Suchanek wrote:
> > +static int sr_block_open_finish(struct block_device *bdev, fmode_t mode,
> > + int ret)
> > +{
> > + struct scsi_cd *cd = scsi_cd(bdev->bd_disk);
> > +
> > + /* wait for drive to get ready */
> > + if ((ret == -ENOMEDIUM) && !(mode & FMODE_NDELAY)) {
> > + struct scsi_device *sdev = cd->device;
> > + /*
> > + * Cannot use sr_block_ioctl because it locks sr_mutex blocking
> > + * out any processes trying to access the drive
> > + */
> > + scsi_autopm_get_device(sdev);
> > + cdrom_ioctl(&cd->cdi, bdev, mode, CDROM_AUTOCLOSE, 0);
> > + ret = __sr_block_open(bdev, mode);
> > + scsi_autopm_put_device(sdev);
>
> Ioctls should never be used from kernel space. We have a few leftovers,
> but we need to get rid of that and not add more.
What is the alternative?
Thanks
Michal
On Thu, Oct 24, 2019 at 07:46:57AM +0200, Hannes Reinecke wrote:
> On 10/23/19 6:23 PM, Michal Such?nek wrote:
> > On Wed, Oct 23, 2019 at 04:13:15PM +0200, Hannes Reinecke wrote:
> >> On 10/23/19 2:52 PM, Michal Suchanek wrote:
> >>> The WMware ESXi cdrom identifies itself as:
> >>> sr 0:0:0:0: [sr0] scsi3-mmc drive: vendor: "NECVMWarVMware SATA CD001.00"
> >>> model: "VMware SATA CD001.00"
> >>> with the following get_capabilities print in sr.c:
> >>> sr_printk(KERN_INFO, cd,
> >>> "scsi3-mmc drive: vendor: \"%s\" model: \"%s\"\n",
> >>> cd->device->vendor, cd->device->model);
> >>>
> >>> So the model looks like reliable identification while vendor does not.
> >>>
> >>> The drive claims to have a tray and claims to be able to close it.
> >>> However, the UI has no notion of a tray - when medium is ejected it is
> >>> dropped in the floor and the user must select a medium again before the
> >>> drive can be re-loaded. On the kernel side the tray_move call to close
> >>> the tray succeeds but the drive state does not change as a result of the
> >>> call.
> >>>
> >>> The drive does not in fact emulate the tray state. There are two ways to
> >>> get the medium state. One is the SCSI status:
> >>>
> >>> Physical drive:
> >>>
> >>> Fixed format, current; Sense key: Not Ready
> >>> Additional sense: Medium not present - tray open
> >>> Raw sense data (in hex):
> >>> 70 00 02 00 00 00 00 0a 00 00 00 00 3a 02 00 00
> >>> 00 00
> >>>
> >>> Fixed format, current; Sense key: Not Ready
> >>> Additional sense: Medium not present - tray closed
> >>> Raw sense data (in hex):
> >>> 70 00 02 00 00 00 00 0a 00 00 00 00 3a 01 00 00
> >>> 00 00
> >>>
> >>> VMware ESXi:
> >>>
> >>> Fixed format, current; Sense key: Not Ready
> >>> Additional sense: Medium not present
> >>> Info fld=0x0 [0]
> >>> Raw sense data (in hex):
> >>> f0 00 02 00 00 00 00 0a 00 00 00 00 3a 00 00 00
> >>> 00 00
> >>>
> >>> So the tray state is not reported here. Other is medium status which the
> >>> kernel prefers if available. Adding a print here gives:
> >>>
> >>> cdrom: get_media_event success: code = 0, door_open = 1, medium_present = 0
> >>>
> >>> door_open is interpreted as open tray. This is fine so long as tray_move
> >>> would close the tray when requested or report an error which never
> >>> happens on VMware ESXi servers (5.5 and 6.5 tested).
> >>>
> >>> This is a popular virtualization platform so a workaround is worthwhile.
> >>>
> >>> Signed-off-by: Michal Suchanek <[email protected]>
> >>> ---
> >>> drivers/scsi/sr.c | 6 ++++++
> >>> 1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> >>> index 4664fdf75c0f..8090c5bdec09 100644
> >>> --- a/drivers/scsi/sr.c
> >>> +++ b/drivers/scsi/sr.c
> >>> @@ -867,6 +867,7 @@ static void get_capabilities(struct scsi_cd *cd)
> >>> unsigned int ms_len = 128;
> >>> int rc, n;
> >>>
> >>> + static const char *model_vmware = "VMware";
> >>> static const char *loadmech[] =
> >>> {
> >>> "caddy",
> >>> @@ -922,6 +923,11 @@ static void get_capabilities(struct scsi_cd *cd)
> >>> buffer[n + 4] & 0x20 ? "xa/form2 " : "", /* can read xa/from2 */
> >>> buffer[n + 5] & 0x01 ? "cdda " : "", /* can read audio data */
> >>> loadmech[buffer[n + 6] >> 5]);
> >>> + if (!strncmp(cd->device->model, model_vmware, strlen(model_vmware))) {
> >>> + buffer[n + 6] &= ~(0xff << 5);
> >>> + sr_printk(KERN_INFO, cd,
> >>> + "VMware ESXi bug workaround: tray -> caddy\n");
> >>> + }
> >>> if ((buffer[n + 6] >> 5) == 0)
> >>> /* caddy drives can't close tray... */
> >>> cd->cdi.mask |= CDC_CLOSE_TRAY;
> >>>
> >> This looks something which should be handled via a blacklist flag, not
> >> some inline hack which everyone forgets about it...
> >
> > AFAIK we used to have a blacklist but don't have anymore. So either it
> > has to be resurrected for this one flag or an inline hack should be good
> > enough.
> >
> But we do have one for generic scsi; cf drivers/scsi/scsi_devinfo.c.
> And this pretty much falls into the category of SCSI quirks, so I'd
> prefer have it hooked into that.
But generic scsi does not know about cdrom trays, does it?
Thanks
Michal
On 10/24/19 10:56 AM, Michal Suchánek wrote:
> On Thu, Oct 24, 2019 at 07:46:57AM +0200, Hannes Reinecke wrote:
>> On 10/23/19 6:23 PM, Michal Suchánek wrote:
>>> On Wed, Oct 23, 2019 at 04:13:15PM +0200, Hannes Reinecke wrote:
[ .. ]>>>> This looks something which should be handled via a blacklist
flag, not
>>>> some inline hack which everyone forgets about it...
>>>
>>> AFAIK we used to have a blacklist but don't have anymore. So either it
>>> has to be resurrected for this one flag or an inline hack should be good
>>> enough.
>>>
>> But we do have one for generic scsi; cf drivers/scsi/scsi_devinfo.c.
>> And this pretty much falls into the category of SCSI quirks, so I'd
>> prefer have it hooked into that.
>
> But generic scsi does not know about cdrom trays, does it?
>
No, just about 'flags'. What you _do_ with those flags is up to you.
Or, rather, the driver.
Just define a 'tray detection broken' flag, and evaluate it in sr.c.
Where is the problem with that?
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer
On Thu, Oct 24, 2019 at 11:41:38AM +0200, Hannes Reinecke wrote:
> On 10/24/19 10:56 AM, Michal Such?nek wrote:
> > On Thu, Oct 24, 2019 at 07:46:57AM +0200, Hannes Reinecke wrote:
> >> On 10/23/19 6:23 PM, Michal Such?nek wrote:
> >>> On Wed, Oct 23, 2019 at 04:13:15PM +0200, Hannes Reinecke wrote:
> [ .. ]>>>> This looks something which should be handled via a blacklist
> flag, not
> >>>> some inline hack which everyone forgets about it...
> >>>
> >>> AFAIK we used to have a blacklist but don't have anymore. So either it
> >>> has to be resurrected for this one flag or an inline hack should be good
> >>> enough.
> >>>
> >> But we do have one for generic scsi; cf drivers/scsi/scsi_devinfo.c.
> >> And this pretty much falls into the category of SCSI quirks, so I'd
> >> prefer have it hooked into that.
> >
> > But generic scsi does not know about cdrom trays, does it?
> >
> No, just about 'flags'. What you _do_ with those flags is up to you.
> Or, rather, the driver.
> Just define a 'tray detection broken' flag, and evaluate it in sr.c.
>
> Where is the problem with that?
And how do you set the flag?
The blacklist lists exact manufacturer and model while this rule only
depends on model because manufacturer is bogus. Also the blacklist
itself is deprecated:
/*
* scsi_static_device_list: deprecated list of devices that require
* settings that differ from the default, includes black-listed (broken)
* devices. The entries here are added to the tail of scsi_dev_info_list
* via scsi_dev_info_list_init.
*
* Do not add to this list, use the command line or proc interface to add
* to the scsi_dev_info_list. This table will eventually go away.
*/
Thanks
Michal
The WMware ESXi cdrom identifies itself as:
sr 0:0:0:0: [sr0] scsi3-mmc drive: vendor: "NECVMWarVMware SATA CD001.00"
model: "VMware SATA CD001.00"
with the following get_capabilities print in sr.c:
sr_printk(KERN_INFO, cd,
"scsi3-mmc drive: vendor: \"%s\" model: \"%s\"\n",
cd->device->vendor, cd->device->model);
The model looks like reliable identification while vendor does not.
The drive claims to have a tray and claims to be able to close it.
However, the UI has no notion of a tray - when medium is ejected it is
dropped in the floor and the user must select a medium again before the
drive can be re-loaded. On the kernel side the tray_move call to close
the tray succeeds but the drive state does not change as a result of the
call.
The drive does not in fact emulate the tray state. There are two ways to
get the medium state. One is the SCSI status:
Physical drive:
Fixed format, current; Sense key: Not Ready
Additional sense: Medium not present - tray open
Raw sense data (in hex):
70 00 02 00 00 00 00 0a 00 00 00 00 3a 02 00 00
00 00
Fixed format, current; Sense key: Not Ready
Additional sense: Medium not present - tray closed
Raw sense data (in hex):
70 00 02 00 00 00 00 0a 00 00 00 00 3a 01 00 00
00 00
VMware ESXi:
Fixed format, current; Sense key: Not Ready
Additional sense: Medium not present
Info fld=0x0 [0]
Raw sense data (in hex):
f0 00 02 00 00 00 00 0a 00 00 00 00 3a 00 00 00
00 00
So the tray state is not reported here. Other is medium status which the
kernel prefers if available. Adding a print here gives:
cdrom: get_media_event success: code = 0, door_open = 1, medium_present = 0
door_open is interpreted as open tray. This is fine so long as tray_move
would close the tray when requested or report an error which never
happens on VMware ESXi servers (5.5 and 6.5 tested).
This is a popular virtualization platform so a workaround is worthwhile.
Signed-off-by: Michal Suchanek <[email protected]>
---
drivers/scsi/scsi_devinfo.c | 15 +++++++++------
drivers/scsi/sr.c | 6 ++++++
include/scsi/scsi_devinfo.h | 7 ++++++-
3 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index df14597752ec..8c1cb38f862b 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -252,6 +252,7 @@ static struct {
{"TOSHIBA", "CD-ROM", NULL, BLIST_ISROM},
{"Traxdata", "CDR4120", NULL, BLIST_NOLUN}, /* locks up */
{"USB2.0", "SMARTMEDIA/XD", NULL, BLIST_FORCELUN | BLIST_INQUIRY_36},
+ {"VMware", "VMware", NULL, BLIST_MATCH_VENDOR | BLIST_TRAY},
{"WangDAT", "Model 2600", "01.7", BLIST_SELECT_NO_ATN},
{"WangDAT", "Model 3200", "02.2", BLIST_SELECT_NO_ATN},
{"WangDAT", "Model 1300", "02.4", BLIST_SELECT_NO_ATN},
@@ -454,10 +455,11 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
/*
* vendor strings must be an exact match
*/
- if (vmax != strnlen(devinfo->vendor,
- sizeof(devinfo->vendor)) ||
- memcmp(devinfo->vendor, vskip, vmax))
- continue;
+ if (!devinfo->flags & BLIST_MATCH_VENDOR)
+ if (vmax != strnlen(devinfo->vendor,
+ sizeof(devinfo->vendor)) ||
+ memcmp(devinfo->vendor, vskip, vmax))
+ continue;
/*
* @model specifies the full string, and
@@ -468,8 +470,9 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
continue;
return devinfo;
} else {
- if (!memcmp(devinfo->vendor, vendor,
- sizeof(devinfo->vendor)) &&
+ if ((!memcmp(devinfo->vendor, vendor,
+ sizeof(devinfo->vendor))
+ || (devinfo->flags & BLIST_MATCH_VENDOR)) &&
!memcmp(devinfo->model, model,
sizeof(devinfo->model)))
return devinfo;
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 4664fdf75c0f..c5468829be34 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -58,6 +58,7 @@
#include <scsi/scsi_eh.h>
#include <scsi/scsi_host.h>
#include <scsi/scsi_ioctl.h> /* For the door lock/unlock commands */
+#include <scsi/scsi_devinfo.h>
#include "scsi_logging.h"
#include "sr.h"
@@ -922,6 +923,11 @@ static void get_capabilities(struct scsi_cd *cd)
buffer[n + 4] & 0x20 ? "xa/form2 " : "", /* can read xa/from2 */
buffer[n + 5] & 0x01 ? "cdda " : "", /* can read audio data */
loadmech[buffer[n + 6] >> 5]);
+ if (cd->device->sdev_bflags & BLIST_TRAY) {
+ buffer[n + 6] &= ~(0xff << 5);
+ sr_printk(KERN_INFO, cd,
+ "Tray emulation bug workaround: tray -> caddy\n");
+ }
if ((buffer[n + 6] >> 5) == 0)
/* caddy drives can't close tray... */
cd->cdi.mask |= CDC_CLOSE_TRAY;
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index 3fdb322d4c4b..dbaf07888c16 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -67,8 +67,13 @@
#define BLIST_RETRY_ITF ((__force blist_flags_t)(1ULL << 32))
/* Always retry ABORTED_COMMAND with ASC 0xc1 */
#define BLIST_RETRY_ASC_C1 ((__force blist_flags_t)(1ULL << 33))
+/* Device reports to have a tray but it cannot be opearted reliably */
+#define BLIST_TRAY ((__force blist_flags_t)(1ULL << 34))
+/* Vendor string is bogus */
+#define BLIST_MATCH_VENDOR ((__force blist_flags_t)(1ULL << 35))
-#define __BLIST_LAST_USED BLIST_RETRY_ASC_C1
+
+#define __BLIST_LAST_USED BLIST_MATCH_VENDOR
#define __BLIST_HIGH_UNUSED (~(__BLIST_LAST_USED | \
(__force blist_flags_t) \
--
2.23.0
On Thu, Oct 24, 2019 at 10:51:36AM +0200, Michal Such?nek wrote:
> On Wed, Oct 23, 2019 at 07:24:06PM -0700, Christoph Hellwig wrote:
> > On Wed, Oct 23, 2019 at 02:52:47PM +0200, Michal Suchanek wrote:
> > > +static int sr_block_open_finish(struct block_device *bdev, fmode_t mode,
> > > + int ret)
> > > +{
> > > + struct scsi_cd *cd = scsi_cd(bdev->bd_disk);
> > > +
> > > + /* wait for drive to get ready */
> > > + if ((ret == -ENOMEDIUM) && !(mode & FMODE_NDELAY)) {
> > > + struct scsi_device *sdev = cd->device;
> > > + /*
> > > + * Cannot use sr_block_ioctl because it locks sr_mutex blocking
> > > + * out any processes trying to access the drive
> > > + */
> > > + scsi_autopm_get_device(sdev);
> > > + cdrom_ioctl(&cd->cdi, bdev, mode, CDROM_AUTOCLOSE, 0);
> > > + ret = __sr_block_open(bdev, mode);
> > > + scsi_autopm_put_device(sdev);
> >
> > Ioctls should never be used from kernel space. We have a few leftovers,
> > but we need to get rid of that and not add more.
>
> What is the alternative?
Call the function that would be called by the ioctl instead.
On Thu, Oct 24, 2019 at 10:55:14AM +0200, Michal Such?nek wrote:
> On Wed, Oct 23, 2019 at 07:22:32PM -0700, Christoph Hellwig wrote:
> > On Wed, Oct 23, 2019 at 02:52:45PM +0200, Michal Suchanek wrote:
> > > Opening a block device may require a long operation such as waiting for
> > > the cdrom tray to close. Performing this operation with locks held locks
> > > out other attempts to open the device. These processes waiting to open
> > > the device are not killable.
You can use mutex_lock_killable() to fix that.
On Thu, Oct 24, 2019 at 06:12:54AM -0700, Matthew Wilcox wrote:
> On Thu, Oct 24, 2019 at 10:55:14AM +0200, Michal Such?nek wrote:
> > On Wed, Oct 23, 2019 at 07:22:32PM -0700, Christoph Hellwig wrote:
> > > On Wed, Oct 23, 2019 at 02:52:45PM +0200, Michal Suchanek wrote:
> > > > Opening a block device may require a long operation such as waiting for
> > > > the cdrom tray to close. Performing this operation with locks held locks
> > > > out other attempts to open the device. These processes waiting to open
> > > > the device are not killable.
>
> You can use mutex_lock_killable() to fix that.
>
That solves only half of the problem.
It still does not give access to the device to processes that open with
O_NONBLOCK.
Thanks
Michal
On Wed, Oct 23, 2019 at 07:19:58PM -0700, Christoph Hellwig wrote:
> > static
> > -int open_for_data(struct cdrom_device_info *cdi)
> > +int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
>
> Please fix the coding style. static never should be on a line of its
> own..
It's OK to have the static on a line by itself; it's having 'static int'
on a line by itself that Linus gets unhappy about because he can't use
grep to see the return type.
But there's no need for it to be on a line by itself here, it all fits fine
in 80 columns.
On Thu, Oct 24, 2019 at 10:50:14AM +0200, Michal Such?nek wrote:
> Then I will get complaints I do unrelated changes and it's hard to
> review. The code gets removed later anyway.
If you refactor you you pretty much have a card blanche for the
refactored code and the direct surroundings.
On Thu, Oct 24, 2019 at 06:23:14AM -0700, Matthew Wilcox wrote:
> On Wed, Oct 23, 2019 at 07:19:58PM -0700, Christoph Hellwig wrote:
> > > static
> > > -int open_for_data(struct cdrom_device_info *cdi)
> > > +int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
> >
> > Please fix the coding style. static never should be on a line of its
> > own..
>
> It's OK to have the static on a line by itself; it's having 'static int'
> on a line by itself that Linus gets unhappy about because he can't use
> grep to see the return type.
Sorry, but independent of any preference just looking at the codebases
proves you wrong. All on one line is the most common style, but not
by much, followed by static + type. Just static is just in a few crazy
On Thu, Oct 24, 2019 at 07:39:08PM -0700, Christoph Hellwig wrote:
> On Thu, Oct 24, 2019 at 10:50:14AM +0200, Michal Such?nek wrote:
> > Then I will get complaints I do unrelated changes and it's hard to
> > review. The code gets removed later anyway.
>
> If you refactor you you pretty much have a card blanche for the
> refactored code and the direct surroundings.
This is different from what other reviewers say:
https://lore.kernel.org/lkml/[email protected]/
Either way, this code is removed in a later patch so this discussion is
moot. It makes sense to have a bisection point here in case something
goes wrong but it is pointless to argue about the code structure
inherited from the previous revision.
Thanks
Michal
On Fri, 25 Oct 2019, Michal Such?nek wrote:
> On Thu, Oct 24, 2019 at 07:39:08PM -0700, Christoph Hellwig wrote:
> > On Thu, Oct 24, 2019 at 10:50:14AM +0200, Michal Such?nek wrote:
> > > Then I will get complaints I do unrelated changes and it's hard to
> > > review. The code gets removed later anyway.
> >
> > If you refactor you you pretty much have a card blanche for the
> > refactored code and the direct surroundings.
>
> This is different from what other reviewers say:
>
> https://lore.kernel.org/lkml/[email protected]/
>
I don't see any inconsistency there. Both reviews are valuable.
In general, different reviewers may give contradictory advice. Reviewers
probably even contradict themselves eventually. Yet it rarely happens that
the same patch gets contradictory reviews. If it did, you might well
complain.
> Either way, this code is removed in a later patch so this discussion is
> moot.
>
> It makes sense to have a bisection point here in case something
> goes wrong but it is pointless to argue about the code structure
> inherited from the previous revision.
A patch may refactor some code only to have the next patch remove that
code. This doesn't generally mean that the former patch is redundant.
The latter patch may end up committed and subsequently reverted. The
latter patch may become easier to review because of the former. The former
patch may be eligible for -stable. The former patch may be the result of
an automatic process. And so on.
I don't know what Christoph had in mind here but he's usually right, so
it's worth asking.
--
>
> Thanks
>
> Michal
>
FYI, we noticed the following commit (built with gcc-7):
commit: 9ed2563662a7eccd0dd3e4cfcaa58c776effe8cc ("[PATCH v2 8/8] scsi: sr: wait for the medium to become ready")
url: https://github.com/0day-ci/linux/commits/Michal-Suchanek/Fix-cdrom-autoclose/20191025-100818
in testcase: blktests
with following parameters:
disk: 1SSD
test: block-group1
on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 8G
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
+--------------------------------------------------------------------------+------------+------------+
| | 71afe2ff77 | 9ed2563662 |
+--------------------------------------------------------------------------+------------+------------+
| boot_successes | 8 | 0 |
| boot_failures | 0 | 232 |
| BUG:kernel_NULL_pointer_dereference,address | 0 | 220 |
| Oops:#[##] | 0 | 223 |
| RIP:cdrom_release[cdrom] | 0 | 208 |
| Kernel_panic-not_syncing:Fatal_exception | 0 | 225 |
| WARNING:at_fs/kernfs/dir.c:#kernfs_remove_by_name_ns | 0 | 49 |
| RIP:kernfs_remove_by_name_ns | 0 | 49 |
| WARNING:at_kernel/module.c:#module_put | 0 | 46 |
| RIP:module_put | 0 | 46 |
| RIP:__pm_runtime_resume | 0 | 8 |
| RIP:kobject_uevent_env | 0 | 12 |
| WARNING:at_lib/list_debug.c:#__list_del_entry_valid | 0 | 5 |
| RIP:__list_del_entry_valid | 0 | 9 |
| WARNING:at_net/sched/sch_generic.c:#dev_watchdog | 0 | 1 |
| RIP:dev_watchdog | 0 | 1 |
| RIP:native_safe_halt | 0 | 1 |
| BUG:soft_lockup-CPU##stuck_for#s | 0 | 5 |
| RIP:_raw_spin_unlock_irqrestore | 0 | 1 |
| Kernel_panic-not_syncing:softlockup:hung_tasks | 0 | 5 |
| BUG:kernel_hang_in_boot_stage | 0 | 1 |
| WARNING:at_fs/sysfs/group.c:#internal_create_group | 0 | 5 |
| RIP:internal_create_group | 0 | 5 |
| WARNING:at_fs/sysfs/file.c:#sysfs_create_file_ns | 0 | 5 |
| RIP:sysfs_create_file_ns | 0 | 5 |
| RIP:smp_call_function_single | 0 | 4 |
| BUG:unable_to_handle_page_fault_for_address | 0 | 7 |
| Kernel_panic-not_syncing:Fatal_exception_in_interrupt | 0 | 1 |
| RIP:device_del | 0 | 1 |
| WARNING:at_lib/kobject.c:#kobject_put | 0 | 2 |
| RIP:kobject_put | 0 | 2 |
| WARNING:at_block/genhd.c:#__device_add_disk | 0 | 6 |
| RIP:__device_add_disk | 0 | 6 |
| BUG:sleeping_function_called_from_invalid_context_at_arch/x86/mm/fault.c | 0 | 1 |
| RIP:rpm_resume | 0 | 1 |
| general_protection_fault:#[##] | 0 | 3 |
| RIP:sysfs_remove_groups | 0 | 2 |
| RIP:driver_deferred_probe_del | 0 | 3 |
| INFO:rcu_sched_self-detected_stall_on_CPU | 0 | 1 |
| RIP:console_unlock | 0 | 1 |
| RIP:kernfs_find_ns | 0 | 1 |
+--------------------------------------------------------------------------+------------+------------+
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
[ 36.496568] BUG: kernel NULL pointer dereference, address: 0000000000000038
[ 36.498508] #PF: supervisor read access in kernel mode
[ 36.499995] #PF: error_code(0x0000) - not-present page
[ 36.500181] sr 6:0:0:0: Attached scsi CD-ROM sr3
[ 36.501914] PGD 0 P4D 0
[ 36.501919] Oops: 0000 [#1] SMP PTI
[ 36.501922] CPU: 0 PID: 2604 Comm: scsi_id Not tainted 5.4.0-rc4-00112-g9ed2563662a7e #1
[ 36.501924] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
[ 36.503631] sr 6:0:0:0: Attached scsi generic sg13 type 5
[ 36.504444] RIP: 0010:cdrom_release+0x19/0x2b0 [cdrom]
[ 36.504446] Code: e8 dc 3a 18 f2 8b 44 24 04 eb 99 e8 b1 f4 d8 f1 90 66 66 66 66 90 41 57 41 56 41 55 41 54 41 89 f4 55 53 48 89 fb 48 83 ec 48 <48> 8b 2f 65 48 8b 04 25 28 00 00 00 48 89 44 24 40 31 c0 80 3d 4b
[ 36.504447] RSP: 0018:ffffaf2c4035fb68 EFLAGS: 00010292
[ 36.504448] RAX: 0000000000000000 RBX: 0000000000000038 RCX: 0000000000000000
[ 36.504449] RDX: ffff9f59418e0000 RSI: 000000000800005d RDI: 0000000000000038
[ 36.504450] RBP: 000000000800005d R08: 0000000000000001 R09: ffff9f58c1464cf0
[ 36.504450] R10: 0000000000000001 R11: 0000000000327273 R12: 000000000800005d
[ 36.504451] R13: ffff9f594277e000 R14: ffff9f58878123c0 R15: ffff9f5887812498
[ 36.504452] FS: 00007fc6049f0740(0000) GS:ffff9f59bfc00000(0000) knlGS:0000000000000000
[ 36.504454] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 36.540978] CR2: 0000000000000038 CR3: 00000001c2860000 CR4: 00000000000406f0
[ 36.543223] Call Trace:
[ 36.544362] ? del_timer+0x53/0x80
[ 36.545695] ? lock_timer_base+0x67/0x80
[ 36.547100] sr_block_release+0x27/0x40 [sr_mod]
[ 36.548940] __blkdev_put+0x192/0x1e0
[ 36.551285] __blkdev_get+0x28b/0x630
[ 36.553191] ? bd_acquire+0xe0/0xe0
[ 36.555268] do_dentry_open+0x1ce/0x380
[ 36.557160] path_openat+0x2e5/0x1550
[ 36.558524] ? __get_locked_pte+0x1c7/0x260
[ 36.559951] do_filp_open+0x9b/0x110
[ 36.561452] ? __check_object_size+0xd4/0x1a0
[ 36.563651] ? do_sys_open+0x1bd/0x250
[ 36.565807] do_sys_open+0x1bd/0x250
[ 36.567814] do_syscall_64+0x5b/0x1d0
[ 36.569951] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 36.572287] RIP: 0033:0x7fc604bc5c8b
[ 36.574365] Code: 4e 89 f0 25 00 00 41 00 3d 00 00 41 00 74 40 8b 05 ca e7 00 00 85 c0 75 61 89 f2 b8 01 01 00 00 48 89 fe bf 9c ff ff ff 0f 05 <48> 3d 00 f0 ff ff 0f 87 99 00 00 00 48 8b 4c 24 28 64 48 33 0c 25
[ 36.581602] RSP: 002b:00007ffd8656b750 EFLAGS: 00000246 ORIG_RAX: 0000000000000101
[ 36.584113] RAX: ffffffffffffffda RBX: 000055fd844820d0 RCX: 00007fc604bc5c8b
[ 36.586157] RDX: 0000000000080800 RSI: 00007ffd8656b930 RDI: 00000000ffffff9c
[ 36.588165] RBP: 00007ffd8656b930 R08: 00007fc604bae1d0 R09: 00007fc604bae240
[ 36.590203] R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffd8656bb30
[ 36.592218] R13: 00007ffd8656b820 R14: 0000000000000014 R15: 0000000000000064
[ 36.594430] Modules linked in: scsi_debug loop intel_rapl_msr sr_mod intel_rapl_common cdrom crct10dif_pclmul sd_mod crc32_pclmul sg crc32c_intel ghash_clmulni_intel ppdev bochs_drm ata_generic pata_acpi drm_vram_helper ttm drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops snd_pcm aesni_intel drm crypto_simd snd_timer snd cryptd glue_helper ata_piix libata soundcore joydev pcspkr serio_raw virtio_scsi i2c_piix4 floppy parport_pc parport ip_tables [last unloaded: scsi_debug]
[ 36.606610] CR2: 0000000000000038
[ 36.669085] ---[ end trace 716cd1ac8d8f8945 ]---
To reproduce:
# build kernel
cd linux
cp config-5.4.0-rc4-00112-g9ed2563662a7e .config
make HOSTCC=gcc-7 CC=gcc-7 ARCH=x86_64 olddefconfig prepare modules_prepare bzImage
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> job-script # job-script is attached in this email
Thanks,
lkp
On Thu, Oct 24, 2019 at 06:12:54AM -0700, Matthew Wilcox wrote:
> On Thu, Oct 24, 2019 at 10:55:14AM +0200, Michal Such?nek wrote:
> > On Wed, Oct 23, 2019 at 07:22:32PM -0700, Christoph Hellwig wrote:
> > > On Wed, Oct 23, 2019 at 02:52:45PM +0200, Michal Suchanek wrote:
> > > > Opening a block device may require a long operation such as waiting for
> > > > the cdrom tray to close. Performing this operation with locks held locks
> > > > out other attempts to open the device. These processes waiting to open
> > > > the device are not killable.
>
> You can use mutex_lock_killable() to fix that.
>
That fixes only half of the problem.
Other processes still cannot access the device while you wait on
mutex_lock_killable
Thanks
Michal
On Wed, Oct 23, 2019 at 07:23:07PM -0700, Christoph Hellwig wrote:
> On Wed, Oct 23, 2019 at 02:52:46PM +0200, Michal Suchanek wrote:
> >
> > The drive claims to have a tray and claims to be able to close it.
> > However, the UI has no notion of a tray - when medium is ejected it is
> > dropped in the floor and the user must select a medium again before the
> > drive can be re-loaded. On the kernel side the tray_move call to close
> > the tray succeeds but the drive state does not change as a result of the
> > call.
> >
> > The drive does not in fact emulate the tray state. There are two ways to
> > get the medium state. One is the SCSI status:
>
> Given that this is a buggy software emulation we should not add more
> than 100 lines of kernel code to work around it. Ask VMware to fix
> their mess instead.
Where do you see 100 lines of code?
The patch has exactly 4.
Thanks
Michal