Hello, James, Jens and Christoph.
This patchset is reworked version of the previous REQ_SPECIAL update
patchset. Patches #01 and #05 update blk layer. The other patches
update SCSI midlayer.
I've opted for automatically setting REQ_SOFTBARRIER together with
REQ_STARTED in elv_next_request(). Reordering prep-deferred or
requeued requests doesn't have any real benefit and actually both as
and cfq don't reorder requeued requests. The only behavior change is
that prep-deferred requests can't be passed by others. I think the
change is a good thing. The only affected driver other than SCSI is
i2o_block. So, Jens, please let me know what you think.
This patchset does the following things.
#01 : make elv_next_request() set REQ_SOFTBARRIER in addition to
REQ_STARTED.
#02-04 : decouple REQ_SPECIAL from scsi_cmnd->special and make it
mean special requests (non-fs/pc).
#05 : remove requeue feature from blk_insert_request().
Previously, REQ_SPECIAL duplicately meant the request has been
prepp'ed by SCSI midlayer and/or the request is a special request.
This left special requests handling in the midlayer subtley
inconsistent.
Also, the setting of REQ_SPECIAL was done by the block layer using
blk_insert_request() mostly but sometimes by the SCSI midlayer (when
returning BLK_PREP_DEFER from scsi_prep_fn()). blk_insert_request()
was used for two different purposes.
* enqueue special requests
* turn on REQ_SPECIAL|REQ_SOFTBARRIER and call blk_requeue_request().
The second somewhat unobvious feature of blk_insert_request() is used
only by SCSI midlayer and SCSI midlayer depended on it to set
REQ_SOFTBARRIER. Unfortunately, when the SCSI midlayer sets
REQ_SPECIAL explicitly (sg allocation failure path) it didn't set
REQ_SOFTBARRIER, creating a *highly* unlikely but still existing dead
lock condition caused by allowing reorder of a request which has its
cmd allocated. IMHO, this proves the subtlety of current situation.
This patchset makes blk layer set REQ_SOFTBARRIER automatically when
a request is dispatched from its request queue and SCSI midlayer use
blk_requeue_request() for requeueing.
To prevent more misuses, the requeue feature of blk_insert_request()
is removed. Requeueing should be done with blk_requeue_request() not
blk_insert_request().
[ Start of patch descriptions ]
01_scsi_blk_make_started_requests_ordered.patch
: make blk layer set REQ_SOFTBARRIER when a request is dispatched
Reordering already started requests is without any real
benefit and causes problems if the request has its
driver-specific resources allocated (as in SCSI). This patch
makes elv_next_request() set REQ_SOFTBARRIER automatically
when a request is dispatched.
As both as and cfq schedulers don't allow passing requeued
requests, the only behavior change is that requests deferred
by prep_fn won't be passed by other requests. This change
shouldn't cause any problem. The only affected driver other
than SCSI is i2o_block.
02_scsi_REQ_SPECIAL_semantic_scsi_init_io.patch
: remove REQ_SPECIAL in scsi_init_io()
scsi_init_io() used to set REQ_SPECIAL when it fails sg
allocation before requeueing the request by returning
BLKPREP_DEFER. REQ_SPECIAL is being updated to mean special
requests. So, remove REQ_SPECIAL setting.
03_scsi_REQ_SPECIAL_semantic_scsi_queue_insert.patch
: make scsi_queue_insert() use blk_requeue_request()
scsi_queue_insert() used to use blk_insert_request() for
requeueing requests. This depends on the unobvious behavior
of blk_insert_request() setting REQ_SPECIAL and
REQ_SOFTBARRIER when requeueing. This patch makes
scsi_queue_insert() use blk_requeue_request(). As REQ_SPECIAL
means special requests and REQ_SOFTBARRIER is automatically
handled by blk layer now, no flag needs to be set.
Note that scsi_queue_insert() now calls scsi_run_queue()
itself, and the prototype of the function is added right above
scsi_queue_insert(). This is temporary, as later requeue path
consolidation patchset removes scsi_queue_insert(). By adding
temporary prototype, we can do away with unnecessarily moving
functions.
04_scsi_REQ_SPECIAL_semantic_scsi_requeue_command.patch
: make scsi_requeue_request() use blk_requeue_request()
scsi_requeue_request() used to use blk_insert_request() for
requeueing requests. This depends on the unobvious behavior
of blk_insert_request() setting REQ_SPECIAL and
REQ_SOFTBARRIER when requeueing. This patch makes
scsi_queue_insert() use blk_requeue_request(). As REQ_SPECIAL
means special requests and REQ_SOFTBARRIER is automatically
handled by blk layer now, no flag needs to be set.
05_scsi_blk_insert_request_no_requeue.patch
: remove requeue feature from blk_insert_request()
blk_insert_request() has a unobivous feature of requeuing a
request setting REQ_SPECIAL|REQ_SOFTBARRIER. SCSI midlayer
was the only user and as previous patches removed the usage,
remove the feature from blk_insert_request(). Only special
requests should be queued with blk_insert_request(). All
requeueing should go through blk_requeue_request().
[ End of patch descriptions ]
Thanks.
01_scsi_blk_make_started_requests_ordered.patch
Reordering already started requests is without any real
benefit and causes problems if the request has its
driver-specific resources allocated (as in SCSI). This patch
makes elv_next_request() set REQ_SOFTBARRIER automatically
when a request is dispatched.
As both as and cfq schedulers don't allow passing requeued
requests, the only behavior change is that requests deferred
by prep_fn won't be passed by other requests. This change
shouldn't cause any problem. The only affected driver other
than SCSI is i2o_block.
Signed-off-by: Tejun Heo <[email protected]>
elevator.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
Index: scsi-reqfn-export/drivers/block/elevator.c
===================================================================
--- scsi-reqfn-export.orig/drivers/block/elevator.c 2005-04-20 08:13:01.000000000 +0900
+++ scsi-reqfn-export/drivers/block/elevator.c 2005-04-20 08:13:33.000000000 +0900
@@ -370,11 +370,11 @@ struct request *elv_next_request(request
while ((rq = __elv_next_request(q)) != NULL) {
/*
- * just mark as started even if we don't start it, a request
- * that has been delayed should not be passed by new incoming
- * requests
+ * just mark as started even if we don't start it.
+ * also, as a request that has been delayed should not
+ * be passed by new incoming requests, set softbarrier.
*/
- rq->flags |= REQ_STARTED;
+ rq->flags |= REQ_STARTED | REQ_SOFTBARRIER;
if (rq == q->last_merge)
q->last_merge = NULL;
03_scsi_REQ_SPECIAL_semantic_scsi_queue_insert.patch
scsi_queue_insert() used to use blk_insert_request() for
requeueing requests. This depends on the unobvious behavior
of blk_insert_request() setting REQ_SPECIAL and
REQ_SOFTBARRIER when requeueing. This patch makes
scsi_queue_insert() use blk_requeue_request(). As REQ_SPECIAL
means special requests and REQ_SOFTBARRIER is automatically
handled by blk layer now, no flag needs to be set.
Note that scsi_queue_insert() now calls scsi_run_queue()
itself, and the prototype of the function is added right above
scsi_queue_insert(). This is temporary, as later requeue path
consolidation patchset removes scsi_queue_insert(). By adding
temporary prototype, we can do away with unnecessarily moving
functions.
Signed-off-by: Tejun Heo <[email protected]>
scsi_lib.c | 21 +++++++++++----------
1 files changed, 11 insertions(+), 10 deletions(-)
Index: scsi-reqfn-export/drivers/scsi/scsi_lib.c
===================================================================
--- scsi-reqfn-export.orig/drivers/scsi/scsi_lib.c 2005-04-20 08:13:34.000000000 +0900
+++ scsi-reqfn-export/drivers/scsi/scsi_lib.c 2005-04-20 08:13:34.000000000 +0900
@@ -96,6 +96,8 @@ int scsi_insert_special_req(struct scsi_
return 0;
}
+static void scsi_run_queue(struct request_queue *q);
+
/*
* Function: scsi_queue_insert()
*
@@ -119,6 +121,8 @@ int scsi_queue_insert(struct scsi_cmnd *
{
struct Scsi_Host *host = cmd->device->host;
struct scsi_device *device = cmd->device;
+ struct request_queue *q = device->request_queue;
+ unsigned long flags;
SCSI_LOG_MLQUEUE(1,
printk("Inserting command %p into mlqueue\n", cmd));
@@ -154,17 +158,14 @@ int scsi_queue_insert(struct scsi_cmnd *
scsi_device_unbusy(device);
/*
- * Insert this command at the head of the queue for it's device.
- * It will go before all other commands that are already in the queue.
- *
- * NOTE: there is magic here about the way the queue is plugged if
- * we have no outstanding commands.
- *
- * Although this *doesn't* plug the queue, it does call the request
- * function. The SCSI request function detects the blocked condition
- * and plugs the queue appropriately.
+ * Requeue the command.
*/
- blk_insert_request(device->request_queue, cmd->request, 1, cmd, 1);
+ spin_lock_irqsave(q->queue_lock, flags);
+ blk_requeue_request(q, cmd->request);
+ spin_unlock_irqrestore(q->queue_lock, flags);
+
+ scsi_run_queue(q);
+
return 0;
}
05_scsi_blk_insert_request_no_requeue.patch
blk_insert_request() has a unobivous feature of requeuing a
request setting REQ_SPECIAL|REQ_SOFTBARRIER. SCSI midlayer
was the only user and as previous patches removed the usage,
remove the feature from blk_insert_request(). Only special
requests should be queued with blk_insert_request(). All
requeueing should go through blk_requeue_request().
Signed-off-by: Tejun Heo <[email protected]>
drivers/block/ll_rw_blk.c | 20 ++++++--------------
drivers/block/paride/pd.c | 2 +-
drivers/block/sx8.c | 4 ++--
drivers/scsi/scsi_lib.c | 2 +-
include/linux/blkdev.h | 2 +-
5 files changed, 11 insertions(+), 19 deletions(-)
Index: scsi-reqfn-export/drivers/block/ll_rw_blk.c
===================================================================
--- scsi-reqfn-export.orig/drivers/block/ll_rw_blk.c 2005-04-20 08:13:01.000000000 +0900
+++ scsi-reqfn-export/drivers/block/ll_rw_blk.c 2005-04-20 08:13:35.000000000 +0900
@@ -2037,7 +2037,6 @@ EXPORT_SYMBOL(blk_requeue_request);
* @rq: request to be inserted
* @at_head: insert request at head or tail of queue
* @data: private data
- * @reinsert: true if request it a reinsertion of previously processed one
*
* Description:
* Many block devices need to execute commands asynchronously, so they don't
@@ -2052,8 +2051,9 @@ EXPORT_SYMBOL(blk_requeue_request);
* host that is unable to accept a particular command.
*/
void blk_insert_request(request_queue_t *q, struct request *rq,
- int at_head, void *data, int reinsert)
+ int at_head, void *data)
{
+ int where = at_head ? ELEVATOR_INSERT_FRONT : ELEVATOR_INSERT_BACK;
unsigned long flags;
/*
@@ -2070,20 +2070,12 @@ void blk_insert_request(request_queue_t
/*
* If command is tagged, release the tag
*/
- if (reinsert)
- blk_requeue_request(q, rq);
- else {
- int where = ELEVATOR_INSERT_BACK;
+ if (blk_rq_tagged(rq))
+ blk_queue_end_tag(q, rq);
- if (at_head)
- where = ELEVATOR_INSERT_FRONT;
+ drive_stat_acct(rq, rq->nr_sectors, 1);
+ __elv_add_request(q, rq, where, 0);
- if (blk_rq_tagged(rq))
- blk_queue_end_tag(q, rq);
-
- drive_stat_acct(rq, rq->nr_sectors, 1);
- __elv_add_request(q, rq, where, 0);
- }
if (blk_queue_plugged(q))
__generic_unplug_device(q);
else
Index: scsi-reqfn-export/drivers/block/paride/pd.c
===================================================================
--- scsi-reqfn-export.orig/drivers/block/paride/pd.c 2005-04-20 08:13:01.000000000 +0900
+++ scsi-reqfn-export/drivers/block/paride/pd.c 2005-04-20 08:13:35.000000000 +0900
@@ -723,7 +723,7 @@ static int pd_special_command(struct pd_
rq.ref_count = 1;
rq.waiting = &wait;
rq.end_io = blk_end_sync_rq;
- blk_insert_request(disk->gd->queue, &rq, 0, func, 0);
+ blk_insert_request(disk->gd->queue, &rq, 0, func);
wait_for_completion(&wait);
rq.waiting = NULL;
if (rq.errors)
Index: scsi-reqfn-export/drivers/block/sx8.c
===================================================================
--- scsi-reqfn-export.orig/drivers/block/sx8.c 2005-04-20 08:13:01.000000000 +0900
+++ scsi-reqfn-export/drivers/block/sx8.c 2005-04-20 08:13:35.000000000 +0900
@@ -614,7 +614,7 @@ static int carm_array_info (struct carm_
spin_unlock_irq(&host->lock);
DPRINTK("blk_insert_request, tag == %u\n", idx);
- blk_insert_request(host->oob_q, crq->rq, 1, crq, 0);
+ blk_insert_request(host->oob_q, crq->rq, 1, crq);
return 0;
@@ -653,7 +653,7 @@ static int carm_send_special (struct car
crq->msg_bucket = (u32) rc;
DPRINTK("blk_insert_request, tag == %u\n", idx);
- blk_insert_request(host->oob_q, crq->rq, 1, crq, 0);
+ blk_insert_request(host->oob_q, crq->rq, 1, crq);
return 0;
}
Index: scsi-reqfn-export/drivers/scsi/scsi_lib.c
===================================================================
--- scsi-reqfn-export.orig/drivers/scsi/scsi_lib.c 2005-04-20 08:13:35.000000000 +0900
+++ scsi-reqfn-export/drivers/scsi/scsi_lib.c 2005-04-20 08:13:35.000000000 +0900
@@ -92,7 +92,7 @@ int scsi_insert_special_req(struct scsi_
*/
sreq->sr_request->flags &= ~REQ_DONTPREP;
blk_insert_request(sreq->sr_device->request_queue, sreq->sr_request,
- at_head, sreq, 0);
+ at_head, sreq);
return 0;
}
Index: scsi-reqfn-export/include/linux/blkdev.h
===================================================================
--- scsi-reqfn-export.orig/include/linux/blkdev.h 2005-04-20 08:13:01.000000000 +0900
+++ scsi-reqfn-export/include/linux/blkdev.h 2005-04-20 08:13:35.000000000 +0900
@@ -544,7 +544,7 @@ extern void blk_end_sync_rq(struct reque
extern void blk_attempt_remerge(request_queue_t *, struct request *);
extern void __blk_attempt_remerge(request_queue_t *, struct request *);
extern struct request *blk_get_request(request_queue_t *, int, int);
-extern void blk_insert_request(request_queue_t *, struct request *, int, void *, int);
+extern void blk_insert_request(request_queue_t *, struct request *, int, void *);
extern void blk_requeue_request(request_queue_t *, struct request *);
extern void blk_plug_device(request_queue_t *);
extern int blk_remove_plug(request_queue_t *);
04_scsi_REQ_SPECIAL_semantic_scsi_requeue_command.patch
scsi_requeue_request() used to use blk_insert_request() for
requeueing requests. This depends on the unobvious behavior
of blk_insert_request() setting REQ_SPECIAL and
REQ_SOFTBARRIER when requeueing. This patch makes
scsi_queue_insert() use blk_requeue_request(). As REQ_SPECIAL
means special requests and REQ_SOFTBARRIER is automatically
handled by blk layer now, no flag needs to be set.
Signed-off-by: Tejun Heo <[email protected]>
scsi_lib.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletion(-)
Index: scsi-reqfn-export/drivers/scsi/scsi_lib.c
===================================================================
--- scsi-reqfn-export.orig/drivers/scsi/scsi_lib.c 2005-04-20 08:13:34.000000000 +0900
+++ scsi-reqfn-export/drivers/scsi/scsi_lib.c 2005-04-20 08:13:35.000000000 +0900
@@ -480,8 +480,13 @@ static void scsi_run_queue(struct reques
*/
static void scsi_requeue_command(struct request_queue *q, struct scsi_cmnd *cmd)
{
+ unsigned long flags;
+
cmd->request->flags &= ~REQ_DONTPREP;
- blk_insert_request(q, cmd->request, 1, cmd, 1);
+
+ spin_lock_irqsave(q->queue_lock, flags);
+ blk_requeue_request(q, cmd->request);
+ spin_unlock_irqrestore(q->queue_lock, flags);
scsi_run_queue(q);
}
02_scsi_REQ_SPECIAL_semantic_scsi_init_io.patch
scsi_init_io() used to set REQ_SPECIAL when it fails sg
allocation before requeueing the request by returning
BLKPREP_DEFER. REQ_SPECIAL is being updated to mean special
requests. So, remove REQ_SPECIAL setting.
Signed-off-by: Tejun Heo <[email protected]>
scsi_lib.c | 4 +---
1 files changed, 1 insertion(+), 3 deletions(-)
Index: scsi-reqfn-export/drivers/scsi/scsi_lib.c
===================================================================
--- scsi-reqfn-export.orig/drivers/scsi/scsi_lib.c 2005-04-20 08:13:33.000000000 +0900
+++ scsi-reqfn-export/drivers/scsi/scsi_lib.c 2005-04-20 08:13:34.000000000 +0900
@@ -935,10 +935,8 @@ static int scsi_init_io(struct scsi_cmnd
* if sg table allocation fails, requeue request later.
*/
sgpnt = scsi_alloc_sgtable(cmd, GFP_ATOMIC);
- if (unlikely(!sgpnt)) {
- req->flags |= REQ_SPECIAL;
+ if (unlikely(!sgpnt))
return BLKPREP_DEFER;
- }
cmd->request_buffer = (char *) sgpnt;
cmd->request_bufflen = req->nr_sectors << 9;
On Wed, Apr 20 2005, Tejun Heo wrote:
> 01_scsi_blk_make_started_requests_ordered.patch
>
> Reordering already started requests is without any real
> benefit and causes problems if the request has its
> driver-specific resources allocated (as in SCSI). This patch
> makes elv_next_request() set REQ_SOFTBARRIER automatically
> when a request is dispatched.
>
> As both as and cfq schedulers don't allow passing requeued
> requests, the only behavior change is that requests deferred
> by prep_fn won't be passed by other requests. This change
> shouldn't cause any problem. The only affected driver other
> than SCSI is i2o_block.
>
> Signed-off-by: Tejun Heo <[email protected]>
>
> elevator.c | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> Index: scsi-reqfn-export/drivers/block/elevator.c
> ===================================================================
> --- scsi-reqfn-export.orig/drivers/block/elevator.c 2005-04-20 08:13:01.000000000 +0900
> +++ scsi-reqfn-export/drivers/block/elevator.c 2005-04-20 08:13:33.000000000 +0900
> @@ -370,11 +370,11 @@ struct request *elv_next_request(request
>
> while ((rq = __elv_next_request(q)) != NULL) {
> /*
> - * just mark as started even if we don't start it, a request
> - * that has been delayed should not be passed by new incoming
> - * requests
> + * just mark as started even if we don't start it.
> + * also, as a request that has been delayed should not
> + * be passed by new incoming requests, set softbarrier.
> */
> - rq->flags |= REQ_STARTED;
> + rq->flags |= REQ_STARTED | REQ_SOFTBARRIER;
>
> if (rq == q->last_merge)
> q->last_merge = NULL;
Do it on requeue, please - not on the initial spotting of the request.
--
Jens Axboe
Jens Axboe wrote:
> On Wed, Apr 20 2005, Tejun Heo wrote:
>
>>01_scsi_blk_make_started_requests_ordered.patch
>>
>> Reordering already started requests is without any real
>> benefit and causes problems if the request has its
>> driver-specific resources allocated (as in SCSI). This patch
>> makes elv_next_request() set REQ_SOFTBARRIER automatically
>> when a request is dispatched.
>>
>> As both as and cfq schedulers don't allow passing requeued
>> requests, the only behavior change is that requests deferred
>> by prep_fn won't be passed by other requests. This change
>> shouldn't cause any problem. The only affected driver other
>> than SCSI is i2o_block.
>>
>>Signed-off-by: Tejun Heo <[email protected]>
>>
>> elevator.c | 8 ++++----
>> 1 files changed, 4 insertions(+), 4 deletions(-)
>>
>>Index: scsi-reqfn-export/drivers/block/elevator.c
>>===================================================================
>>--- scsi-reqfn-export.orig/drivers/block/elevator.c 2005-04-20 08:13:01.000000000 +0900
>>+++ scsi-reqfn-export/drivers/block/elevator.c 2005-04-20 08:13:33.000000000 +0900
>>@@ -370,11 +370,11 @@ struct request *elv_next_request(request
>>
>> while ((rq = __elv_next_request(q)) != NULL) {
>> /*
>>- * just mark as started even if we don't start it, a request
>>- * that has been delayed should not be passed by new incoming
>>- * requests
>>+ * just mark as started even if we don't start it.
>>+ * also, as a request that has been delayed should not
>>+ * be passed by new incoming requests, set softbarrier.
>> */
>>- rq->flags |= REQ_STARTED;
>>+ rq->flags |= REQ_STARTED | REQ_SOFTBARRIER;
>>
>> if (rq == q->last_merge)
>> q->last_merge = NULL;
>
>
> Do it on requeue, please - not on the initial spotting of the request.
>
The thing is that we also need to set REQ_SOFTBARRIER on
BLKPREP_DEFER. So, it will be two places - in elv_next_request and
blk_requeue_request. The end result will be the same. Do you think
doing on requeue paths is better?
--
tejun
Hello, Jens.
On Wed, Apr 20, 2005 at 08:30:10AM +0200, Jens Axboe wrote:
> Do it on requeue, please - not on the initial spotting of the request.
This is the reworked version of the patch. It sets REQ_SOFTBARRIER
in two places - in elv_next_request() on BLKPREP_DEFER and in
blk_requeue_request().
Other patches apply cleanly with this patch or the original one and
the end result is the same, so take your pick. :-)
Signed-off-by: Tejun Heo <[email protected]>
Index: scsi-reqfn-export/drivers/block/elevator.c
===================================================================
--- scsi-reqfn-export.orig/drivers/block/elevator.c 2005-04-20 16:24:26.000000000 +0900
+++ scsi-reqfn-export/drivers/block/elevator.c 2005-04-20 16:31:36.000000000 +0900
@@ -291,6 +291,13 @@ void elv_requeue_request(request_queue_t
}
/*
+ * the request is prepped and may have some resources allocated.
+ * allowing unprepped requests to pass this one may cause resource
+ * deadlock. turn on softbarrier.
+ */
+ rq->flags |= REQ_SOFTBARRIER;
+
+ /*
* if iosched has an explicit requeue hook, then use that. otherwise
* just put the request at the front of the queue
*/
@@ -386,6 +393,12 @@ struct request *elv_next_request(request
if (ret == BLKPREP_OK) {
break;
} else if (ret == BLKPREP_DEFER) {
+ /*
+ * the request may have been (partially) prepped.
+ * we need to keep this request in the front to
+ * avoid resource deadlock. turn on softbarrier.
+ */
+ rq->flags |= REQ_SOFTBARRIER;
rq = NULL;
break;
} else if (ret == BLKPREP_KILL) {
On Wed, 2005-04-20 at 16:40 +0900, Tejun Heo wrote:
> Hello, Jens.
>
> On Wed, Apr 20, 2005 at 08:30:10AM +0200, Jens Axboe wrote:
> > Do it on requeue, please - not on the initial spotting of the request.
>
> This is the reworked version of the patch. It sets REQ_SOFTBARRIER
> in two places - in elv_next_request() on BLKPREP_DEFER and in
> blk_requeue_request().
>
> Other patches apply cleanly with this patch or the original one and
> the end result is the same, so take your pick. :-)
>
I'm not sure that you need *either* one.
As far as I'm aware, REQ_SOFTBARRIER is used when feeding requests
into the top of the block layer, and is used to guarantee the device
driver gets the requests in a specific ordering.
When dealing with the requests at the other end (ie.
elevator_next_req_fn, blk_requeue_request), then ordering does not
change.
That is - if you call elevator_next_req_fn and don't dequeue the
request, then that's the same request you'll get next time.
And blk_requeue_request will push the request back onto the end of
the queue in a LIFO manner.
So I think adding barriers, apart from not doing anything, confuses
the issue because it suggests there *could* be reordering without
them.
Or am I completely wrong? It's been a while since I last got into
the code.
--
SUSE Labs, Novell Inc.
Nick Piggin wrote:
> On Wed, 2005-04-20 at 16:40 +0900, Tejun Heo wrote:
>
>> Hello, Jens.
>>
>>On Wed, Apr 20, 2005 at 08:30:10AM +0200, Jens Axboe wrote:
>>
>>>Do it on requeue, please - not on the initial spotting of the request.
>>
>> This is the reworked version of the patch. It sets REQ_SOFTBARRIER
>>in two places - in elv_next_request() on BLKPREP_DEFER and in
>>blk_requeue_request().
>>
>> Other patches apply cleanly with this patch or the original one and
>>the end result is the same, so take your pick. :-)
>>
>
>
> I'm not sure that you need *either* one.
>
> As far as I'm aware, REQ_SOFTBARRIER is used when feeding requests
> into the top of the block layer, and is used to guarantee the device
> driver gets the requests in a specific ordering.
>
> When dealing with the requests at the other end (ie.
> elevator_next_req_fn, blk_requeue_request), then ordering does not
> change.
>
> That is - if you call elevator_next_req_fn and don't dequeue the
> request, then that's the same request you'll get next time.
>
> And blk_requeue_request will push the request back onto the end of
> the queue in a LIFO manner.
>
> So I think adding barriers, apart from not doing anything, confuses
> the issue because it suggests there *could* be reordering without
> them.
>
> Or am I completely wrong? It's been a while since I last got into
> the code.
Well, yeah, all schedulers have dispatch queue (noop has only the
dispatch queue) and use them to defer/requeue, so no reordering will
happen, but I'm not sure they are required to be like this or just
happen to be implemented so.
Hmm, well, it seems that setting REQ_SOFTBARRIER on requeue path isn't
necessary as we have INSERT_FRONT policy on requeue, and if
elv_next_req_fn() is required to return the same request when the
request isn't dequeued, you're right and we don't need this patch at
all. We are guaranteed that all requeued requests are served in LIFO
manner.
BTW, the same un-dequeued request rule is sort of already broken as
INSERT_FRONT request passes a returned but un-dequeued request, but,
then again, we need this behavior as we have to favor fully-prepped
requests over partially-prepped one.
--
tejun
On Wed, Apr 20 2005, Tejun Heo wrote:
> Nick Piggin wrote:
> > On Wed, 2005-04-20 at 16:40 +0900, Tejun Heo wrote:
> >
> >> Hello, Jens.
> >>
> >>On Wed, Apr 20, 2005 at 08:30:10AM +0200, Jens Axboe wrote:
> >>
> >>>Do it on requeue, please - not on the initial spotting of the request.
> >>
> >> This is the reworked version of the patch. It sets REQ_SOFTBARRIER
> >>in two places - in elv_next_request() on BLKPREP_DEFER and in
> >>blk_requeue_request().
> >>
> >> Other patches apply cleanly with this patch or the original one and
> >>the end result is the same, so take your pick. :-)
> >>
> >
> >
> > I'm not sure that you need *either* one.
> >
> > As far as I'm aware, REQ_SOFTBARRIER is used when feeding requests
> > into the top of the block layer, and is used to guarantee the device
> > driver gets the requests in a specific ordering.
> >
> > When dealing with the requests at the other end (ie.
> > elevator_next_req_fn, blk_requeue_request), then ordering does not
> > change.
> >
> > That is - if you call elevator_next_req_fn and don't dequeue the
> > request, then that's the same request you'll get next time.
> >
> > And blk_requeue_request will push the request back onto the end of
> > the queue in a LIFO manner.
> >
> > So I think adding barriers, apart from not doing anything, confuses
> > the issue because it suggests there *could* be reordering without
> > them.
> >
> > Or am I completely wrong? It's been a while since I last got into
> > the code.
>
> Well, yeah, all schedulers have dispatch queue (noop has only the
> dispatch queue) and use them to defer/requeue, so no reordering will
> happen, but I'm not sure they are required to be like this or just
> happen to be implemented so.
Precisely, I feel much better making sure SOFTBARRIER is set so that we
_know_ that a scheduler following the outlined rules will do the right
thing.
> Hmm, well, it seems that setting REQ_SOFTBARRIER on requeue path isn't
> necessary as we have INSERT_FRONT policy on requeue, and if
> elv_next_req_fn() is required to return the same request when the
> request isn't dequeued, you're right and we don't need this patch at
> all. We are guaranteed that all requeued requests are served in LIFO
> manner.
After a requeue, it is not required to return the same request again.
> BTW, the same un-dequeued request rule is sort of already broken as
> INSERT_FRONT request passes a returned but un-dequeued request, but,
> then again, we need this behavior as we have to favor fully-prepped
> requests over partially-prepped one.
INSERT_FRONT really should skip requests with REQ_STARTED on the
dispatch list to be fully safe.
--
Jens Axboe
Jens Axboe wrote:
> On Wed, Apr 20 2005, Tejun Heo wrote:
>> Well, yeah, all schedulers have dispatch queue (noop has only the
>>dispatch queue) and use them to defer/requeue, so no reordering will
>>happen, but I'm not sure they are required to be like this or just
>>happen to be implemented so.
>
>
> Precisely, I feel much better making sure SOFTBARRIER is set so that we
> _know_ that a scheduler following the outlined rules will do the right
> thing.
>
Well yeah, at the moment I am just following implementations as
defining the standard.
>
>> Hmm, well, it seems that setting REQ_SOFTBARRIER on requeue path isn't
>>necessary as we have INSERT_FRONT policy on requeue, and if
>>elv_next_req_fn() is required to return the same request when the
>>request isn't dequeued, you're right and we don't need this patch at
>>all. We are guaranteed that all requeued requests are served in LIFO
>>manner.
>
>
> After a requeue, it is not required to return the same request again.
>
Well I guess not.
Would there be any benefit to reordering after a requeue?
>
>> BTW, the same un-dequeued request rule is sort of already broken as
>>INSERT_FRONT request passes a returned but un-dequeued request, but,
>>then again, we need this behavior as we have to favor fully-prepped
>>requests over partially-prepped one.
>
>
> INSERT_FRONT really should skip requests with REQ_STARTED on the
> dispatch list to be fully safe.
>
I guess this could be one use of 'reordering' after a requeue.
I'm not sure this would need a REQ_SOFTBARRIER either though, really.
Your basic io scheduler framework - ie. a FIFO dispatch list which
can have requests requeued on the front models pretty well what the
block layer needs of the elevator.
Considering all requeues and all elv_next_request but not dequeued
requests would have this REQ_SOFTBARRIER bit set, any other model
that theoretically would allow reordering would degenerate to this
dispatch list behaviour, right?
In which case, the dispatch list is effectively basically the most
efficient way to do it? In which case should we just explicitly
document that in the API?
--
SUSE Labs, Novell Inc.
On Wed, Apr 20 2005, Nick Piggin wrote:
> >>Hmm, well, it seems that setting REQ_SOFTBARRIER on requeue path isn't
> >>necessary as we have INSERT_FRONT policy on requeue, and if
> >>elv_next_req_fn() is required to return the same request when the
> >>request isn't dequeued, you're right and we don't need this patch at
> >>all. We are guaranteed that all requeued requests are served in LIFO
> >>manner.
> >
> >
> >After a requeue, it is not required to return the same request again.
> >
>
> Well I guess not.
>
> Would there be any benefit to reordering after a requeue?
Logic dictates that requeues should maintain ordering, since we don't
want to reorder around the original io scheduler decisions. But you
could be requeuing more than one request.
> >>BTW, the same un-dequeued request rule is sort of already broken as
> >>INSERT_FRONT request passes a returned but un-dequeued request, but,
> >>then again, we need this behavior as we have to favor fully-prepped
> >>requests over partially-prepped one.
> >
> >
> >INSERT_FRONT really should skip requests with REQ_STARTED on the
> >dispatch list to be fully safe.
> >
>
> I guess this could be one use of 'reordering' after a requeue.
Yeah, or perhaps the io scheduler might determine that a request has
higher prio than a requeued one. I'm not sure what semantics to place
on soft-barrier, I've always taken it to mean 'maintain ordering if
convenient' where the hard-barrier must be followed.
> I'm not sure this would need a REQ_SOFTBARRIER either though, really.
>
> Your basic io scheduler framework - ie. a FIFO dispatch list which
> can have requests requeued on the front models pretty well what the
> block layer needs of the elevator.
>
> Considering all requeues and all elv_next_request but not dequeued
> requests would have this REQ_SOFTBARRIER bit set, any other model
> that theoretically would allow reordering would degenerate to this
> dispatch list behaviour, right?
Not sure I follow this - I don't want REQ_SOFTBARRIER set automatically
on elv_next_request() return, it should only happen on requeues.
REQ_STARTED implies that you should not pass this request, since the io
scheduler is required to return this request again until dequeue is
called. But the result is the same, correct.
> In which case, the dispatch list is effectively basically the most
> efficient way to do it? In which case should we just explicitly
> document that in the API?
You lost me, please detail exactly what behaviour you want documented :)
--
Jens Axboe
Jens Axboe wrote:
> On Wed, Apr 20 2005, Nick Piggin wrote:
>
>>I guess this could be one use of 'reordering' after a requeue.
>
>
> Yeah, or perhaps the io scheduler might determine that a request has
> higher prio than a requeued one. I'm not sure what semantics to place
I guess this is possible. It is often only a single request
that is on the dispatch list though, so I don't know if it
would make sense to reorder it by priority again.
> on soft-barrier, I've always taken it to mean 'maintain ordering if
> convenient' where the hard-barrier must be followed.
>
I've thought it was SOFTBARRIER ensures the device driver (and
hardware?) sees the request in this order, and HARDBARRIER ensures
it reaches stable storage in this order.
Not exactly sure why you would want a softbarrier and not a
hardbarrier. Maybe for special commands.
>
>>I'm not sure this would need a REQ_SOFTBARRIER either though, really.
>>
>>Your basic io scheduler framework - ie. a FIFO dispatch list which
>>can have requests requeued on the front models pretty well what the
>>block layer needs of the elevator.
>>
>>Considering all requeues and all elv_next_request but not dequeued
>>requests would have this REQ_SOFTBARRIER bit set, any other model
>>that theoretically would allow reordering would degenerate to this
>>dispatch list behaviour, right?
>
>
> Not sure I follow this - I don't want REQ_SOFTBARRIER set automatically
> on elv_next_request() return, it should only happen on requeues.
> REQ_STARTED implies that you should not pass this request, since the io
> scheduler is required to return this request again until dequeue is
> called. But the result is the same, correct.
>
OK - but I'm wondering would it ever make sense to do it any
other way? I would have thought no, in which case we can document
that requests seen by 'elv_next_request', and those requeued back
into the device will not be reordered, and so Tejun does not need
to set REQ_SOFTBARRIER.
But I'm not so sure now... it isn't really that big a deal ;)
So whatever you're happy with is fine. Sorry for the nose.
--
SUSE Labs, Novell Inc.
On Wed, Apr 20 2005, Nick Piggin wrote:
> Jens Axboe wrote:
> >On Wed, Apr 20 2005, Nick Piggin wrote:
> >
>
> >>I guess this could be one use of 'reordering' after a requeue.
> >
> >
> >Yeah, or perhaps the io scheduler might determine that a request has
> >higher prio than a requeued one. I'm not sure what semantics to place
>
> I guess this is possible. It is often only a single request
> that is on the dispatch list though, so I don't know if it
> would make sense to reorder it by priority again.
Depends entirely on the io scheduler. CFQ may put several on the
dispatch list.
> >on soft-barrier, I've always taken it to mean 'maintain ordering if
> >convenient' where the hard-barrier must be followed.
> >
>
> I've thought it was SOFTBARRIER ensures the device driver (and
> hardware?) sees the request in this order, and HARDBARRIER ensures
> it reaches stable storage in this order.
>
> Not exactly sure why you would want a softbarrier and not a
> hardbarrier. Maybe for special commands.
It is the cleaner interpretation. CFQ marks requests as requeued
internally and gives preference to them for reissue, but it may return
another first (actually, I think it even checks for ->requeued on
dispatch sort, so it wont right now).
> >>I'm not sure this would need a REQ_SOFTBARRIER either though, really.
> >>
> >>Your basic io scheduler framework - ie. a FIFO dispatch list which
> >>can have requests requeued on the front models pretty well what the
> >>block layer needs of the elevator.
> >>
> >>Considering all requeues and all elv_next_request but not dequeued
> >>requests would have this REQ_SOFTBARRIER bit set, any other model
> >>that theoretically would allow reordering would degenerate to this
> >>dispatch list behaviour, right?
> >
> >
> >Not sure I follow this - I don't want REQ_SOFTBARRIER set automatically
> >on elv_next_request() return, it should only happen on requeues.
> >REQ_STARTED implies that you should not pass this request, since the io
> >scheduler is required to return this request again until dequeue is
> >called. But the result is the same, correct.
> >
>
> OK - but I'm wondering would it ever make sense to do it any
> other way? I would have thought no, in which case we can document
> that requests seen by 'elv_next_request', and those requeued back
> into the device will not be reordered, and so Tejun does not need
> to set REQ_SOFTBARRIER.
>
> But I'm not so sure now... it isn't really that big a deal ;)
> So whatever you're happy with is fine. Sorry for the nose.
It's not noise, it would be nice to have this entirely documented so
that there isn't any confusion on what is guaranteed vs what currently
happens in most places.
But I don't want to document that they are never reordered. For requeues
it make sense to maintain ordering in most cases, but it also may make
sense to reorder for higher priority io. If the driver does _not_ want a
particular request reordered for data integrity reasons, then that needs
to be explicitly specified.
--
Jens Axboe
Hello, guys.
Jens Axboe wrote:
> On Wed, Apr 20 2005, Nick Piggin wrote:
>
>>Jens Axboe wrote:
>>
>>>On Wed, Apr 20 2005, Nick Piggin wrote:
>>>
>>
>>>>I guess this could be one use of 'reordering' after a requeue.
>>>
>>>
>>>Yeah, or perhaps the io scheduler might determine that a request has
>>>higher prio than a requeued one. I'm not sure what semantics to place
>>
>>I guess this is possible. It is often only a single request
>>that is on the dispatch list though, so I don't know if it
>>would make sense to reorder it by priority again.
>
>
> Depends entirely on the io scheduler. CFQ may put several on the
> dispatch list.
>
>
>>>on soft-barrier, I've always taken it to mean 'maintain ordering if
>>>convenient' where the hard-barrier must be followed.
>>>
>>
>>I've thought it was SOFTBARRIER ensures the device driver (and
>>hardware?) sees the request in this order, and HARDBARRIER ensures
>>it reaches stable storage in this order.
>>
>>Not exactly sure why you would want a softbarrier and not a
>>hardbarrier. Maybe for special commands.
>
>
> It is the cleaner interpretation. CFQ marks requests as requeued
> internally and gives preference to them for reissue, but it may return
> another first (actually, I think it even checks for ->requeued on
> dispatch sort, so it wont right now).
>
>
>>>>I'm not sure this would need a REQ_SOFTBARRIER either though, really.
>>>>
>>>>Your basic io scheduler framework - ie. a FIFO dispatch list which
>>>>can have requests requeued on the front models pretty well what the
>>>>block layer needs of the elevator.
>>>>
>>>>Considering all requeues and all elv_next_request but not dequeued
>>>>requests would have this REQ_SOFTBARRIER bit set, any other model
>>>>that theoretically would allow reordering would degenerate to this
>>>>dispatch list behaviour, right?
>>>
>>>
>>>Not sure I follow this - I don't want REQ_SOFTBARRIER set automatically
>>>on elv_next_request() return, it should only happen on requeues.
>>>REQ_STARTED implies that you should not pass this request, since the io
>>>scheduler is required to return this request again until dequeue is
>>>called. But the result is the same, correct.
>>>
>>
>>OK - but I'm wondering would it ever make sense to do it any
>>other way? I would have thought no, in which case we can document
>>that requests seen by 'elv_next_request', and those requeued back
>>into the device will not be reordered, and so Tejun does not need
>>to set REQ_SOFTBARRIER.
>>
>>But I'm not so sure now... it isn't really that big a deal ;)
>>So whatever you're happy with is fine. Sorry for the nose.
>
>
> It's not noise, it would be nice to have this entirely documented so
> that there isn't any confusion on what is guaranteed vs what currently
> happens in most places.
>
> But I don't want to document that they are never reordered. For requeues
> it make sense to maintain ordering in most cases, but it also may make
> sense to reorder for higher priority io. If the driver does _not_ want a
> particular request reordered for data integrity reasons, then that needs
> to be explicitly specified.
>
So, I guess this is settled now. James, what do you think about the
rest of this patchset? If you're okay, I think we can proceed merging
as there doesn't seem to be any issue left. Do we put this into the
SCSI tree? Or separate out blk and SCSI changes?
Once we're done merging this patchset, I'll regenerate & repost the
reqfn reimpl patchset.
Thanks a lot. :-)
--
tejun
On Wed, 2005-04-20 at 08:15 +0900, Tejun Heo wrote:
> - * Insert this command at the head of the queue for it's device.
> - * It will go before all other commands that are already in the queue.
> - *
> - * NOTE: there is magic here about the way the queue is plugged if
> - * we have no outstanding commands.
> - *
> - * Although this *doesn't* plug the queue, it does call the request
> - * function. The SCSI request function detects the blocked condition
> - * and plugs the queue appropriately.
This comment still looks appropriate to me ... why do you want to remove
it?
> + * Requeue the command.
> */
> - blk_insert_request(device->request_queue, cmd->request, 1, cmd, 1);
> + spin_lock_irqsave(q->queue_lock, flags);
> + blk_requeue_request(q, cmd->request);
> + spin_unlock_irqrestore(q->queue_lock, flags);
> +
> + scsi_run_queue(q);
Really, wouldn't it be much more efficient simply to call blk_run_queue
()? since the blocked flags were set above, that's pretty much what
scsi_run_queue() collapses to.
James
Hello, James.
James Bottomley wrote:
> On Wed, 2005-04-20 at 08:15 +0900, Tejun Heo wrote:
>
>>- * Insert this command at the head of the queue for it's device.
>>- * It will go before all other commands that are already in the queue.
>>- *
>>- * NOTE: there is magic here about the way the queue is plugged if
>>- * we have no outstanding commands.
>>- *
>>- * Although this *doesn't* plug the queue, it does call the request
>>- * function. The SCSI request function detects the blocked condition
>>- * and plugs the queue appropriately.
>
>
> This comment still looks appropriate to me ... why do you want to remove
> it?
>
Well, the thing is that we don't really care what exactly happens to
the queue or how the queue is plugged or not. All we need to do are to
requeue the request and kick the queue in the ass. Hmmm, maybe I should
keep the comment about how the request will be put at the head of the
queue, but the second part about plugging doesn't really belong here, I
think.
>
>>+ * Requeue the command.
>> */
>>- blk_insert_request(device->request_queue, cmd->request, 1, cmd, 1);
>>+ spin_lock_irqsave(q->queue_lock, flags);
>>+ blk_requeue_request(q, cmd->request);
>>+ spin_unlock_irqrestore(q->queue_lock, flags);
>>+
>>+ scsi_run_queue(q);
>
>
> Really, wouldn't it be much more efficient simply to call blk_run_queue
> ()? since the blocked flags were set above, that's pretty much what
> scsi_run_queue() collapses to.
>
Yes, that will be more efficient but I don't think it would make any
noticeable difference. IMO, universally using scsi_run_queue() to kick
scsi request queues is better than mixing blk_run_queue() and
scsi_run_queue() for probably unnoticeable optimization. If we start to
mix'em, we need to rationalize why specific one is chosen in specific
places and that's just unnecessary.
Thanks.
--
tejun
On Thu, 2005-04-21 at 09:20 +0900, Tejun Heo wrote:
> Hello, James.
>
> James Bottomley wrote:
> > On Wed, 2005-04-20 at 08:15 +0900, Tejun Heo wrote:
> >
> >>- * Insert this command at the head of the queue for it's device.
> >>- * It will go before all other commands that are already in the queue.
> >>- *
> >>- * NOTE: there is magic here about the way the queue is plugged if
> >>- * we have no outstanding commands.
> >>- *
> >>- * Although this *doesn't* plug the queue, it does call the request
> >>- * function. The SCSI request function detects the blocked condition
> >>- * and plugs the queue appropriately.
> >
> >
> > This comment still looks appropriate to me ... why do you want to remove
> > it?
> >
>
> Well, the thing is that we don't really care what exactly happens to
> the queue or how the queue is plugged or not. All we need to do are to
> requeue the request and kick the queue in the ass. Hmmm, maybe I should
> keep the comment about how the request will be put at the head of the
> queue, but the second part about plugging doesn't really belong here, I
> think.
Really? We do care greatly. If you requeue with no other outstanding
commands to the device, the block queue will never restart unless it's
plugged, and the device will hang. The comment is explaining how this
happens.
> Yes, that will be more efficient but I don't think it would make
> any
> noticeable difference. IMO, universally using scsi_run_queue() to
> kick
> scsi request queues is better than mixing blk_run_queue() and
> scsi_run_queue() for probably unnoticeable optimization. If we start
> to
> mix'em, we need to rationalize why specific one is chosen in specific
> places and that's just unnecessary.
Fair enough.
James
James Bottomley wrote:
> On Thu, 2005-04-21 at 09:20 +0900, Tejun Heo wrote:
>
>> Hello, James.
>>
>>James Bottomley wrote:
>>
>>>On Wed, 2005-04-20 at 08:15 +0900, Tejun Heo wrote:
>>>
>>>
>>>>- * Insert this command at the head of the queue for it's device.
>>>>- * It will go before all other commands that are already in the queue.
>>>>- *
>>>>- * NOTE: there is magic here about the way the queue is plugged if
>>>>- * we have no outstanding commands.
>>>>- *
>>>>- * Although this *doesn't* plug the queue, it does call the request
>>>>- * function. The SCSI request function detects the blocked condition
>>>>- * and plugs the queue appropriately.
>>>
>>>
>>>This comment still looks appropriate to me ... why do you want to remove
>>>it?
>>>
>>
>> Well, the thing is that we don't really care what exactly happens to
>>the queue or how the queue is plugged or not. All we need to do are to
>>requeue the request and kick the queue in the ass. Hmmm, maybe I should
>>keep the comment about how the request will be put at the head of the
>>queue, but the second part about plugging doesn't really belong here, I
>>think.
>
>
> Really? We do care greatly. If you requeue with no other outstanding
> commands to the device, the block queue will never restart unless it's
> plugged, and the device will hang. The comment is explaining how this
> happens.
>
Yes, you're right. My point was that that's scsi_run_queue()'s
business. We don't need to comment that deep when we're requeueing a
request. After we put a request on a queue, we kick the queue. It's
the queue running function's responsibility to determine whether to run
the request right away or to defer processing (and thus plug). I wasn't
saying that the eventual plugging isn't necessary, but that the comment
is sort of excessive.
Anyways, if you think the comment is necessary, I don't feel strong
against it. I'll rewrite above comment to fit the new code and repost
this patch soon.
>
>> Yes, that will be more efficient but I don't think it would make
>>any
>>noticeable difference. IMO, universally using scsi_run_queue() to
>>kick
>>scsi request queues is better than mixing blk_run_queue() and
>>scsi_run_queue() for probably unnoticeable optimization. If we start
>>to
>>mix'em, we need to rationalize why specific one is chosen in specific
>>places and that's just unnecessary.
>
>
> Fair enough.
Thanks.
--
tejun
Hello, James.
This is the modified patch with the comment (slightly modified).
With this patch, the next patch in this patchset complains about line
offset but it's okay.
Thanks.
Signed-off-by: Tejun Heo <[email protected]>
Index: scsi-reqfn-export/drivers/scsi/scsi_lib.c
===================================================================
--- scsi-reqfn-export.orig/drivers/scsi/scsi_lib.c 2005-04-21 11:33:13.000000000 +0900
+++ scsi-reqfn-export/drivers/scsi/scsi_lib.c 2005-04-21 11:39:17.000000000 +0900
@@ -96,6 +96,8 @@ int scsi_insert_special_req(struct scsi_
return 0;
}
+static void scsi_run_queue(struct request_queue *q);
+
/*
* Function: scsi_queue_insert()
*
@@ -119,6 +121,8 @@ int scsi_queue_insert(struct scsi_cmnd *
{
struct Scsi_Host *host = cmd->device->host;
struct scsi_device *device = cmd->device;
+ struct request_queue *q = device->request_queue;
+ unsigned long flags;
SCSI_LOG_MLQUEUE(1,
printk("Inserting command %p into mlqueue\n", cmd));
@@ -154,17 +158,22 @@ int scsi_queue_insert(struct scsi_cmnd *
scsi_device_unbusy(device);
/*
- * Insert this command at the head of the queue for it's device.
- * It will go before all other commands that are already in the queue.
+ * Requeue this command. It will go before all other commands
+ * that are already in the queue.
*
* NOTE: there is magic here about the way the queue is plugged if
* we have no outstanding commands.
*
- * Although this *doesn't* plug the queue, it does call the request
+ * Although we *don't* plug the queue, we call the request
* function. The SCSI request function detects the blocked condition
* and plugs the queue appropriately.
- */
- blk_insert_request(device->request_queue, cmd->request, 1, cmd, 1);
+ */
+ spin_lock_irqsave(q->queue_lock, flags);
+ blk_requeue_request(q, cmd->request);
+ spin_unlock_irqrestore(q->queue_lock, flags);
+
+ scsi_run_queue(q);
+
return 0;
}
On Wed, Apr 20 2005, James Bottomley wrote:
> On Wed, 2005-04-20 at 08:15 +0900, Tejun Heo wrote:
> > - * Insert this command at the head of the queue for it's device.
> > - * It will go before all other commands that are already in the queue.
> > - *
> > - * NOTE: there is magic here about the way the queue is plugged if
> > - * we have no outstanding commands.
> > - *
> > - * Although this *doesn't* plug the queue, it does call the request
> > - * function. The SCSI request function detects the blocked condition
> > - * and plugs the queue appropriately.
>
> This comment still looks appropriate to me ... why do you want to remove
> it?
>
> > + * Requeue the command.
> > */
> > - blk_insert_request(device->request_queue, cmd->request, 1, cmd, 1);
> > + spin_lock_irqsave(q->queue_lock, flags);
> > + blk_requeue_request(q, cmd->request);
> > + spin_unlock_irqrestore(q->queue_lock, flags);
> > +
> > + scsi_run_queue(q);
>
> Really, wouldn't it be much more efficient simply to call blk_run_queue
> ()? since the blocked flags were set above, that's pretty much what
> scsi_run_queue() collapses to.
I wondered about this action recently myself. What is the point in
requeueing this request, only to call scsi_run_queue() ->
blk_run_queue() -> issue same request. If the point really is to reissue
the request immediately, I can think of many ways more efficient than
this :-)
--
Jens Axboe
On Thu, 2005-04-21 at 08:10 +0200, Jens Axboe wrote:
> I wondered about this action recently myself. What is the point in
> requeueing this request, only to call scsi_run_queue() ->
> blk_run_queue() -> issue same request. If the point really is to reissue
> the request immediately, I can think of many ways more efficient than
> this :-)
Well ... that's because the logic that decides whether to plug the queue
or simply exit is in the scsi_request_fn(). That's what the comment is
about. We could abstract the check into a function, but (unless you
have any suggestions on rewording it) I thought the comment made what
was going on reasonably clear.
James
On Thu, Apr 21 2005, James Bottomley wrote:
> On Thu, 2005-04-21 at 08:10 +0200, Jens Axboe wrote:
> > I wondered about this action recently myself. What is the point in
> > requeueing this request, only to call scsi_run_queue() ->
> > blk_run_queue() -> issue same request. If the point really is to reissue
> > the request immediately, I can think of many ways more efficient than
> > this :-)
>
> Well ... that's because the logic that decides whether to plug the queue
> or simply exit is in the scsi_request_fn(). That's what the comment is
> about. We could abstract the check into a function, but (unless you
> have any suggestions on rewording it) I thought the comment made what
> was going on reasonably clear.
>
Looks fine, I just missed enough code context in the patch to see this.
Since requeuing probably isn't all that uncommon, it may make sense to
optimize this a little though.
--
Jens Axboe