Skip to contentSkip to conversation

smp dead lock of io_request_lock/queue_lock patch

PATCH Peter Yao 5 participants 11 messages Jan 12, 2004 – Jan 15, 2004 SCSI

lore
Share & tools
lore

Peter Yao

author
#0 lore raw

Hi,
I found a smp dead lock in io_request_lock/queue_lock patch in redhat's 2.4.18-4 kernel. I don't know how this patch is going on, just put my fix for it here. :)
The dead lock is for scsi host->lock and scsi q->queue_lock between scsi_restart_operations@scsi_error.c and scsi_request_fn@scsi_lib.c.

Index: scsi_error.c
===================================================================
RCS file:
/home/cvsroot/ieee1394_driver/linux-2.4.18-3/drivers/scsi/scsi_error.c,v retrieving revision 1.13
retrieving revision 1.13.8.1
diff -Llinux-2.4.18-3/drivers/scsi/scsi_error.c -Llinux-2.4.18-3/drivers/scsi/scsi_error.c -u -d -r1.13 -r1.13.8.1

drivers/scsi/scsi_error.c +2 -2
@@ -1293,11 +1293,11 @@
 			break;
 		}
 		q = &SDpnt->request_queue;
-		spin_lock(q->queue_lock);
 		spin_unlock(host->lock);
+		spin_lock(q->queue_lock);
 		q->request_fn(q);
-		spin_lock(host->lock);
 		spin_unlock(q->queue_lock);
+		spin_lock(host->lock);
 	}
 	spin_unlock_irqrestore(host->lock, flags);
 }
#1 lore raw
Quoted 3 lines · Peter Yao

Hi,
I found a smp dead lock in io_request_lock/queue_lock patch in redhat's 2.4.18-4 kernel.

1) Bugs in vendor kernels are best reported to the vendor, in this case in http://bugzilla.redhat.com
2) 2.4.18-4 is really old and has been superceeded by updates a few dozen times
3) 2.4.18-4 has both remote and local security issues and datacorruption issues

#2 lore raw
Quoted 11 lines · Arjan van de Ven
Quoted 3 lines · Peter Yao

Hi,
I found a smp dead lock in io_request_lock/queue_lock patch in redhat's 2.4.18-4 kernel.

1) Bugs in vendor kernels are best reported to the vendor, in this case in http://bugzilla.redhat.com
2) 2.4.18-4 is really old and has been superceeded by updates a few dozen times
3) 2.4.18-4 has both remote and local security issues and datacorruption issues

4) The problem reported and patch looks real, though, maybe a little credit and thanks would be appropriate as well?

Signature · 1 line
Jens Axboe
#3 lore raw
Quoted 13 lines · Jens Axboe
Quoted 11 lines · Arjan van de Ven
Quoted 3 lines · Peter Yao

Hi,
I found a smp dead lock in io_request_lock/queue_lock patch in redhat's 2.4.18-4 kernel.

1) Bugs in vendor kernels are best reported to the vendor, in this case in http://bugzilla.redhat.com
2) 2.4.18-4 is really old and has been superceeded by updates a few dozen times
3) 2.4.18-4 has both remote and local security issues and datacorruption issues

Quoted 2 lines

4) The problem reported and patch looks real, though, maybe a little credit and thanks would be appropriate as well?

... and still exists in your 2.4.21 based kernel.

Signature · 1 line
Jens Axboe
#4 lore raw
Quoted 2 lines · Jens Axboe

... and still exists in your 2.4.21 based kernel.

The RHL 2.4.21 kernels don't have the locking patch at all...

#6 lore raw
Quoted 5 lines · Jens Axboe
Quoted 4 lines · Arjan van de Ven
Quoted 2 lines · Jens Axboe

... and still exists in your 2.4.21 based kernel.

Quoted 7 lines
Quoted 1 line

The RHL 2.4.21 kernels don't have the locking patch at all...

But RHEL3 does:

http://kernelnewbies.org/kernels/rhel3/SOURCES/linux-2.4.21-iorl.patch

and the bug is there.

