Fix aic7xxx del_timer_sync() deadlock

The best fix, I think is to eliminate this timer from the driver. All it was doing was providing a time limited queue freeze for the BUSY and QUEUE_FULL on no outstanding commands cases. These are handled correctly by the mid-layer, so there's no need for the driver do try to handle them on it's own.

1) The mid-layer doesn't correctly handle this situation. The mid-layer uses blk_plug_device() to implement this behavior rather than blk_stop_queue(). The leaves the implementation vulnerable to any code that does a manual unplug (e.g. the SCSI scan code) or a blk_run_queues() (e.g. MD) which can alter the duration of the delay. While the host may be able to tune the delay to HZ, the default values are not. I'm also not sure that leaving the default blk_unplug delay as 0 gives you a deterministic jiffy long delay before the request function will be hit again.

2) These drivers both include Domain Validation support. These commands do not go through the mid-layer at all yet require proper queue full and busy handling.

There's no guaranteed minimum delay time required by the specs. We pause for a predetermined number of unplugs if you look at the code, which is actually under the control of the driver. The trade off is that we want to try faster under heavy I/O pressure, so the delay isn't fixed, it's bounded by the I/O pressure in the system, which is as it should be.

Quoted 3 lines

2) These drivers both include Domain Validation support. These commands do not go through the mid-layer at all yet require proper queue full and busy handling.

Well, if the driver used the correct command handling infrastructure for that, they would go through the mid-layer and there would be no problem.

Well, experience shows that if you implement a SCSI system based solely

There are lots of devices out there that require a delay of at least 250ms in order to not deadlock their internal SCSI processor. The I/O load of the system has no bearing on when a device will become "unbusy" (we can't even say why it is "busy"), so I fail to see why it should have any effect on how long we wait in response to this condition.

In order to issue a DV command to the end device via the mid-layer, the host queue and the device queue must not be blocked. But, for DV to be effective, it must be the only activity occurring on that device. How do you reconcile the two while using the mid-layer to do your I/O? The mid-layer has no concept of allowing a client to freeze the queue, wait for the active count to go to zero, effectively pre-empt the command stream with a series of special commands, and then unblock everyone else only at the end. The closest the mid-layer comes to this is in some of its error recovery handling but those are internal interfaces.

Quoted 6 lines

There are lots of devices out there that require a delay of at least 250ms in order to not deadlock their internal SCSI processor. The I/O load of the system has no bearing on when a device will become "unbusy" (we can't even say why it is "busy"), so I fail to see why it should have any effect on how long we wait in response to this condition.

Could you give the most common example ... I'll see if I can persuade the OSDL test people to try it out with the current stack?

If you tell me what the inquiry strings of these devices are, I can blacklist them to have a much larger max_device_blocked count, so if there is a problem with them, *all* drivers will work rather than just the Adaptec ones.

Quoted 5 lines

In order to issue a DV command to the end device via the mid-layer, the host queue and the device queue must not be blocked. But, for DV to be effective, it must be the only activity occurring on that device. How do you reconcile the two while using the mid-layer to do your I/O? The mid-layer has no concept of allowing [this] ...

But domain validation is a pretty intrusive thing. It's only really supposed to be run in two places:

1. At start of day, which you should do from slave_configure, where you are guaranteed that nothing else is using the device

2. On indication of transport problems. This you would run for a single target from the bus or device reset handler after issuing the command and pausing for the settle time (OK, that's bad because the settle time is also built into the error handler, but that will improve when error handling becomes more transport specific and I can build domain validation directly into the SPI transport error handling).

In both of these cases, you are guaranteed a quiescent device queue, so I don't see what the problem is.

This is not something worth black-listing. It is not a special case. Busy and/or queue full with no I/O pending is a rare event. The user will never notice this in practice other than their devices that need this delay will work correctly in this situation. To put it another way, the aic7xxx and aic79xx drivers have enforced this delay for almost four years in Linux and I have yet to have someone complain that they had poor device performance due to this delay. It is just not worth the code complexity or potential of missing a broken device to "optimize" this delay.

Quoted 9 lines

This is not something worth black-listing. It is not a special case. Busy and/or queue full with no I/O pending is a rare event. The user will never notice this in practice other than their devices that need this delay will work correctly in this situation. To put it another way, the aic7xxx and aic79xx drivers have enforced this delay for almost four years in Linux and I have yet to have someone complain that they had poor device performance due to this delay. It is just not worth the code complexity or potential of missing a broken device to "optimize" this delay.

Well, actually, it is: there are certain array vendors (who should justifiably remain nameless) who implemented the array queue resources as global controller pools. Thus, under heavy I/O to multiple LUNs, they become highly likely to throw BUSY or QUEUE FULL at zero depth and do it quite often. Pausing for fractions of a second here will cause nasty performance glitches in the benchmarks.

What about putting a rate limited printk in when the stutter is triggered? That way if someone still has one of the problem devices we should have a very good trace when they report the hang.

While it is certainly possible to move domain validation into the mid-layer and remove it from LLDs, I doubt that is the type of change you want to include in 2.6. It would require at minimum:

1) A generic method for fetching and changing the transport parameters for devices attached to LLDs.

2) A generic method for freezing the execution queue and "single-stepping" things like domain validation and error recovery commands.

3) A way to wait for the active count on a device or target (including all luns) to drop to 0.

