Re: [PATCH scsi-misc-2.6 03/05] scsi: make scsi_queue_insert() use blk_requeue_request()
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 03/05] scsi: make scsi_queue_insert() use blk_requeue_request()
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 03/05] scsi: make scsi_queue_insert() use blk_requeue_request()
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 03/05] scsi: make scsi_queue_insert() use blk_requeue_request()
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.0 +0900
+++ scsi-reqfn-export/drivers/scsi/scsi_lib.c 2005-04-21 11:39:17.0
+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;
}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 03/05] scsi: make scsi_queue_insert() use blk_requeue_request()
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 03/05] scsi: make scsi_queue_insert() use blk_requeue_request()
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 03/05] scsi: make scsi_queue_insert() use blk_requeue_request()
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 03/05] scsi: make scsi_queue_insert() use blk_requeue_request()
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 03/05] scsi: make scsi_queue_insert() use blk_requeue_request()
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.0 +0900
+++ scsi-reqfn-export/drivers/scsi/scsi_lib.c 2005-04-20 08:13:34.0
+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;
}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