But in RHEL3 the bug is fixed already (not in a released kernel, but the fix went into our internal kernel some time back and will be in our next update kernel). From my internal bk tree for this stuff:

[dledford@compaq RHEL3-scsi]$ bk changes -r1.23
[email protected], 2003-11-10 17:19:54-05:00, [email protected]
drivers/scsi/scsi_error.c
Don't panic if the eh thread is dead, instead do the same thing that scsi_softirq_handler does and just complete the command as a failed command.
Change when we wake the eh thread in scsi_times_out to accomodate the changes to the mlqueue operations.
Clear blocked status on the host and all devices in scsi_restart_operations
-> Don't grab the host_lock in scsi_restart_operations, we aren't doing
anything that needs it. Just goose the queues unconditionally, scsi_request_fn() will know to not send commands if they shouldn't go for some reason.
Make sure we account SCSI_STATE_MLQUEUE commands as not being failed commands in scsi_unjam_host.

But, Jens is right, it's a real bug. I just fixed it in a different way. And my fix is dependent on other changes in our scsi stack as well.

Signature · 4 lines
  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc.
         1801 Varsity Dr.
         Raleigh, NC 27606
#7 lore raw
Quoted 12 lines · Doug Ledford
Quoted 10 lines · Jens Axboe
Quoted 5 lines · Arjan van de Ven
Quoted 1 line · Jens Axboe

... and still exists in your 2.4.21 based kernel.

The RHL 2.4.21 kernels don't have the locking patch at all...

But RHEL3 does:

Quoted 8 lines
Quoted 3 lines

http://kernelnewbies.org/kernels/rhel3/SOURCES/linux-2.4.21-iorl.patch

and the bug is there.

But in RHEL3 the bug is fixed already (not in a released kernel, but the fix went into our internal kernel some time back and will be in our next update kernel). From my internal bk tree for this stuff:

"not in a released kernel..." Do I read this right? That you have a fix for a critical bug and it hasn't been pushed to customers yet? How about security bugs, has the fix you pushed in RH-9.0 been push to EL customers?

Quoted 16 lines

[dledford@compaq RHEL3-scsi]$ bk changes -r1.23
[email protected], 2003-11-10 17:19:54-05:00, [email protected]
drivers/scsi/scsi_error.c
Don't panic if the eh thread is dead, instead do the same thing that scsi_softirq_handler does and just complete the command as a failed command.
Change when we wake the eh thread in scsi_times_out to accomodate the changes to the mlqueue operations.
Clear blocked status on the host and all devices in scsi_restart_operations
-> Don't grab the host_lock in scsi_restart_operations, we aren't doing
anything that needs it. Just goose the queues unconditionally, scsi_request_fn() will know to not send commands if they shouldn't go for some reason.
Make sure we account SCSI_STATE_MLQUEUE commands as not being failed commands in scsi_unjam_host.

Quoted 3 lines

But, Jens is right, it's a real bug. I just fixed it in a different way. And my fix is dependent on other changes in our scsi stack as well.

Yes, thanks to Peter for that fix, nice that someone provides timely fixes...

Signature · 3 lines
bill davidsen <[email protected]>
  CTO TMR Associates, Inc
  Doing interesting things with small computers since 1979
#8 lore raw
Quoted 18 lines · Bill Davidsen
Quoted 17 lines · Doug Ledford
Quoted 13 lines · Jens Axboe
Quoted 5 lines · Arjan van de Ven
Quoted 1 line · Jens Axboe

... and still exists in your 2.4.21 based kernel.

The RHL 2.4.21 kernels don't have the locking patch at all...

But RHEL3 does:

http://kernelnewbies.org/kernels/rhel3/SOURCES/linux-2.4.21-iorl.patch

and the bug is there.

Quoted 7 lines
Quoted 3 lines

But in RHEL3 the bug is fixed already (not in a released kernel, but the fix went into our internal kernel some time back and will be in our next update kernel). From my internal bk tree for this stuff:

"not in a released kernel..." Do I read this right? That you have a fix for a critical bug and it hasn't been pushed to customers yet? How about security bugs, has the fix you pushed in RH-9.0 been push to EL customers?

