[PATCH 0/3] RFC: libsas queuecommand fixups

[PATCH 0/3] RFC: libsas queuecommand fixups

[Christoph Hellwig]

Fix up some lose ends in sas_queuecommand that I noticed during the isci
review.  The first two are pretty straight forward, but the third is a bit
more nasty and needs some serious review and testing.  Note that I could
only compile-test the bunch as I don't actually have a SAS controller
using libsas.

[PATCH 1/3] libsas: sas_queuecommand doesnt need host_lock

[Christoph Hellwig]

[-- Attachment #1: libsas-unlocked-queuecommand --]
[-- Type: text/plain, Size: 2221 bytes --]


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/scsi/libsas/sas_scsi_host.c
===================================================================
--- linux-2.6.orig/drivers/scsi/libsas/sas_scsi_host.c	2011-07-02 13:40:58.777796133 +0200
+++ linux-2.6/drivers/scsi/libsas/sas_scsi_host.c	2011-07-02 13:42:22.504461759 +0200
@@ -182,27 +182,12 @@ int sas_queue_up(struct sas_task *task)
 	return 0;
 }
 
-/**
- * sas_queuecommand -- Enqueue a command for processing
- * @parameters: See SCSI Core documentation
- *
- * Note: XXX: Remove the host unlock/lock pair when SCSI Core can
- * call us without holding an IRQ spinlock...
- */
-static int sas_queuecommand_lck(struct scsi_cmnd *cmd,
-		     void (*scsi_done)(struct scsi_cmnd *))
-	__releases(host->host_lock)
-	__acquires(dev->sata_dev.ap->lock)
-	__releases(dev->sata_dev.ap->lock)
-	__acquires(host->host_lock)
+int sas_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 {
 	int res = 0;
 	struct domain_device *dev = cmd_to_domain_dev(cmd);
-	struct Scsi_Host *host = cmd->device->host;
 	struct sas_internal *i = to_sas_internal(host->transportt);
 
-	spin_unlock_irq(host->host_lock);
-
 	{
 		struct sas_ha_struct *sas_ha = dev->port->ha;
 		struct sas_task *task;
@@ -210,7 +195,7 @@ static int sas_queuecommand_lck(struct s
 		/* If the device fell off, no sense in issuing commands */
 		if (dev->gone) {
 			cmd->result = DID_BAD_TARGET << 16;
-			scsi_done(cmd);
+			cmd->scsi_done(cmd);
 			goto out;
 		}
 
@@ -228,7 +213,6 @@ static int sas_queuecommand_lck(struct s
 		if (!task)
 			goto out;
 
-		cmd->scsi_done = scsi_done;
 		/* Queue up, Direct Mode or Task Collector Mode. */
 		if (sas_ha->lldd_max_execute_num < 2)
 			res = i->dft->lldd_execute_task(task, 1, GFP_ATOMIC);
@@ -243,18 +227,15 @@ static int sas_queuecommand_lck(struct s
 			if (res == -SAS_QUEUE_FULL) {
 				cmd->result = DID_SOFT_ERROR << 16; /* retry */
 				res = 0;
-				scsi_done(cmd);
+				cmd->scsi_done(cmd);
 			}
 			goto out;
 		}
 	}
 out:
-	spin_lock_irq(host->host_lock);
 	return res;
 }
 
-DEF_SCSI_QCMD(sas_queuecommand)
-
 static void sas_eh_finish_cmd(struct scsi_cmnd *cmd)
 {
 	struct sas_task *task = TO_SAS_TASK(cmd);

Re: [PATCH 1/3] libsas: sas_queuecommand doesnt need host_lock

[Dan Williams]

On Mon, Jul 11, 2011 at 11:49 AM, Christoph Hellwig <hch@infradead.org> wrote:
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Acked-by: Dan Williams <dan.j.williams@intel.com>

Dave can add his Tested-by, but looks like a nice bump in iops.

--
Dan

RE: [PATCH 1/3] libsas: sas_queuecommand doesnt need host_lock

[Jiang, Dave]

> -----Original Message-----
> From: dan.j.williams@gmail.com [mailto:dan.j.williams@gmail.com] On
> Behalf Of Dan Williams
> Sent: Monday, July 18, 2011 4:40 PM
> To: Christoph Hellwig
> Cc: James.Bottomley@hansenpartnership.com; linux-scsi@vger.kernel.org;
> Jiang, Dave
> Subject: Re: [PATCH 1/3] libsas: sas_queuecommand doesnt need host_lock
> 
> On Mon, Jul 11, 2011 at 11:49 AM, Christoph Hellwig <hch@infradead.org>
> wrote:
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Acked-by: Dan Williams <dan.j.williams@intel.com>
> 
> Dave can add his Tested-by, but looks like a nice bump in iops.
> 
> --
> Dan

Tested-by: Dave Jiang <dave.jiang@intel.com>

RE: [PATCH 1/3] libsas: sas_queuecommand doesnt need host_lock

[Jack Wang]

> On Mon, Jul 11, 2011 at 11:49 AM, Christoph Hellwig <hch@infradead.org>
wrote:
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Acked-by: Dan Williams <dan.j.williams@intel.com>
> 
> Dave can add his Tested-by, but looks like a nice bump in iops.
[Jack Wang] 
I test it with pm8001, using 512b read with 12 6G sas disks bumps ~460K iops
from 340K iops, but using 512b write performance drops from 340K to 240K.

Dan and Dave , could you share your number?
> 
> --
> Dan
> --
> 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 1/3] libsas: sas_queuecommand doesnt need host_lock

['Christoph Hellwig']

On Tue, Jul 19, 2011 at 04:56:22PM +0800, Jack Wang wrote:
> [Jack Wang] 
> I test it with pm8001, using 512b read with 12 6G sas disks bumps ~460K iops
> from 340K iops, but using 512b write performance drops from 340K to 240K.

Very strange.  Is this repeatable, and did you test with just patch 1,
or the whole series?

[PATCH 2/3] libsas: reindent sas_queuecommand

[Christoph Hellwig]

[-- Attachment #1: libsas-reformat-queuecommand --]
[-- Type: text/plain, Size: 2955 bytes --]

Switch sas_queuecommand to a normal indentation and goto based error handling.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/scsi/libsas/sas_scsi_host.c
===================================================================
--- linux-2.6.orig/drivers/scsi/libsas/sas_scsi_host.c	2011-07-02 13:46:15.714458862 +0200
+++ linux-2.6/drivers/scsi/libsas/sas_scsi_host.c	2011-07-02 13:46:45.267791828 +0200
@@ -184,56 +184,51 @@ int sas_queue_up(struct sas_task *task)
 
 int sas_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 {
-	int res = 0;
-	struct domain_device *dev = cmd_to_domain_dev(cmd);
 	struct sas_internal *i = to_sas_internal(host->transportt);
+	struct domain_device *dev = cmd_to_domain_dev(cmd);
+	struct sas_ha_struct *sas_ha = dev->port->ha;
+	struct sas_task *task;
+	int res = 0;
 
-	{
-		struct sas_ha_struct *sas_ha = dev->port->ha;
-		struct sas_task *task;
-
-		/* If the device fell off, no sense in issuing commands */
-		if (dev->gone) {
-			cmd->result = DID_BAD_TARGET << 16;
-			cmd->scsi_done(cmd);
-			goto out;
-		}
-
-		if (dev_is_sata(dev)) {
-			unsigned long flags;
-
-			spin_lock_irqsave(dev->sata_dev.ap->lock, flags);
-			res = ata_sas_queuecmd(cmd, dev->sata_dev.ap);
-			spin_unlock_irqrestore(dev->sata_dev.ap->lock, flags);
-			goto out;
-		}
-
-		res = -ENOMEM;
-		task = sas_create_task(cmd, dev, GFP_ATOMIC);
-		if (!task)
-			goto out;
-
-		/* Queue up, Direct Mode or Task Collector Mode. */
-		if (sas_ha->lldd_max_execute_num < 2)
-			res = i->dft->lldd_execute_task(task, 1, GFP_ATOMIC);
-		else
-			res = sas_queue_up(task);
-
-		/* Examine */
-		if (res) {
-			SAS_DPRINTK("lldd_execute_task returned: %d\n", res);
-			ASSIGN_SAS_TASK(cmd, NULL);
-			sas_free_task(task);
-			if (res == -SAS_QUEUE_FULL) {
-				cmd->result = DID_SOFT_ERROR << 16; /* retry */
-				res = 0;
-				cmd->scsi_done(cmd);
-			}
-			goto out;
-		}
+	/* If the device fell off, no sense in issuing commands */
+	if (dev->gone) {
+		cmd->result = DID_BAD_TARGET << 16;
+		goto out_done;
 	}
-out:
-	return res;
+
+	if (dev_is_sata(dev)) {
+		unsigned long flags;
+
+		spin_lock_irqsave(dev->sata_dev.ap->lock, flags);
+		res = ata_sas_queuecmd(cmd, dev->sata_dev.ap);
+		spin_unlock_irqrestore(dev->sata_dev.ap->lock, flags);
+		return res;
+	}
+
+	task = sas_create_task(cmd, dev, GFP_ATOMIC);
+	if (!task)
+		return -ENOMEM;
+
+	/* Queue up, Direct Mode or Task Collector Mode. */
+	if (sas_ha->lldd_max_execute_num < 2)
+		res = i->dft->lldd_execute_task(task, 1, GFP_ATOMIC);
+	else
+		res = sas_queue_up(task);
+
+	if (res)
+		goto out_free_task;
+	return 0;
+
+out_free_task:
+	SAS_DPRINTK("lldd_execute_task returned: %d\n", res);
+	ASSIGN_SAS_TASK(cmd, NULL);
+	sas_free_task(task);
+	if (res != -SAS_QUEUE_FULL)
+		return res;
+	cmd->result = DID_SOFT_ERROR << 16; /* retry */
+out_done:
+	cmd->scsi_done(cmd);
+	return 0;
 }
 
 static void sas_eh_finish_cmd(struct scsi_cmnd *cmd)

[PATCH 3/3] libsas: fix sas_queuecommand return values

[Christoph Hellwig]

[-- Attachment #1: libsas-fix-queuecommand-return --]
[-- Type: text/plain, Size: 1575 bytes --]

->queuecommand must return either 0, or one of the SCSI_MLQUEUE_* return
values.  Non-transient errors are indicated by setting cmd->result before
calling ->scsi_done and returning 0.  Fix libsas to adhere to this calling
convention.  Note that the DID_ERROR for returns from the low-level driver
might not be correct for all cases, but it's the best we can do with
the current layering in libsas.  I also suspect that the pre-existing
handling of -SAS_QUEUE_FULL should really be SCSI_MLQUEUE_HOST_BUSY, but
I'll leave that for a separate change.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/scsi/libsas/sas_scsi_host.c
===================================================================
--- linux-2.6.orig/drivers/scsi/libsas/sas_scsi_host.c	2011-07-11 20:25:25.529428978 +0200
+++ linux-2.6/drivers/scsi/libsas/sas_scsi_host.c	2011-07-11 20:29:22.321479498 +0200
@@ -207,7 +207,7 @@ int sas_queuecommand(struct Scsi_Host *h
 
 	task = sas_create_task(cmd, dev, GFP_ATOMIC);
 	if (!task)
-		return -ENOMEM;
+		return SCSI_MLQUEUE_HOST_BUSY;
 
 	/* Queue up, Direct Mode or Task Collector Mode. */
 	if (sas_ha->lldd_max_execute_num < 2)
@@ -223,9 +223,10 @@ out_free_task:
 	SAS_DPRINTK("lldd_execute_task returned: %d\n", res);
 	ASSIGN_SAS_TASK(cmd, NULL);
 	sas_free_task(task);
-	if (res != -SAS_QUEUE_FULL)
-		return res;
-	cmd->result = DID_SOFT_ERROR << 16; /* retry */
+	if (res = -SAS_QUEUE_FULL)
+		cmd->result = DID_SOFT_ERROR << 16; /* retry */
+	else
+		cmd->result = DID_ERROR << 16;
 out_done:
 	cmd->scsi_done(cmd);
 	return 0;

RE: [PATCH 3/3] libsas: fix sas_queuecommand return values

[Jack Wang]

> ->queuecommand must return either 0, or one of the SCSI_MLQUEUE_* return
> values.  Non-transient errors are indicated by setting cmd->result before
> calling ->scsi_done and returning 0.  Fix libsas to adhere to this calling
> convention.  Note that the DID_ERROR for returns from the low-level driver
> might not be correct for all cases, but it's the best we can do with
> the current layering in libsas.  I also suspect that the pre-existing
> handling of -SAS_QUEUE_FULL should really be SCSI_MLQUEUE_HOST_BUSY, but
> I'll leave that for a separate change.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: linux-2.6/drivers/scsi/libsas/sas_scsi_host.c
> ===================================================================
> --- linux-2.6.orig/drivers/scsi/libsas/sas_scsi_host.c	2011-07-11
> 20:25:25.529428978 +0200
> +++ linux-2.6/drivers/scsi/libsas/sas_scsi_host.c	2011-07-11
> 20:29:22.321479498 +0200
> @@ -207,7 +207,7 @@ int sas_queuecommand(struct Scsi_Host *h
> 
>  	task = sas_create_task(cmd, dev, GFP_ATOMIC);
>  	if (!task)
> -		return -ENOMEM;
> +		return SCSI_MLQUEUE_HOST_BUSY;
> 
>  	/* Queue up, Direct Mode or Task Collector Mode. */
>  	if (sas_ha->lldd_max_execute_num < 2)
> @@ -223,9 +223,10 @@ out_free_task:
>  	SAS_DPRINTK("lldd_execute_task returned: %d\n", res);
>  	ASSIGN_SAS_TASK(cmd, NULL);
>  	sas_free_task(task);
> -	if (res != -SAS_QUEUE_FULL)
> -		return res;
> -	cmd->result = DID_SOFT_ERROR << 16; /* retry */
> +	if (res = -SAS_QUEUE_FULL)
[Jack Wang] Seems you should use res == -SAS_QUEUE_FULL ? 
I will test the patchset and report result later.
> +		cmd->result = DID_SOFT_ERROR << 16; /* retry */
> +	else
> +		cmd->result = DID_ERROR << 16;
>  out_done:
>  	cmd->scsi_done(cmd);
>  	return 0;
> 

[PATCH 3/3 v2] libsas: fix sas_queuecommand return values

[Christoph Hellwig]

->queuecommand must return either 0, or one of the SCSI_MLQUEUE_* return
values.  Non-transient errors are indicated by setting cmd->result before
calling ->scsi_done and returning 0.  Fix libsas to adhere to this calling
convention.  Note that the DID_ERROR for returns from the low-level driver
might not be correct for all cases, but it's the best we can do with
the current layering in libsas.  I also suspect that the pre-existing
handling of -SAS_QUEUE_FULL should really be SCSI_MLQUEUE_HOST_BUSY, but
I'll leave that for a separate change.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/scsi/libsas/sas_scsi_host.c
===================================================================
--- linux-2.6.orig/drivers/scsi/libsas/sas_scsi_host.c	2011-07-16 11:24:36.852182194 -0700
+++ linux-2.6/drivers/scsi/libsas/sas_scsi_host.c	2011-07-16 11:24:50.022110847 -0700
@@ -207,7 +207,7 @@ int sas_queuecommand(struct Scsi_Host *h
 
 	task = sas_create_task(cmd, dev, GFP_ATOMIC);
 	if (!task)
-		return -ENOMEM;
+		return SCSI_MLQUEUE_HOST_BUSY;
 
 	/* Queue up, Direct Mode or Task Collector Mode. */
 	if (sas_ha->lldd_max_execute_num < 2)
@@ -223,9 +223,10 @@ out_free_task:
 	SAS_DPRINTK("lldd_execute_task returned: %d\n", res);
 	ASSIGN_SAS_TASK(cmd, NULL);
 	sas_free_task(task);
-	if (res != -SAS_QUEUE_FULL)
-		return res;
-	cmd->result = DID_SOFT_ERROR << 16; /* retry */
+	if (res == -SAS_QUEUE_FULL)
+		cmd->result = DID_SOFT_ERROR << 16; /* retry */
+	else
+		cmd->result = DID_ERROR << 16;
 out_done:
 	cmd->scsi_done(cmd);
 	return 0;

Re: [PATCH 0/3] RFC: libsas queuecommand fixups

[ersatz splatt]

I ran some simple tests (see the recent pm8001 e-mail for specifics)
-- fio on each of the patches applied incrementally to libsas.  This
is only a simple "smoke test" really.
Things seemed to work fine with the patches.

If someone wants some more specific testing, I'm open to running it if
you give me the details.



On Mon, Jul 11, 2011 at 11:49 AM, Christoph Hellwig <hch@infradead.org> wrote:
> Fix up some lose ends in sas_queuecommand that I noticed during the isci
> review.  The first two are pretty straight forward, but the third is a bit
> more nasty and needs some serious review and testing.  Note that I could
> only compile-test the bunch as I don't actually have a SAS controller
> using libsas.
> --
> 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
>
--
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 0/3] RFC: libsas queuecommand fixups

[Christoph Hellwig]

ping?

On Mon, Jul 11, 2011 at 02:49:22PM -0400, Christoph Hellwig wrote:
> Fix up some lose ends in sas_queuecommand that I noticed during the isci
> review.  The first two are pretty straight forward, but the third is a bit
> more nasty and needs some serious review and testing.  Note that I could
> only compile-test the bunch as I don't actually have a SAS controller
> using libsas.
> --
> 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
---end quoted text---

Re: [PATCH 0/3] RFC: libsas queuecommand fixups

[James Bottomley]

On Mon, 2011-08-22 at 10:45 -0400, Christoph Hellwig wrote:
> ping?

It seems to work for me too.  I'll put them in scsi misc and see if
anyone complains.

James

Re: [PATCH 0/3] RFC: libsas queuecommand fixups

[ersatz splatt]

Christoph, if you want anything more than the basic testing I did,
just let me know what you would like done.

David


On Tue, Aug 23, 2011 at 6:57 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Mon, 2011-08-22 at 10:45 -0400, Christoph Hellwig wrote:
>> ping?
>
> It seems to work for me too.  I'll put them in scsi misc and see if
> anyone complains.
>
> James
>
>
> --
> 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
>
--
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