[PATCH 0/6] first batch of SCSI data path micro-optimizations

[PATCH 0/6] first batch of SCSI data path micro-optimizations

[Christoph Hellwig]

This series contains a few optimizations for the SCSI data I/O path.
These are the patches acceptable to James as-is from the previous series.
No separate benchmarking has been performed for these patches.

This work was sponsored by the ION division of Fusion IO.

[PATCH 1/6] scsi: avoid useless free_list lock roundtrips

[Christoph Hellwig]

[-- Attachment #1: 0001-scsi-avoid-useless-free_list-lock-roundtrips.patch --]
[-- Type: text/plain, Size: 1095 bytes --]

Avoid hitting the host-wide free_list lock unless we need to put a command
back onto the freelist.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index d8afec8..fb86479 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -320,13 +320,14 @@ void __scsi_put_command(struct Scsi_Host *shost, struct scsi_cmnd *cmd,
 {
 	unsigned long flags;
 
-	/* changing locks here, don't need to restore the irq state */
-	spin_lock_irqsave(&shost->free_list_lock, flags);
 	if (unlikely(list_empty(&shost->free_list))) {
-		list_add(&cmd->list, &shost->free_list);
-		cmd = NULL;
+		spin_lock_irqsave(&shost->free_list_lock, flags);
+		if (list_empty(&shost->free_list)) {
+			list_add(&cmd->list, &shost->free_list);
+			cmd = NULL;
+		}
+		spin_unlock_irqrestore(&shost->free_list_lock, flags);
 	}
-	spin_unlock_irqrestore(&shost->free_list_lock, flags);
 
 	if (likely(cmd != NULL))
 		scsi_pool_free_command(shost->cmd_pool, cmd);
-- 
1.7.10.4

[PATCH 2/6] scsi: avoid taking host_lock in scsi_run_queue unless nessecary

[Christoph Hellwig]

[-- Attachment #1: 0002-scsi-avoid-taking-host_lock-in-scsi_run_queue-unless.patch --]
[-- Type: text/plain, Size: 2111 bytes --]

If we don't have starved devices we don't need to take the host lock
to iterate over them.  Also split the function up to be more clear.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_lib.c |   43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7bd7f0d..ad516c0 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -385,29 +385,12 @@ static inline int scsi_host_is_busy(struct Scsi_Host *shost)
 	return 0;
 }
 
-/*
- * Function:	scsi_run_queue()
- *
- * Purpose:	Select a proper request queue to serve next
- *
- * Arguments:	q	- last request's queue
- *
- * Returns:     Nothing
- *
- * Notes:	The previous command was completely finished, start
- *		a new one if possible.
- */
-static void scsi_run_queue(struct request_queue *q)
+static void scsi_starved_list_run(struct Scsi_Host *shost)
 {
-	struct scsi_device *sdev = q->queuedata;
-	struct Scsi_Host *shost;
 	LIST_HEAD(starved_list);
+	struct scsi_device *sdev;
 	unsigned long flags;
 
-	shost = sdev->host;
-	if (scsi_target(sdev)->single_lun)
-		scsi_single_lun_run(sdev);
-
 	spin_lock_irqsave(shost->host_lock, flags);
 	list_splice_init(&shost->starved_list, &starved_list);
 
@@ -459,6 +442,28 @@ static void scsi_run_queue(struct request_queue *q)
 	/* put any unprocessed entries back */
 	list_splice(&starved_list, &shost->starved_list);
 	spin_unlock_irqrestore(shost->host_lock, flags);
+}
+
+/*
+ * Function:   scsi_run_queue()
+ *
+ * Purpose:    Select a proper request queue to serve next
+ *
+ * Arguments:  q       - last request's queue
+ *
+ * Returns:     Nothing
+ *
+ * Notes:      The previous command was completely finished, start
+ *             a new one if possible.
+ */
+static void scsi_run_queue(struct request_queue *q)
+{
+	struct scsi_device *sdev = q->queuedata;
+
+	if (scsi_target(sdev)->single_lun)
+		scsi_single_lun_run(sdev);
+	if (!list_empty(&sdev->host->starved_list))
+		scsi_starved_list_run(sdev->host);
 
 	blk_run_queue(q);
 }
-- 
1.7.10.4

[PATCH 3/6] scsi: do not manipulate device reference counts in scsi_get/put_command

[Christoph Hellwig]

[-- Attachment #1: 0003-scsi-do-not-manipulate-device-reference-counts-in-sc.patch --]
[-- Type: text/plain, Size: 6394 bytes --]

Many callers won't need this and we can optimize them away.  In addition
the handling in the __-prefixed variants was inconsistant to start with.

Based on an earlier patch from Bart Van Assche.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi.c         |   36 ++++++++++++------------------------
 drivers/scsi/scsi_error.c   |    6 ++++++
 drivers/scsi/scsi_lib.c     |   12 +++++++++++-
 drivers/scsi/scsi_tgt_lib.c |    3 ++-
 include/scsi/scsi_cmnd.h    |    3 +--
 5 files changed, 32 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index fb86479..843b4f1 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -284,27 +284,19 @@ EXPORT_SYMBOL_GPL(__scsi_get_command);
  */
 struct scsi_cmnd *scsi_get_command(struct scsi_device *dev, gfp_t gfp_mask)
 {
-	struct scsi_cmnd *cmd;
+	struct scsi_cmnd *cmd = __scsi_get_command(dev->host, gfp_mask);
+	unsigned long flags;
 
-	/* Bail if we can't get a reference to the device */
-	if (!get_device(&dev->sdev_gendev))
+	if (unlikely(cmd == NULL))
 		return NULL;
 
-	cmd = __scsi_get_command(dev->host, gfp_mask);
-
-	if (likely(cmd != NULL)) {
-		unsigned long flags;
-
-		cmd->device = dev;
-		INIT_LIST_HEAD(&cmd->list);
-		INIT_DELAYED_WORK(&cmd->abort_work, scmd_eh_abort_handler);
-		spin_lock_irqsave(&dev->list_lock, flags);
-		list_add_tail(&cmd->list, &dev->cmd_list);
-		spin_unlock_irqrestore(&dev->list_lock, flags);
-		cmd->jiffies_at_alloc = jiffies;
-	} else
-		put_device(&dev->sdev_gendev);
-
+	cmd->device = dev;
+	INIT_LIST_HEAD(&cmd->list);
+	INIT_DELAYED_WORK(&cmd->abort_work, scmd_eh_abort_handler);
+	spin_lock_irqsave(&dev->list_lock, flags);
+	list_add_tail(&cmd->list, &dev->cmd_list);
+	spin_unlock_irqrestore(&dev->list_lock, flags);
+	cmd->jiffies_at_alloc = jiffies;
 	return cmd;
 }
 EXPORT_SYMBOL(scsi_get_command);
@@ -315,8 +307,7 @@ EXPORT_SYMBOL(scsi_get_command);
  * @cmd: Command to free
  * @dev: parent scsi device
  */
-void __scsi_put_command(struct Scsi_Host *shost, struct scsi_cmnd *cmd,
-			struct device *dev)
+void __scsi_put_command(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
 {
 	unsigned long flags;
 
@@ -331,8 +322,6 @@ void __scsi_put_command(struct Scsi_Host *shost, struct scsi_cmnd *cmd,
 
 	if (likely(cmd != NULL))
 		scsi_pool_free_command(shost->cmd_pool, cmd);
-
-	put_device(dev);
 }
 EXPORT_SYMBOL(__scsi_put_command);
 
@@ -346,7 +335,6 @@ EXPORT_SYMBOL(__scsi_put_command);
  */
 void scsi_put_command(struct scsi_cmnd *cmd)
 {
-	struct scsi_device *sdev = cmd->device;
 	unsigned long flags;
 
 	/* serious error if the command hasn't come from a device list */
@@ -357,7 +345,7 @@ void scsi_put_command(struct scsi_cmnd *cmd)
 
 	cancel_delayed_work(&cmd->abort_work);
 
-	__scsi_put_command(cmd->device->host, cmd, &sdev->sdev_gendev);
+	__scsi_put_command(cmd->device->host, cmd);
 }
 EXPORT_SYMBOL(scsi_put_command);
 
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 78b004d..771c16b 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -2288,6 +2288,11 @@ scsi_reset_provider(struct scsi_device *dev, int flag)
 	if (scsi_autopm_get_host(shost) < 0)
 		return FAILED;
 
+	if (!get_device(&dev->sdev_gendev)) {
+		rtn = FAILED;
+		goto out_put_autopm_host;
+	}
+
 	scmd = scsi_get_command(dev, GFP_KERNEL);
 	blk_rq_init(NULL, &req);
 	scmd->request = &req;
@@ -2345,6 +2350,7 @@ scsi_reset_provider(struct scsi_device *dev, int flag)
 	scsi_run_host_queues(shost);
 
 	scsi_next_command(scmd);
+out_put_autopm_host:
 	scsi_autopm_put_host(shost);
 	return rtn;
 }
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ad516c0..500178c 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -95,6 +95,7 @@ static void scsi_unprep_request(struct request *req)
 	req->special = NULL;
 
 	scsi_put_command(cmd);
+	put_device(&cmd->device->sdev_gendev);
 }
 
 /**
@@ -529,6 +530,7 @@ void scsi_next_command(struct scsi_cmnd *cmd)
 	get_device(&sdev->sdev_gendev);
 
 	scsi_put_command(cmd);
+	put_device(&sdev->sdev_gendev);
 	scsi_run_queue(q);
 
 	/* ok to remove device now */
@@ -1116,6 +1118,7 @@ err_exit:
 	scsi_release_buffers(cmd);
 	cmd->request->special = NULL;
 	scsi_put_command(cmd);
+	put_device(&cmd->device->sdev_gendev);
 	return error;
 }
 EXPORT_SYMBOL(scsi_init_io);
@@ -1126,9 +1129,15 @@ static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
 	struct scsi_cmnd *cmd;
 
 	if (!req->special) {
+		/* Bail if we can't get a reference to the device */
+		if (!get_device(&sdev->sdev_gendev))
+			return NULL;
+
 		cmd = scsi_get_command(sdev, GFP_ATOMIC);
-		if (unlikely(!cmd))
+		if (unlikely(!cmd)) {
+			put_device(&sdev->sdev_gendev);
 			return NULL;
+		}
 		req->special = cmd;
 	} else {
 		cmd = req->special;
@@ -1291,6 +1300,7 @@ int scsi_prep_return(struct request_queue *q, struct request *req, int ret)
 			struct scsi_cmnd *cmd = req->special;
 			scsi_release_buffers(cmd);
 			scsi_put_command(cmd);
+			put_device(&cmd->device->sdev_gendev);
 			req->special = NULL;
 		}
 		break;
diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
index 84a1fdf..e51add0 100644
--- a/drivers/scsi/scsi_tgt_lib.c
+++ b/drivers/scsi/scsi_tgt_lib.c
@@ -155,7 +155,8 @@ void scsi_host_put_command(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
 	__blk_put_request(q, rq);
 	spin_unlock_irqrestore(q->queue_lock, flags);
 
-	__scsi_put_command(shost, cmd, &shost->shost_gendev);
+	__scsi_put_command(shost, cmd);
+	put_device(&shost->shost_gendev);
 }
 EXPORT_SYMBOL_GPL(scsi_host_put_command);
 
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 91558a1..414edf9 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -142,8 +142,7 @@ static inline struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd)
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
 extern struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *, gfp_t);
 extern void scsi_put_command(struct scsi_cmnd *);
-extern void __scsi_put_command(struct Scsi_Host *, struct scsi_cmnd *,
-			       struct device *);
+extern void __scsi_put_command(struct Scsi_Host *, struct scsi_cmnd *);
 extern void scsi_finish_command(struct scsi_cmnd *cmd);
 
 extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
-- 
1.7.10.4

[PATCH 4/6] scsi: remove a useless get/put_device pair in scsi_request_fn

[Christoph Hellwig]

[-- Attachment #1: 0004-scsi-remove-a-useless-get-put_device-pair-in-scsi_re.patch --]
[-- Type: text/plain, Size: 2084 bytes --]

From: Bart Van Assche <bvanassche@acm.org>

SCSI devices may only be removed by calling scsi_remove_device().
That function must invoke blk_cleanup_queue() before the final put
of sdev->sdev_gendev. Since blk_cleanup_queue() waits for the
block queue to drain and then tears it down, scsi_request_fn cannot
be active anymore after blk_cleanup_queue() has returned and hence
the get_device()/put_device() pair in scsi_request_fn is unnecessary.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Acked-by: Tejun Heo <tj@kernel.org>
Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>
---
 drivers/scsi/scsi_lib.c |   14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 500178c..7d35678 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1558,16 +1558,14 @@ static void scsi_softirq_done(struct request *rq)
  * Lock status: IO request lock assumed to be held when called.
  */
 static void scsi_request_fn(struct request_queue *q)
+	__releases(q->queue_lock)
+	__acquires(q->queue_lock)
 {
 	struct scsi_device *sdev = q->queuedata;
 	struct Scsi_Host *shost;
 	struct scsi_cmnd *cmd;
 	struct request *req;
 
-	if(!get_device(&sdev->sdev_gendev))
-		/* We must be tearing the block queue down already */
-		return;
-
 	/*
 	 * To start with, we keep looping until the queue is empty, or until
 	 * the host is no longer able to accept any more requests.
@@ -1656,7 +1654,7 @@ static void scsi_request_fn(struct request_queue *q)
 			goto out_delay;
 	}
 
-	goto out;
+	return;
 
  not_ready:
 	spin_unlock_irq(shost->host_lock);
@@ -1675,12 +1673,6 @@ static void scsi_request_fn(struct request_queue *q)
 out_delay:
 	if (sdev->device_busy == 0)
 		blk_delay_queue(q, SCSI_QUEUE_DELAY);
-out:
-	/* must be careful here...if we trigger the ->remove() function
-	 * we cannot be holding the q lock */
-	spin_unlock_irq(q->queue_lock);
-	put_device(&sdev->sdev_gendev);
-	spin_lock_irq(q->queue_lock);
 }
 
 u64 scsi_calculate_bounce_limit(struct Scsi_Host *shost)
-- 
1.7.10.4

[PATCH 5/6] scsi: remove a useless get/put_device pair in scsi_next_command

[Christoph Hellwig]

[-- Attachment #1: 0005-scsi-remove-a-useless-get-put_device-pair-in-scsi_ne.patch --]
[-- Type: text/plain, Size: 1008 bytes --]

From: Bart Van Assche <bvanassche@acm.org>

Eliminate a get_device() / put_device() pair from scsi_next_command().
Both are atomic operations hence removing these slightly improves
performance.

[hch: slight changes due to different context]

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_lib.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7d35678..91ca414 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -526,14 +526,9 @@ void scsi_next_command(struct scsi_cmnd *cmd)
 	struct scsi_device *sdev = cmd->device;
 	struct request_queue *q = sdev->request_queue;
 
-	/* need to hold a reference on the device before we let go of the cmd */
-	get_device(&sdev->sdev_gendev);
-
 	scsi_put_command(cmd);
-	put_device(&sdev->sdev_gendev);
 	scsi_run_queue(q);
 
-	/* ok to remove device now */
 	put_device(&sdev->sdev_gendev);
 }
 
-- 
1.7.10.4

[PATCH 6/6] scsi: remove a useless get/put_device pair in scsi_requeue_command

[Christoph Hellwig]

[-- Attachment #1: 0006-scsi-remove-a-useless-get-put_device-pair-in-scsi_re.patch --]
[-- Type: text/plain, Size: 1875 bytes --]

Avoid a spurious device get/put cycle by using scsi_put_command and folding
scsi_unprep_request into scsi_requeue_command.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_lib.c |   35 +++--------------------------------
 1 file changed, 3 insertions(+), 32 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 91ca414..9350691 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -75,29 +75,6 @@ struct kmem_cache *scsi_sdb_cache;
  */
 #define SCSI_QUEUE_DELAY	3
 
-/*
- * Function:	scsi_unprep_request()
- *
- * Purpose:	Remove all preparation done for a request, including its
- *		associated scsi_cmnd, so that it can be requeued.
- *
- * Arguments:	req	- request to unprepare
- *
- * Lock status:	Assumed that no locks are held upon entry.
- *
- * Returns:	Nothing.
- */
-static void scsi_unprep_request(struct request *req)
-{
-	struct scsi_cmnd *cmd = req->special;
-
-	blk_unprep_request(req);
-	req->special = NULL;
-
-	scsi_put_command(cmd);
-	put_device(&cmd->device->sdev_gendev);
-}
-
 /**
  * __scsi_queue_insert - private queue insertion
  * @cmd: The SCSI command being requeued
@@ -503,16 +480,10 @@ static void scsi_requeue_command(struct request_queue *q, struct scsi_cmnd *cmd)
 	struct request *req = cmd->request;
 	unsigned long flags;
 
-	/*
-	 * We need to hold a reference on the device to avoid the queue being
-	 * killed after the unlock and before scsi_run_queue is invoked which
-	 * may happen because scsi_unprep_request() puts the command which
-	 * releases its reference on the device.
-	 */
-	get_device(&sdev->sdev_gendev);
-
 	spin_lock_irqsave(q->queue_lock, flags);
-	scsi_unprep_request(req);
+	blk_unprep_request(req);
+	req->special = NULL;
+	scsi_put_command(cmd);
 	blk_requeue_request(q, req);
 	spin_unlock_irqrestore(q->queue_lock, flags);
 
-- 
1.7.10.4

Re: [PATCH 1/6] scsi: avoid useless free_list lock roundtrips

[Hannes Reinecke]

On 02/06/2014 07:43 PM, Christoph Hellwig wrote:
> Avoid hitting the host-wide free_list lock unless we need to put a command
> back onto the freelist.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Looks good.

Acked-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] scsi: avoid taking host_lock in scsi_run_queue unless nessecary

[Hannes Reinecke]

On 02/06/2014 07:43 PM, Christoph Hellwig wrote:
> If we don't have starved devices we don't need to take the host lock
> to iterate over them.  Also split the function up to be more clear.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/scsi_lib.c |   43 ++++++++++++++++++++++++-------------------
>  1 file changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 7bd7f0d..ad516c0 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -385,29 +385,12 @@ static inline int scsi_host_is_busy(struct Scsi_Host *shost)
>  	return 0;
>  }
>  
> -/*
> - * Function:	scsi_run_queue()
> - *
> - * Purpose:	Select a proper request queue to serve next
> - *
> - * Arguments:	q	- last request's queue
> - *
> - * Returns:     Nothing
> - *
> - * Notes:	The previous command was completely finished, start
> - *		a new one if possible.
> - */
> -static void scsi_run_queue(struct request_queue *q)
> +static void scsi_starved_list_run(struct Scsi_Host *shost)
>  {
> -	struct scsi_device *sdev = q->queuedata;
> -	struct Scsi_Host *shost;
>  	LIST_HEAD(starved_list);
> +	struct scsi_device *sdev;
>  	unsigned long flags;
>  
> -	shost = sdev->host;
> -	if (scsi_target(sdev)->single_lun)
> -		scsi_single_lun_run(sdev);
> -
>  	spin_lock_irqsave(shost->host_lock, flags);
>  	list_splice_init(&shost->starved_list, &starved_list);
>  
> @@ -459,6 +442,28 @@ static void scsi_run_queue(struct request_queue *q)
>  	/* put any unprocessed entries back */
>  	list_splice(&starved_list, &shost->starved_list);
>  	spin_unlock_irqrestore(shost->host_lock, flags);
> +}
> +
> +/*
> + * Function:   scsi_run_queue()
> + *
> + * Purpose:    Select a proper request queue to serve next
> + *
> + * Arguments:  q       - last request's queue
> + *
> + * Returns:     Nothing
> + *
> + * Notes:      The previous command was completely finished, start
> + *             a new one if possible.
> + */
> +static void scsi_run_queue(struct request_queue *q)
> +{
> +	struct scsi_device *sdev = q->queuedata;
> +
> +	if (scsi_target(sdev)->single_lun)
> +		scsi_single_lun_run(sdev);
> +	if (!list_empty(&sdev->host->starved_list))
> +		scsi_starved_list_run(sdev->host);
>  
>  	blk_run_queue(q);
>  }
> -- 1.7.10.4 --
Hmm.

What happens when another CPU is just modifying the starved list
at this point?
We probably won't be seeing the update until when the next command
completed.
Which probably doesn't matter if the HBA has run out of resources
(which means there are plenty of other commands outstanding),
but it'll surely influence the load balancing when using several
devices, won't it?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] scsi: do not manipulate device reference counts in scsi_get/put_command

[Hannes Reinecke]

On 02/06/2014 07:43 PM, Christoph Hellwig wrote:
> Many callers won't need this and we can optimize them away.  In addition
> the handling in the __-prefixed variants was inconsistant to start with.
> 
> Based on an earlier patch from Bart Van Assche.
> 
But this decouples the scsi_device refcount from the number of
outstanding commands, right?
While I'm sure you've audited all call sites, it's now up to the
callers to provide proper reference counting.
Shouldn't we document it somewhere?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] scsi: remove a useless get/put_device pair in scsi_request_fn

[Hannes Reinecke]

On 02/06/2014 07:43 PM, Christoph Hellwig wrote:
> From: Bart Van Assche <bvanassche@acm.org>
> 
> SCSI devices may only be removed by calling scsi_remove_device().
> That function must invoke blk_cleanup_queue() before the final put
> of sdev->sdev_gendev. Since blk_cleanup_queue() waits for the
> block queue to drain and then tears it down, scsi_request_fn cannot
> be active anymore after blk_cleanup_queue() has returned and hence
> the get_device()/put_device() pair in scsi_request_fn is unnecessary.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> Acked-by: Tejun Heo <tj@kernel.org>
> Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>
Acked-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] scsi: remove a useless get/put_device pair in scsi_next_command

