Skip to contentSkip to conversation

staging: hv: Fixed lockup problem with bounce_buffer scatter list - RESEND

PATCH In review Hank Janssen 2 participants 4 messages Aug 31, 2010 – Sep 1, 2010

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 2/5 staging: hv: Fixed lockup problem with bounce_buffer scatter list - RESEND

(Send from a linux machine, not routed through exchange)

Fixed lockup problem with bounce_buffer scatter list which caused crashes in heavy loads.

---
 drivers/staging/hv/storvsc_drv.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)
drivers/staging/hv/storvsc_drv.c +6 -3
index 3b9ccb0..169d701 100644
@@ -615,6 +615,7 @@ static int storvsc_queuecommand(struct scsi_cmnd *scmnd,
 	unsigned int request_size = 0;
 	int i;
 	struct scatterlist *sgl;
+	unsigned int sg_count = 0;
 
 	DPRINT_DBG(STORVSC_DRV, "scmnd %p dir %d, use_sg %d buf %p len %d "
 		   "queue depth %d tagged %d", scmnd, scmnd->sc_data_direction,
@@ -697,6 +698,7 @@ static int storvsc_queuecommand(struct scsi_cmnd *scmnd,
 	request->DataBuffer.Length = scsi_bufflen(scmnd);
 	if (scsi_sg_count(scmnd)) {
 		sgl = (struct scatterlist *)scsi_sglist(scmnd);
+		sg_count = scsi_sg_count(scmnd);
 
 		/* check if we need to bounce the sgl */
 		if (do_bounce_buffer(sgl, scsi_sg_count(scmnd)) != -1) {
@@ -731,15 +733,16 @@ static int storvsc_queuecommand(struct scsi_cmnd *scmnd,
 					      scsi_sg_count(scmnd));
 
 			sgl = cmd_request->bounce_sgl;
+			sg_count = cmd_request->bounce_sgl_count;
 		}
 
 		request->DataBuffer.Offset = sgl[0].offset;
 
-		for (i = 0; i < scsi_sg_count(scmnd); i++) {
+		for (i = 0; i < sg_count; i++) {
 			DPRINT_DBG(STORVSC_DRV, "sgl[%d] len %d offset %d\n",
 				   i, sgl[i].length, sgl[i].offset);
-			request->DataBuffer.PfnArray[i] =
-					page_to_pfn(sg_page((&sgl[i])));
+			request->DataBuffer.PfnArray[i] = 
+				page_to_pfn(sg_page((&sgl[i])));
 		}
 	} else if (scsi_sglist(scmnd)) {
 		/* ASSERT(scsi_bufflen(scmnd) <= PAGE_SIZE); */
Signature · 1 line
1.6.0.2
#1 lore raw

drivers/staging/hv/storvsc_drv.c

Quoted 1 line · Hank Janssen
drivers/staging/hv/storvsc_drv.c +0 -0

[]

drivers/staging/hv/storvsc_drv.c static int storvsc_queuecommand(struct scsi_cmnd *scmnd, · line 745

Quoted 4 lines
drivers/staging/hv/storvsc_drv.c +2 -2
-			request->DataBuffer.PfnArray[i] =
-					page_to_pfn(sg_page((&sgl[i])));
+			request->DataBuffer.PfnArray[i] = 
+				page_to_pfn(sg_page((&sgl[i])));

I did a bit of a doubletake reading this last change.

This isn't actually a part of your fix and it introduces a whitespace error.

#2 lore raw
Quoted 11 lines · Joe Perches

drivers/staging/hv/storvsc_drv.c

Quoted 1 line · Hank Janssen
drivers/staging/hv/storvsc_drv.c +0 -0

[]

drivers/staging/hv/storvsc_drv.c static int storvsc_queuecommand(struct scsi_cmnd *scmnd, · line 745

Quoted 4 lines
drivers/staging/hv/storvsc_drv.c +2 -2
-			request->DataBuffer.PfnArray[i] =
-					page_to_pfn(sg_page((&sgl[i])));
+			request->DataBuffer.PfnArray[i] = 
+				page_to_pfn(sg_page((&sgl[i])));

I did a bit of a doubletake reading this last change.

Quoted 3 lines

This isn't actually a part of your fix and it introduces a whitespace error.

I will resubmit this patch and clean up the whitespace error.

When change the code I normally also change the indentation to the correct format for that area of the code. That is how the whitespace snuck in there.

Thanks,

Hank.

#3 lore raw
Quoted 10 lines · Hank Janssen
Quoted 9 lines · Joe Perches

drivers/staging/hv/storvsc_drv.c

Quoted 5 lines · Hank Janssen
drivers/staging/hv/storvsc_drv.c +2 -2
-			request->DataBuffer.PfnArray[i] =
-					page_to_pfn(sg_page((&sgl[i])));
+			request->DataBuffer.PfnArray[i] = 
+				page_to_pfn(sg_page((&sgl[i])));

I did a bit of a doubletake reading this last change. This isn't actually a part of your fix and it introduces a whitespace error.

Quoted 5 lines

I will resubmit this patch and clean up the whitespace error.

When change the code I normally also change the indentation to the correct format for that area of the code. That is how the whitespace snuck in there.

At least mention the whitespace change in the description or perhaps use a different coding style for readability that uses temporaries to avoid the split lines.

Something like:

 drivers/staging/hv/storvsc_drv.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)
drivers/staging/hv/storvsc_drv.c +9 -8
index 075b61b..874891b 100644
@@ -611,6 +611,8 @@ static int storvsc_queuecommand(struct scsi_cmnd *scmnd,
 	struct storvsc_driver_object *storvsc_drv_obj =
 		&storvsc_drv_ctx->drv_obj;
 	struct hv_storvsc_request *request;
+	struct hv_multipage_buffer *rmpb;
+	u64 *rmpba;
 	struct storvsc_cmd_request *cmd_request;
 	unsigned int request_size = 0;
 	int i;
@@ -693,8 +695,10 @@ static int storvsc_queuecommand(struct scsi_cmnd *scmnd,
 	request->SenseBuffer = scmnd->sense_buffer;
 	request->SenseBufferSize = SCSI_SENSE_BUFFERSIZE;
 
+	rmpb = &request->DataBuffer;
+	rmpba = rmpb->PfnArray;
 
-	request->DataBuffer.Length = scsi_bufflen(scmnd);
+	rmpb->Length = scsi_bufflen(scmnd);
 	if (scsi_sg_count(scmnd)) {
 		sgl = (struct scatterlist *)scsi_sglist(scmnd);
 
@@ -733,20 +737,17 @@ static int storvsc_queuecommand(struct scsi_cmnd *scmnd,
 			sgl = cmd_request->bounce_sgl;
 		}
 
-		request->DataBuffer.Offset = sgl[0].offset;
+		rmpb->Offset = sgl[0].offset;
 
 		for (i = 0; i < scsi_sg_count(scmnd); i++) {
 			DPRINT_DBG(STORVSC_DRV, "sgl[%d] len %d offset %d\n",
 				   i, sgl[i].length, sgl[i].offset);
-			request->DataBuffer.PfnArray[i] =
-					page_to_pfn(sg_page((&sgl[i])));
+			*rmpba++ = page_to_pfn(sg_page((&sgl[i])));
 		}
 	} else if (scsi_sglist(scmnd)) {
 		/* ASSERT(scsi_bufflen(scmnd) <= PAGE_SIZE); */
-		request->DataBuffer.Offset =
-			virt_to_phys(scsi_sglist(scmnd)) & (PAGE_SIZE-1);
-		request->DataBuffer.PfnArray[0] =
-			virt_to_phys(scsi_sglist(scmnd)) >> PAGE_SHIFT;
+		rmpb->Offset = virt_to_phys(scsi_sglist(scmnd)) & (PAGE_SIZE-1);
+		*rmpba = virt_to_phys(scsi_sglist(scmnd)) >> PAGE_SHIFT;
 	}
 
 retry_request: