[PATCH 0000/0005] Staging: hv: storvsc cleanup

[PATCH 0000/0005] Staging: hv: storvsc cleanup

[K. Y. Srinivasan]

Cleanup storvsc driver  based on review comments from James.

	1) While the existing per (virtual) HBA memory pools mechanism
	   does address the deadlock issues raised earlier since for IDE
	   devices there is a HBA per device and presently only IDE devices
	   can be drain devices, here we implement a generic per-LUN memory
	   pools mechanism.

	2) Fix a couple of bugs in the bounce buffer handling code (now that I
	   have been able to trigger these code paths!)

	3) Cleanup some unnecessary code.

With this, I believe I have addressed all of the review comments I have received
on this driver. I will submit a separate patch for moving the driver out of staging
(that includes these patches).


Regards,

K. Y

[PATCH 1/5] Staging: hv: storvsc: Disable clustering

[K. Y. Srinivasan]

Disable clustering, since the host side on Hyper-V requires that
each I/O element not exceed the page size. As part of this
cleanup, get rid of the function to merge bvecs, as the primary
reason for this function was to avoid having an element exceed
the page size. 

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/staging/hv/storvsc_drv.c |   18 +-----------------
 1 files changed, 1 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/hv/storvsc_drv.c b/drivers/staging/hv/storvsc_drv.c
index 0245143..9153f98 100644
--- a/drivers/staging/hv/storvsc_drv.c
+++ b/drivers/staging/hv/storvsc_drv.c
@@ -798,13 +798,6 @@ static int storvsc_device_alloc(struct scsi_device *sdevice)
 	return 0;
 }
 