[Hannes Reinecke]

On 02/06/2014 07:43 PM, Christoph Hellwig wrote:
> From: Bart Van Assche <bvanassche@acm.org>
> 
> Eliminate a get_device() / put_device() pair from scsi_next_command().
> Both are atomic operations hence removing these slightly improves
> performance.
> 
> [hch: slight changes due to different context]
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Dependent on Patch 3/6, but as I just wanted some clarification there:

Acked-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] scsi: remove a useless get/put_device pair in scsi_requeue_command

[Hannes Reinecke]

On 02/06/2014 07:43 PM, Christoph Hellwig wrote:
> Avoid a spurious device get/put cycle by using scsi_put_command and folding
> scsi_unprep_request into scsi_requeue_command.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Makes one wonder why we didn't do this to start with.

Acked-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] scsi: avoid taking host_lock in scsi_run_queue unless nessecary

[Christoph Hellwig]

On Wed, Feb 12, 2014 at 12:08:35PM +0100, Hannes Reinecke wrote:
> What happens when another CPU is just modifying the starved list
> at this point?
> We probably won't be seeing the update until when the next command
> completed.

That's correct if the last was emptry previous.  list_empty won't
return true when adding an additional command.

> Which probably doesn't matter if the HBA has run out of resources
> (which means there are plenty of other commands outstanding),
> but it'll surely influence the load balancing when using several
> devices, won't it?

