[PATCH v2 0/8] Fix cdrom autoclose.

[PATCH v2 0/8] Fix cdrom autoclose.

[Michal Suchanek]

Link: https://lore.kernel.org/lkml/cover.1513263482.git.msuchanek@suse.de/

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

[PATCH v2 1/8] cdrom: add poll_event_interruptible

[Michal Suchanek]

Add convenience macro for polling an event that does not have a
waitqueue.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 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

[PATCH v2 2/8] cdrom: factor out common open_for_* code

[Michal Suchanek]

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 <msuchanek@suse.de>
---
 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

[PATCH v2 3/8] cdrom: wait for the tray to close

[Michal Suchanek]

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 <msuchanek@suse.de>
---
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

[PATCH v2 4/8] cdrom: separate autoclose into an IOCTL

[Michal Suchanek]

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 <msuchanek@suse.de>
---
 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

[PATCH v2 5/8] docs: cdrom: Add autoclose IOCTL

[Michal Suchanek]

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 <msuchanek@suse.de>
---
 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

[PATCH v2 6/8] bdev: add open_finish.

[Michal Suchanek]

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 <msuchanek@suse.de>
---
 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

[PATCH v2 7/8] scsi: sr: workaround VMware ESXi cdrom emulation bug

[Michal Suchanek]

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 <msuchanek@suse.de>
---
 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

[PATCH v2 8/8] scsi: sr: wait for the medium to become ready

[Michal Suchanek]

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 <msuchanek@suse.de>
---
 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

Re: [PATCH v2 7/8] scsi: sr: workaround VMware ESXi cdrom emulation bug

[Hannes Reinecke]

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 <msuchanek@suse.de>
> ---
>  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
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer

Re: [PATCH v2 7/8] scsi: sr: workaround VMware ESXi cdrom emulation bug

[Michal Suchánek]

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 <msuchanek@suse.de>
> > ---
> >  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

Re: [PATCH v2 7/8] scsi: sr: workaround VMware ESXi cdrom emulation bug

[Ewan D. Milne]

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 <msuchanek@suse.de>
> > > ---
> > >  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

Re: [PATCH v2 2/8] cdrom: factor out common open_for_* code

[Christoph Hellwig]

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

Re: [PATCH v2 6/8] bdev: add open_finish.

[Christoph Hellwig]

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.

Re: [PATCH v2 7/8] scsi: sr: workaround VMware ESXi cdrom emulation bug

[Christoph Hellwig]

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.

Re: [PATCH v2 8/8] scsi: sr: wait for the medium to become ready

[Christoph Hellwig]

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.

Re: [PATCH v2 7/8] scsi: sr: workaround VMware ESXi cdrom emulation bug

[Hannes Reinecke]

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 <msuchanek@suse.de>
>>> ---
>>>  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
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer

Re: [PATCH v2 2/8] cdrom: factor out common open_for_* code

[Michal Suchánek]

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

Re: [PATCH v2 8/8] scsi: sr: wait for the medium to become ready

[Michal Suchánek]

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

Re: [PATCH v2 7/8] scsi: sr: workaround VMware ESXi cdrom emulation bug

[Michal Suchánek]

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

Re: [PATCH v2 6/8] bdev: add open_finish.

[Michal Suchánek]

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

Re: [PATCH v2 7/8] scsi: sr: workaround VMware ESXi cdrom emulation bug

[Michal Suchánek]

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 <msuchanek@suse.de>
> >>> ---
> >>>  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

Re: [PATCH v2 7/8] scsi: sr: workaround VMware ESXi cdrom emulation bug

[Hannes Reinecke]

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
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer

Re: [PATCH v2 7/8] scsi: sr: workaround VMware ESXi cdrom emulation bug

[Michal Suchánek]

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

Re: [PATCH v2 6/8] bdev: add open_finish.

[Matthew Wilcox]

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.

Re: [PATCH v2 8/8] scsi: sr: wait for the medium to become ready

[Matthew Wilcox]

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.

Re: [PATCH v2 6/8] bdev: add open_finish.

[Michal Suchánek]

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

Re: [PATCH v2 2/8] cdrom: factor out common open_for_* code

[Matthew Wilcox]

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.

Re: [PATCH v2 2/8] cdrom: factor out common open_for_* code

[Christoph Hellwig]

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

Re: [PATCH v2 2/8] cdrom: factor out common open_for_* code

[Christoph Hellwig]

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.

Re: [PATCH v2 2/8] cdrom: factor out common open_for_* code

[Michal Suchánek]

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/1517245320.2687.14.camel@wdc.com/

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

Re: [PATCH v2 2/8] cdrom: factor out common open_for_* code

[Finn Thain]

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/1517245320.2687.14.camel@wdc.com/
> 

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
> 

Re: [PATCH v2 6/8] bdev: add open_finish.

[Michal Suchánek]

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

Re: [PATCH v2 7/8] scsi: sr: workaround VMware ESXi cdrom emulation bug

[Michal Suchánek]

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