2001-11-28 18:48:37

by Ron Lawrence

[permalink] [raw]
Subject: Re: CDROM ioctl bug (fwd)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi all,

Jens told me to go ahead and post to this list, since he has been very
busy. Here are the symptoms of my problem : doing reads from a CDROM
device intermingled with CDROM_MEDIA_CHANGED ioctls causes long pauses
during the ioctl. This behavior started in 2.4.10. The ioctl can take a
very long time to return, especially if reading large chunks.

FYI, I tried adjusting the max/min-readahead that appeared in 2.4.16, but
these seemed to have no effect on the problem. If anyone has time to look
into this, or can point me at a likely place to start looking, I would
appreciate it.

Ron Lawrence
[email protected]

Here is a program that will recreate the problem consistently (larger
numbers produce it more consistently):

- -------- %< ------------
#include <stdio.h>
#include <fcntl.h>
#include <linux/cdrom.h>
/* usage cdr [cd-device [bytes to read] ] */
int
main(int argc, char *argv[]) {
int cdfd, i, j, k;
char *infile;
char c;
int read_bytes = argc > 2 ? atoi(argv[2]) : 65535;
char buf[500000];
char *openit = argc > 1 ? argv[1] : "/dev/cdrom";

printf("opening %s, reading %d at a time\n", openit, read_bytes);
if ((cdfd = open(openit, O_RDONLY) ) == -1) {
exit(2);
}

for (i=0; i<50 ;i++) {
j=time();
printf("%2.2d Reading %d bytes\n", i, read_bytes);
read(cdfd, buf, read_bytes);
printf("%2.2d Calling media change ioctl:\n",i);
ioctl(cdfd, CDROM_MEDIA_CHANGED, CDSL_CURRENT);
k=time();
printf("%2.2d done (%d)\n", i, (k-j));
}
return 0;
}
- -------- %< ------------

- ---------- Forwarded message ----------
Date: Wed, 28 Nov 2001 08:01:20 +0100
From: Jens Axboe <[email protected]>
To: Ron Lawrence <[email protected]>
Subject: Re: CDROM ioctl bug

On Tue, Nov 27 2001, Ron Lawrence wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi Jens,
>
> I'm just writing to ask whether you have had any luck finding the
> CDROM
> MEDIA_CHANGE ioctl bug yet. If not, would you mind my posting of the
> sample program to the kernel list to see if anyone else has time to
> debug it?

No time yet, but please go ahead and post the sample program so someone
else can debug it for now.

--
Jens Axboe
- --------------------------------------------

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.6 (GNU/Linux)
Comment: For info see http://www.gnupg.org

iD8DBQE8BTBzU0yq8UBYK2oRAmQfAJ4/YaXWrDcWfLcVRREHetBGwAolfwCfeJIt
Z3/z1rseKLU1N1p/gspjieU=
=+NEN
-----END PGP SIGNATURE-----



2001-11-28 23:52:44

by Peter Osterlund

[permalink] [raw]
Subject: Re: CDROM ioctl bug (fwd)

Ron Lawrence <[email protected]> writes:

> busy. Here are the symptoms of my problem : doing reads from a CDROM
> device intermingled with CDROM_MEDIA_CHANGED ioctls causes long pauses
> during the ioctl. This behavior started in 2.4.10. The ioctl can take a
> very long time to return, especially if reading large chunks.

This patch fixes the problem for my USB CDROM device. Maybe a similar
patch is needed for the IDE case, I haven't looked yet.

In general, who is responsible for unplugging the request queue after
queuing an ioctl command?