Only when first adding an item to the starved list.  Load balancing
isn't that important when just dealing with two commands but more
for a long lasting overload.

Re: [PATCH 3/6] scsi: do not manipulate device reference counts in scsi_get/put_command

[Christoph Hellwig]

On Wed, Feb 12, 2014 at 12:37:26PM +0100, Hannes Reinecke wrote:
> But this decouples the scsi_device refcount from the number of
> outstanding commands, right?

We still take a reference for each command in scsi_get_cmd_from_req,
which is helper used in each prep_fn, so we still have a reference
for each command.  The big simplication is that we avoid useless
roundtrips on the reference count during I/O completion by not hiding
them in unrelated functions.

> While I'm sure you've audited all call sites, it's now up to the
> callers to provide proper reference counting.
> Shouldn't we document it somewhere?

Not sure what'd we would want to document.  We grab a reference when
first getting the command from the block layer, and drop it when
finishing the command.  We could write this setence down somewhere if it
really helps people, but I'm not sure where a good place for that would
be.

Re: [PATCH 6/6] scsi: remove a useless get/put_device pair in scsi_requeue_command

[Christoph Hellwig]

On Wed, Feb 12, 2014 at 12:40:24PM +0100, Hannes Reinecke wrote:
> On 02/06/2014 07:43 PM, Christoph Hellwig wrote:
> > Avoid a spurious device get/put cycle by using scsi_put_command and folding
> > scsi_unprep_request into scsi_requeue_command.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> Makes one wonder why we didn't do this to start with.

Because we couldn't do it when the device get/put was hidden in the
scsi_get_command/scsi_put_command.