Don't get me wrong. One of my main complaints of the Linux SCSI layer is its intrinsic lack of consistency. In my opinion, the code path for sending commands to and processing the results from a LLD should be identical regardless of whether the commands are from error recovery, domain validation, a peripheral driver, or the probe code. Instead we only get certain behavior (like a bus settle delay) for some clients and not others. Fixing this would allow the behavior to be defined in one place and these "workarounds" could be removed from the LLDs. Unfortunately, the LLDs are the only "code path" common to all of the clients of the mid-layer, so you wind up with the LLDs enforcing correct behavior.

1) The mid-layer doesn't correctly handle this situation. The mid-layer uses blk_plug_device() to implement this behavior rather than blk_stop_queue(). The leaves the implementation vulnerable to any code that does a manual unplug (e.g. the SCSI scan code) or a blk_run_queues() (e.g. MD) which can alter the duration of the delay.

Quoted 5 lines · Justin T. Gibbs

1) The mid-layer doesn't correctly handle this situation. The mid-layer uses blk_plug_device() to implement this behavior rather than blk_stop_queue(). The leaves the implementation vulnerable to any code that does a manual unplug (e.g. the SCSI scan code) or a blk_run_queues() (e.g. MD) which can alter the duration of the delay.

hum.

Long term we definitely want to use blk_{start,stop}_queue(). I had to work on Jens for a little while before he was kind enough to add it :) so let's put it to use.

The only nasty right now is that you cannot unconditionally call blk_stop_queue(), it requires a manual blk_start_queue() on io completion which will only happen if io is pending of course. I just got an idea on how to do that properly, I'll make sure it works in any case.

Quoted 4 lines · Jens Axboe

The only nasty right now is that you cannot unconditionally call blk_stop_queue(), it requires a manual blk_start_queue() on io completion which will only happen if io is pending of course. I just got an idea on how to do that properly, I'll make sure it works in any case.

hmmm... I thought the model was to call blk_stop_queue() when you can accept no more requests, and call blk_start_queue() once you can start accepting more requests.

That would imply that blk_start_queue() is OK when there is no more I/O pending...

Quoted 7 lines · Jeff Garzik

Quoted 4 lines · Jens Axboe

The only nasty right now is that you cannot unconditionally call blk_stop_queue(), it requires a manual blk_start_queue() on io completion which will only happen if io is pending of course. I just got an idea on how to do that properly, I'll make sure it works in any case.

Quoted 3 lines

hmmm... I thought the model was to call blk_stop_queue() when you can accept no more requests, and call blk_start_queue() once you can start accepting more requests.

That would imply that blk_start_queue() is OK when there is no more I/O pending...

That is the model. The problem is that you don't have an opportunitty to call blk_start_queue() if there's no more IO pending (you typicall do it from io completion). There's no problem with doing that.

Quoted 17 lines · Jens Axboe

Quoted 13 lines · Jeff Garzik

Quoted 4 lines · Jens Axboe

The only nasty right now is that you cannot unconditionally call blk_stop_queue(), it requires a manual blk_start_queue() on io completion which will only happen if io is pending of course. I just got an idea on how to do that properly, I'll make sure it works in any case.

hmmm... I thought the model was to call blk_stop_queue() when you can accept no more requests, and call blk_start_queue() once you can start accepting more requests.

That would imply that blk_start_queue() is OK when there is no more I/O pending...

Quoted 3 lines

That is the model. The problem is that you don't have an opportunitty to call blk_start_queue() if there's no more IO pending (you typicall do it from io completion). There's no problem with doing that.

Help me out here, I must be dumb :)

If you call blk_start_queue() from io completion -- as you should -- then it gets called unconditionally, regardless of whether there is IO pending or not. The LLD should only care if it can accept more requests. The opportunity seems to be there, to me...

Quoted 1 line · Jeff Garzik

If you call blk_start_queue() from io completion -- as you should -- then it gets called unconditionally, regardless of whether there is IO pending or not. The LLD should only care if it can accept more requests. The opportunity seems to be there, to me...

You are still assuming you have io pending when you call blk_stop_queue(). If some condition prevents you from queueing the first request, you call blk_stop_queue() without having anything to start the queue again.

Quoted 11 lines · Jens Axboe

Quoted 4 lines · Jeff Garzik

If you call blk_start_queue() from io completion -- as you should -- then it gets called unconditionally, regardless of whether there is IO pending or not. The LLD should only care if it can accept more requests. The opportunity seems to be there, to me...

