linux-scsi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] Fix automatic CD tray loading for data reading via sr
@ 2020-10-06  9:40 Thomas Schmitt
  2020-10-06  9:40 ` [PATCH v2 1/2] cdrom: delegate automatic CD tray loading to callers of cdrom_open() Thomas Schmitt
  2020-10-06  9:40 ` [PATCH v2 2/2] sr: fix automatic tray loading for data reading Thomas Schmitt
  0 siblings, 2 replies; 3+ messages in thread
From: Thomas Schmitt @ 2020-10-06  9:40 UTC (permalink / raw)
  To: linux-scsi; +Cc: axboe, linux-kernel, jejb, martin.petersen, Thomas Schmitt

Hi,

if
  open("/dev/sr0", O_RDONLY);
pulls in the tray of an optical drive, it immediately returns -1 with
errno ENOMEDIUM, or the first read(2) fails with EIO. Later, when the
drive has stopped blinking, another open() yields success and read()
works.
This affects not only userland reading of the device file but also
mounting the device.

The fundamental problem of the current implementation is that function
sr_block_open() does medium assessment before calling cdrom_open(), under
which open_for_data() performs the tray loading too late.
A minor problem is that there is currently no waiting loop for the drive's
decision about the medium.

Please regard my proposal as follow-up to commit 2bbea6e11735 ("cdrom: do
not call check_disk_change() inside cdrom_open()") of march 2008. It moved
medium assessment out ouf cdrom_open() but left behind tray loading.

--------------------------------------------------------------------------
For now my proposal fixes only data reading via sr, because that's what i
am able to test:

Factor out from open_for_data() a new function cdrom_handle_open_tray()
and export it.
It decides whether it can and should load the tray. If so, it emits the
tray loading command and waits for up to 40 seconds for the drive to make
its decision. Plus 1 second of waiting in case of a usable medium, in
order to avoid hard-to-test narrow race conditions.

Let cdrom_open() not attempt to load the tray any more, but rather just
return -ENOMEDIUM if it detects an open tray.

Let sr_block_open() call cdrom_handle_open_tray() under mutex cd->lock
which it gives up immediately thereafter.
Then it calls the same sequence of medium assessment, mutex cd->lock and
cdrom_open() as it does currently.

Thanks to commit 51a858817dcd ("scsi: sr: get rid of sr global mutex") of
february 2020, the blocking of at most 41 seconds only affects the
particular drive and not all other sr devices.

-----------------------------------------------------------------------
Successful tests:

I tested with 3 different drives at SATA and USB (ASUS DRW-24D5MT,
Pioneer BDR-S09, LG BH16NS40 in USB box) simultaneously by
  time dd if=/dev/sr"$N" bs=2048 count=1 of=/dev/null
with CD, DVD, and BD media. Waiting times ranged from 9 to 23 seconds.

The same times were measured when each drive was the only busy one.

Simultaneous full reads succeeded starting from open tray by
  time dd if=/dev/sr"$N" bs=2048 status=progress of=/dev/null

I tested mounting CD, DVD and BD with ISO 9660 from open tray by
  mount /dev/sr"$N" /mnt/iso

To make sure that no unwanted tray loading happens, i tested with open
tray by a program which prevents loading by O_NONBLOCK:
  fd = open(path, O_RDONLY | O_NONBLOCK);
  ret = ioctl(fd, CDROMREADTOCHDR, &hdr);
The open() call succeeds, whereas the ioctl() fails with ENOMEDIUM.
The tray does not move.

The patches were checked on Debian 10 by
  ./scripts/checkpatch.pl --strict --codespell --codespellfile \
       /usr/lib/python3/dist-packages/codespell_lib/data/dictionary.txt

---------------------------------------------------------------------
Patch version:

v2 is based on git://git.kernel.dk/linux-block branch for-next to take
into respect commit afd35c4f573d ("sr: use bdev_check_media_change") and
commit 38a2b557e238 ("sr: simplify sr_block_revalidate_disk").

Newest in the branch was commit 73f2e37b498a ("Merge branch
'for-5.10/drivers' into for-next").

v2 improves the concurrency behavior of its tray loader with competing
tray loading by the human user. v1 reported "no medium" when encountering
a drive which is already in the process of decision. v2 waits for this
decision.

--------------------------------------------------------------------
The rest of this mail is about these aspects:
- What about CD audio and non-sr CD drivers.
- Why not (yet) waiting without mutex after tray loading.
- Why i can only test data reading via sr.
- Why the current code not always yields ENOMEDIUM at open() but sometimes
  EIO at the first read(2).

--------------------------------------------------------------------
What about CD audio and non-sr CD drivers:

Because i lack of testing opportunities (see below), i omit:

- Calling cdrom_handle_open_tray() in the bdops.open() functions of
  cdrom/gdrom.c, block/paride/pcd.c, ide/ide-cd.c.

- Calling cdrom_handle_open_tray() in the callers of
  check_for_audio_disc(): cdrom_ioctl_audioctl() and
  cdrom_ioctl_play_trkind() which control audio playback by the drive to
  some (traditionally analog) audio outlet.

- Replacing the tray loading code of check_for_audio_disc() in cdrom.c
  by a mere status checker like by my proposal in open_for_data().
  (It could stay in place but would be of no use any more.)

My changes in sr_block_open() and open_for_data() could serve as
templates for these omitted tasks.

I would prepare patches if there is acceptance for untested code.

--------------------------------------------------------------------
Why not (yet) waiting without mutex after tray loading:

The automatic tray loading happened under the mutex for cdrom_open().
My proposal upholds this locking by acquiring cd->lock before calling
cdrom_handle_open_tray() and giving it up before medium assessment.

It seems to me that it should be safe to run the new function
cdrom_handle_open_tray() without cd->lock, as it mainly emits drive
commands START/STOP UNIT and TEST UNIT READY. It does not manipulate
members of device structs.
Without lock it would enable access to the drive by other threads during
the waiting for a drive decision.

But i have no idea how to prove that it is safe.

---------------------------------------------------------------------
Why i can only test data reading via sr:

None of my DVD and BD drives has a socket for plugging loudspeakers or
earphones. So i cannot test the consequences of my proposal to the ability
of playing audio tracks.

sr is the only driver i can test, because of the extinction status of the
devices of the other drivers which call cdrom_open():
- gdrom seems to be for ancient gaming consoles. It is unclear to me
  whether they could load a tray by own motor power.
- paride is described as ATAPI via an old printer cable.
- ide-cd ... was not that the CD part of hd swallowed by sr, as the HDD
  part of hd was swallowed by sd ?

sr is well alive. BD drives cost ~80 EUR, 25 GB BD-RE media <1 EUR.
Not a competitor to HDD, but to USB sticks.

---------------------------------------------------------------------
Why the current code not always yields ENOMEDIUM at open() but sometimes
EIO at the first read(2):

A newly bought Pioneer BDR-S09 shows this behavior with the current code.
dd reports I/O error rather than no medium.

Inspection by libburn shows that it waits with its reply to START/STOP
UNIT until it has decided over the medium. The first following TEST UNIT
READY yields key=6, asc=28h, ascq=00h "Not Ready to Ready change, medium
may have changed" and the second TEST UNIT READY yields success.

This substitutes for the missing waiting loop, but not for the lack of
medium assessment after loading. So open(2) succeeds, but read(2) faces
the medium assessment which was made when the tray was out. The result was
in my experiments always EIO on the first read().

This behavior is peculiar among my drives. Seven others reply to
START/STOP UNIT as soon as the tray has moved in. Then for several seconds
they reply to TEST UNIT READY by key=2, asc=04h, ascq=01h "Logical unit is
in process of becoming ready".

Have a nice day :)

Thomas

Thomas Schmitt (2):
  cdrom: delegate automatic CD tray loading to callers of cdrom_open()
  sr: fix automatic tray loading for data reading

 drivers/cdrom/cdrom.c | 173 +++++++++++++++++++++++++++++++-----------
 drivers/scsi/sr.c     |  12 ++-
 include/linux/cdrom.h |   3 +
 3 files changed, 141 insertions(+), 47 deletions(-)

--
2.20.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH v2 1/2] cdrom: delegate automatic CD tray loading to callers of cdrom_open()
  2020-10-06  9:40 [PATCH v2 0/2] Fix automatic CD tray loading for data reading via sr Thomas Schmitt
