Re: Prevent busy looping

[Elias Oltmanns]

Jens Axboe <jens.axboe@xxxxxxxxxx> wrote:
> On Thu, Apr 17 2008, Elias Oltmanns wrote:
>> Jens Axboe <jens.axboe@xxxxxxxxxx> wrote:
>> > On Wed, Apr 16 2008, Elias Oltmanns wrote:
>> >> blk_run_queue() as well as blk_start_queue() plug the device on reentry
>> >> and schedule blk_unplug_work() right afterwards. However,
>> >> blk_plug_device() takes care of that already and makes sure that there is
>> >> a short delay before blk_unplug_work() is scheduled. This is important
>> >> to prevent busy looping and possibly system lockups as observed here:
>> >> <http://permalink.gmane.org/gmane.linux.ide/28351>.
>> >
>> > If you call blk_start_queue() and blk_run_queue(), you better mean it.
>> > There should be no delay. The only reason it does blk_plug_device() is
>> > so that the work queue function will actually do some work.
>> 
>> Well, I'm mainly concerned with blk_run_queue(). In a comment it says
>> that it should recurse only once so as not to overrun the stack. On my
>> machine, however, immediate rescheduling may have exactly as disastrous
>> consequences as an overrunning stack would have since the system locks
>> up completely.
>> 
>> Just to get this straight: Are low level drivers allowed to rely on
>> blk_run_queue() that there will be no loops or do they have to make sure
>> that this function is not called from the request_fn() of the same
>> queue?
>
> It's not really designed for being called recursively. Which isn't the
> problem imo, the problem is SCSI apparently being dumb and calling
> blk_run_queue() all the time. blk_run_queue() must run the queue NOW. If
> SCSI wants something like 'run the queue in a bit', it should use
> blk_plug_device() instead.

James would probably argue that this is alright as long as
max_device_blocked and max_host_blocked are bigger than one.

>
>> > In the newer kernels we just do:
>> >
>> >         set_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags);
>> >         kblockd_schedule_work(q, &q->unplug_work);
>> >
>> > instead, which is much better.
>> 
>> Only as long as it doesn't get called from the request_fn() of the same
>> queue. Otherwise, there may be no chance for other threads to clear the
>> condition that caused blk_run_queue() to be called in the first place.
>
> Broken usage.

Right. Tejun, would it be possible to apply the patch below (2.6.25) or
do you see any alternative?

Regards,

Elias

---

 drivers/ata/libata-scsi.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 1579539..ce865e9 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -831,7 +831,7 @@ static void ata_scsi_sdev_config(struct scsi_device *sdev)
 	 * prevent SCSI midlayer from automatically deferring
 	 * requests.
 	 */
-	sdev->max_device_blocked = 1;
+	sdev->max_device_blocked = 2;
 }
 
 /**
@@ -3206,7 +3206,7 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
 		 * Set host_blocked to 1 to prevent SCSI midlayer from
 		 * automatically deferring requests.
 		 */
-		shost->max_host_blocked = 1;
+		shost->max_host_blocked = 2;
 
 		rc = scsi_add_host(ap->scsi_host, ap->host->dev);
 		if (rc)