You are still assuming you have io pending when you call blk_stop_queue(). If some condition prevents you from queueing the first request, you call blk_stop_queue() without having anything to start the queue again.

ok, I got it. I thought your "io pending" was referring to requests in the request_queue, not requests active in the LLD.

Still, this is the responsibility of the LLD, right? For example, before any requests are ever sent to the request_queue, the LLD may choose to call blk_stop_queue() to prevent queueing while it probes or resets the bus. That would be a case where there is no io pending, for both definitions of the phrase :) And it would be up to the LLD to call blk_start_queue() again. Right?

Quoted 2 lines

That would imply that blk_start_queue() is OK when there is no more I/O pending...

That is the model. The problem is that you don't have an opportunitty to call blk_start_queue() if there's no more IO pending (you typicall do it from io completion). There's no problem with doing that.

Quoted 5 lines

Quoted 1 line

That would imply that blk_start_queue() is OK when there is no more I/O pending...

That is the model. The problem is that you don't have an opportunitty to call blk_start_queue() if there's no more IO pending (you typicall do it from io completion). There's no problem with doing that.

You just need to schedule a timer to do the blk_start_queue() call. The point of doing this is so that the delay is deterministic.

Quoted 2 lines

You just need to schedule a timer to do the blk_start_queue() call. The point of doing this is so that the delay is deterministic.

Now that I understand what Jens is referring to... this depends on the LLD.

Quoted 5 lines

Quoted 2 lines

You just need to schedule a timer to do the blk_start_queue() call. The point of doing this is so that the delay is deterministic.

Now that I understand what Jens is referring to... this depends on the LLD.

Exactly. The block layer shouldn't be involved in this at all. In the particular case I was talking about, busy or queue full with no I/O pending, you need to schedule a timer to unfreeze the queue since there is no other event that can be used to give you a deterministic delay.

Quoted 2 lines

Exactly. The block layer shouldn't be involved in this at all. In the particular case I was talking about, busy or queue full with no I/O pending,

Agreed for busy.

But queue full?

Quoted 2 lines

Quoted 2 lines

Exactly. The block layer shouldn't be involved in this at all. In the particular case I was talking about, busy or queue full with no I/O pending,

                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Quoted 3 lines

Agreed for busy.

But queue full?

Some SCSI devices send Queue Full instead of Busy in the case of a "busy" condition with no I/O pending.

Quoted 9 lines · Justin T. Gibbs

Quoted 3 lines

Quoted 3 lines

Exactly. The block layer shouldn't be involved in this at all. In the particular case I was talking about, busy or queue full with no I/O pending,

                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Quoted 2 lines

Agreed for busy.

Quoted 5 lines

Quoted 1 line

But queue full?

Some SCSI devices send Queue Full instead of Busy in the case of a "busy" condition with no I/O pending.

Gotcha, thanks.

Don't forget that this isn't the only condition. What about pci dma mapping failures?

Quoted 2 lines · Jens Axboe

Don't forget that this isn't the only condition. What about pci dma mapping failures?

hehe, granted there might be other conditions, but for PCI DMA mapping specifically, it is (unfortunately) currently defined to not fail.

That wants changing for ppc64 and x86-64 iommu, which are a -lot- more likely to fail an iommu transaction than parisc or sparc64... but that's the way it is for now. Way too many drivers assume PCI DMA mapping always succeeds.

Quoted 6 lines · Jeff Garzik

Quoted 2 lines · Jens Axboe

Don't forget that this isn't the only condition. What about pci dma mapping failures?

Quoted 1 line

hehe, granted there might be other conditions, but for PCI DMA mapping specifically, it is (unfortunately) currently defined to not fail.

Which is bogus and must be changed anyways... Besides, that was just one example. There could be other resource shortages that can happen which prevent queueing.

Quoted 1 line

That wants changing for ppc64 and x86-64 iommu, which are a -lot- more likely to fail an iommu transaction than parisc or sparc64... but that's the way it is for now. Way too many drivers assume PCI DMA mapping always succeeds.

I just don't like _at all_ that it's assumed it cannot fail, when in fact it _can_ fail. Likeliness just means it's less of a problem for those archs, it's still an eye sore.

But queue full? I hope you clear out the queue first and do some error handling, before calling blk_start_queue()?

Normally, calling blk_start_queue() when the queue is full would be the wrong thing to do :) You call blk_stop_queue() when the queue becomes full, then blk_start_queue() only when the LLD's hardware signals it has completed a transaction...

Quoted 6 lines

Quoted 2 lines

That would imply that blk_start_queue() is OK when there is no more I/O pending...

That is the model. The problem is that you don't have an opportunitty to call blk_start_queue() if there's no more IO pending (you typicall do it from io completion). There's no problem with doing that.

You just need to schedule a timer to do the blk_start_queue() call. The point of doing this is so that the delay is deterministic.