@ 2020-10-06  9:40 ` Thomas Schmitt
  2020-10-06  9:40 ` [PATCH v2 2/2] sr: fix automatic tray loading for data reading Thomas Schmitt
  1 sibling, 0 replies; 3+ messages in thread
From: Thomas Schmitt @ 2020-10-06  9:40 UTC (permalink / raw)
  To: linux-scsi; +Cc: axboe, linux-kernel, jejb, martin.petersen, Thomas Schmitt

If
  open("/dev/sr0", O_RDONLY);
pulls in the tray of an optical drive, it immediately returns -1 with
errno ENOMEDIUM or the first read(2) fails with EIO. Later, when the drive
has stopped blinking, another open() yields success and read() works.
This affects not only userland reading of the device file but also
mounting the device.

Since commit 210ba1d1724f ("[SCSI] sr: update to follow tray status
correctly") of january 2008 the necessary waiting loop after emitting the
tray loading command is not performed, because sr_do_ioctl() is not called
any more.
Commit 2bbea6e11735 ("cdrom: do not call check_disk_change() inside
cdrom_open()") of march 2008 moved medium assessment out of cdrom_open()
and thus inevitable before automatic tray loading.

Factor out a new function cdrom_handle_open_tray() in cdrom.c from
open_for_data() and export it, so that callers of cdrom_open() can call
it before their call of check_disk_change(). It decides whether it can and
should load the tray. If so, it emits the tray loading command and waits
for the drive to make its decision.

Replace automatic tray loading in cdrom_open() by a mere check whether the
drive reports a usable medium in a loaded tray.
Unaware callers of cdrom_open() will not cause automatic tray loading
any more, but rather will reliably see -ENOMEDIUM if the tray is open.

Signed-off-by: Thomas Schmitt <scdbackup@gmx.net>
---
 drivers/cdrom/cdrom.c | 173 +++++++++++++++++++++++++++++++-----------
 include/linux/cdrom.h |   3 +
 2 files changed, 132 insertions(+), 44 deletions(-)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 0c271b9e3c5b..45bfe76129ef 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -286,6 +286,18 @@
 #include <scsi/scsi_common.h>
 #include <scsi/scsi_request.h>

+/*
+ * For the wait-and-retry loop after possibly having loaded the drive tray.
+ * 10 retries in 20 seconds are hardcoded in sr_do_ioctl() which was used
+ * up to 2008.
+ * But time spans up to 25 seconds were measured by libburn on
+ * drives connected via SATA or USB-SATA bridges.
+ * So 20 retries * 2000 ms = 40 seconds seems more appropriate.
+ */
+#define CD_OPEN_MEDIUM_RETRY_MAX 20
+#define CD_OPEN_MEDIUM_RETRY_MSLEEP 2000
+#include <linux/delay.h>
+
 /* used to tell the module to turn on full debugging messages */
 static bool debug;
 /* default compatibility mode */
@@ -1040,6 +1052,114 @@ static void cdrom_count_tracks(struct cdrom_device_info *cdi, tracktype *tracks)
 	       tracks->cdi, tracks->xa);
 }