-static int storvsc_merge_bvec(struct request_queue *q,
-			      struct bvec_merge_data *bmd, struct bio_vec *bvec)
-{
-	/* checking done by caller. */
-	return bvec->bv_len;
-}
-
 static int storvsc_device_configure(struct scsi_device *sdevice)
 {
 	scsi_adjust_queue_depth(sdevice, MSG_SIMPLE_TAG,
@@ -812,8 +805,6 @@ static int storvsc_device_configure(struct scsi_device *sdevice)
 
 	blk_queue_max_segment_size(sdevice->request_queue, PAGE_SIZE);
 
-	blk_queue_merge_bvec(sdevice->request_queue, storvsc_merge_bvec);
-
 	blk_queue_bounce_limit(sdevice->request_queue, BLK_BOUNCE_ANY);
 
 	return 0;
@@ -1375,14 +1366,7 @@ static struct scsi_host_template scsi_driver = {
 	/* no use setting to 0 since ll_blk_rw reset it to 1 */
 	/* currently 32 */
 	.sg_tablesize =		MAX_MULTIPAGE_BUFFER_COUNT,
-	/*
-	 * ENABLE_CLUSTERING allows mutiple physically contig bio_vecs to merge
-	 * into 1 sg element. If set, we must limit the max_segment_size to
-	 * PAGE_SIZE, otherwise we may get 1 sg element that represents
-	 * multiple
-	 */
-	/* physically contig pfns (ie sg[x].length > PAGE_SIZE). */
-	.use_clustering =	ENABLE_CLUSTERING,
+	.use_clustering =	DISABLE_CLUSTERING,
 	/* Make sure we dont get a sg segment crosses a page boundary */
 	.dma_boundary =		PAGE_SIZE-1,
 };
-- 
1.7.4.1

[PATCH 2/5] Staging: hv: storvsc: Cleanup storvsc_device_alloc()

[K. Y. Srinivasan]

The code in storvsc_device_alloc() is not needed as this would be
done by default. Get rid of it. We still keep the function as we use
this hook to allocate per-LUN memory pools in a later patch.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/staging/hv/storvsc_drv.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/hv/storvsc_drv.c b/drivers/staging/hv/storvsc_drv.c
index 9153f98..14ecb69 100644
--- a/drivers/staging/hv/storvsc_drv.c
+++ b/drivers/staging/hv/storvsc_drv.c
@@ -790,11 +790,6 @@ static void storvsc_get_ide_info(struct hv_device *dev, int *target, int *path)
 
 static int storvsc_device_alloc(struct scsi_device *sdevice)
 {
-	/*
-	 * This enables luns to be located sparsely. Otherwise, we may not
-	 * discovered them.
-	 */
-	sdevice->sdev_bflags |= BLIST_SPARSELUN | BLIST_LARGELUN;
 	return 0;
 }
 
-- 
1.7.4.1

[PATCH 3/5] Staging: hv: storvsc: Fix a bug in storvsc_command_completion()

[K. Y. Srinivasan]

Fix a bug in storvsc_command_completion() that leaks memory when scatter/gather
lists are used on the "write" side.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/staging/hv/storvsc_drv.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/hv/storvsc_drv.c b/drivers/staging/hv/storvsc_drv.c
index 14ecb69..8dafe52 100644
--- a/drivers/staging/hv/storvsc_drv.c
+++ b/drivers/staging/hv/storvsc_drv.c
@@ -1123,13 +1123,12 @@ static void storvsc_command_completion(struct hv_storvsc_request *request)
 
 	vm_srb = &request->vstor_packet.vm_srb;
 	if (cmd_request->bounce_sgl_count) {
-		if (vm_srb->data_in == READ_TYPE) {
+		if (vm_srb->data_in == READ_TYPE)
 			copy_from_bounce_buffer(scsi_sglist(scmnd),
 					cmd_request->bounce_sgl,
 					scsi_sg_count(scmnd));
-			destroy_bounce_buffer(cmd_request->bounce_sgl,
+		destroy_bounce_buffer(cmd_request->bounce_sgl,
 					cmd_request->bounce_sgl_count);
-		}
 	}
 
 	/*
-- 
1.7.4.1

[PATCH 4/5] Staging: hv: storvsc: Fix a bug in copy_from_bounce_buffer()

[K. Y. Srinivasan]

Fix a bug in copy_from_bounce_buffer().

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/staging/hv/storvsc_drv.c |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/hv/storvsc_drv.c b/drivers/staging/hv/storvsc_drv.c
index 8dafe52..c22de06 100644
--- a/drivers/staging/hv/storvsc_drv.c
+++ b/drivers/staging/hv/storvsc_drv.c
@@ -880,7 +880,8 @@ cleanup:
 /* Assume the original sgl has enough room */
 static unsigned int copy_from_bounce_buffer(struct scatterlist *orig_sgl,
 					    struct scatterlist *bounce_sgl,
-					    unsigned int orig_sgl_count)
+					    unsigned int orig_sgl_count,
+					    unsigned int bounce_sgl_count)
 {
 	int i;
 	int j = 0;
@@ -921,6 +922,24 @@ static unsigned int copy_from_bounce_buffer(struct scatterlist *orig_sgl,
 				kunmap_atomic((void *)bounce_addr, KM_IRQ0);
 				j++;
 
+				/*
+				 * It is possible that the number of elements
+				 * in the bounce buffer may not be equal to
+				 * the number of elements in the original
+				 * scatter list. Handle this correctly.
+				 */
+
+				if (j == bounce_sgl_count) {
+					/*
+					 * We are done; cleanup and return.
+					 */
+					kunmap_atomic((void *)(dest_addr -
+							orig_sgl[i].offset),
+							KM_IRQ0);
+					local_irq_restore(flags);
+					return total_copied;
+				}
+
 				/* if we need to use another bounce buffer */
 				if (destlen || i != orig_sgl_count - 1)
 					bounce_addr =
@@ -1126,7 +1145,8 @@ static void storvsc_command_completion(struct hv_storvsc_request *request)
 		if (vm_srb->data_in == READ_TYPE)
 			copy_from_bounce_buffer(scsi_sglist(scmnd),
 					cmd_request->bounce_sgl,
-					scsi_sg_count(scmnd));
+					scsi_sg_count(scmnd),
+					cmd_request->bounce_sgl_count);
 		destroy_bounce_buffer(cmd_request->bounce_sgl,
 					cmd_request->bounce_sgl_count);
 	}
-- 
1.7.4.1

Re: [PATCH 4/5] Staging: hv: storvsc: Fix a bug in copy_from_bounce_buffer()

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 322 bytes --]

On Thu, Dec 01, 2011 at 04:59:19AM -0800, K. Y. Srinivasan wrote:
> Fix a bug in copy_from_bounce_buffer().
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>

Seriously? ...

This is a crap changelog.  What did the bug do?

regards,
dan carpenter

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH 4/5] Staging: hv: storvsc: Fix a bug in copy_from_bounce_buffer()

