linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: CDROM ioctl bug (fwd)
@ 2001-11-28 18:44 Ron Lawrence
  2001-11-28 23:51 ` Peter Osterlund
  0 siblings, 1 reply; 7+ messages in thread
From: Ron Lawrence @ 2001-11-28 18:44 UTC (permalink / raw)
  To: linux-kernel

-----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
rlawrence@netraverse.com

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 <axboe@suse.de>
 To: Ron Lawrence <rlawrence@netraverse.com>
 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-----



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

* Re: CDROM ioctl bug (fwd)
  2001-11-28 18:44 CDROM ioctl bug (fwd) Ron Lawrence
@ 2001-11-28 23:51 ` Peter Osterlund
  2001-11-29 17:17   ` Ron Lawrence
  2001-11-29 17:27   ` Jens Axboe
  0 siblings, 2 replies; 7+ messages in thread
From: Peter Osterlund @ 2001-11-28 23:51 UTC (permalink / raw)
  To: Ron Lawrence; +Cc: linux-kernel, Jens Axboe

Ron Lawrence <rlawrence@netraverse.com> 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             petero2@telia.com
Sköndalsvägen 35            http://w1.894.telia.com/~u89404340
S-128 66 Sköndal            +46 8 942647
Sweden

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

* Re: CDROM ioctl bug (fwd)
  2001-11-28 23:51 ` Peter Osterlund
@ 2001-11-29 17:17   ` Ron Lawrence
  2001-11-29 17:27   ` Jens Axboe
  1 sibling, 0 replies; 7+ messages in thread
From: Ron Lawrence @ 2001-11-29 17:17 UTC (permalink / raw)
  To: Peter Osterlund; +Cc: linux-kernel, Jens Axboe

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 1397 bytes --]

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

On 29 Nov 2001, Peter Osterlund wrote:
>Ron Lawrence <rlawrence@netraverse.com> 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             petero2@telia.com
>Sköndalsvägen 35            http://w1.894.telia.com/~u89404340
>S-128 66 Sköndal            +46 8 942647
>Sweden

Ron Lawrence
rlawrence@NeTraverse.com
-----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-----



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

* Re: CDROM ioctl bug (fwd)
  2001-11-28 23:51 ` Peter Osterlund
  2001-11-29 17:17   ` Ron Lawrence
@ 2001-11-29 17:27   ` Jens Axboe
  2001-11-29 18:38     ` Ron Lawrence
  1 sibling, 1 reply; 7+ messages in thread
From: Jens Axboe @ 2001-11-29 17:27 UTC (permalink / raw)
  To: Peter Osterlund; +Cc: Ron Lawrence, linux-kernel

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


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

* Re: CDROM ioctl bug (fwd)
  2001-11-29 17:27   ` Jens Axboe
@ 2001-11-29 18:38     ` Ron Lawrence
  2001-11-29 18:48       ` Jens Axboe
  0 siblings, 1 reply; 7+ messages in thread
From: Ron Lawrence @ 2001-11-29 18:38 UTC (permalink / raw)
  To: Douglas Gilbert; +Cc: Peter Osterlund, Jens Axboe, linux-kernel

-----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
rlawrence@NeTraverse.com


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



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

* Re: CDROM ioctl bug (fwd)
  2001-11-29 18:38     ` Ron Lawrence
@ 2001-11-29 18:48       ` Jens Axboe
  0 siblings, 0 replies; 7+ messages in thread
From: Jens Axboe @ 2001-11-29 18:48 UTC (permalink / raw)
  To: Ron Lawrence; +Cc: Douglas Gilbert, Peter Osterlund, linux-kernel

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


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

* Re: CDROM ioctl bug (fwd)
@ 2001-11-29 14:21 Douglas Gilbert
  0 siblings, 0 replies; 7+ messages in thread
From: Douglas Gilbert @ 2001-11-29 14:21 UTC (permalink / raw)
  To: Peter Osterlund, linux-kernel; +Cc: linux-scsi, Jens Axboe

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


vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
Peter Osterlund <petero2@telia.com> wrote:

Ron Lawrence <rlawrence@netraverse.com> 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 )
        {

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

end of thread, other threads:[~2001-11-29 18:49 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2001-11-28 18:44 CDROM ioctl bug (fwd) Ron Lawrence
2001-11-28 23:51 ` Peter Osterlund
2001-11-29 17:17   ` Ron Lawrence
2001-11-29 17:27   ` Jens Axboe
2001-11-29 18:38     ` Ron Lawrence
2001-11-29 18:48       ` Jens Axboe
2001-11-29 14:21 Douglas Gilbert

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