2006-12-19 10:32:55

by Dan Aloni

[permalink] [raw]
Subject: [PATCH] scsi_execute_async() should add to the tail of the queue

Hello,

scsi_execute_async() has replaced scsi_do_req() a few versions ago,
but it also incurred a change of behavior. I noticed that over-queuing
a SCSI device using that function causes I/Os to be starved from
low-level queuing for no justified reason.

I think it makes much more sense to perserve the original behaviour
of scsi_do_req() and add the request to the tail of the queue.

Signed-off-by: Dan Aloni <[email protected]>

diff -p -urN a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
--- a/drivers/scsi/scsi_lib.c 2006-12-19 01:48:50.000000000 +0200
+++ b/drivers/scsi/scsi_lib.c 2006-12-19 01:49:35.000000000 +0200
@@ -421,7 +421,7 @@ int scsi_execute_async(struct scsi_devic
sioc->data = privdata;
sioc->done = done;

- blk_execute_rq_nowait(req->q, NULL, req, 1, scsi_end_async);
+ blk_execute_rq_nowait(req->q, NULL, req, 0, scsi_end_async);
return 0;

free_req:


--
Dan Aloni
XIV, http://www.xivstorage.com


2006-12-19 10:35:05

by Dan Aloni

[permalink] [raw]
Subject: Re: [PATCH] scsi_execute_async() should add to the tail of the queue

Arjan van de Ven wrote:
> On Tue, 2006-12-19 at 10:35 +0200, Dan Aloni wrote:
>
>> Hello,
>>
>> scsi_execute_async() has replaced scsi_do_req() a few versions ago,
>> but it also incurred a change of behavior. I noticed that over-queuing
>> a SCSI device using that function causes I/Os to be starved from
>> low-level queuing for no justified reason.
>>
>> I think it makes much more sense to perserve the original behaviour
>> of scsi_do_req() and add the request to the tail of the queue.
>>
>
> Hi,
>
> some things should really be added to the head of the queue, like
> maintenance requests and error handling requests. Are you sure this is
> the right change? At least I'd expect 2 apis, one for a head and one for
> a "normal" queueing...
>
Since a user of scsi_execute_async() would most likely want to have
control over this, it would be better to add a parameter and fix the
current users of the function.

However, if we take this route we might have duplicate code
across mid-layer drivers (sg, st, osst), because they may choose to
prioritize I/Os in similar ways.

So instead of adding a parameter, we can make scsi_execute_async()
decide for itself based on the SCSI command, with read/write I/Os
taking the lowest priority.

Suggestions?

2006-12-20 21:54:35

by Steven Hayter

[permalink] [raw]
Subject: Re: [PATCH] scsi_execute_async() should add to the tail of the queue

Dan Aloni wrote:
> Hello,
>
> scsi_execute_async() has replaced scsi_do_req() a few versions ago,
> but it also incurred a change of behavior. I noticed that over-queuing
> a SCSI device using that function causes I/Os to be starved from
> low-level queuing for no justified reason.
>
> I think it makes much more sense to perserve the original behaviour
> of scsi_do_req() and add the request to the tail of the queue.

As far as I'm aware the way in which scsi_do_req() was to insert at the
head of the queue, leading to projects like SCST to come up with
scsi_do_req_fifo() as queuing multiple commands using scsi_do_req() with
constant head insertion might lead to out of order execution.

Just thought I'd throw some light on the history and what others have
done in the past.

Steve

2006-12-21 00:06:21

by Jeremy Linton

[permalink] [raw]
Subject: Re: [PATCH] scsi_execute_async() should add to the tail of the queue

> So instead of adding a parameter, we can make scsi_execute_async()
> decide for itself based on the SCSI command, with read/write I/Os
> taking the lowest priority.
This seems like a bad idea, I can come up with a number of cases where
the priority of a request would better be optimized by a higher level
subsystem, rather than a simple prioritization based on the command type.

The original suggestion to provide both head and tail insertion options
seems like the obvious solution, short of a full priority queuing system.




2006-12-21 07:12:55

by Dan Aloni

[permalink] [raw]
Subject: Re: [PATCH] scsi_execute_async() should add to the tail of the queue

Steven Hayter wrote:
> Dan Aloni wrote:
>> Hello,
>>
>> scsi_execute_async() has replaced scsi_do_req() a few versions ago,
>> but it also incurred a change of behavior. I noticed that
>> over-queuing a SCSI device using that function causes I/Os to be
>> starved from low-level queuing for no justified reason.
>>
>> I think it makes much more sense to perserve the original behaviour
>> of scsi_do_req() and add the request to the tail of the queue.
>
> As far as I'm aware the way in which scsi_do_req() was to insert at
> the head of the queue, leading to projects like SCST to come up with
> scsi_do_req_fifo() as queuing multiple commands using scsi_do_req()
> with constant head insertion might lead to out of order execution.
>
> Just thought I'd throw some light on the history and what others have
> done in the past.
>
In Linux 2.4.31 scsi_do_req() still inserts at the tail. This was also
valid until 2.6.5.
James, is the change you inserted in Linux 2.6.5 still relevant in 2.6
today?

<[email protected]>
[PATCH] add device quiescing to the SCSI API

This patch adds the ability to quiesce a SCSI device. The idea
is that
user issued commands (including filesystem ones) would get blocked,
while mid-layer and device issued ones would be allowed to proceed.
This is for things like Domain Validation which like to operate
on an
otherwise quiet device.

There is one big change: to get all of this to happen correctly,
scsi_do_req() has to queue on the *head* of the request queue,
not the
tail as it was doing previously. The reason is that deferred
requests
block the queue, so anything needing executing after a deferred
request
has to go in front of it. I don't think there are any untoward
consequences of this.

--
Dan Aloni
XIV LTD, http://www.xivstorage.com
da-x (at) monatomic.org, dan (at) xiv.co.il