[KY Srinivasan]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Thursday, December 01, 2011 4:03 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> James.Bottomley@HansenPartnership.com; hch@infradead.org; linux-
> scsi@vger.kernel.org; Haiyang Zhang
> Subject: Re: [PATCH 4/5] Staging: hv: storvsc: Fix a bug in
> copy_from_bounce_buffer()
> 
> On Thu, Dec 01, 2011 at 04:59:19AM -0800, K. Y. Srinivasan wrote:
> > Fix a bug in copy_from_bounce_buffer().
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> 
> Seriously? ...
> 
> This is a crap changelog.  What did the bug do?

Sorry Dan. I put in some comments in the code describing the bug. Perhaps I 
should have copied that in the changelog as well.

Regards,

K. Y

Re: [PATCH 4/5] Staging: hv: storvsc: Fix a bug in copy_from_bounce_buffer()

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 315 bytes --]

On Thu, Dec 01, 2011 at 09:07:46PM +0000, KY Srinivasan wrote:
> Sorry Dan. I put in some comments in the code describing the bug. Perhaps I 
> should have copied that in the changelog as well.

Yes.  That's the entire point of the changelog, in fact.  Could you
fix it and resend?

regards,
dan carpenter

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH 4/5] Staging: hv: storvsc: Fix a bug in copy_from_bounce_buffer()

[KY Srinivasan]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Thursday, December 01, 2011 4:12 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> James.Bottomley@HansenPartnership.com; hch@infradead.org; linux-
> scsi@vger.kernel.org; Haiyang Zhang
> Subject: Re: [PATCH 4/5] Staging: hv: storvsc: Fix a bug in
> copy_from_bounce_buffer()
> 
> On Thu, Dec 01, 2011 at 09:07:46PM +0000, KY Srinivasan wrote:
> > Sorry Dan. I put in some comments in the code describing the bug. Perhaps I
> > should have copied that in the changelog as well.
> 
> Yes.  That's the entire point of the changelog, in fact.  Could you
> fix it and resend?

I could do that, but Greg has already checked in this patch. 

Regards,

K. Y

[PATCH 5/5] Staging: hv: storvsc: Implement per device memory pools

[K. Y. Srinivasan]

The current code implemented a per-HBA memory pool mechanism. For IDE disks
managed by this driver, there is a one to one correspondance between the
block device and the associated virtual HBA and since currently only IDE devices
can be the boot device, this addressed the deadlock issues that were raised during
the review process. This patch implements a per-lun memory pool mechanism.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/staging/hv/storvsc_drv.c |  106 ++++++++++++++++++++++----------------
 1 files changed, 62 insertions(+), 44 deletions(-)

diff --git a/drivers/staging/hv/storvsc_drv.c b/drivers/staging/hv/storvsc_drv.c
index c22de06..18f8771 100644
--- a/drivers/staging/hv/storvsc_drv.c
+++ b/drivers/staging/hv/storvsc_drv.c
@@ -285,10 +285,13 @@ struct storvsc_device {
 	struct hv_storvsc_request reset_request;
 };
 
