[PATCH] libmount: if ENOMEDIUM and tray is open, close tray and retry

[PATCH] libmount: if ENOMEDIUM and tray is open, close tray and retry

[Stanislav Brabec]

In past, d50c5917 introduced an interesting behavior: If mount was called on a
CDROM with open tray, the tray was closed and mount was retried. But the
implementation caused 15 seconds delay, so ca55a451 reverted it.

This is another attempt to implement that:
If tray is closed: No change, no delay, no retry, simply fail.
If tray is open: Close tray and retry after 3 seconds.

It can never cause delay more than time to close tray + 3 sec.

Signed-off-by: Stanislav Brabec <sbrabec@suse.cz>
---
 libmount/src/context_mount.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/libmount/src/context_mount.c b/libmount/src/context_mount.c
index 65f7dbfd0..80cc2e8c8 100644
--- a/libmount/src/context_mount.c
+++ b/libmount/src/context_mount.c
@@ -18,11 +18,15 @@
 
 #include <sys/wait.h>
 #include <sys/mount.h>
+#include <linux/cdrom.h>
 
 #include "linux_version.h"
 #include "mountP.h"
 #include "strutils.h"
 
+/* open() retries when errno is ENOMEDIUM and tray is open */
+#define CRDOM_TRAYOPEN_RETRIES    5
+
 /*
  * Kernel supports only one MS_PROPAGATION flag change by one mount(2) syscall,
  * to bypass this restriction we call mount(2) per flag. It's really not a perfect
@@ -1292,6 +1296,8 @@ int mnt_context_get_mount_excode(
 	int syserr;
 	struct stat st;
 	unsigned long uflags = 0, mflags = 0;
+	int cdrom;
+	unsigned int retries = 0;
 
 	int restricted = mnt_context_is_restricted(cxt);
 	const char *tgt = mnt_context_get_target(cxt);
@@ -1399,6 +1405,7 @@ int mnt_context_get_mount_excode(
 	/*
 	 * mount(2) errors
 	 */
+mount_retry:
 	syserr = mnt_context_get_syscall_errno(cxt);
 
 
