2.4.X Avoid excessive recursion when setting device offline

PATCH In review Justin T. Gibbs 4 participants 6 messages Mar 29, 2004 – Mar 30, 2004 SCSI

lore

Share & tools

lore

Justin T. Gibbs

author

2004-03-29 17:07 UTC #0 lore raw

PATCH 1/1 2.4.X Avoid excessive recursion when setting device offline

How to reproduce: Queue up lots of I/O to a device. Set that device
offline.

Result: The SCSI mid-layer blows out the kernel stack due to the inherent

	recursion in its request function.  The repeating stack trace goes
	something like:

	scsi_request_fn
	__scsi_end_request
	scsi_release_command
	scsi_queue_next_request
	scsi_request_fn
	...

This patch prevents recursive calls through the scsi_request_fn for the same queue:

===== scsi.h 1.9 vs edited =====

scsi.h renamed from drivers/scsi/scsi.h +1 -0

@@ -609,6 +609,7 @@
 	unsigned starved:1;	/* unable to process commands because
 				   host busy */
 	unsigned no_start_on_add:1;	/* do not issue start on add */
+	unsigned running_request_fn:1;
 
 	// Flag to allow revalidate to succeed in sd_open
 	int allow_revalidate;

===== scsi_lib.c 1.17 vs edited =====

scsi_lib.c renamed from drivers/scsi/scsi_lib.c +11 -2

@@ -850,12 +850,20 @@
 	if (!SDpnt) {
 		panic("Missing device");
 	}
+
 	SHpnt = SDpnt->host;
 
 	/*
 	 * To start with, we keep looping until the queue is empty, or until
-	 * the host is no longer able to accept any more requests.
+	 * the host is no longer able to accept any more requests.  We avoid
+	 * excessive recursion through the SCSI layer by recording that we
+	 * are running this queue and rejecting requests to run it again. This
+	 * avoids blowing out the stack when multiple commands are failed
+	 * during our loop.
 	 */
+	if (SDpnt->running_request_fn)
+		return;
+	SDpnt->running_request_fn = 1;
 	while (1 == 1) {
 		/*
 		 * Check this again - each time we loop through we will have

@@ -863,7 +871,7 @@
 		 * we need to check to see if the queue is plugged or not.
 		 */
 		if (SHpnt->in_recovery || q->plugged)
-			return;
+			break;
 
 		/*
 		 * If the device cannot accept another request, then quit.

@@ -1099,6 +1107,7 @@
 		 */
 		spin_lock_irq(&io_request_lock);
 	}