-struct hv_host_device {
-	struct hv_device *dev;
+struct stor_mem_pools {
 	struct kmem_cache *request_pool;
 	mempool_t *request_mempool;
+};
+
+struct hv_host_device {
+	struct hv_device *dev;
 	unsigned int port;
 	unsigned char path;
 	unsigned char target;
@@ -790,7 +793,48 @@ static void storvsc_get_ide_info(struct hv_device *dev, int *target, int *path)
 
 static int storvsc_device_alloc(struct scsi_device *sdevice)
 {
+	struct stor_mem_pools *memp;
+	int number = STORVSC_MIN_BUF_NR;
+
+	memp = kzalloc(sizeof(struct stor_mem_pools), GFP_KERNEL);
+	if (!memp)
+		return -ENOMEM;
+
+	memp->request_pool =
+		kmem_cache_create(dev_name(&sdevice->sdev_dev),
+				sizeof(struct storvsc_cmd_request), 0,
+				SLAB_HWCACHE_ALIGN, NULL);
+
+	if (!memp->request_pool)
+		goto err0;
+
+	memp->request_mempool = mempool_create(number, mempool_alloc_slab,
+						mempool_free_slab,
+						memp->request_pool);
+
+	if (!memp->request_mempool)
+		goto err1;
+
+	sdevice->hostdata = memp;
+
 	return 0;
+
+err1:
+	kmem_cache_destroy(memp->request_pool);
+
+err0:
+	kfree(memp);
+	return -ENOMEM;
+}
+
+static void storvsc_device_destroy(struct scsi_device *sdevice)
+{
+	struct stor_mem_pools *memp = sdevice->hostdata;
+
+	mempool_destroy(memp->request_mempool);
+	kmem_cache_destroy(memp->request_pool);
+	kfree(memp);
+	sdevice->hostdata = NULL;
 }
 
 static int storvsc_device_configure(struct scsi_device *sdevice)
@@ -1031,19 +1075,13 @@ static int storvsc_remove(struct hv_device *dev)
 {
 	struct storvsc_device *stor_device = hv_get_drvdata(dev);
 	struct Scsi_Host *host = stor_device->host;
-	struct hv_host_device *host_dev = shost_priv(host);
 
 	scsi_remove_host(host);
 
 	scsi_host_put(host);
 
 	storvsc_dev_remove(dev);
-	if (host_dev->request_pool) {
-		mempool_destroy(host_dev->request_mempool);
-		kmem_cache_destroy(host_dev->request_pool);
-		host_dev->request_pool = NULL;
-		host_dev->request_mempool = NULL;
-	}
+
 	return 0;
 }
 
@@ -1139,6 +1177,7 @@ static void storvsc_command_completion(struct hv_storvsc_request *request)
 	struct scsi_sense_hdr sense_hdr;
 	struct vmscsi_request *vm_srb;
 	struct storvsc_scan_work *wrk;
+	struct stor_mem_pools *memp = scmnd->device->hostdata;
 
 	vm_srb = &request->vstor_packet.vm_srb;
 	if (cmd_request->bounce_sgl_count) {
@@ -1201,7 +1240,7 @@ static void storvsc_command_completion(struct hv_storvsc_request *request)
 
 	scsi_done_fn(scmnd);
 
-	mempool_free(cmd_request, host_dev->request_mempool);
+	mempool_free(cmd_request, memp->request_mempool);
 }
 
 static bool storvsc_check_scsi_cmd(struct scsi_cmnd *scmnd)
@@ -1236,6 +1275,7 @@ static int storvsc_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *scmnd)
 	struct scatterlist *sgl;
 	unsigned int sg_count = 0;
 	struct vmscsi_request *vm_srb;
+	struct stor_mem_pools *memp = scmnd->device->hostdata;
 
 	if (storvsc_check_scsi_cmd(scmnd) == false) {
 		scmnd->scsi_done(scmnd);
@@ -1253,7 +1293,7 @@ static int storvsc_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *scmnd)
 
 	request_size = sizeof(struct storvsc_cmd_request);
 
-	cmd_request = mempool_alloc(host_dev->request_mempool,
+	cmd_request = mempool_alloc(memp->request_mempool,
 				       GFP_ATOMIC);
 	if (!cmd_request)
 		return SCSI_MLQUEUE_DEVICE_BUSY;
@@ -1312,7 +1352,7 @@ static int storvsc_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *scmnd)
 			if (!cmd_request->bounce_sgl) {
 				scmnd->host_scribble = NULL;
 				mempool_free(cmd_request,
-						host_dev->request_mempool);
+						memp->request_mempool);
 
 				return SCSI_MLQUEUE_HOST_BUSY;
 			}