--- linux/drivers/scsi/scsi.c.old Thu Nov 29 00:42:16 2001
+++ linux/drivers/scsi/scsi.c Thu Nov 29 00:32:28 2001
@@ -767,14 +767,17 @@
void scsi_wait_req (Scsi_Request * SRpnt, const void *cmnd ,
void *buffer, unsigned bufflen,
int timeout, int retries)
{
+ request_queue_t *q;
DECLARE_COMPLETION(wait);

SRpnt->sr_request.waiting = &wait;
SRpnt->sr_request.rq_status = RQ_SCSI_BUSY;
scsi_do_req (SRpnt, (void *) cmnd,
buffer, bufflen, scsi_wait_done, timeout, retries);
+ q = &SRpnt->sr_device->request_queue;
+ generic_unplug_device(q);
wait_for_completion(&wait);
SRpnt->sr_request.waiting = NULL;
if( SRpnt->sr_command != NULL )
{

--
Peter ?sterlund [email protected]
Sk?ndalsv?gen 35 http://w1.894.telia.com/~u89404340
S-128 66 Sk?ndal +46 8 942647
Sweden

2001-11-29 17:25:41

by Ron Lawrence

[permalink] [raw]
Subject: Re: CDROM ioctl bug (fwd)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 29 Nov 2001, Peter Osterlund wrote:
>Ron Lawrence <[email protected]> writes:
>> busy. Here are the symptoms of my problem : doing reads from a CDROM
>> device intermingled with CDROM_MEDIA_CHANGED ioctls causes long pauses
>> during the ioctl. This behavior started in 2.4.10. The ioctl can take a
>> very long time to return, especially if reading large chunks.
>This patch fixes the problem for my USB CDROM device. Maybe a similar
>patch is needed for the IDE case, I haven't looked yet.

Thanks! I should have mentioned that it affects CDROM drives when accessed
via ide-scsi, but not when accessed "normally" via ide. So, your patch
helps this case too. It is still significantly slower than "normal"
access, and I will run some tests to see if it's still slower in this case
than it was in 2.4.9.

>In general, who is responsible for unplugging the request queue after
>queuing an ioctl command?
>
>Peter ?sterlund [email protected]
>Sk?ndalsv?gen 35 http://w1.894.telia.com/~u89404340
>S-128 66 Sk?ndal +46 8 942647
>Sweden

Ron Lawrence
[email protected]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.6 (GNU/Linux)
Comment: For info see http://www.gnupg.org

iD8DBQE8Bm3GU0yq8UBYK2oRAsH/AJ9fy5LQSTiES5PD0BczAb82EXrsYgCaA3sI
zeX3IuZnQzh7B80TT4oJH4M=
=+UKy
-----END PGP SIGNATURE-----


2001-11-29 17:28:31

by Jens Axboe

[permalink] [raw]
Subject: Re: CDROM ioctl bug (fwd)

On Thu, Nov 29 2001, Peter Osterlund wrote:
> In general, who is responsible for unplugging the request queue after
> queuing an ioctl command?

The queuer is responsible for that. As Doug mentioned, you have the same
race that was long standing in sg as well which I fixed some months ago.

--
Jens Axboe

2001-11-29 18:49:05

by Ron Lawrence

[permalink] [raw]
Subject: Re: CDROM ioctl bug (fwd)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Thu, 29 Nov 2001, Douglas Gilbert wrote:

> Peter,
> That patch is flawed as Jens and I found out the hard way
> in the sg driver. The scsi_do_req() can lead to the pointer
> chain on the following assignment into q being invalid (in
> the worst case).
>
> The easy fix is to move the assignment into q _before_
> the call to scsi_do_req().
>
> Doug Gilbert

Douglas, Moving the assignment up did the trick. The responsiveness is
back to it's old self again.

Thanks.

Jens, will you take care of submitting this patch, so it can be fixed in
the mainline kernel, or do I need to do something? I'm happy to do
whatever it takes to get this in.

Ron Lawrence
[email protected]


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.6 (GNU/Linux)
Comment: For info see http://www.gnupg.org

iD8DBQE8BoCaU0yq8UBYK2oRAuENAKCWIZ2+ulSLsC7rG7+hjo2vy6UsYgCgsGIm
wRS6Pkb6G3mITKZ1aciMKtM=
=Z/Cy
-----END PGP SIGNATURE-----


2001-11-29 18:49:15

by Jens Axboe

[permalink] [raw]
Subject: Re: CDROM ioctl bug (fwd)

On Thu, Nov 29 2001, Ron Lawrence wrote:
> Jens, will you take care of submitting this patch, so it can be fixed in
> the mainline kernel, or do I need to do something? I'm happy to do
> whatever it takes to get this in.

Sure I'll include it right away.

--
Jens Axboe