+static
+int wait_for_medium_decision(struct cdrom_device_info *cdi)
+{
+	int retry = 0, ret;
+	const struct cdrom_device_ops *cdo = cdi->ops;
+
+	/* Wait until the intermediate drive status CDS_DRIVE_NOT_READY ends */
+	while (1) {
+		ret = cdo->drive_status(cdi, CDSL_CURRENT);
+		if (ret == CDS_DRIVE_NOT_READY &&
+		    retry++ < CD_OPEN_MEDIUM_RETRY_MAX)
+			msleep(CD_OPEN_MEDIUM_RETRY_MSLEEP);
+		else
+			break;
+	}
+	if (ret != CDS_DISC_OK)
+		return ret;
+	/*
+	 * It is hard to test whether very recent readiness can cause race
+	 * conditions with media change events. So wait a while to never
+	 * undercut the average delay between actual readiness and detection
+	 * which was tested without this additional msleep().
+	 */
+	msleep(CD_OPEN_MEDIUM_RETRY_MSLEEP / 2);
+
+	return CDS_DISC_OK;
+}
+
+/*
+ * To be called by expectant callers of cdrom_open(), before they call
+ * check_disk_change() and then cdrom_open().
+ *
+ * If the mode is right, the drive capable, the tray out, and autoclose
+ * enabled, try to move in the tray and wait for the drive's decision about
+ * the medium.
+ * Return 0 if cdrom_open() would not want to know the tray status, or the
+ * drive cannot report its tray status at all, or the decision is CDS_DISC_OK.
+ * Else return a negative error number.
+ * Input parameter mode decides whether cdrom_open() will want to know or
+ * change the tray status at all.
+ * Input parameter leave_open == 1 suppresses the try to close and the waiting
+ * for a decision. It rather just assesses the situation. Submit mode == 0
+ * to not hamper assessment.
+ */
+int cdrom_handle_open_tray(struct cdrom_device_info *cdi, fmode_t mode,
+			   int leave_open)
+{
+	int ret;
+	const struct cdrom_device_ops *cdo = cdi->ops;
+
+	if ((mode & FMODE_NDELAY) && (cdi->options & CDO_USE_FFLAGS))
+		return 0;
+	if (!cdo->drive_status)
+		return 0;
+
+	ret = cdo->drive_status(cdi, CDSL_CURRENT);
+	cd_dbg(CD_OPEN, "drive_status=%d\n", ret);
+	if (ret == CDS_DRIVE_NOT_READY) {
+		/* Probably some other entity is loading the tray */
+		if (leave_open)
+			return -ENOMEDIUM;
+		wait_for_medium_decision(cdi);
+		goto assess_and_return;
+	}
+	if (ret != CDS_TRAY_OPEN)
+		goto assess_and_return;
+
+	cd_dbg(CD_OPEN, "the tray is open...\n");
+	if (leave_open)
+		return -ENOMEDIUM;
+	/* 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 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 {
+		if (!CDROM_CAN(CDC_CLOSE_TRAY))
+			cd_dbg(CD_OPEN,
+			       "bummer. this drive can't close the tray.\n");
+		return -ENOMEDIUM;
+	}
+
+	ret = wait_for_medium_decision(cdi);
+	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");
+
+assess_and_return:
+	ret = cdo->drive_status(cdi, CDSL_CURRENT);
+	if (ret != CDS_DISC_OK)
+		return -ENOMEDIUM;
+	return 0;
+}
+EXPORT_SYMBOL(cdrom_handle_open_tray);
+
 static
 int open_for_data(struct cdrom_device_info *cdi)
 {
@@ -1047,50 +1167,15 @@ int open_for_data(struct cdrom_device_info *cdi)
 	const struct cdrom_device_ops *cdo = cdi->ops;
 	tracktype tracks;
 	cd_dbg(CD_OPEN, "entering open_for_data\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) {
-		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 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.*/
-					ret=-ENOMEDIUM;
-					goto clean_up_and_return;
-				}
-			} else {
-				cd_dbg(CD_OPEN, "bummer. this drive can't close the tray.\n");
-				ret=-ENOMEDIUM;
-				goto clean_up_and_return;
-			}
-			/* 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;
-			}
-			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;
-		}
-	}
+
+	/*
+	 * Check for open tray, but do not close it. The caller should
+	 * have cared to call cdrom_handle_open_tray(,,0) in advance.
+	 */
+	ret = cdrom_handle_open_tray(cdi, (fmode_t)0, 1);
+	if (ret)
+		goto clean_up_and_return;
+
 	cdrom_count_tracks(cdi, &tracks);
 	if (tracks.error == CDS_NO_DISC) {
 		cd_dbg(CD_OPEN, "bummer. no disc.\n");
diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
index f48d0a31deae..cf2b5fc9c6fd 100644
--- a/include/linux/cdrom.h
+++ b/include/linux/cdrom.h
@@ -98,6 +98,9 @@ int cdrom_multisession(struct cdrom_device_info *cdi,
 int cdrom_read_tocentry(struct cdrom_device_info *cdi,
 		struct cdrom_tocentry *entry);

+int cdrom_handle_open_tray(struct cdrom_device_info *cdi, fmode_t mode,
+			   int leave_open);
+
 /* the general block_device operations structure: */
 extern int cdrom_open(struct cdrom_device_info *cdi, struct block_device *bdev,
 			fmode_t mode);
--
2.20.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH v2 2/2] sr: fix automatic tray loading for data reading
  2020-10-06  9:40 [PATCH v2 0/2] Fix automatic CD tray loading for data reading via sr Thomas Schmitt
  2020-10-06  9:40 ` [PATCH v2 1/2] cdrom: delegate automatic CD tray loading to callers of cdrom_open() Thomas Schmitt
@ 2020-10-06  9:40 ` Thomas Schmitt
  1 sibling, 0 replies; 3+ messages in thread
From: Thomas Schmitt @ 2020-10-06  9:40 UTC (permalink / raw)
  To: linux-scsi; +Cc: axboe, linux-kernel, jejb, martin.petersen, Thomas Schmitt

If
  open("/dev/sr0", O_RDONLY);
pulls in the tray of an optical drive, it immediately returns -1 with
errno ENOMEDIUM or the first read(2) fails with EIO. Later, when the drive
has stopped blinking, another open() yields success and read() works.
This affects not only userland reading of the device file but also
mounting the device.
The reason is that medium assessment happens before automatic tray
loading.

Use the new function cdrom_handle_open_tray() for deciding and performing
tray loading before doing medium assessment.

Signed-off-by: Thomas Schmitt <scdbackup@gmx.net>
---
 drivers/scsi/sr.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 2b43c0f97442..1c3806f59865 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -540,12 +540,18 @@ static int sr_block_open(struct block_device *bdev, fmode_t mode)

 	sdev = cd->device;
 	scsi_autopm_get_device(sdev);
-	if (bdev_check_media_change(bdev))
-		sr_revalidate_disk(cd);

 	mutex_lock(&cd->lock);
-	ret = cdrom_open(&cd->cdi, bdev, mode);
+	ret = cdrom_handle_open_tray(&cd->cdi, mode, 0);
 	mutex_unlock(&cd->lock);
+	if (!ret) {
+		if (bdev_check_media_change(bdev))
+			sr_revalidate_disk(cd);
+
+		mutex_lock(&cd->lock);
+		ret = cdrom_open(&cd->cdi, bdev, mode);
+		mutex_unlock(&cd->lock);
+	}

 	scsi_autopm_put_device(sdev);
 	if (ret)
--
2.20.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-10-06  9:43 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-06  9:40 [PATCH v2 0/2] Fix automatic CD tray loading for data reading via sr Thomas Schmitt
2020-10-06  9:40 ` [PATCH v2 1/2] cdrom: delegate automatic CD tray loading to callers of cdrom_open() Thomas Schmitt
2020-10-06  9:40 ` [PATCH v2 2/2] sr: fix automatic tray loading for data reading Thomas Schmitt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).