@@ -1354,7 +1394,7 @@ retry_request:
 			destroy_bounce_buffer(cmd_request->bounce_sgl,
 					cmd_request->bounce_sgl_count);
 
-		mempool_free(cmd_request, host_dev->request_mempool);
+		mempool_free(cmd_request, memp->request_mempool);
 
 		scmnd->host_scribble = NULL;
 
@@ -1372,6 +1412,7 @@ static struct scsi_host_template scsi_driver = {
 	.queuecommand =		storvsc_queuecommand,
 	.eh_host_reset_handler =	storvsc_host_reset_handler,
 	.slave_alloc =		storvsc_device_alloc,
+	.slave_destroy =	storvsc_device_destroy,
 	.slave_configure =	storvsc_device_configure,
 	.cmd_per_lun =		1,
 	/* 64 max_queue * 1 target */
@@ -1413,7 +1454,6 @@ static int storvsc_probe(struct hv_device *device,
 			const struct hv_vmbus_device_id *dev_id)
 {
 	int ret;
-	int number = STORVSC_MIN_BUF_NR;
 	struct Scsi_Host *host;
 	struct hv_host_device *host_dev;
 	bool dev_is_ide = ((dev_id->driver_data == IDE_GUID) ? true : false);
@@ -1432,29 +1472,11 @@ static int storvsc_probe(struct hv_device *device,
 	host_dev->port = host->host_no;
 	host_dev->dev = device;
 
-	host_dev->request_pool =
-				kmem_cache_create(dev_name(&device->device),
-					sizeof(struct storvsc_cmd_request), 0,
-					SLAB_HWCACHE_ALIGN, NULL);
-
-	if (!host_dev->request_pool) {
-		scsi_host_put(host);
-		return -ENOMEM;
-	}
-
-	host_dev->request_mempool = mempool_create(number, mempool_alloc_slab,
-						mempool_free_slab,
-						host_dev->request_pool);
-
-	if (!host_dev->request_mempool) {
-		ret = -ENOMEM;
-		goto err_out0;
-	}
 
 	stor_device = kzalloc(sizeof(struct storvsc_device), GFP_KERNEL);
 	if (!stor_device) {
 		ret = -ENOMEM;
-		goto err_out1;
+		goto err_out0;
 	}
 
 	stor_device->destroy = false;
@@ -1466,7 +1488,7 @@ static int storvsc_probe(struct hv_device *device,
 	stor_device->port_number = host->host_no;
 	ret = storvsc_connect_to_vsp(device, storvsc_ringbuffer_size);
 	if (ret)
-		goto err_out2;
+		goto err_out1;
 
 	if (dev_is_ide)
 		storvsc_get_ide_info(device, &target, &path);
@@ -1486,7 +1508,7 @@ static int storvsc_probe(struct hv_device *device,
 	/* Register the HBA and start the scsi bus scan */
 	ret = scsi_add_host(host, &device->device);
 	if (ret != 0)
-		goto err_out3;
+		goto err_out2;
 
 	if (!dev_is_ide) {
 		scsi_scan_host(host);
@@ -1495,28 +1517,24 @@ static int storvsc_probe(struct hv_device *device,
 	ret = scsi_add_device(host, 0, target, 0);
 	if (ret) {
 		scsi_remove_host(host);
-		goto err_out3;
+		goto err_out2;
 	}
 	return 0;
 
-err_out3:
+err_out2:
 	/*
 	 * Once we have connected with the host, we would need to
 	 * to invoke storvsc_dev_remove() to rollback this state and
 	 * this call also frees up the stor_device; hence the jump around
-	 * err_out2 label.
+	 * err_out1 label.
 	 */
 	storvsc_dev_remove(device);
-	goto err_out1;
-
-err_out2:
-	kfree(stor_device);
+	goto err_out0;
 
 err_out1:
-	mempool_destroy(host_dev->request_mempool);
+	kfree(stor_device);
 
 err_out0:
-	kmem_cache_destroy(host_dev->request_pool);
 	scsi_host_put(host);
 	return ret;
 }
-- 
1.7.4.1