[PATCH] scsi_execute_async() should add to the tail of the queue

[PATCH] scsi_execute_async() should add to the tail of the queue

[Dan Aloni]

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 <da-x@monatomic.org>

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

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

[Arjan van de Ven]

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...

Greetings,
   Arjan van de Ven

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

[Dan Aloni]

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?

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

[Jens Axboe]

On Tue, Dec 19 2006, 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...

It does sounds broken - head insertion should only be used for careful
internal commands, not be the default way user issued commands. Looking
at the current users, the patch makes sense to me.

-- 
Jens Axboe

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

[Jon Escombe]

Jens Axboe wrote:
 > On Tue, Dec 19 2006, 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...
 >
 > It does sounds broken - head insertion should only be used for careful
 > internal commands, not be the default way user issued commands. Looking
 > at the current users, the patch makes sense to me.
 >

It's worth noting that the hdaps disk protection patches rely on the 
current behaviour to add 'IDLE IMMEDIATE WITH UNLOAD' commands to the 
head of the queue.. Another function, or a new parameter for queue 
position would be needed to retain this functionality - any preference 
for either?

Regards,
Jon.

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

[Jens Axboe]

On Tue, Dec 19 2006, Jon Escombe wrote:
> Jens Axboe wrote:
> > On Tue, Dec 19 2006, 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...
> >
> > It does sounds broken - head insertion should only be used for careful
> > internal commands, not be the default way user issued commands. Looking
> > at the current users, the patch makes sense to me.
> >
> 
> It's worth noting that the hdaps disk protection patches rely on the 
> current behaviour to add 'IDLE IMMEDIATE WITH UNLOAD' commands to the 
> head of the queue.. Another function, or a new parameter for queue 
> position would be needed to retain this functionality - any preference 
> for either?

The hdaps disk protection should not be using the SCSI internal
function, so it should not be an issue. The block layer API exposes both
front/back/sort insertion possibilities.

-- 
Jens Axboe

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

[Jeremy Linton]

> 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.

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

[Dan Aloni]

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?

<James.Bottomley@steeleye.com>
        [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

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

[Steven Hayter]

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

[PATCH] scsi_execute_async() should add to the tail of the queue

[Dan Aloni]

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 <da-x@monatomic.org>

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