@@ -1563,6 +1570,18 @@ int mnt_context_get_mount_excode(
 		break;
 
 	case ENOMEDIUM:
+		cdrom = open(mnt_context_get_source(cxt), O_RDONLY | O_NONBLOCK);
+		if (cdrom != -1) {
+			if (retries < CRDOM_TRAYOPEN_RETRIES &&
+			    ioctl(cdrom, CDROM_DRIVE_STATUS) == CDS_TRAY_OPEN) {
+				ioctl(cdrom, CDROMCLOSETRAY);
+				close(cdrom);
+				sleep(3);
+				++retries;
+				goto mount_retry;
+			} else
+				close(cdrom);
+		}
 		if (uflags & MNT_MS_NOFAIL)
 			return MNT_EX_SUCCESS;
 		if (buf)
-- 
2.13.2

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Křižíkova 148/34 (Corso IIa)                  tel: +49 911 7405384547
186 00 Praha 8-Karlín                          fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Re: [PATCH] libmount: if ENOMEDIUM and tray is open, close tray and retry

[Stanislav Brabec]

Stanislav Brabec wrote:
> In past, d50c5917 introduced an interesting behavior: If mount was called on a
> CDROM with open tray, the tray was closed and mount was retried. But the
> implementation caused 15 seconds delay, so ca55a451 reverted it.
> 
> This is another attempt to implement that:
> If tray is closed: No change, no delay, no retry, simply fail.
> If tray is open: Close tray and retry after 3 seconds.

I was just pointed to an improvement: There are drives that are not
capable to close tray. In such case, fail.

I am sending new version of the patch.

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Křižíkova 148/34 (Corso IIa)                  tel: +49 911 7405384547
186 00 Praha 8-Karlín                          fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

[PATCH] libmount: if ENOMEDIUM and tray is open, close tray and retry

[Stanislav Brabec]

In past, d50c5917 introduced an interesting behavior: If mount was called on a
CDROM with open tray, the tray was closed and mount was retried. But the
implementation caused 15 seconds delay, so ca55a451 reverted it.

This is another attempt to implement that:
If tray is closed: No delay, no retry, simply fail.
If tray is open: Check, whether the drive can close tray.
  If yes, close tray and retry after 3 seconds.
  If not, no delay, no retry, simply fail.

It can never cause delay more than time to close tray + 3 sec.

Signed-off-by: Stanislav Brabec <sbrabec@suse.cz>
---
 libmount/src/context_mount.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/libmount/src/context_mount.c b/libmount/src/context_mount.c
index 65f7dbfd0..ed0b84080 100644
--- a/libmount/src/context_mount.c
+++ b/libmount/src/context_mount.c
@@ -18,11 +18,15 @@
 
 #include <sys/wait.h>
 #include <sys/mount.h>
+#include <linux/cdrom.h>
 
 #include "linux_version.h"
 #include "mountP.h"
 #include "strutils.h"
 
+/* open() retries when errno is ENOMEDIUM and tray is open */
+#define CRDOM_TRAYOPEN_RETRIES    5
+
 /*
  * Kernel supports only one MS_PROPAGATION flag change by one mount(2) syscall,
  * to bypass this restriction we call mount(2) per flag. It's really not a perfect
@@ -1292,6 +1296,8 @@ int mnt_context_get_mount_excode(
 	int syserr;
 	struct stat st;
 	unsigned long uflags = 0, mflags = 0;
+	int cdrom;
+	unsigned int retries = 0;
 
 	int restricted = mnt_context_is_restricted(cxt);
 	const char *tgt = mnt_context_get_target(cxt);
@@ -1399,6 +1405,7 @@ int mnt_context_get_mount_excode(
 	/*
 	 * mount(2) errors
 	 */
+mount_retry:
 	syserr = mnt_context_get_syscall_errno(cxt);
 
 
@@ -1563,6 +1570,19 @@ int mnt_context_get_mount_excode(
 		break;
 
 	case ENOMEDIUM:
+		cdrom = open(mnt_context_get_source(cxt), O_RDONLY | O_NONBLOCK);
+		if (cdrom != -1) {
+			if (retries < CRDOM_TRAYOPEN_RETRIES &&
+			    (ioctl(cdrom, CDROM_GET_CAPABILITY, NULL) & CDC_CLOSE_TRAY) &&
+			    ioctl(cdrom, CDROM_DRIVE_STATUS, NULL) == CDS_TRAY_OPEN) {
+				ioctl(cdrom, CDROMCLOSETRAY);
+				close(cdrom);
+				sleep(3);
+				++retries;
+				goto mount_retry;
+			} else
+				close(cdrom);
+		}
 		if (uflags & MNT_MS_NOFAIL)
 			return MNT_EX_SUCCESS;
 		if (buf)
-- 
2.13.2

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Křižíkova 148/34 (Corso IIa)                  tel: +49 911 7405384547
186 00 Praha 8-Karlín                          fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Re: [PATCH] libmount: if ENOMEDIUM and tray is open, close tray and retry

[Yuriy M. Kaminskiy]

On 07/21/17 21:44 , Stanislav Brabec wrote:
> In past, d50c5917 introduced an interesting behavior: If mount was called on a
> CDROM with open tray, the tray was closed and mount was retried. But the
> implementation caused 15 seconds delay, so ca55a451 reverted it.
>
> This is another attempt to implement that:
> If tray is closed: No delay, no retry, simply fail.
> If tray is open: Check, whether the drive can close tray.
>   If yes, close tray and retry after 3 seconds.
>   If not, no delay, no retry, simply fail.
>
> It can never cause delay more than time to close tray + 3 sec.
>
> Signed-off-by: Stanislav Brabec <sbrabec@suse.cz>
> ---
>  libmount/src/context_mount.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/libmount/src/context_mount.c b/libmount/src/context_mount.c
> index 65f7dbfd0..ed0b84080 100644
> --- a/libmount/src/context_mount.c
> +++ b/libmount/src/context_mount.c
> @@ -18,11 +18,15 @@
>  
>  #include <sys/wait.h>
>  #include <sys/mount.h>
> +#include <linux/cdrom.h>
>  
>  #include "linux_version.h"
>  #include "mountP.h"
>  #include "strutils.h"
>  
> +/* open() retries when errno is ENOMEDIUM and tray is open */
> +#define CRDOM_TRAYOPEN_RETRIES    5
> +
>  /*
>   * Kernel supports only one MS_PROPAGATION flag change by one mount(2) syscall,
>   * to bypass this restriction we call mount(2) per flag. It's really not a perfect
> @@ -1292,6 +1296,8 @@ int mnt_context_get_mount_excode(
>  	int syserr;
>  	struct stat st;
>  	unsigned long uflags = 0, mflags = 0;
> +	int cdrom;
> +	unsigned int retries = 0;
>  
>  	int restricted = mnt_context_is_restricted(cxt);
>  	const char *tgt = mnt_context_get_target(cxt);
> @@ -1399,6 +1405,7 @@ int mnt_context_get_mount_excode(
>  	/*
>  	 * mount(2) errors
>  	 */
> +mount_retry:
>  	syserr = mnt_context_get_syscall_errno(cxt);
>  
>  
> @@ -1563,6 +1570,19 @@ int mnt_context_get_mount_excode(
>  		break;
>  
>  	case ENOMEDIUM:
> +		cdrom = open(mnt_context_get_source(cxt), O_RDONLY | O_NONBLOCK);
> +		if (cdrom != -1) {
> +			if (retries < CRDOM_TRAYOPEN_RETRIES &&
> +			    (ioctl(cdrom, CDROM_GET_CAPABILITY, NULL) & CDC_CLOSE_TRAY) &&

If ioctl is not supported, and ioctl() returns -1, (ioctl() & FOO) is
true.
I guess this should be something like
+                            (rc = ioctl(cdrom, CDROM_GET_CAPABILITY, NULL)) != -1 &&
+                             (rc & CDC_CLOSE_TRAY) &&
instead.

> +			    ioctl(cdrom, CDROM_DRIVE_STATUS, NULL) == CDS_TRAY_OPEN) {
> +				ioctl(cdrom, CDROMCLOSETRAY);

Error check missing?

(And, I used cdrom a long time ago, but IIRC there were some driver
settings to automatically close tray on device open, so that this patch
should not be needed?)

> +				close(cdrom);
> +				sleep(3);
> +				++retries;
> +				goto mount_retry;
> +			} else
> +				close(cdrom);
> +		}
>  		if (uflags & MNT_MS_NOFAIL)
>  			return MNT_EX_SUCCESS;
>  		if (buf)

Re: [PATCH] libmount: if ENOMEDIUM and tray is open, close tray and retry

[Stanislav Brabec]

Dne 27.7.2017 v 10:58 Yuriy M. Kaminskiy napsal(a):
>>  	case ENOMEDIUM:
>> +		cdrom = open(mnt_context_get_source(cxt), O_RDONLY | O_NONBLOCK);
>> +		if (cdrom != -1) {
>> +			if (retries < CRDOM_TRAYOPEN_RETRIES &&
>> +			    (ioctl(cdrom, CDROM_GET_CAPABILITY, NULL) & CDC_CLOSE_TRAY) &&
> 
> If ioctl is not supported, and ioctl() returns -1, (ioctl() & FOO) is
> true.

Yes, ugly code, but even if -1 passes the check, CDROM_DRIVE_STATUS will
not return CDS_TRAY_OPEN.

> I guess this should be something like
> +                            (rc = ioctl(cdrom, CDROM_GET_CAPABILITY, NULL)) != -1 &&
> +                             (rc & CDC_CLOSE_TRAY) &&
> instead.

That would be cleaner but more complicated.

> (And, I used cdrom a long time ago, but IIRC there were some driver
> settings to automatically close tray on device open, so that this patch
> should not be needed?)

Thanks for this hint. Yes, you are true:
/proc/sys/dev/cdrom/autoclose
/proc/sys/dev/cdrom/autoeject

But this does not work with the current mount. The mount request will
expire on timeout earlier than the tray of ordinary CDROM closes.

But yes, it is possible to implement a different, simpler patch that
will make the autoclose feature working, and not cause delays in case of
no medium.

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Křižíkova 148/34 (Corso IIa)                  tel: +49 911 7405384547
186 00 Praha 8-Karlín                          fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Re: [PATCH] libmount: if ENOMEDIUM and tray is open, close tray and retry

[Stanislav Brabec]

Dne 28.7.2017 v 17:26 Stanislav Brabec napsal(a):
> Dne 27.7.2017 v 10:58 Yuriy M. Kaminskiy napsal(a):
>> (And, I used cdrom a long time ago, but IIRC there were some driver
>> settings to automatically close tray on device open, so that this patch
>> should not be needed?)
> 
> Thanks for this hint. Yes, you are true:
> /proc/sys/dev/cdrom/autoclose
> /proc/sys/dev/cdrom/autoeject
> 
> But this does not work with the current mount. The mount request will
> expire on timeout earlier than the tray of ordinary CDROM closes.
> 
> But yes, it is possible to implement a different, simpler patch that
> will make the autoclose feature working, and not cause delays in case of
> no medium.
> 

I did some testing.

The kernel autoclose feature should do exactly the same as my patch
attempts. But kernel seems to be broken. Before the tray actually
closes, kernel returns ENOMEDIUM.

>From strace log when a standard DVD drive is open with a DVD laying on
the tray:

17:30:32.719041 access("/dev/sr0", F_OK) = 0 <0.000005>
(here the tray starts to close)
17:30:32.719054 open("/dev/sr0", O_RDONLY|O_CLOEXEC) = -1 ENOMEDIUM (No
medium found) <1.450436>
...
17:30:34.169943 mount("/dev/sr0", "/mnt", "vfat", MS_MGC_VAL|MS_SILENT,
NULL) = -1 ENOMEDIUM (No medium found) <0.083741>

I am nearly sure that it worked ~20 years ago.

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Křižíkova 148/34 (Corso IIa)                  tel: +49 911 7405384547
186 00 Praha 8-Karlín                          fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Re: [PATCH] libmount: if ENOMEDIUM and tray is open, close tray and retry

[Michal Suchánek]

On Fri, 28 Jul 2017 17:48:22 +0200
Stanislav Brabec <sbrabec@suse.cz> wrote:

> Dne 28.7.2017 v 17:26 Stanislav Brabec napsal(a):
> > Dne 27.7.2017 v 10:58 Yuriy M. Kaminskiy napsal(a):  
> >> (And, I used cdrom a long time ago, but IIRC there were some driver
> >> settings to automatically close tray on device open, so that this
> >> patch should not be needed?)  
> > 
> > Thanks for this hint. Yes, you are true:
> > /proc/sys/dev/cdrom/autoclose
> > /proc/sys/dev/cdrom/autoeject
> > 
> > But this does not work with the current mount. The mount request
> > will expire on timeout earlier than the tray of ordinary CDROM
> > closes.
> > 
> > But yes, it is possible to implement a different, simpler patch that
> > will make the autoclose feature working, and not cause delays in
> > case of no medium.
> >   
> 
> I did some testing.
> 
> The kernel autoclose feature should do exactly the same as my patch
> attempts. But kernel seems to be broken. Before the tray actually
> closes, kernel returns ENOMEDIUM.
> 
> From strace log when a standard DVD drive is open with a DVD laying on
> the tray:
> 
> 17:30:32.719041 access("/dev/sr0", F_OK) = 0 <0.000005>
> (here the tray starts to close)
> 17:30:32.719054 open("/dev/sr0", O_RDONLY|O_CLOEXEC) = -1 ENOMEDIUM
> (No medium found) <1.450436>
> ...
> 17:30:34.169943 mount("/dev/sr0", "/mnt", "vfat",
> MS_MGC_VAL|MS_SILENT, NULL) = -1 ENOMEDIUM (No medium found)
> <0.083741>
> 
> I am nearly sure that it worked ~20 years ago.
> 

Since we have facility for

 - telling if the drive is closed
 - telling if the drive can be closed
 - closing the drive

it is completely reasonable to return ENOMEDIUM from kernel when the
tray is open and let userspace decide if it wants to attempt to close
the tray and how long it wants to wait for the tray to close (it may be
blocked/broken).

Thanks

Michal

Re: [PATCH] libmount: if ENOMEDIUM and tray is open, close tray and retry

[Stanislav Brabec]

Michal Suchánek wrote:
> it is completely reasonable to return ENOMEDIUM from kernel when the
> tray is open and let userspace decide if it wants to attempt to close
> the tray and how long it wants to wait for the tray to close (it may be
> blocked/broken).
> 
But then, what is the purpose of /proc/sys/dev/cdrom/autoclose?

Just to start closing the tray?

If it is true, the logic should be:

If (error is ENOMEDIUM
    AND /proc/sys/dev/cdrom/autoclose is 1
    AND tray can be closed)
THEN
    sleep few seconds and retry


Alternatively, implement also:

If (error is ENOMEDIUM
    AND /proc/sys/dev/cdrom/autoclose is 0
    AND tray can be closed)
THEN
    close tray
    IF (close tray succeeded)
    THEN sleep few seconds and retry

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Křižíkova 148/34 (Corso IIa)                  tel: +49 911 7405384547
186 00 Praha 8-Karlín                          fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Re: [PATCH] libmount: if ENOMEDIUM and tray is open, close tray and retry

[Michal Suchánek]

On Mon, 2 Oct 2017 15:06:33 +0200
Stanislav Brabec <sbrabec@suse.cz> wrote:

> Michal Suchánek wrote:
> > it is completely reasonable to return ENOMEDIUM from kernel when the
> > tray is open and let userspace decide if it wants to attempt to
> > close the tray and how long it wants to wait for the tray to close
> > (it may be blocked/broken).
> >   
> But then, what is the purpose of /proc/sys/dev/cdrom/autoclose?

It can be used as a hint in mount if closing the tray is desired or
not.

I guess this used to work with IDE drivers not using SCSI emulation.

With SCSI the close tray command is executed like any other with
default timeout and obviously does time out in most cases. 

But how long should the kernel wait? Is 1 minute ok? Should it wait 5
minutes in case the user did not align the medium properly and needs to
realign it? Who should set this timeout, where?

Thanks

Michal

Re: [PATCH] libmount: if ENOMEDIUM and tray is open, close tray and retry

[Stanislav Brabec]

Michal Suchánek wrote:
> On Mon, 2 Oct 2017 15:06:33 +0200
> Stanislav Brabec <sbrabec@suse.cz> wrote:
> 
>> Michal Suchánek wrote:
>>> it is completely reasonable to return ENOMEDIUM from kernel when the
>>> tray is open and let userspace decide if it wants to attempt to
>>> close the tray and how long it wants to wait for the tray to close
>>> (it may be blocked/broken).
>>>   
>> But then, what is the purpose of /proc/sys/dev/cdrom/autoclose?
> 
> It can be used as a hint in mount if closing the tray is desired or
> not.

Such feature could be completely implemented in the user space, so there
is no reason to stay in the kernel in such a limited form.

20 years ago, it was a completely working implementation inside the
kernel. User space just called mount(), and it succeeded.

If that requires user space loop loop, then there is no reason to
implement sysfs entry just to perform tray close inside mount() syscall
and then fail anyway.

> I guess this used to work with IDE drivers not using SCSI emulation.
> 
> With SCSI the close tray command is executed like any other with
> default timeout and obviously does time out in most cases. 
> 
> But how long should the kernel wait? Is 1 minute ok? Should it wait 5
> minutes in case the user did not align the medium properly and needs to
> realign it? Who should set this timeout, where?
> 
The timeout should be equal to the time to close tray on the slowest
device. Guessing something about 3 to 5 seconds may be fine. After that
time (or several times until that time), just check when the tray starts
to report that it is closed.

By the way, most drives open the tray in the case of medium misalignment.

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Křižíkova 148/34 (Corso IIa)                  tel: +49 911 7405384547
186 00 Praha 8-Karlín                          fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Re: [PATCH] libmount: if ENOMEDIUM and tray is open, close tray and retry

[Michal Suchánek]

On Mon, 2 Oct 2017 16:11:04 +0200
Stanislav Brabec <sbrabec@suse.cz> wrote:

> Michal Suchánek wrote:
> > On Mon, 2 Oct 2017 15:06:33 +0200
> > Stanislav Brabec <sbrabec@suse.cz> wrote:
> >   
> >> Michal Suchánek wrote:  
> >>> it is completely reasonable to return ENOMEDIUM from kernel when
> >>> the tray is open and let userspace decide if it wants to attempt
> >>> to close the tray and how long it wants to wait for the tray to
> >>> close (it may be blocked/broken).
> >>>     
> >> But then, what is the purpose of /proc/sys/dev/cdrom/autoclose?  
> > 
> > It can be used as a hint in mount if closing the tray is desired or
> > not.  
> 
> Such feature could be completely implemented in the user space, so
> there is no reason to stay in the kernel in such a limited form.

Then mount should probably get an option to attempt to close the tray
or not. Closing the tray is not always desirable.

Thanks

Michal

Re: [PATCH] libmount: if ENOMEDIUM and tray is open, close tray and retry

[Michal Suchánek]

On Mon, 2 Oct 2017 16:11:04 +0200
Stanislav Brabec <sbrabec@suse.cz> wrote:

> Michal Suchánek wrote:
> > On Mon, 2 Oct 2017 15:06:33 +0200
> > Stanislav Brabec <sbrabec@suse.cz> wrote:
> >   
> >> Michal Suchánek wrote:  
> >>> it is completely reasonable to return ENOMEDIUM from kernel when
> >>> the tray is open and let userspace decide if it wants to attempt
> >>> to close the tray and how long it wants to wait for the tray to
> >>> close (it may be blocked/broken).
> >>>     
> >> But then, what is the purpose of /proc/sys/dev/cdrom/autoclose?  
> > 
> > It can be used as a hint in mount if closing the tray is desired or
> > not.  
> 
> Such feature could be completely implemented in the user space, so
> there is no reason to stay in the kernel in such a limited form.
> 
> 20 years ago, it was a completely working implementation inside the
> kernel. User space just called mount(), and it succeeded.
> 
> If that requires user space loop loop, then there is no reason to
> implement sysfs entry just to perform tray close inside mount()
> syscall and then fail anyway.
> 
> > I guess this used to work with IDE drivers not using SCSI emulation.
> > 
> > With SCSI the close tray command is executed like any other with
> > default timeout and obviously does time out in most cases. 
> > 
> > But how long should the kernel wait? Is 1 minute ok? Should it wait
> > 5 minutes in case the user did not align the medium properly and
> > needs to realign it? Who should set this timeout, where?
> >   
> The timeout should be equal to the time to close tray on the slowest
> device. Guessing something about 3 to 5 seconds may be fine. After
> that time (or several times until that time), just check when the
> tray starts to report that it is closed.

It may take 5s to physically close the drive but it takes much longer
until the medium is detected and can be read. I suspect there might be
some issue with interpreting the timeout in SCSI ioctls, though.

You get 

/* The CDROM is fairly slow, so we need a little extra time */
/* In fact, it is very slow if it has to spin up first */
#define IOCTL_TIMEOUT 30*HZ

which looks like the intended timeout is 30s.

However, the timeout is passed like

cgc.timeout = IOCTL_TIMEOUT (units for struct packet_command
unspecified)

result = scsi_execute(SDev, cgc->cmd, cgc->data_direction,
                              cgc->buffer, cgc->buflen,
                              (unsigned char *)cgc->sense, &sshdr,
                              cgc->timeout, IOCTL_RETRIES, 0, 0, NULL);

and finally

**
 * scsi_execute - insert request and wait for the result
 * @sdev:       scsi device
 * @cmd:        scsi command
 * @data_direction: data direction
 * @buffer:     data buffer
 * @bufflen:    len of buffer
 * @sense:      optional sense buffer
 * @sshdr:      optional decoded sense header
 * @timeout:    request timeout in seconds
 * @retries:    number of times to retry request
 * @flags:      flags for ->cmd_flags
 * @rq_flags:   flags for ->rq_flags
 * @resid:      optional residual length
 *
 * Returns the scsi_cmnd result field if a command was executed, or a
negative
 * Linux error code if we didn't get that far.
 */
int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
                 int data_direction, void *buffer, unsigned bufflen,
                 unsigned char *sense, struct scsi_sense_hdr *sshdr,
                 int timeout, int retries, u64 flags, req_flags_t
rq_flags, int *resid)


meaning that the 30*HZ value is interpreted as seconds if the docs are
right. Looks fishy to me.

Thanks

Michal

Re: [PATCH] libmount: if ENOMEDIUM and tray is open, close tray and retry

[Michal Suchánek]

On Mon, 2 Oct 2017 16:11:04 +0200
Stanislav Brabec <sbrabec@suse.cz> wrote:

> Michal Suchánek wrote:
> > On Mon, 2 Oct 2017 15:06:33 +0200
> > Stanislav Brabec <sbrabec@suse.cz> wrote:
> >   
> >> Michal Suchánek wrote:  
> >>> it is completely reasonable to return ENOMEDIUM from kernel when
> >>> the tray is open and let userspace decide if it wants to attempt
> >>> to close the tray and how long it wants to wait for the tray to
> >>> close (it may be blocked/broken).
> >>>     
> >> But then, what is the purpose of /proc/sys/dev/cdrom/autoclose?  
> > 
> > It can be used as a hint in mount if closing the tray is desired or
> > not.  
> 
> Such feature could be completely implemented in the user space, so
> there is no reason to stay in the kernel in such a limited form.
> 
> 20 years ago, it was a completely working implementation inside the
> kernel. User space just called mount(), and it succeeded.

Ok, if you can point on a particular kernel version and driver that
worked it might be interesting to see the implementation.

> 
> If that requires user space loop loop, then there is no reason to
> implement sysfs entry just to perform tray close inside mount()
> syscall and then fail anyway.
> 

As things are the kernel calls the tray_move function to close the
tray. This function exits before the tray closes. The kernel proceeds
to try to read the medium. There is a wait loop in ioctl calls (but not
block calls) to check for ASC/ASCQ 04/01 (unit becoming ready).
However, my cdrom reports 3A/02 (no medium, tray open) while the tray
is closing. There is no code for 'tray closing'. Hence the open() call
is not aware that the tray is supposed to close shortly and does not
wait for it.

Tracking the 'closing' state in the kernel does not look pretty. Best
kernel-side option would be to wait for some arbitrary time in the
tray_move function until the drive state changes from 'tray open'. I
suspect waiting for arbitrary time or indefinitely until the tray
closes may upset people. Then again, this is CD drive so it is anything
but fast (unless emulated ;-)

Thanks

Michal

Re: [PATCH] libmount: if ENOMEDIUM and tray is open, close tray and retry

[Karel Zak]

On Fri, Jul 21, 2017 at 08:44:03PM +0200, Stanislav Brabec wrote:
> @@ -1399,6 +1405,7 @@ int mnt_context_get_mount_excode(
>  	/*
>  	 * mount(2) errors
>  	 */
> +mount_retry:
>  	syserr = mnt_context_get_syscall_errno(cxt);
>  
>  
> @@ -1563,6 +1570,19 @@ int mnt_context_get_mount_excode(
>  		break;
>  
>  	case ENOMEDIUM:
> +		cdrom = open(mnt_context_get_source(cxt), O_RDONLY | O_NONBLOCK);
> +		if (cdrom != -1) {
> +			if (retries < CRDOM_TRAYOPEN_RETRIES &&
> +			    (ioctl(cdrom, CDROM_GET_CAPABILITY, NULL) & CDC_CLOSE_TRAY) &&
> +			    ioctl(cdrom, CDROM_DRIVE_STATUS, NULL) == CDS_TRAY_OPEN) {
> +				ioctl(cdrom, CDROMCLOSETRAY);
> +				close(cdrom);
> +				sleep(3);
> +				++retries;
> +				goto mount_retry;
> +			} else
> +				close(cdrom);
> +		}

This cannot work.

It seem you have tested the tray-close only, but I guess your goal is
also *mount* the device. The "goto mount_retry" does not call any
mount(2) stuff :-)

The function mnt_context_get_mount_excode() is wrong place. This
optional function is there to generate error message and exit code.
Nothing else.

The right place for your code is mnt_context_mount() where we already
have the same logic ("try mount again") for read-only devices. You
need something like:

 if (mnt_context_syscall_called(cxt) &&
     mnt_context_get_syscall_errno(cxt) == ENOMEDIUM) {

    ... your open() and ioctls ...

    goto again;
 }


Use findmnt(8) to be sure that CDROM is really mounted.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] libmount: if ENOMEDIUM and tray is open, close tray and retry

[Karel Zak]

On Tue, Aug 01, 2017 at 02:16:15PM +0200, Karel Zak wrote:
> On Fri, Jul 21, 2017 at 08:44:03PM +0200, Stanislav Brabec wrote:
> > @@ -1399,6 +1405,7 @@ int mnt_context_get_mount_excode(
> >  	/*
> >  	 * mount(2) errors
> >  	 */
> > +mount_retry:
> >  	syserr = mnt_context_get_syscall_errno(cxt);
> >  
> >  
> > @@ -1563,6 +1570,19 @@ int mnt_context_get_mount_excode(
> >  		break;
> >  
> >  	case ENOMEDIUM:
> > +		cdrom = open(mnt_context_get_source(cxt), O_RDONLY | O_NONBLOCK);
> > +		if (cdrom != -1) {
> > +			if (retries < CRDOM_TRAYOPEN_RETRIES &&
> > +			    (ioctl(cdrom, CDROM_GET_CAPABILITY, NULL) & CDC_CLOSE_TRAY) &&
> > +			    ioctl(cdrom, CDROM_DRIVE_STATUS, NULL) == CDS_TRAY_OPEN) {
> > +				ioctl(cdrom, CDROMCLOSETRAY);
> > +				close(cdrom);
> > +				sleep(3);
> > +				++retries;
> > +				goto mount_retry;
> > +			} else
> > +				close(cdrom);
> > +		}
> 
> This cannot work.

Stanislav, do you want to update the patch before the next release?

    Karel

> 
> It seem you have tested the tray-close only, but I guess your goal is
> also *mount* the device. The "goto mount_retry" does not call any
> mount(2) stuff :-)
> 
> The function mnt_context_get_mount_excode() is wrong place. This
> optional function is there to generate error message and exit code.
> Nothing else.
> 
> The right place for your code is mnt_context_mount() where we already
> have the same logic ("try mount again") for read-only devices. You
> need something like:
> 
>  if (mnt_context_syscall_called(cxt) &&
>      mnt_context_get_syscall_errno(cxt) == ENOMEDIUM) {
> 
>     ... your open() and ioctls ...
> 
>     goto again;
>  }
> 
> 
> Use findmnt(8) to be sure that CDROM is really mounted.
> 
>     Karel
> 
> -- 
>  Karel Zak  <kzak@redhat.com>
>  http://karelzak.blogspot.com

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] libmount: if ENOMEDIUM and tray is open, close tray and retry

[Stanislav Brabec]

Karel Zak wrote:
> Stanislav, do you want to update the patch before the next release?

Probably not. Kernel autoclose feature should be fixed instead, and this
patch will not be needed.

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Křižíkova 148/34 (Corso IIa)                  tel: +49 911 7405384547
186 00 Praha 8-Karlín                          fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76