scsi_execute_async() should add to the tail of the queue
PATCH 1/1 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.
drivers/scsi/scsi_lib.c +1 -1
@@ -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
Quoted 9 lines · Dan Aloni
Hello,
scsi_execute_async()has replacedscsi_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
Quoted 9 lines · Arjan van de Ven
Quoted 6 lines · Dan Aloni
Hello,
scsi_execute_async()has replacedscsi_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.
Quoted 7 lines
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?
Quoted 3 lines
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.
Quoted 11 lines · Arjan van de Ven
Quoted 9 lines · Dan Aloni
Hello,
scsi_execute_async()has replacedscsi_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.
Quoted 6 lines
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.
Signature · 1 line
Jens Axboe
Quoted 13 lines · Jens Axboe
Quoted 12 lines · Arjan van de Ven
Quoted 9 lines · Dan Aloni
Hello,
scsi_execute_async()has replacedscsi_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,
Quoted 9 lines
Quoted 4 lines
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.
Quoted 24 lines · Jon Escombe
Quoted 13 lines · Jens Axboe
Quoted 12 lines · Arjan van de Ven
Quoted 9 lines · Dan Aloni
Hello,
scsi_execute_async()has replacedscsi_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,
Quoted 9 lines
Quoted 4 lines
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.
Quoted 5 lines
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.
Signature · 1 line
Jens Axboe