+	SDpnt->running_request_fn = 0;
 }
 
 /*

Arjan van de Ven

2004-03-29 17:26 UTC #1 lore raw

Quoted 14 lines · Justin T. Gibbs

How to reproduce: Queue up lots of I/O to a device. Set that device
offline.
Result: The SCSI mid-layer blows out the kernel stack due to the inherent
	recursion in its request function.  The repeating stack trace goes
	something like:

	scsi_request_fn
	__scsi_end_request
	scsi_release_command
	scsi_queue_next_request
	scsi_request_fn
	...

Quoted 2 lines

This patch prevents recursive calls through the scsi_request_fn for the same queue:

won't this achieve the same ?

diff -urN Linux/drivers/scsi/scsi_lib.c linux/drivers/scsi/scsi_lib.c
--- Linux/drivers/scsi/scsi_lib.c 2002-10-03 16:50:07.000000000
+0200
+++ linux/drivers/scsi/scsi_lib.c 2002-10-03 17:33:20.000000000
+0200
@@ -442,7 +442,7 @@
* This will goose the queue request function at the end, so we
don't
* need to worry about launching another command.
*/
- scsi_release_command(SCpnt);
+ __scsi_release_command(SCpnt);

        if( frequeue ) {
                request_queue_t *q;

drivers/scsi/scsi_lib.c +1 -1

diff -urN Linux/drivers/scsi/scsi_lib.c linux/drivers/scsi/scsi_lib.c
@@ -442,7 +442,7 @@
 	 * This will goose the queue request function at the end, so we don't
 	 * need to worry about launching another command.
 	 */
-	scsi_release_command(SCpnt);
+	__scsi_release_command(SCpnt);
 
 	if( frequeue ) {
 		request_queue_t *q;

Justin T. Gibbs

author

2004-03-29 17:42 UTC #2 lore raw

Quoted 4 lines

Quoted 2 lines
This patch prevents recursive calls through the scsi_request_fn for the same queue:
won't this achieve the same ?

Yes, it appears it will and that the latest 2.4.X kernels already have this change. My only concern stems from the need to audit all of the callers of __scsi_end_request to ensure they are setting the frequeue flag correctly.

--
Justin

James Bottomley

2004-03-29 17:48 UTC #3 lore raw

In reply to Arjan van de Ven

drivers/scsi/scsi_lib.c line 445

Quoted 8 lines · Arjan van de Ven

won't this achieve the same ?
diff -urN Linux/drivers/scsi/scsi_lib.c linux/drivers/scsi/scsi_lib.c
--- Linux/drivers/scsi/scsi_lib.c 2002-10-03 16:50:07.000000000
+0200
+++ linux/drivers/scsi/scsi_lib.c 2002-10-03 17:33:20.000000000
drivers/scsi/scsi_lib.c +1 -1
-       scsi_release_command(SCpnt);
+       __scsi_release_command(SCpnt);

Yes, this patch is in 2.4.25 already. I seem to recall that's why it was applied several years ago (to prevent recursion), but it's too long ago for me to remember (or even bitkeeper, since it's also in scsi_lib.c version 1.1).

Exactly which version of 2.4 is exhibiting this problem?

James

Justin T. Gibbs

author

2004-03-29 17:52 UTC #4 lore raw

Quoted 6 lines

Yes, this patch is in 2.4.25 already. I seem to recall that's why it was applied several years ago (to prevent recursion), but it's too long ago for me to remember (or even bitkeeper, since it's also in scsi_lib.c version 1.1).
Exactly which version of 2.4 is exhibiting this problem?

I was looking at RHEL3. Sorry for the noise on the lists. Arjan. Please refer to bugzilla 119334.

--
Justin

Jens Axboe

2004-03-30 07:04 UTC #5 lore raw

In reply to Arjan van de Ven

Quoted 41 lines · Arjan van de Ven

Quoted 14 lines · Justin T. Gibbs
How to reproduce: Queue up lots of I/O to a device. Set that device
offline.
Result: The SCSI mid-layer blows out the kernel stack due to the inherent
	recursion in its request function.  The repeating stack trace goes
	something like:

	scsi_request_fn
	__scsi_end_request
	scsi_release_command
	scsi_queue_next_request
	scsi_request_fn
	...
Quoted 2 lines
This patch prevents recursive calls through the scsi_request_fn for the same queue:
won't this achieve the same ?
diff -urN Linux/drivers/scsi/scsi_lib.c linux/drivers/scsi/scsi_lib.c
--- Linux/drivers/scsi/scsi_lib.c 2002-10-03 16:50:07.000000000
+0200
+++ linux/drivers/scsi/scsi_lib.c 2002-10-03 17:33:20.000000000
+0200
(quoted diff) +0 -0
@@ -442,7 +442,7 @@
         * This will goose the queue request function at the end, so we
don't
* need to worry about launching another command. */
drivers/scsi/scsi_lib.c +1 -1
-       scsi_release_command(SCpnt);
+       __scsi_release_command(SCpnt);
 
        if( frequeue ) {
                request_queue_t *q;
diff -urN Linux/drivers/scsi/scsi_lib.c linux/drivers/scsi/scsi_lib.c

drivers/scsi/scsi_lib.c line 448

Quoted 8 lines

drivers/scsi/scsi_lib.c +1 -1

@@ -442,7 +442,7 @@
 	 * This will goose the queue request function at the end, so we don't
 	 * need to worry about launching another command.
 	 */
-	scsi_release_command(SCpnt);
+	__scsi_release_command(SCpnt);
 
 	if( frequeue ) {
 		request_queue_t *q;

That is better. If SCSI used blk_start_queue/stop_queue, it has recursion detection as well (only allowing one reenter).

Signature · 1 line

Jens Axboe