Calm down, it's a bug fix for a deadlock that _could_ trigger on SMP only in SCSI error recovery.

Signature · 1 line
Jens Axboe
#9 lore raw
In reply to Bill Davidsen
Quoted 5 lines

"not in a released kernel..." Do I read this right? That you have a fix for a critical bug and it hasn't been pushed to customers yet? How about security bugs, has the fix you pushed in RH-9.0 been push to EL customers?

RHL9 does not suffer this bug by virtue of not having the code in the first place.

RHEL has the corner case fixed pushed already

#10 lore raw
In reply to Bill Davidsen
Quoted 18 lines · Bill Davidsen
Quoted 17 lines · Doug Ledford
Quoted 13 lines · Jens Axboe
Quoted 5 lines · Arjan van de Ven
Quoted 1 line · Jens Axboe

... and still exists in your 2.4.21 based kernel.

The RHL 2.4.21 kernels don't have the locking patch at all...

But RHEL3 does:

http://kernelnewbies.org/kernels/rhel3/SOURCES/linux-2.4.21-iorl.patch

and the bug is there.

Quoted 6 lines
Quoted 3 lines

But in RHEL3 the bug is fixed already (not in a released kernel, but the fix went into our internal kernel some time back and will be in our next update kernel). From my internal bk tree for this stuff:

"not in a released kernel..." Do I read this right? That you have a fix for a critical bug and it hasn't been pushed to customers yet?

No, you don't read this right. We have a fix for a correctness issue that has almost 0% chance of ever triggering in real life, has exactly 0 bug reports of it ever happening, and which has been integrated into our tree. Obviously, we always push new kernels to all of our customers every time we have this situation, or about twice a day...

Quoted 19 lines

How about
security bugs, has the fix you pushed in RH-9.0 been push to EL customers?

Quoted 16 lines

[dledford@compaq RHEL3-scsi]$ bk changes -r1.23
[email protected], 2003-11-10 17:19:54-05:00, [email protected]
drivers/scsi/scsi_error.c
Don't panic if the eh thread is dead, instead do the same thing that scsi_softirq_handler does and just complete the command as a failed command.
Change when we wake the eh thread in scsi_times_out to accomodate the changes to the mlqueue operations.
Clear blocked status on the host and all devices in scsi_restart_operations
-> Don't grab the host_lock in scsi_restart_operations, we aren't doing
anything that needs it. Just goose the queues unconditionally, scsi_request_fn() will know to not send commands if they shouldn't go for some reason.
Make sure we account SCSI_STATE_MLQUEUE commands as not being failed commands in scsi_unjam_host.

Quoted 6 lines
Quoted 3 lines

But, Jens is right, it's a real bug. I just fixed it in a different way. And my fix is dependent on other changes in our scsi stack as well.

Yes, thanks to Peter for that fix, nice that someone provides timely fixes...

Puh-Leeze. If you actually read the source code, you would see just how damn near impossible this bug is. But, since you obviously didn't, let me enlighten you:

1) The offending code is part of the error handler code, specifically the section that kicks drives back into gear after recovery is complete.

2) The only section of code it's racing against is scsi_request_fn.

3) The race only happens if scsi_request_fn() detects that host->in_recovery is not set.

So, a sample would be something like this

CPU1

spin_lock_irqsave(device_lock)
scsi_request_fn()
check host->in_recovery == 0
[ start window ]
make a couple other sanity checks
[ end window ]
spin_lock(host_lock)
modify host command counts
spin_unlock(host_lock)
finish prepping command
spin_unlock(device_lock)
scsi_dispatch_cmd()
spin_lock(device_lock)
return
spin_unlock(device_lock)

Now, in that little, itty, bitty window in scsi_request_fn(), we have to
A) get some kind of error that starts the error handler subsystem, B) all outstanding commands have to complete (since we don't start the error handler thread until the controller is quiescient), C) we have to resolve the problem and get to the point of kicking the host back in gear, and finally D) we have to grab the host_lock before scsi_request_fn tries to and then we have to try and grab the specific device_lock that scsi_request_fn is holding.

Signature · 4 lines
  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc.
         1801 Varsity Dr.
         Raleigh, NC 27606