SCSI regression in 4.11

Something in SCSI in 4.11 broke booting on Hyper-V Generation 2 VM with 8 VCPU and 4G of memory. Both Linus's current tree (4.11 pre-rc1) and linux-next fail in a similar manner. It looks like some error in SCSI device detection because there is only a single device.

The offending commit causing the regression is:

$ git bisect bad
e9c787e65c0c36529745be47d490d998b4b6e589 is the first bad commit
commit e9c787e65c0c36529745be47d490d998b4b6e589
Author: Christoph Hellwig <[email protected]>
Date:   Mon Jan 2 21:55:26 2017 +0300

scsi: allocate scsi_cmnd structures as part of struct request

Rely on the new block layer functionality to allocate additional driver specific data behind struct request instead of implementing it in SCSI itѕelf.

Signed-off-by Christoph Hellwig <[email protected]>

Acked-by Martin K. Petersen <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Signed-off-by Jens Axboe <[email protected]>

:040000 040000 6ff016fcdae227efeb19c1c301b17ccd7ea35da6 70d79f99d9b79ecf4dccbe067fc697219f5c78da M drivers
:040000 040000 a672ff52df8b2c211b3f98cae4a88d8a96ccde0b 1aaaed7de0994f597c7f8290c722a0b4a7789429 M include

I checked and tree is current and up to date and includes commit ee5242360424b9b967454e9183767323d10cf985

Author: Christoph Hellwig <[email protected]>
Date:   Tue Feb 21 10:04:55 2017 +0100

scsi: zero per-cmd driver data before each I/O

Kernel config is attached. It started with Ubuntu config, but then did localmodconfig and pruned out unnecessary stuff.

Quoted 28 lines · On Mon, 27 Feb 2017 15:30:30 -0800 Stephen Hemminger…

Something in SCSI in 4.11 broke booting on Hyper-V Generation 2 VM with 8 VCPU and 4G of memory. Both Linus's current tree (4.11 pre-rc1) and linux-next fail in a similar manner. It looks like some error in SCSI device detection because there is only a single device.

The offending commit causing the regression is:

$ git bisect bad
e9c787e65c0c36529745be47d490d998b4b6e589 is the first bad commit
commit e9c787e65c0c36529745be47d490d998b4b6e589
Author: Christoph Hellwig <[email protected]>
Date:   Mon Jan 2 21:55:26 2017 +0300

scsi: allocate scsi_cmnd structures as part of struct request

Rely on the new block layer functionality to allocate additional driver specific data behind struct request instead of implementing it in SCSI itѕelf.

Signed-off-by Christoph Hellwig <[email protected]>

Acked-by Martin K. Petersen <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Signed-off-by Jens Axboe <[email protected]>

:040000 040000 6ff016fcdae227efeb19c1c301b17ccd7ea35da6 70d79f99d9b79ecf4dccbe067fc697219f5c78da M drivers
:040000 040000 a672ff52df8b2c211b3f98cae4a88d8a96ccde0b 1aaaed7de0994f597c7f8290c722a0b4a7789429 M include

I checked and tree is current and up to date and includes commit ee5242360424b9b967454e9183767323d10cf985

Quoted 8 lines

Author: Christoph Hellwig <[email protected]>
Date:   Tue Feb 21 10:04:55 2017 +0100

scsi: zero per-cmd driver data before each I/O

Kernel config is attached. It started with Ubuntu config, but then did localmodconfig and pruned out unnecessary stuff.

This problem I am seeing looks like the one addressed by:

Fixes ee5242360424 ("scsi: zero per-cmd driver data before each I/O")

but that is already in linux-next.

Noticed another place where memset(of the data was being done not the extra bits. Tried this, but didn't fix it either...

Fixes ee5242360424 ("scsi: zero per-cmd driver data before each I/O")

but that is already in linux-next.

Noticed another place where memset(of the data was being done not the extra bits. Tried this, but didn't fix it either...

Quoted 6 lines · Stephen Hemminger

Fixes ee5242360424 ("scsi: zero per-cmd driver data before each I/O")

but that is already in linux-next.

Noticed another place where memset(of the data was being done not the extra bits. Tried this, but didn't fix it either...

Are you using blk-mq or the legacy request code?

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Stephen Hemminger

Fixes ee5242360424 ("scsi: zero per-cmd driver data before each

I/O")

but that is already in linux-next.

Noticed another place where memset(of the data was being done not the extra bits.
Tried this, but didn't fix it either...

Quoted 1 line

Are you using blk-mq or the legacy request code?

Stephen doesn't have MQ set in the config he posted, I'm assuming he didn't boot with scsi_mod.use_blk_mq=true. In a previous email, I asked if turning on MQ makes a difference.

Quoted 11 lines · Jens Axboe

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Stephen Hemminger

Fixes ee5242360424 ("scsi: zero per-cmd driver data before each

I/O")

but that is already in linux-next.

Noticed another place where memset(of the data was being done not the extra bits.
Tried this, but didn't fix it either...

Quoted 5 lines

Quoted 1 line

Are you using blk-mq or the legacy request code?

Stephen doesn't have MQ set in the config he posted, I'm assuming he didn't boot with scsi_mod.use_blk_mq=true. In a previous email, I asked if turning on MQ makes a difference.

OK, since we're not making much progress, Stephen, could you insert some debugging into the storvsc driver? The trace clearly shows we're getting zeros back in the buffer when we should have data from the initial scan. Firstly, does the vmbus think it's transferring any data for the INQUIRY and READ_CAPACITY commands (looks like storvsc_command_completion() data_transfer_length)? If it does, there's probably an issue initialising the sg list. If it doesn't, we're probably sending bogus commands.

James

Quoted 27 lines · On Tue, 28 Feb 2017 09:06:13 -0800 James Bottomley <[email protected]>…

Quoted 11 lines · Jens Axboe

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Stephen Hemminger

Fixes ee5242360424 ("scsi: zero per-cmd driver data before each

I/O")

but that is already in linux-next.

Noticed another place where memset(of the data was being done not the extra bits.
Tried this, but didn't fix it either...

Quoted 5 lines

Quoted 1 line

Are you using blk-mq or the legacy request code?

Stephen doesn't have MQ set in the config he posted, I'm assuming he didn't boot with scsi_mod.use_blk_mq=true. In a previous email, I asked if turning on MQ makes a difference.

OK, since we're not making much progress, Stephen, could you insert some debugging into the storvsc driver? The trace clearly shows we're getting zeros back in the buffer when we should have data from the initial scan. Firstly, does the vmbus think it's transferring any data for the INQUIRY and READ_CAPACITY commands (looks like storvsc_command_completion() data_transfer_length)? If it does, there's probably an issue initialising the sg list. If it doesn't, we're probably sending bogus commands.

Quoted 1 line

James

I tried booting with scsi_mod.use_blk_mq=true and that did nothing. Rebuilding now with
CONFIG_SCSI_MQ_DEFAULT=y

Sure, can instrument. after that test.

Quoted 11 lines · Jens Axboe

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Stephen Hemminger

Fixes ee5242360424 ("scsi: zero per-cmd driver data before each

I/O")

but that is already in linux-next.

Noticed another place where memset(of the data was being done not the extra bits.
Tried this, but didn't fix it either...

Quoted 5 lines

Quoted 1 line

Are you using blk-mq or the legacy request code?

Stephen doesn't have MQ set in the config he posted, I'm assuming he didn't boot with scsi_mod.use_blk_mq=true. In a previous email, I asked if turning on MQ makes a difference.

OK, since we're not making much progress, Stephen, could you insert some debugging into the storvsc driver? The trace clearly shows we're getting zeros back in the buffer when we should have data from the initial scan. Firstly, does the vmbus think it's transferring any data for the INQUIRY and READ_CAPACITY commands (looks like storvsc_command_completion() data_transfer_length)? If it does, there's probably an issue initialising the sg list. If it doesn't, we're probably sending bogus commands.

James

From 216c0364bd42f2f5c57623dbfa8f20f28b4bb3b7 Mon Sep 17 00:00:00 2001

[    1.652279] hv_storvsc:  IO cmd 0x12 0x0 0x0 scsi status 0x0 srb
status 0x20 length 36
[    1.652297] scsi host1: scsi scan: INQUIRY result too short (5),

using 36

Quoted 11 lines · Jens Axboe

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Stephen Hemminger

Fixes ee5242360424 ("scsi: zero per-cmd driver data before each

I/O")

but that is already in linux-next.

Noticed another place where memset(of the data was being done not the extra bits.
Tried this, but didn't fix it either...

Quoted 5 lines

Quoted 1 line

Are you using blk-mq or the legacy request code?

Stephen doesn't have MQ set in the config he posted, I'm assuming he didn't boot with scsi_mod.use_blk_mq=true. In a previous email, I asked if turning on MQ makes a difference.

OK, since we're not making much progress, Stephen, could you insert some debugging into the storvsc driver? The trace clearly shows we're getting zeros back in the buffer when we should have data from the initial scan. Firstly, does the vmbus think it's transferring any data for the INQUIRY and READ_CAPACITY commands (looks like storvsc_command_completion() data_transfer_length)? If it does, there's probably an issue initialising the sg list. If it doesn't, we're probably sending bogus commands.

James

Quoted 33 lines · On Tue, 28 Feb 2017 09:06:13 -0800 James Bottomley <[email protected]>…

Quoted 14 lines · Jens Axboe

Quoted 13 lines · Christoph Hellwig

Quoted 10 lines · Stephen Hemminger

Fixes ee5242360424 ("scsi: zero per-cmd driver data before

each
I/O")

but that is already in linux-next.

Noticed another place where memset(of the data was being done not
the extra bits.
Tried this, but didn't fix it either...

Quoted 6 lines

Quoted 1 line

Are you using blk-mq or the legacy request code?

Stephen doesn't have MQ set in the config he posted, I'm assuming he
didn't boot with scsi_mod.use_blk_mq=true. In a previous email, I asked if turning on MQ makes a difference.

OK, since we're not making much progress, Stephen, could you insert some debugging into the storvsc driver? The trace clearly shows we're
getting zeros back in the buffer when we should have data from the initial scan. Firstly, does the vmbus think it's transferring any data
for the INQUIRY and READ_CAPACITY commands (looks like storvsc_command_completion() data_transfer_length)? If it does, there's probably an issue initialising the sg list. If it doesn't, we're probably sending bogus commands.

Quoted 2 lines

James

The following code in storvsc looks suspicious

static void storvsc_on_io_completion(struct storvsc_device *stor_device,
struct vstor_packet *vstor_packet, struct storvsc_cmd_request *request)

{
	struct vstor_packet *stor_pkt;
	struct hv_device *device = stor_device->device;

	stor_pkt = &request->vstor_packet;

	/*
	 * The current SCSI handling on the host side does
	 * not correctly handle:
	 * INQUIRY command with page code parameter set to 0x80
	 * MODE_SENSE command with cmd[2] == 0x1c
	 *
	 * Setup srb and scsi status so this won't be fatal.
	 * We do this so we can distinguish truly fatal failues
	 * (srb status == 0x4) and off-line the device in that case.
	 */

	if ((stor_pkt->vm_srb.cdb[0] == INQUIRY) ||
	   (stor_pkt->vm_srb.cdb[0] == MODE_SENSE)) {
		vstor_packet->vm_srb.scsi_status = 0;
		vstor_packet->vm_srb.srb_status = SRB_STATUS_SUCCESS;
	}

If SCSI layer is sending inquiry about devices to do scanning then wouldn't this workaround break things? Maybe a better to fully test for the broken command.

Let's concentrate on INQUIRY since that's the first command in the probe sequence. I think it's completing successfully because your hyperv layer says it has 36 bytes of transfer and that's the size of a successful initial INQUIRY, so the fact that the code above would break stuff if the INQUIRY failed is orthogonal to the the current problem.

can you print out some of the DMA buffer in storvsc_on_io_completion()?

I think just the stor_pkt->vm_srb.cdb[0] (to identify the command completing) and byte 5 of the buffer will tell us what we need to know.
It's going to be complex to get byte 5, you'll need to do a
kmap_atomic_pfn on request->payload->range.pfn_array[0] and then look at byte 5. If that's zero it means there's some problem with hyperv writing to the pfn if it's 0x24 (expected value for an initial inquiry) we've got a problem somewhere in bio completion not copying the value back.

[    1.346023] hv_storvsc:  IO cmd 0x12 0x0 0x0 scsi status 0x0 srb
status 0x20 length 36
[    1.352913] inquiry data: 00000000: 00 aa be f1 5c 98 ff ff f0 64
02 89 ff ff ff ff
[    1.356543] inquiry data: 00000010: 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00
[    1.359996] inquiry data: 00000020: 00 00 00 00
[    1.361835] scsi host1: scsi scan: INQUIRY result too short (5),
using 36
[    1.361888] hv_storvsc:  IO cmd 0xa0 0x0 0x0 scsi status 0x0 srb
status 0x1 length 16
[    1.362307] hv_storvsc:  IO cmd 0x12 0x0 0x0 scsi status 0x0 srb

status 0x1 length 36

[    1.362308] inquiry data: 00000000: 00 23 34 f1 5c 98 ff ff f0 64
02 89 ff ff ff ff
[    1.362309] inquiry data: 00000010: 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00
[    1.362309] inquiry data: 00000020: 00 00 00 00
[    1.377423] scsi 1:0:0:1: Direct-Access                           
         PQ: 0 ANSI: 0

Quoted 12 lines · Stephen Hemminger

[    1.346023] hv_storvsc:  IO cmd 0x12 0x0 0x0 scsi status 0x0 srb
status 0x20 length 36
[    1.352913] inquiry data: 00000000: 00 aa be f1 5c 98 ff ff f0 64
02 89 ff ff ff ff
[    1.356543] inquiry data: 00000010: 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00
[    1.359996] inquiry data: 00000020: 00 00 00 00
[    1.361835] scsi host1: scsi scan: INQUIRY result too short (5),
using 36
[    1.361888] hv_storvsc:  IO cmd 0xa0 0x0 0x0 scsi status 0x0 srb
status 0x1 length 16
[    1.362307] hv_storvsc:  IO cmd 0x12 0x0 0x0 scsi status 0x0 srb

Quoted 8 lines

status 0x1 length 36

[    1.362308] inquiry data: 00000000: 00 23 34 f1 5c 98 ff ff f0 64
02 89 ff ff ff ff
[    1.362309] inquiry data: 00000010: 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00
[    1.362309] inquiry data: 00000020: 00 00 00 00
[    1.377423] scsi 1:0:0:1: Direct-Access                           
         PQ: 0 ANSI: 0  

Well, this pinpoints the fault to the block uncopy, I think. The Inquiry data is clearly correct in the page frame, so it's not getting copied to the scsi_execute() buffer for some reason.

James

Let me know, I can run another test and dump more data.

Quoted 1 line · Stephen Hemminger

Let me know, I can run another test and dump more data.

Could it be that we keep the old sense buffer values around because my commit change the way how sense buffers are handled. A while ago I suggested this patch to fix it, so maybe you can try it:

	http://git.infradead.org/users/hch/block.git/commitdiff/148cff67b401e2229c076c0ea418712654be77e4

Quoted 1 line

	http://git.infradead.org/users/hch/block.git/commitdiff/148cff67b401e2229c076c0ea418712654be77e4

It appears that is already in the code I am testing in linux-next...

Quoted 3 lines · Stephen Hemminger

Quoted 1 line

	http://git.infradead.org/users/hch/block.git/commitdiff/148cff67b401e2229c076c0ea418712654be77e4

It appears that is already in the code I am testing in linux-next...

It's in -next now, but it wasn't at the time you reported the bug.

And it would sortof explain the bug if the INQUIRY data is correct in the scatterlist, but we ignore it, given that scsi_probe_lun ignores the result based on sense data.

Can you check what happens with the horrible hack below:

Quoted 5 lines · Christoph Hellwig

Quoted 3 lines · Stephen Hemminger

Quoted 1 line

	http://git.infradead.org/users/hch/block.git/commitdiff/148cff67b401e2229c076c0ea418712654be77e4  

It appears that is already in the code I am testing in linux-next...

Quoted 7 lines

It's in -next now, but it wasn't at the time you reported the bug.

And it would sortof explain the bug if the INQUIRY data is correct in the scatterlist, but we ignore it, given that scsi_probe_lun ignores the result based on sense data.

Can you check what happens with the horrible hack below:

Strike that - we're checking result later, so this can't be the case.

Now the other interesting thing is the memset in __scsi_exectute, which looks very suspicious. Try the following please:

drivers/scsi/scsi_lib.c +0 -0

index 3e32dc954c3c..22f4fb550561 100644

Quoted 31 lines · On Thu, 2 Mar 2017 01:56:15 +0100 Christoph Hellwig <[email protected]> wrote:

Quoted 5 lines · Christoph Hellwig

Quoted 3 lines · Stephen Hemminger

Quoted 1 line

	http://git.infradead.org/users/hch/block.git/commitdiff/148cff67b401e2229c076c0ea418712654be77e4

It appears that is already in the code I am testing in linux-next...

Quoted 7 lines

It's in -next now, but it wasn't at the time you reported the bug.

And it would sortof explain the bug if the INQUIRY data is correct in the scatterlist, but we ignore it, given that scsi_probe_lun ignores the result based on sense data.

Can you check what happens with the horrible hack below:

Strike that - we're checking result later, so this can't be the case.

Now the other interesting thing is the memset in __scsi_exectute, which looks very suspicious. Try the following please:

drivers/scsi/scsi_lib.c +2 -1

index 3e32dc954c3c..22f4fb550561 100644
@@ -253,7 +253,8 @@ static int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 	 * and prevent security leaks by zeroing out the excess data.
 	 */
 	if (unlikely(rq->resid_len > 0 && rq->resid_len <= bufflen))
-		memset(buffer + (bufflen - rq->resid_len), 0, rq->resid_len);
+//		memset(buffer + (bufflen - rq->resid_len), 0, rq->resid_len);
+		printk_ratelimited("%s: got resid %d\n", __func__, rq->resid_len);
 

drivers/scsi/scsi_lib.c static int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, · line 260

Quoted 2 lines

drivers/scsi/scsi_lib.c +0 -0

 	if (resid)
 		*resid = rq->resid_len;

Still fails but does print resid on some of the later INQUIRY commands (not the initial one).

Quoted 34 lines · Stephen Hemminger

Quoted 31 lines · On Thu, 2 Mar 2017 01:56:15 +0100 Christoph Hellwig <[email protected]> wrote:

Quoted 5 lines · Christoph Hellwig

Quoted 3 lines · Stephen Hemminger

Quoted 1 line

	http://git.infradead.org/users/hch/block.git/commitdiff/148cff67b401e2229c076c0ea418712654be77e4  

It appears that is already in the code I am testing in linux-next...

Quoted 7 lines

It's in -next now, but it wasn't at the time you reported the bug.

And it would sortof explain the bug if the INQUIRY data is correct in the scatterlist, but we ignore it, given that scsi_probe_lun ignores the result based on sense data.

Can you check what happens with the horrible hack below:

Strike that - we're checking result later, so this can't be the case.

Now the other interesting thing is the memset in __scsi_exectute, which looks very suspicious. Try the following please:

drivers/scsi/scsi_lib.c +2 -1

index 3e32dc954c3c..22f4fb550561 100644
@@ -253,7 +253,8 @@ static int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 	 * and prevent security leaks by zeroing out the excess data.
 	 */
 	if (unlikely(rq->resid_len > 0 && rq->resid_len <= bufflen))
-		memset(buffer + (bufflen - rq->resid_len), 0, rq->resid_len);
+//		memset(buffer + (bufflen - rq->resid_len), 0, rq->resid_len);
+		printk_ratelimited("%s: got resid %d\n", __func__, rq->resid_len);
 

Quoted 6 lines

drivers/scsi/scsi_lib.c static int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, · line 260

Quoted 2 lines

drivers/scsi/scsi_lib.c +0 -0

 	if (resid)
 		*resid = rq->resid_len;  

Still fails but does print resid on some of the later INQUIRY commands (not the initial one).

Can you test what happens if you blank out the storvsc_drv workaround:

drivers/scsi/storvsc_drv.c +2 -2

index 585e54f..c36f42d 100644
@@ -1060,13 +1060,13 @@ static void storvsc_on_io_completion(struct 
 storvsc_device *stor_device,
          * We do this so we can distinguish truly fatal failues
          * (srb status == 0x4) and off-line the device in that case.
          */
-
+#if 0
         if ((stor_pkt->vm_srb.cdb[0] == INQUIRY) ||
            (stor_pkt->vm_srb.cdb[0] == MODE_SENSE)) {
                 vstor_packet->vm_srb.scsi_status = 0;
                 vstor_packet->vm_srb.srb_status = SRB_STATUS_SUCCESS;
         }
-
+#endif
 
         /* Copy over the status...etc */

         stor_pkt->vm_srb.scsi_status = vstor_packet->vm_srb.scsi_status;

It might thappen that we're fail to interpret the 'Device not present' status correctly (which will happen for non-connected DVDs) causing the SCSI stack to make incorrect decisions later on.

Cheers,

Hannes

Quoted 34 lines · Stephen Hemminger

Quoted 31 lines · On Thu, 2 Mar 2017 01:56:15 +0100 Christoph Hellwig <[email protected]> wrote:

Quoted 5 lines · Christoph Hellwig

Quoted 3 lines · Stephen Hemminger

Quoted 1 line

	http://git.infradead.org/users/hch/block.git/commitdiff/148cff67b401e2229c076c0ea418712654be77e4  

It appears that is already in the code I am testing in linux-next...

Quoted 7 lines

It's in -next now, but it wasn't at the time you reported the bug.

And it would sortof explain the bug if the INQUIRY data is correct in the scatterlist, but we ignore it, given that scsi_probe_lun ignores the result based on sense data.

Can you check what happens with the horrible hack below:

Strike that - we're checking result later, so this can't be the case.

Now the other interesting thing is the memset in __scsi_exectute, which looks very suspicious. Try the following please:

drivers/scsi/scsi_lib.c +2 -1

index 3e32dc954c3c..22f4fb550561 100644
@@ -253,7 +253,8 @@ static int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 	 * and prevent security leaks by zeroing out the excess data.
 	 */
 	if (unlikely(rq->resid_len > 0 && rq->resid_len <= bufflen))
-		memset(buffer + (bufflen - rq->resid_len), 0, rq->resid_len);
+//		memset(buffer + (bufflen - rq->resid_len), 0, rq->resid_len);
+		printk_ratelimited("%s: got resid %d\n", __func__, rq->resid_len);
 

Quoted 6 lines

drivers/scsi/scsi_lib.c static int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, · line 260

Quoted 2 lines

drivers/scsi/scsi_lib.c +0 -0

 	if (resid)
 		*resid = rq->resid_len;  

Still fails but does print resid on some of the later INQUIRY commands (not the initial one).

Can you test what happens if you blank out the storvsc_drv workaround:

drivers/scsi/storvsc_drv.c +2 -2

index 585e54f..c36f42d 100644
@@ -1060,13 +1060,13 @@ static void storvsc_on_io_completion(struct 
 storvsc_device *stor_device,
          * We do this so we can distinguish truly fatal failues
          * (srb status == 0x4) and off-line the device in that case.
          */
-
+#if 0
         if ((stor_pkt->vm_srb.cdb[0] == INQUIRY) ||
            (stor_pkt->vm_srb.cdb[0] == MODE_SENSE)) {
                 vstor_packet->vm_srb.scsi_status = 0;
                 vstor_packet->vm_srb.srb_status = SRB_STATUS_SUCCESS;
         }
-
+#endif
 
         /* Copy over the status...etc */

         stor_pkt->vm_srb.scsi_status = vstor_packet->vm_srb.scsi_status;

It might thappen that we're fail to interpret the 'Device not present' status correctly (which will happen for non-connected DVDs) causing the SCSI stack to make incorrect decisions later on.

Cheers,

Hannes

Quoted 7 lines · On Thu, 2 Mar 2017 14:25:14 +0100 Hannes Reinecke <[email protected]> wrote:

Quoted 6 lines · Stephen Hemminger

Quoted 3 lines · On Thu, 2 Mar 2017 01:56:15 +0100 Christoph Hellwig <[email protected]> wrote:

Quoted 1 line · Christoph Hellwig

On Wed, Mar 01, 2017 at 07:54:12AM -0800, Stephen Hemminger

Quoted 1 line · wrote:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

	http://git.infradead.org/users/hch/block.git/commitdiff/148cff67b401e2229c076c0ea418712654be77e4

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

It appears that is already in the code I am testing in

linux-next...

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

It's in -next now, but it wasn't at the time you reported the

bug.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

And it would sortof explain the bug if the INQUIRY data is

correct

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

in the scatterlist, but we ignore it, given that scsi_probe_lun ignores the result based on sense data.

Can you check what happens with the horrible hack below:

Strike that - we're checking result later, so this can't be the

case.

Quoted 9 lines

Quoted 9 lines

drivers/scsi/scsi_lib.c static int __scsi_execute(struct scsi_device

Quoted 9 lines

Now the other interesting thing is the memset in __scsi_exectute, which looks very suspicious. Try the following please:

drivers/scsi/scsi_lib.c +0 -0

index 3e32dc954c3c..22f4fb550561 100644
@@ -253,7 +253,8 @@ static int __scsi_execute(struct scsi_device

*sdev, const unsigned char *cmd,

Quoted 6 lines

Quoted 6 lines

drivers/scsi/scsi_lib.c static int __scsi_execute(struct scsi_device · line 257

Quoted 6 lines

drivers/scsi/scsi_lib.c +2 -1

 	 * and prevent security leaks by zeroing out the excess data.
 	 */
 	if (unlikely(rq->resid_len > 0 && rq->resid_len <= bufflen))
-		memset(buffer + (bufflen - rq->resid_len), 0, rq->resid_len);
+//		memset(buffer + (bufflen - rq->resid_len), 0, rq->resid_len);
+		printk_ratelimited("%s: got resid %d\n", __func__,

rq->resid_len);

Quoted 6 lines

Quoted 6 lines

drivers/scsi/scsi_lib.c static int __scsi_execute(struct scsi_device · line 259

Quoted 3 lines

drivers/scsi/scsi_lib.c +0 -0

 	if (resid)
 		*resid = rq->resid_len;  

Still fails but does print resid on some of the later INQUIRY

commands (not the initial one).

Quoted 2 lines

Quoted 1 line

Can you test what happens if you blank out the storvsc_drv

workaround:

Quoted 6 lines

drivers/scsi/storvsc_drv.c +0 -0

index 585e54f..c36f42d 100644

drivers/scsi/storvsc_drv.c static void storvsc_on_io_completion(struct · line 1068

Quoted 10 lines

drivers/scsi/storvsc_drv.c +1 -1

@@ -1060,13 +1060,13 @@ static void storvsc_on_io_completion(struct 
 storvsc_device *stor_device,
          * We do this so we can distinguish truly fatal failues
          * (srb status == 0x4) and off-line the device in that case.
          */
-
+#if 0
         if ((stor_pkt->vm_srb.cdb[0] == INQUIRY) ||
            (stor_pkt->vm_srb.cdb[0] == MODE_SENSE)) {
                 vstor_packet->vm_srb.scsi_status = 0;
                 vstor_packet->vm_srb.srb_status =

SRB_STATUS_SUCCESS;

Quoted 6 lines

drivers/scsi/storvsc_drv.c +1 -1

         }
-
+#endif
 
         /* Copy over the status...etc */

         stor_pkt->vm_srb.scsi_status =

vstor_packet->vm_srb.scsi_status;

Quoted 2 lines

It might thappen that we're fail to interpret the 'Device not

present'

Quoted 1 line

status correctly (which will happen for non-connected DVDs) causing

the

Quoted 5 lines

SCSI stack to make incorrect decisions later on.

Cheers,

Hannes

There are several oddities about the host SCSI interface that I see:
1. The host bus seems to report up to 6 devices even though only 2 are present (Disk and CDROM).
2. The CDROM emulation doesn't report the same status as a real device.
3. The host emulation of SCSI doesn't support all the page codes which is why there is the hack.

But as James said, these don't appear to be related to the failure because
the code worked before and only in post 4.11 merege is there a problem.

Quoted 99 lines · Stephen Hemminger

Quoted 7 lines · On Thu, 2 Mar 2017 14:25:14 +0100 Hannes Reinecke <[email protected]> wrote:

Quoted 6 lines · Stephen Hemminger

Quoted 3 lines · On Thu, 2 Mar 2017 01:56:15 +0100 Christoph Hellwig <[email protected]> wrote:

Quoted 1 line · Christoph Hellwig

On Wed, Mar 01, 2017 at 07:54:12AM -0800, Stephen Hemminger

Quoted 1 line · wrote:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

	http://git.infradead.org/users/hch/block.git/commitdiff/148cff67b401e2229c076c0ea418712654be77e4

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

It appears that is already in the code I am testing in

linux-next...

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

It's in -next now, but it wasn't at the time you reported the

bug.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

And it would sortof explain the bug if the INQUIRY data is

correct

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

in the scatterlist, but we ignore it, given that scsi_probe_lun ignores the result based on sense data.

Can you check what happens with the horrible hack below:

Strike that - we're checking result later, so this can't be the

case.

Quoted 9 lines

Quoted 9 lines

drivers/scsi/scsi_lib.c static int __scsi_execute(struct scsi_device

Quoted 9 lines

Now the other interesting thing is the memset in __scsi_exectute, which looks very suspicious. Try the following please:

drivers/scsi/scsi_lib.c +0 -0

index 3e32dc954c3c..22f4fb550561 100644
@@ -253,7 +253,8 @@ static int __scsi_execute(struct scsi_device  

*sdev, const unsigned char *cmd,

Quoted 6 lines

Quoted 6 lines

drivers/scsi/scsi_lib.c static int __scsi_execute(struct scsi_device · line 257

Quoted 6 lines

drivers/scsi/scsi_lib.c +2 -1

 	 * and prevent security leaks by zeroing out the excess data.
 	 */
 	if (unlikely(rq->resid_len > 0 && rq->resid_len <= bufflen))
-		memset(buffer + (bufflen - rq->resid_len), 0, rq->resid_len);
+//		memset(buffer + (bufflen - rq->resid_len), 0, rq->resid_len);
+		printk_ratelimited("%s: got resid %d\n", __func__,  

rq->resid_len);

Quoted 6 lines

Quoted 6 lines

drivers/scsi/scsi_lib.c static int __scsi_execute(struct scsi_device · line 259

Quoted 3 lines

drivers/scsi/scsi_lib.c +0 -0

 	if (resid)
 		*resid = rq->resid_len;    

Still fails but does print resid on some of the later INQUIRY

commands (not the initial one).

Quoted 2 lines

Quoted 1 line

Can you test what happens if you blank out the storvsc_drv

workaround:

Quoted 6 lines

drivers/scsi/storvsc_drv.c +0 -0

index 585e54f..c36f42d 100644

drivers/scsi/storvsc_drv.c static void storvsc_on_io_completion(struct · line 1068

Quoted 10 lines

drivers/scsi/storvsc_drv.c +1 -1

@@ -1060,13 +1060,13 @@ static void storvsc_on_io_completion(struct 
 storvsc_device *stor_device,
          * We do this so we can distinguish truly fatal failues
          * (srb status == 0x4) and off-line the device in that case.
          */
-
+#if 0
         if ((stor_pkt->vm_srb.cdb[0] == INQUIRY) ||
            (stor_pkt->vm_srb.cdb[0] == MODE_SENSE)) {
                 vstor_packet->vm_srb.scsi_status = 0;
                 vstor_packet->vm_srb.srb_status =  

SRB_STATUS_SUCCESS;

Quoted 6 lines

drivers/scsi/storvsc_drv.c +1 -1

         }
-
+#endif
 
         /* Copy over the status...etc */

         stor_pkt->vm_srb.scsi_status =  

vstor_packet->vm_srb.scsi_status;

Quoted 2 lines

It might thappen that we're fail to interpret the 'Device not

present'

Quoted 1 line

status correctly (which will happen for non-connected DVDs) causing

the

Quoted 5 lines

SCSI stack to make incorrect decisions later on.

Cheers,

Hannes

There are several oddities about the host SCSI interface that I see:
1. The host bus seems to report up to 6 devices even though only 2 are present (Disk and CDROM).
2. The CDROM emulation doesn't report the same status as a real device.
3. The host emulation of SCSI doesn't support all the page codes which is why there is the hack.

Quoted 3 lines

But as James said, these don't appear to be related to the failure because
the code worked before and only in post 4.11 merege is there a problem.

Your wait for the hang trace is the most suggestive. It says we're waiting for a partition read to the spurious device. Previously this would have failed or timed out, so this seems to be the root cause.

James

Quoted 1 line · On Thu, 02 Mar 2017 10:36:17 -0800 James Bottomley…

On March 2, 2017 10:23:24 AM PST, Stephen Hemminger

Quoted 8 lines · <[email protected]> wrote:

Quoted 8 lines

Quoted 5 lines · On Thu, 2 Mar 2017 14:25:14 +0100 Hannes Reinecke <[email protected]> wrote:

Quoted 4 lines · Stephen Hemminger

Quoted 1 line · On Thu, 2 Mar 2017 01:56:15 +0100 Christoph Hellwig <[email protected]> wrote:

On Thu, Mar 02, 2017 at 01:01:35AM +0100, Christoph Hellwig

Quoted 18 lines · wrote:

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

On Wed, Mar 01, 2017 at 07:54:12AM -0800, Stephen Hemminger

Quoted 1 line · wrote:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

	http://git.infradead.org/users/hch/block.git/commitdiff/148cff67b401e2229c076c0ea418712654be77e4

Quoted 12 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

It appears that is already in the code I am testing in

linux-next...

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

It's in -next now, but it wasn't at the time you reported the

bug.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

And it would sortof explain the bug if the INQUIRY data is

correct

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

in the scatterlist, but we ignore it, given that

scsi_probe_lun

Quoted 9 lines

Quoted 9 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

ignores the result based on sense data.

Can you check what happens with the horrible hack below:

Strike that - we're checking result later, so this can't be the

case.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Now the other interesting thing is the memset in

__scsi_exectute,

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

drivers/scsi/scsi_lib.c static int __scsi_execute(struct

Quoted 7 lines

which looks very suspicious. Try the following please:

drivers/scsi/scsi_lib.c +0 -0

index 3e32dc954c3c..22f4fb550561 100644
@@ -253,7 +253,8 @@ static int __scsi_execute(struct

scsi_device

Quoted 5 lines

Quoted 5 lines

*sdev, const unsigned char *cmd,

Quoted 4 lines

Quoted 4 lines

drivers/scsi/scsi_lib.c static int __scsi_execute(struct · line 256

Quoted 4 lines

drivers/scsi/scsi_lib.c +0 -1

 	 * and prevent security leaks by zeroing out the excess data.
 	 */
 	if (unlikely(rq->resid_len > 0 && rq->resid_len <= bufflen))
-		memset(buffer + (bufflen - rq->resid_len), 0,

rq->resid_len);

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

drivers/scsi/scsi_lib.c static int __scsi_execute(struct · line 256

Quoted 1 line

drivers/scsi/scsi_lib.c +1 -0

+//		memset(buffer + (bufflen - rq->resid_len), 0,

rq->resid_len);

Quoted 7 lines

Quoted 7 lines

Quoted 1 line

Quoted 1 line

drivers/scsi/scsi_lib.c static int __scsi_execute(struct · line 257

Quoted 1 line

drivers/scsi/scsi_lib.c +1 -0

+		printk_ratelimited("%s: got resid %d\n", __func__,  

rq->resid_len);

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

drivers/scsi/scsi_lib.c +0 -0

 	if (resid)
 		*resid = rq->resid_len;    

Quoted 7 lines

Quoted 7 lines

Quoted 1 line

Quoted 1 line

Still fails but does print resid on some of the later INQUIRY

commands (not the initial one).

Quoted 2 lines

Quoted 1 line

Can you test what happens if you blank out the storvsc_drv

workaround:

Review

Quoted 2 lines

(quoted diff) +0 -0

b/drivers/scsi/storvsc_drv.c

Quoted 4 lines

Quoted 4 lines

drivers/scsi/storvsc_drv.c static void

Quoted 4 lines

index 585e54f..c36f42d 100644

drivers/scsi/storvsc_drv.c +0 -0

@@ -1060,13 +1060,13 @@ static void

storvsc_on_io_completion(struct

Quoted 3 lines

Quoted 3 lines

drivers/scsi/storvsc_drv.c static void · line 1062

Quoted 3 lines

drivers/scsi/storvsc_drv.c +0 -0

 storvsc_device *stor_device,
          * We do this so we can distinguish truly fatal failues
          * (srb status == 0x4) and off-line the device in that

case.

Quoted 12 lines

Quoted 12 lines

drivers/scsi/storvsc_drv.c static void storvsc_on_io_completion(struct storvsc_device *stor_device,

Quoted 7 lines

drivers/scsi/storvsc_drv.c +1 -1

          */
-
+#if 0
         if ((stor_pkt->vm_srb.cdb[0] == INQUIRY) ||
            (stor_pkt->vm_srb.cdb[0] == MODE_SENSE)) {
                 vstor_packet->vm_srb.scsi_status = 0;
                 vstor_packet->vm_srb.srb_status =  

SRB_STATUS_SUCCESS;

Quoted 4 lines

drivers/scsi/storvsc_drv.c +1 -1

         }
-
+#endif
 

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

drivers/scsi/storvsc_drv.c +0 -0

         /* Copy over the status...etc */

         stor_pkt->vm_srb.scsi_status =  

vstor_packet->vm_srb.scsi_status;

Quoted 2 lines

It might thappen that we're fail to interpret the 'Device not

present'

Quoted 1 line

status correctly (which will happen for non-connected DVDs)

causing

Quoted 9 lines

Quoted 9 lines

the

Quoted 5 lines

SCSI stack to make incorrect decisions later on.

Cheers,

Hannes

There are several oddities about the host SCSI interface that I see:
1. The host bus seems to report up to 6 devices even though only 2

are

Quoted 2 lines

Quoted 2 lines

present (Disk and CDROM).
2. The CDROM emulation doesn't report the same status as a real

device.

Quoted 1 line

Quoted 1 line

3. The host emulation of SCSI doesn't support all the page codes

which

Quoted 5 lines

Quoted 5 lines

is why there is the hack.

But as James said, these don't appear to be related to the failure because
the code worked before and only in post 4.11 merege is there a

problem.

Quoted 2 lines

Your wait for the hang trace is the most suggestive. It says we're

waiting for a partition read to the spurious device. Previously this would have failed or timed out, so this seems to be the root cause.

Quoted 4 lines

James

Where is the number of valid LUN's determined during the scan process?

Quoted 129 lines · Stephen Hemminger

Quoted 1 line · On Thu, 02 Mar 2017 10:36:17 -0800 James Bottomley…

On March 2, 2017 10:23:24 AM PST, Stephen Hemminger

Quoted 8 lines · <[email protected]> wrote:

Quoted 8 lines

Quoted 5 lines · On Thu, 2 Mar 2017 14:25:14 +0100 Hannes Reinecke <[email protected]> wrote:

Quoted 4 lines · Stephen Hemminger

Quoted 1 line · On Thu, 2 Mar 2017 01:56:15 +0100 Christoph Hellwig <[email protected]> wrote:

On Thu, Mar 02, 2017 at 01:01:35AM +0100, Christoph Hellwig

Quoted 18 lines · wrote:

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

On Wed, Mar 01, 2017 at 07:54:12AM -0800, Stephen Hemminger

Quoted 1 line · wrote:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

	http://git.infradead.org/users/hch/block.git/commitdiff/148cff67b401e2229c076c0ea418712654be77e4  

Quoted 12 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

It appears that is already in the code I am testing in

linux-next...

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

It's in -next now, but it wasn't at the time you reported the

bug.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

And it would sortof explain the bug if the INQUIRY data is

correct

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

in the scatterlist, but we ignore it, given that

scsi_probe_lun

Quoted 9 lines

Quoted 9 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

ignores the result based on sense data.

Can you check what happens with the horrible hack below:

Strike that - we're checking result later, so this can't be the

case.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Now the other interesting thing is the memset in

__scsi_exectute,

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

drivers/scsi/scsi_lib.c static int __scsi_execute(struct

Quoted 7 lines

which looks very suspicious. Try the following please:

drivers/scsi/scsi_lib.c +0 -0

index 3e32dc954c3c..22f4fb550561 100644
@@ -253,7 +253,8 @@ static int __scsi_execute(struct  

scsi_device

Quoted 5 lines

Quoted 5 lines

*sdev, const unsigned char *cmd,

Quoted 4 lines

Quoted 4 lines

drivers/scsi/scsi_lib.c static int __scsi_execute(struct · line 256

Quoted 4 lines

drivers/scsi/scsi_lib.c +0 -1

 	 * and prevent security leaks by zeroing out the excess data.
 	 */
 	if (unlikely(rq->resid_len > 0 && rq->resid_len <= bufflen))
-		memset(buffer + (bufflen - rq->resid_len), 0,  

rq->resid_len);

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

drivers/scsi/scsi_lib.c static int __scsi_execute(struct · line 256

Quoted 1 line

drivers/scsi/scsi_lib.c +1 -0

+//		memset(buffer + (bufflen - rq->resid_len), 0,  

rq->resid_len);

Quoted 7 lines

Quoted 7 lines

Quoted 1 line

Quoted 1 line

drivers/scsi/scsi_lib.c static int __scsi_execute(struct · line 257

Quoted 1 line

drivers/scsi/scsi_lib.c +1 -0

+		printk_ratelimited("%s: got resid %d\n", __func__,    

rq->resid_len);

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

drivers/scsi/scsi_lib.c +0 -0

 	if (resid)
 		*resid = rq->resid_len;      

Quoted 7 lines

Quoted 7 lines

Quoted 1 line

Quoted 1 line

Still fails but does print resid on some of the later INQUIRY

commands (not the initial one).

Quoted 2 lines

Quoted 1 line

Can you test what happens if you blank out the storvsc_drv

workaround:

Review

Quoted 2 lines

(quoted diff) +0 -0

b/drivers/scsi/storvsc_drv.c

Quoted 4 lines

Quoted 4 lines

drivers/scsi/storvsc_drv.c static void

Quoted 4 lines

index 585e54f..c36f42d 100644

drivers/scsi/storvsc_drv.c +0 -0

@@ -1060,13 +1060,13 @@ static void  

storvsc_on_io_completion(struct

Quoted 3 lines

Quoted 3 lines

drivers/scsi/storvsc_drv.c static void · line 1062

Quoted 3 lines

drivers/scsi/storvsc_drv.c +0 -0

 storvsc_device *stor_device,
          * We do this so we can distinguish truly fatal failues
          * (srb status == 0x4) and off-line the device in that  

case.

Quoted 12 lines

Quoted 12 lines

drivers/scsi/storvsc_drv.c static void storvsc_on_io_completion(struct storvsc_device *stor_device,

Quoted 7 lines

drivers/scsi/storvsc_drv.c +1 -1

          */
-
+#if 0
         if ((stor_pkt->vm_srb.cdb[0] == INQUIRY) ||
            (stor_pkt->vm_srb.cdb[0] == MODE_SENSE)) {
                 vstor_packet->vm_srb.scsi_status = 0;
                 vstor_packet->vm_srb.srb_status =    

SRB_STATUS_SUCCESS;

Quoted 4 lines

drivers/scsi/storvsc_drv.c +1 -1

         }
-
+#endif
 

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

drivers/scsi/storvsc_drv.c +0 -0

         /* Copy over the status...etc */

         stor_pkt->vm_srb.scsi_status =    

vstor_packet->vm_srb.scsi_status;

Quoted 2 lines

It might thappen that we're fail to interpret the 'Device not

present'

Quoted 1 line

status correctly (which will happen for non-connected DVDs)

causing

Quoted 9 lines

Quoted 9 lines

the

Quoted 5 lines

SCSI stack to make incorrect decisions later on.

Cheers,

Hannes

There are several oddities about the host SCSI interface that I see:
1. The host bus seems to report up to 6 devices even though only 2

are

Quoted 2 lines

Quoted 2 lines

present (Disk and CDROM).
2. The CDROM emulation doesn't report the same status as a real

device.

Quoted 1 line

Quoted 1 line

3. The host emulation of SCSI doesn't support all the page codes

which

Quoted 5 lines

Quoted 5 lines

is why there is the hack.

But as James said, these don't appear to be related to the failure because
the code worked before and only in post 4.11 merege is there a

problem.

Quoted 2 lines

Your wait for the hang trace is the most suggestive. It says we're

waiting for a partition read to the spurious device. Previously this would have failed or timed out, so this seems to be the root cause.

Quoted 1 line

Quoted 5 lines

Quoted 3 lines

James

Where is the number of valid LUN's determined during the scan process?

Depends. If you can do a report lun scan then that's definitive. You seem to be probing (SCSI_probe_and_add_lun) and you make us think there's something there by responding wrongly to the initial inquiry.

Needs more testing but this does fix the observed problem.

From: Stephen Hemminger <[email protected]>

Subject: [PATCH] hv_storvsc: fix error handling

The Hyper-V storvsc SCSI driver was hiding all errors in INQUIRY and MODE_SENSE commands. This caused the scan process to incorrectly think devices were present and online. Also invalid LUN errors were not being handled correctly.

This fixes problems booting a GEN2 VM on Hyper-V. It effectively reverts commit 4ed51a21c0f69 ("Staging: hv: storvsc: Fixup srb and scsi status for INQUIRY and MODE_SENSE")

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/scsi/storvsc_drv.c | 48 ++++------------------------------------------
 1 file changed, 4 insertions(+), 44 deletions(-)

drivers/scsi/storvsc_drv.c +1 -42

index 638e5f427c90..8cc241fc54b8 100644
@@ -543,28 +543,6 @@ static void storvsc_host_scan(struct work_struct *work)
 	kfree(wrk);
 }
 
-static void storvsc_remove_lun(struct work_struct *work)
-{
-	struct storvsc_scan_work *wrk;
-	struct scsi_device *sdev;
-
-	wrk = container_of(work, struct storvsc_scan_work, work);
-	if (!scsi_host_get(wrk->host))
-		goto done;
-
-	sdev = scsi_device_lookup(wrk->host, 0, wrk->tgt_id, wrk->lun);
-
-	if (sdev) {
-		scsi_remove_device(sdev);
-		scsi_device_put(sdev);
-	}
-	scsi_host_put(wrk->host);
-
-done:
-	kfree(wrk);
-}
-
-
 /*
  * We can get incoming messages from the host that are not in response to
  * messages that we have sent out. An example of this would be messages

@@ -955,8 +933,7 @@ static void storvsc_handle_error(struct vmscsi_request *vm_srb,
 		}
 		break;
 	case SRB_STATUS_INVALID_LUN:
-		do_work = true;
-		process_err_fn = storvsc_remove_lun;
+		set_host_byte(scmnd, DID_NO_CONNECT);
 		break;
 	case SRB_STATUS_ABORTED:
 		if (vm_srb->srb_status & SRB_STATUS_AUTOSENSE_VALID &&

@@ -1050,32 +1027,15 @@ static void storvsc_on_io_completion(struct storvsc_device *stor_device,
 
 	stor_pkt = &request->vstor_packet;
 
-	/*
-	 * The current SCSI handling on the host side does
-	 * not correctly handle:
-	 * INQUIRY command with page code parameter set to 0x80
-	 * MODE_SENSE command with cmd[2] == 0x1c
-	 *
-	 * Setup srb and scsi status so this won't be fatal.
-	 * We do this so we can distinguish truly fatal failues
-	 * (srb status == 0x4) and off-line the device in that case.
-	 */
-
-	if ((stor_pkt->vm_srb.cdb[0] == INQUIRY) ||
-	   (stor_pkt->vm_srb.cdb[0] == MODE_SENSE)) {
-		vstor_packet->vm_srb.scsi_status = 0;
-		vstor_packet->vm_srb.srb_status = SRB_STATUS_SUCCESS;
-	}
-
-
 	/* Copy over the status...etc */
 	stor_pkt->vm_srb.scsi_status = vstor_packet->vm_srb.scsi_status;
 	stor_pkt->vm_srb.srb_status = vstor_packet->vm_srb.srb_status;
 	stor_pkt->vm_srb.sense_info_length =

-----Original Message-----
From: Stephen Hemminger [mailto:[email protected]]
Sent: Friday, March 3, 2017 4:50 PM
To: James Bottomley <[email protected]>
Cc: Hannes Reinecke <[email protected]>; Christoph Hellwig <[email protected]>; James Bottomley <[email protected]>; Jens Axboe <[email protected]>; Linus Torvalds <[email protected]>; Martin K. Petersen <[email protected]>; KY Srinivasan <[email protected]>; Dexuan Cui <[email protected]>; Long Li <[email protected]>; Josh Poulson <[email protected]>; Adrian Suhov (Cloudbase Solutions SRL) <[email protected]>; linux- [email protected]; Haiyang Zhang <[email protected]>
Subject: [RFC] hv_storvsc: error handling.

Needs more testing but this does fix the observed problem.

From: Stephen Hemminger <[email protected]>

Subject: [PATCH] hv_storvsc: fix error handling

The Hyper-V storvsc SCSI driver was hiding all errors in INQUIRY and MODE_SENSE commands. This caused the scan process to incorrectly think devices were present and online. Also invalid LUN errors were not being handled correctly.

This fixes problems booting a GEN2 VM on Hyper-V. It effectively reverts commit 4ed51a21c0f69 ("Staging: hv: storvsc: Fixup srb and scsi status for INQUIRY and MODE_SENSE")

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/scsi/storvsc_drv.c | 48 ++++------------------------------------------
 1 file changed, 4 insertions(+), 44 deletions(-)

drivers/scsi/storvsc_drv.c +0 -22

index 638e5f427c90..8cc241fc54b8 100644
@@ -543,28 +543,6 @@ static void storvsc_host_scan(struct work_struct
 *work)
 	kfree(wrk);
 }
 
-static void storvsc_remove_lun(struct work_struct *work)
-{
-	struct storvsc_scan_work *wrk;
-	struct scsi_device *sdev;
-
-	wrk = container_of(work, struct storvsc_scan_work, work);
-	if (!scsi_host_get(wrk->host))
-		goto done;
-
-	sdev = scsi_device_lookup(wrk->host, 0, wrk->tgt_id, wrk->lun);
-
-	if (sdev) {
-		scsi_remove_device(sdev);
-		scsi_device_put(sdev);
-	}
-	scsi_host_put(wrk->host);
-
-done:
-	kfree(wrk);
-}
-
-
 /*
  * We can get incoming messages from the host that are not in response to

* messages that we have sent out. An example of this would be messages

drivers/scsi/storvsc_drv.c +1 -2

@@ -955,8 +933,7 @@ static void storvsc_handle_error(struct
 vmscsi_request *vm_srb,
 		}
 		break;
 	case SRB_STATUS_INVALID_LUN:
-		do_work = true;
-		process_err_fn = storvsc_remove_lun;
+		set_host_byte(scmnd, DID_NO_CONNECT);
 		break;
 	case SRB_STATUS_ABORTED:

if (vm_srb->srb_status & SRB_STATUS_AUTOSENSE_VALID
&&

drivers/scsi/storvsc_drv.c +3 -20

@@ -1050,32 +1027,15 @@ static void storvsc_on_io_completion(struct
 storvsc_device *stor_device,
 
 	stor_pkt = &request->vstor_packet;
 
-	/*
-	 * The current SCSI handling on the host side does
-	 * not correctly handle:
-	 * INQUIRY command with page code parameter set to 0x80
-	 * MODE_SENSE command with cmd[2] == 0x1c
-	 *
-	 * Setup srb and scsi status so this won't be fatal.
-	 * We do this so we can distinguish truly fatal failues
-	 * (srb status == 0x4) and off-line the device in that case.
-	 */
-
-	if ((stor_pkt->vm_srb.cdb[0] == INQUIRY) ||
-	   (stor_pkt->vm_srb.cdb[0] == MODE_SENSE)) {
-		vstor_packet->vm_srb.scsi_status = 0;
-		vstor_packet->vm_srb.srb_status = SRB_STATUS_SUCCESS;
-	}
-
-
 	/* Copy over the status...etc */
 	stor_pkt->vm_srb.scsi_status = vstor_packet->vm_srb.scsi_status;
 	stor_pkt->vm_srb.srb_status = vstor_packet->vm_srb.srb_status;
 	stor_pkt->vm_srb.sense_info_length =
 	vstor_packet->vm_srb.sense_info_length;
 
-	if (vstor_packet->vm_srb.scsi_status != 0 ||
-	    vstor_packet->vm_srb.srb_status != SRB_STATUS_SUCCESS)
+	if (stor_pkt->vm_srb.cdb[0] != INQUIRY &&
+	    (vstor_packet->vm_srb.scsi_status != 0 ||
+	     vstor_packet->vm_srb.srb_status != SRB_STATUS_SUCCESS))
 		storvsc_log(device, STORVSC_LOGGING_WARN,
 			"cmd 0x%x scsi status 0x%x srb status 0x%x\n",

stor_pkt->vm_srb.cdb[0],
--

From b97f24f224a71a6e745c42e5640045a553eb407c Mon Sep 17 00:00:00 2001

2.11.0

Quoted 132 lines

-----Original Message-----
From: Stephen Hemminger [mailto:[email protected]]
Sent: Friday, March 3, 2017 4:50 PM
To: James Bottomley <[email protected]>
Cc: Hannes Reinecke <[email protected]>; Christoph Hellwig <[email protected]>; James Bottomley <[email protected]>; Jens Axboe <[email protected]>; Linus Torvalds <[email protected]>; Martin K. Petersen <[email protected]>; KY Srinivasan <[email protected]>; Dexuan Cui <[email protected]>; Long Li <[email protected]>; Josh Poulson <[email protected]>; Adrian
Suhov (Cloudbase Solutions SRL) <[email protected]>; linux- [email protected]; Haiyang Zhang <[email protected]>
Subject: [RFC] hv_storvsc: error handling.

Needs more testing but this does fix the observed problem.

From: Stephen Hemminger <[email protected]>

Subject: [PATCH] hv_storvsc: fix error handling

The Hyper-V storvsc SCSI driver was hiding all errors in INQUIRY and
MODE_SENSE commands. This caused the scan process to incorrectly think
devices were present and online. Also invalid LUN errors were not being handled correctly.

This fixes problems booting a GEN2 VM on Hyper-V. It effectively reverts commit 4ed51a21c0f69 ("Staging: hv: storvsc: Fixup srb and scsi status for INQUIRY and MODE_SENSE")

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/scsi/storvsc_drv.c | 48 ++++------------------------------
------------
 1 file changed, 4 insertions(+), 44 deletions(-)

drivers/scsi/storvsc_drv.c +0 -0

index 638e5f427c90..8cc241fc54b8 100644
@@ -543,28 +543,6 @@ static void storvsc_host_scan(struct

work_struct
*work)

drivers/scsi/storvsc_drv.c +0 -23

 	kfree(wrk);
 }
 
-static void storvsc_remove_lun(struct work_struct *work)
-{
-	struct storvsc_scan_work *wrk;
-	struct scsi_device *sdev;
-
-	wrk = container_of(work, struct storvsc_scan_work, work);
-	if (!scsi_host_get(wrk->host))
-		goto done;
-
-	sdev = scsi_device_lookup(wrk->host, 0, wrk->tgt_id, wrk
->lun);
-
-	if (sdev) {
-		scsi_remove_device(sdev);
-		scsi_device_put(sdev);
-	}
-	scsi_host_put(wrk->host);
-
-done:
-	kfree(wrk);
-}
-
-
 /*
  * We can get incoming messages from the host that are not in

response to
* messages that we have sent out. An example of this would be
messages

drivers/scsi/storvsc_drv.c +1 -2

@@ -955,8 +933,7 @@ static void storvsc_handle_error(struct
 vmscsi_request *vm_srb,
 		}
 		break;
 	case SRB_STATUS_INVALID_LUN:
-		do_work = true;
-		process_err_fn = storvsc_remove_lun;
+		set_host_byte(scmnd, DID_NO_CONNECT);
 		break;
 	case SRB_STATUS_ABORTED:

if (vm_srb->srb_status &
SRB_STATUS_AUTOSENSE_VALID
&&

drivers/scsi/storvsc_drv.c +0 -9

@@ -1050,32 +1027,15 @@ static void storvsc_on_io_completion(struct
 storvsc_device *stor_device,
 
 	stor_pkt = &request->vstor_packet;
 
-	/*
-	 * The current SCSI handling on the host side does
-	 * not correctly handle:
-	 * INQUIRY command with page code parameter set to 0x80
-	 * MODE_SENSE command with cmd[2] == 0x1c
-	 *
-	 * Setup srb and scsi status so this won't be fatal.
-	 * We do this so we can distinguish truly fatal failues
-	 * (srb status == 0x4) and off-line the device in that

case.

drivers/scsi/storvsc_drv.c +0 -6

-	 */
-
-	if ((stor_pkt->vm_srb.cdb[0] == INQUIRY) ||
-	   (stor_pkt->vm_srb.cdb[0] == MODE_SENSE)) {
-		vstor_packet->vm_srb.scsi_status = 0;
-		vstor_packet->vm_srb.srb_status =

SRB_STATUS_SUCCESS;

drivers/scsi/storvsc_drv.c +3 -7

-	}
-
-
 	/* Copy over the status...etc */
 	stor_pkt->vm_srb.scsi_status = vstor_packet
->vm_srb.scsi_status;
 	stor_pkt->vm_srb.srb_status = vstor_packet
->vm_srb.srb_status;
 	stor_pkt->vm_srb.sense_info_length =
 	vstor_packet->vm_srb.sense_info_length;
 
-	if (vstor_packet->vm_srb.scsi_status != 0 ||
-	    vstor_packet->vm_srb.srb_status != SRB_STATUS_SUCCESS)
+	if (stor_pkt->vm_srb.cdb[0] != INQUIRY &&
+	    (vstor_packet->vm_srb.scsi_status != 0 ||
+	     vstor_packet->vm_srb.srb_status !=

SRB_STATUS_SUCCESS))

drivers/scsi/storvsc_drv.c +0 -0

 		storvsc_log(device, STORVSC_LOGGING_WARN,
 			"cmd 0x%x scsi status 0x%x srb status

Quoted 3 lines

0x%x\n",
stor_pkt->vm_srb.cdb[0],
--

This patch gets rid of the ability to "hot remove" LUNs. I don't think that can be part of any
solution. The INQUIRY hack I put in a long time ago was to deal with host bugs on prior versions of
Windows server. WS2016 should not be trigerring this code. Stephen, could you please test this patch -
a quick hack:

From b97f24f224a71a6e745c42e5640045a553eb407c Mon Sep 17 00:00:00 2001
From: K. Y. Srinivasan <[email protected]>
Date: Sat, 4 Mar 2017 14:00:46 -0700
Subject: [PATCH 1/1] scsi: storvsc: Fix a bug in LUN removal code
Reply-To: [email protected]

Signed-off-by K. Y. Srinivasan <[email protected]>

---
 drivers/scsi/storvsc_drv.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

drivers/scsi/storvsc_drv.c +1 -0

index 05526b7..27eb682 100644
@@ -885,6 +885,7 @@ static void storvsc_handle_error(struct
 vmscsi_request *vm_srb,
        struct storvsc_scan_work *wrk;
        void (*process_err_fn)(struct work_struct *work);
        bool do_work = false;
+       struct scsi_device *sdev;
 
        switch (SRB_STATUS(vm_srb->srb_status)) {

case SRB_STATUS_ERROR:

drivers/scsi/storvsc_drv.c +3 -0

@@ -911,6 +912,18 @@ static void storvsc_handle_error(struct
 vmscsi_request *vm_srb,
                }
                break;
        case SRB_STATUS_INVALID_LUN:
+               if (!scsi_host_get(host)) {
+                       set_host_byte(scmnd, DID_NO_CONNECT);
+                       break;

-----Original Message-----
From: James Bottomley [mailto:[email protected]]
Sent: Saturday, March 4, 2017 1:37 PM
To: KY Srinivasan <[email protected]>; Stephen Hemminger <[email protected]>
Cc: Hannes Reinecke <[email protected]>; Christoph Hellwig <[email protected]>; Jens Axboe <[email protected]>; Linus Torvalds <torvalds@linux- foundation.org>; Martin K. Petersen <[email protected]>; Dexuan Cui <[email protected]>; Long Li <[email protected]>; Josh Poulson <[email protected]>; Adrian Suhov (Cloudbase Solutions SRL) <[email protected]>; [email protected]; Haiyang Zhang <[email protected]>
Subject: Re: [RFC] hv_storvsc: error handling.

Quoted 177 lines · KY Srinivasan

Quoted 132 lines

-----Original Message-----
From: Stephen Hemminger [mailto:[email protected]]
Sent: Friday, March 3, 2017 4:50 PM
To: James Bottomley <[email protected]>
Cc: Hannes Reinecke <[email protected]>; Christoph Hellwig <[email protected]>; James Bottomley <[email protected]>; Jens Axboe <[email protected]>; Linus Torvalds <[email protected]>; Martin K. Petersen <[email protected]>; KY Srinivasan <[email protected]>; Dexuan Cui <[email protected]>; Long Li <[email protected]>; Josh Poulson <[email protected]>; Adrian
Suhov (Cloudbase Solutions SRL) <[email protected]>; linux- [email protected]; Haiyang Zhang <[email protected]>
Subject: [RFC] hv_storvsc: error handling.

Needs more testing but this does fix the observed problem.

From: Stephen Hemminger <[email protected]>

Subject: [PATCH] hv_storvsc: fix error handling

The Hyper-V storvsc SCSI driver was hiding all errors in INQUIRY and
MODE_SENSE commands. This caused the scan process to incorrectly think
devices were present and online. Also invalid LUN errors were not being handled correctly.

This fixes problems booting a GEN2 VM on Hyper-V. It effectively reverts commit 4ed51a21c0f69 ("Staging: hv: storvsc: Fixup srb and scsi status for INQUIRY and MODE_SENSE")

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/scsi/storvsc_drv.c | 48 ++++------------------------------
------------
 1 file changed, 4 insertions(+), 44 deletions(-)

drivers/scsi/storvsc_drv.c +0 -0

index 638e5f427c90..8cc241fc54b8 100644
@@ -543,28 +543,6 @@ static void storvsc_host_scan(struct

work_struct
*work)

drivers/scsi/storvsc_drv.c +0 -23

 	kfree(wrk);
 }
 
-static void storvsc_remove_lun(struct work_struct *work)
-{
-	struct storvsc_scan_work *wrk;
-	struct scsi_device *sdev;
-
-	wrk = container_of(work, struct storvsc_scan_work, work);
-	if (!scsi_host_get(wrk->host))
-		goto done;
-
-	sdev = scsi_device_lookup(wrk->host, 0, wrk->tgt_id, wrk
->lun);
-
-	if (sdev) {
-		scsi_remove_device(sdev);
-		scsi_device_put(sdev);
-	}
-	scsi_host_put(wrk->host);
-
-done:
-	kfree(wrk);
-}
-
-
 /*
  * We can get incoming messages from the host that are not in

response to
* messages that we have sent out. An example of this would be
messages

drivers/scsi/storvsc_drv.c +1 -2

@@ -955,8 +933,7 @@ static void storvsc_handle_error(struct
 vmscsi_request *vm_srb,
 		}
 		break;
 	case SRB_STATUS_INVALID_LUN:
-		do_work = true;
-		process_err_fn = storvsc_remove_lun;
+		set_host_byte(scmnd, DID_NO_CONNECT);
 		break;
 	case SRB_STATUS_ABORTED:

if (vm_srb->srb_status &
SRB_STATUS_AUTOSENSE_VALID
&&

drivers/scsi/storvsc_drv.c +0 -9

@@ -1050,32 +1027,15 @@ static void storvsc_on_io_completion(struct
 storvsc_device *stor_device,
 
 	stor_pkt = &request->vstor_packet;
 
-	/*
-	 * The current SCSI handling on the host side does
-	 * not correctly handle:
-	 * INQUIRY command with page code parameter set to 0x80
-	 * MODE_SENSE command with cmd[2] == 0x1c
-	 *
-	 * Setup srb and scsi status so this won't be fatal.
-	 * We do this so we can distinguish truly fatal failues
-	 * (srb status == 0x4) and off-line the device in that

case.

drivers/scsi/storvsc_drv.c +0 -6

-	 */
-
-	if ((stor_pkt->vm_srb.cdb[0] == INQUIRY) ||
-	   (stor_pkt->vm_srb.cdb[0] == MODE_SENSE)) {
-		vstor_packet->vm_srb.scsi_status = 0;
-		vstor_packet->vm_srb.srb_status =

SRB_STATUS_SUCCESS;

drivers/scsi/storvsc_drv.c +3 -7

-	}
-
-
 	/* Copy over the status...etc */
 	stor_pkt->vm_srb.scsi_status = vstor_packet
->vm_srb.scsi_status;
 	stor_pkt->vm_srb.srb_status = vstor_packet
->vm_srb.srb_status;
 	stor_pkt->vm_srb.sense_info_length =
 	vstor_packet->vm_srb.sense_info_length;
 
-	if (vstor_packet->vm_srb.scsi_status != 0 ||
-	    vstor_packet->vm_srb.srb_status != SRB_STATUS_SUCCESS)
+	if (stor_pkt->vm_srb.cdb[0] != INQUIRY &&
+	    (vstor_packet->vm_srb.scsi_status != 0 ||
+	     vstor_packet->vm_srb.srb_status !=

SRB_STATUS_SUCCESS))

drivers/scsi/storvsc_drv.c +0 -0

 		storvsc_log(device, STORVSC_LOGGING_WARN,
 			"cmd 0x%x scsi status 0x%x srb status

Quoted 3 lines

0x%x\n",
stor_pkt->vm_srb.cdb[0],
--

This patch gets rid of the ability to "hot remove" LUNs. I don't think that can be part of any
solution. The INQUIRY hack I put in a long time ago was to deal with host bugs on prior versions of
Windows server. WS2016 should not be trigerring this code. Stephen, could you please test this patch -
a quick hack:

From b97f24f224a71a6e745c42e5640045a553eb407c Mon Sep 17 00:00:00 2001
From: K. Y. Srinivasan <[email protected]>
Date: Sat, 4 Mar 2017 14:00:46 -0700
Subject: [PATCH 1/1] scsi: storvsc: Fix a bug in LUN removal code
Reply-To: [email protected]

Signed-off-by K. Y. Srinivasan <[email protected]>

---
 drivers/scsi/storvsc_drv.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

drivers/scsi/storvsc_drv.c +1 -0

index 05526b7..27eb682 100644
@@ -885,6 +885,7 @@ static void storvsc_handle_error(struct
 vmscsi_request *vm_srb,
        struct storvsc_scan_work *wrk;
        void (*process_err_fn)(struct work_struct *work);
        bool do_work = false;
+       struct scsi_device *sdev;
 
        switch (SRB_STATUS(vm_srb->srb_status)) {

case SRB_STATUS_ERROR:

drivers/scsi/storvsc_drv.c +3 -0

@@ -911,6 +912,18 @@ static void storvsc_handle_error(struct
 vmscsi_request *vm_srb,
                }
                break;
        case SRB_STATUS_INVALID_LUN:
+               if (!scsi_host_get(host)) {
+                       set_host_byte(scmnd, DID_NO_CONNECT);
+                       break;

drivers/scsi/storvsc_drv.c static void storvsc_handle_error(struct · line 928

Quoted 10 lines

drivers/scsi/storvsc_drv.c +9 -0

+               }
+
+               sdev = scsi_device_lookup(wrk->host, 0, wrk->tgt_id,
 wrk->lun);
+
+               if (!sdev) {
+                       set_host_byte(scmnd, DID_NO_CONNECT);
+                       break;
+               }
+

You're now getting two references (one to the host and one to the device) that you don't put either in error handling or in storvsc_remove_lun().

Probably you should eliminate the scsi_host_get and scsi_device_lookup in storvsc_remove_lun() (making it basically remove device put device put host) and add a host put in the !sdev if above.

James

-----Original Message-----
From: KY Srinivasan
Sent: Saturday, March 4, 2017 1:40 PM
To: 'James Bottomley' <[email protected]>; Stephen Hemminger <[email protected]>
Cc: Hannes Reinecke <[email protected]>; Christoph Hellwig <[email protected]>; Jens Axboe <[email protected]>; Linus Torvalds <torvalds@linux- foundation.org>; Martin K. Petersen <[email protected]>; Dexuan Cui <[email protected]>; Long Li <[email protected]>; Josh Poulson <[email protected]>; Adrian Suhov (Cloudbase Solutions SRL) <[email protected]>; [email protected]; Haiyang Zhang <[email protected]>
Subject: RE: [RFC] hv_storvsc: error handling.

Quoted 6 lines

-----Original Message-----
From: James Bottomley [mailto:[email protected]]
Sent: Saturday, March 4, 2017 1:37 PM
To: KY Srinivasan <[email protected]>; Stephen Hemminger <[email protected]>
Cc: Hannes Reinecke <[email protected]>; Christoph Hellwig <[email protected]>;

Jens

Quoted 4 lines

Axboe <[email protected]>; Linus Torvalds <torvalds@linux- foundation.org>; Martin K. Petersen <[email protected]>; Dexuan Cui <[email protected]>; Long Li <[email protected]>; Josh Poulson <[email protected]>; Adrian Suhov (Cloudbase Solutions

SRL)

Quoted 11 lines

<[email protected]>; [email protected]; Haiyang Zhang <[email protected]>
Subject: Re: [RFC] hv_storvsc: error handling.

Quoted 6 lines · KY Srinivasan

Quoted 6 lines

-----Original Message-----
From: Stephen Hemminger [mailto:[email protected]]
Sent: Friday, March 3, 2017 4:50 PM
To: James Bottomley <[email protected]>
Cc: Hannes Reinecke <[email protected]>; Christoph Hellwig

<[email protected]>;

Quoted 186 lines

Quoted 171 lines

Quoted 126 lines

James Bottomley <[email protected]>; Jens Axboe <[email protected]>; Linus Torvalds <[email protected]>; Martin K. Petersen <[email protected]>; KY Srinivasan <[email protected]>; Dexuan Cui <[email protected]>; Long Li <[email protected]>; Josh Poulson <[email protected]>; Adrian
Suhov (Cloudbase Solutions SRL) <[email protected]>; linux- [email protected]; Haiyang Zhang <[email protected]>
Subject: [RFC] hv_storvsc: error handling.

Needs more testing but this does fix the observed problem.

From: Stephen Hemminger <[email protected]>

Subject: [PATCH] hv_storvsc: fix error handling

The Hyper-V storvsc SCSI driver was hiding all errors in INQUIRY and
MODE_SENSE commands. This caused the scan process to incorrectly think
devices were present and online. Also invalid LUN errors were not being handled correctly.

This fixes problems booting a GEN2 VM on Hyper-V. It effectively reverts commit 4ed51a21c0f69 ("Staging: hv: storvsc: Fixup srb and scsi status for INQUIRY and MODE_SENSE")

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/scsi/storvsc_drv.c | 48 ++++------------------------------
------------
 1 file changed, 4 insertions(+), 44 deletions(-)

drivers/scsi/storvsc_drv.c +0 -0

index 638e5f427c90..8cc241fc54b8 100644
@@ -543,28 +543,6 @@ static void storvsc_host_scan(struct

work_struct
*work)

drivers/scsi/storvsc_drv.c +0 -23

 	kfree(wrk);
 }
 
-static void storvsc_remove_lun(struct work_struct *work)
-{
-	struct storvsc_scan_work *wrk;
-	struct scsi_device *sdev;
-
-	wrk = container_of(work, struct storvsc_scan_work, work);
-	if (!scsi_host_get(wrk->host))
-		goto done;
-
-	sdev = scsi_device_lookup(wrk->host, 0, wrk->tgt_id, wrk
->lun);
-
-	if (sdev) {
-		scsi_remove_device(sdev);
-		scsi_device_put(sdev);
-	}
-	scsi_host_put(wrk->host);
-
-done:
-	kfree(wrk);
-}
-
-
 /*
  * We can get incoming messages from the host that are not in

response to
* messages that we have sent out. An example of this would be
messages

drivers/scsi/storvsc_drv.c +1 -2

@@ -955,8 +933,7 @@ static void storvsc_handle_error(struct
 vmscsi_request *vm_srb,
 		}
 		break;
 	case SRB_STATUS_INVALID_LUN:
-		do_work = true;
-		process_err_fn = storvsc_remove_lun;
+		set_host_byte(scmnd, DID_NO_CONNECT);
 		break;
 	case SRB_STATUS_ABORTED:

if (vm_srb->srb_status &
SRB_STATUS_AUTOSENSE_VALID
&&

drivers/scsi/storvsc_drv.c +0 -9

@@ -1050,32 +1027,15 @@ static void storvsc_on_io_completion(struct
 storvsc_device *stor_device,
 
 	stor_pkt = &request->vstor_packet;
 
-	/*
-	 * The current SCSI handling on the host side does
-	 * not correctly handle:
-	 * INQUIRY command with page code parameter set to 0x80
-	 * MODE_SENSE command with cmd[2] == 0x1c
-	 *
-	 * Setup srb and scsi status so this won't be fatal.
-	 * We do this so we can distinguish truly fatal failues
-	 * (srb status == 0x4) and off-line the device in that

case.

drivers/scsi/storvsc_drv.c +0 -6

-	 */
-
-	if ((stor_pkt->vm_srb.cdb[0] == INQUIRY) ||
-	   (stor_pkt->vm_srb.cdb[0] == MODE_SENSE)) {
-		vstor_packet->vm_srb.scsi_status = 0;
-		vstor_packet->vm_srb.srb_status =

SRB_STATUS_SUCCESS;

drivers/scsi/storvsc_drv.c +3 -7

-	}
-
-
 	/* Copy over the status...etc */
 	stor_pkt->vm_srb.scsi_status = vstor_packet
->vm_srb.scsi_status;
 	stor_pkt->vm_srb.srb_status = vstor_packet
->vm_srb.srb_status;
 	stor_pkt->vm_srb.sense_info_length =
 	vstor_packet->vm_srb.sense_info_length;
 
-	if (vstor_packet->vm_srb.scsi_status != 0 ||
-	    vstor_packet->vm_srb.srb_status != SRB_STATUS_SUCCESS)
+	if (stor_pkt->vm_srb.cdb[0] != INQUIRY &&
+	    (vstor_packet->vm_srb.scsi_status != 0 ||
+	     vstor_packet->vm_srb.srb_status !=

SRB_STATUS_SUCCESS))

drivers/scsi/storvsc_drv.c +0 -0

 		storvsc_log(device, STORVSC_LOGGING_WARN,
 			"cmd 0x%x scsi status 0x%x srb status

Quoted 3 lines

0x%x\n",
stor_pkt->vm_srb.cdb[0],
--

This patch gets rid of the ability to "hot remove" LUNs. I don't think that can be part of any
solution. The INQUIRY hack I put in a long time ago was to deal with host bugs on prior versions of
Windows server. WS2016 should not be trigerring this code. Stephen, could you please test this patch -
a quick hack:

From b97f24f224a71a6e745c42e5640045a553eb407c Mon Sep 17 00:00:00 2001
From: K. Y. Srinivasan <[email protected]>
Date: Sat, 4 Mar 2017 14:00:46 -0700
Subject: [PATCH 1/1] scsi: storvsc: Fix a bug in LUN removal code
Reply-To: [email protected]

Signed-off-by K. Y. Srinivasan <[email protected]>

---
 drivers/scsi/storvsc_drv.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

drivers/scsi/storvsc_drv.c +1 -0

index 05526b7..27eb682 100644
@@ -885,6 +885,7 @@ static void storvsc_handle_error(struct
 vmscsi_request *vm_srb,
        struct storvsc_scan_work *wrk;
        void (*process_err_fn)(struct work_struct *work);
        bool do_work = false;
+       struct scsi_device *sdev;
 
        switch (SRB_STATUS(vm_srb->srb_status)) {

case SRB_STATUS_ERROR:

drivers/scsi/storvsc_drv.c +3 -0

@@ -911,6 +912,18 @@ static void storvsc_handle_error(struct
 vmscsi_request *vm_srb,
                }
                break;
        case SRB_STATUS_INVALID_LUN:
+               if (!scsi_host_get(host)) {
+                       set_host_byte(scmnd, DID_NO_CONNECT);
+                       break;

drivers/scsi/storvsc_drv.c static void storvsc_handle_error(struct · line 928

Quoted 10 lines

drivers/scsi/storvsc_drv.c +9 -0

+               }
+
+               sdev = scsi_device_lookup(wrk->host, 0, wrk->tgt_id,
 wrk->lun);
+
+               if (!sdev) {
+                       set_host_byte(scmnd, DID_NO_CONNECT);
+                       break;
+               }
+

You're now getting two references (one to the host and one to the device) that you don't put either in error handling or in storvsc_remove_lun().

Quoted 3 lines

Probably you should eliminate the scsi_host_get and scsi_device_lookup in storvsc_remove_lun() (making it basically remove device put device put host) and add a host put in the !sdev if above.

Yes; this was a quick hack to prove a theory. This should work (I hope); I will cleanup and resend once
Stephen verifies.

K. Y

Quoted 2 lines

James

Quoted 116 lines

-----Original Message-----
From: Stephen Hemminger [mailto:[email protected]]
Sent: Friday, March 3, 2017 4:50 PM
To: James Bottomley <[email protected]>
Cc: Hannes Reinecke <[email protected]>; Christoph Hellwig <[email protected]>; James Bottomley <[email protected]>; Jens Axboe <[email protected]>; Linus Torvalds <[email protected]>; Martin K. Petersen <[email protected]>; KY Srinivasan <[email protected]>; Dexuan Cui <[email protected]>; Long Li <[email protected]>; Josh Poulson <[email protected]>; Adrian Suhov (Cloudbase Solutions SRL) <[email protected]>; linux- [email protected]; Haiyang Zhang <[email protected]>
Subject: [RFC] hv_storvsc: error handling.

Needs more testing but this does fix the observed problem.

From: Stephen Hemminger <[email protected]>

Subject: [PATCH] hv_storvsc: fix error handling

The Hyper-V storvsc SCSI driver was hiding all errors in INQUIRY and MODE_SENSE commands. This caused the scan process to incorrectly think devices were present and online. Also invalid LUN errors were not being handled correctly.

This fixes problems booting a GEN2 VM on Hyper-V. It effectively reverts commit 4ed51a21c0f69 ("Staging: hv: storvsc: Fixup srb and scsi status for INQUIRY and MODE_SENSE")

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/scsi/storvsc_drv.c | 48 ++++------------------------------------------
 1 file changed, 4 insertions(+), 44 deletions(-)

drivers/scsi/storvsc_drv.c +0 -22

index 638e5f427c90..8cc241fc54b8 100644
@@ -543,28 +543,6 @@ static void storvsc_host_scan(struct work_struct
 *work)
 	kfree(wrk);
 }
 
-static void storvsc_remove_lun(struct work_struct *work)
-{
-	struct storvsc_scan_work *wrk;
-	struct scsi_device *sdev;
-
-	wrk = container_of(work, struct storvsc_scan_work, work);
-	if (!scsi_host_get(wrk->host))
-		goto done;
-
-	sdev = scsi_device_lookup(wrk->host, 0, wrk->tgt_id, wrk->lun);
-
-	if (sdev) {
-		scsi_remove_device(sdev);
-		scsi_device_put(sdev);
-	}
-	scsi_host_put(wrk->host);
-
-done:
-	kfree(wrk);
-}
-
-
 /*
  * We can get incoming messages from the host that are not in response to

* messages that we have sent out. An example of this would be messages

drivers/scsi/storvsc_drv.c +1 -2

@@ -955,8 +933,7 @@ static void storvsc_handle_error(struct
 vmscsi_request *vm_srb,
 		}
 		break;
 	case SRB_STATUS_INVALID_LUN:
-		do_work = true;
-		process_err_fn = storvsc_remove_lun;
+		set_host_byte(scmnd, DID_NO_CONNECT);
 		break;
 	case SRB_STATUS_ABORTED:

if (vm_srb->srb_status & SRB_STATUS_AUTOSENSE_VALID
&&

drivers/scsi/storvsc_drv.c +3 -20

@@ -1050,32 +1027,15 @@ static void storvsc_on_io_completion(struct
 storvsc_device *stor_device,
 
 	stor_pkt = &request->vstor_packet;
 
-	/*
-	 * The current SCSI handling on the host side does
-	 * not correctly handle:
-	 * INQUIRY command with page code parameter set to 0x80
-	 * MODE_SENSE command with cmd[2] == 0x1c
-	 *
-	 * Setup srb and scsi status so this won't be fatal.
-	 * We do this so we can distinguish truly fatal failues
-	 * (srb status == 0x4) and off-line the device in that case.
-	 */
-
-	if ((stor_pkt->vm_srb.cdb[0] == INQUIRY) ||
-	   (stor_pkt->vm_srb.cdb[0] == MODE_SENSE)) {
-		vstor_packet->vm_srb.scsi_status = 0;
-		vstor_packet->vm_srb.srb_status = SRB_STATUS_SUCCESS;
-	}
-
-
 	/* Copy over the status...etc */
 	stor_pkt->vm_srb.scsi_status = vstor_packet->vm_srb.scsi_status;
 	stor_pkt->vm_srb.srb_status = vstor_packet->vm_srb.srb_status;
 	stor_pkt->vm_srb.sense_info_length =
 	vstor_packet->vm_srb.sense_info_length;
 
-	if (vstor_packet->vm_srb.scsi_status != 0 ||
-	    vstor_packet->vm_srb.srb_status != SRB_STATUS_SUCCESS)
+	if (stor_pkt->vm_srb.cdb[0] != INQUIRY &&
+	    (vstor_packet->vm_srb.scsi_status != 0 ||
+	     vstor_packet->vm_srb.srb_status != SRB_STATUS_SUCCESS))
 		storvsc_log(device, STORVSC_LOGGING_WARN,
 			"cmd 0x%x scsi status 0x%x srb status 0x%x\n",

Quoted 2 lines

stor_pkt->vm_srb.cdb[0],

This patch gets rid of the ability to "hot remove" LUNs. I don't think that can be part of any solution. The INQUIRY hack I put in a long time ago was to deal with host bugs on prior versions of Windows server. WS2016 should not be trigerring this code. Stephen, could you please test this patch - a quick hack:

From b97f24f224a71a6e745c42e5640045a553eb407c Mon Sep 17 00:00:00 2001
From: K. Y. Srinivasan <[email protected]>
Date: Sat, 4 Mar 2017 14:00:46 -0700
Subject: [PATCH 1/1] scsi: storvsc: Fix a bug in LUN removal code
Reply-To: [email protected]

Signed-off-by K. Y. Srinivasan <[email protected]>

---
 drivers/scsi/storvsc_drv.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

drivers/scsi/storvsc_drv.c +6 -0

index 05526b7..27eb682 100644
@@ -885,6 +885,7 @@ static void storvsc_handle_error(struct vmscsi_request *vm_srb,
        struct storvsc_scan_work *wrk;
        void (*process_err_fn)(struct work_struct *work);
        bool do_work = false;
+       struct scsi_device *sdev;
 
        switch (SRB_STATUS(vm_srb->srb_status)) {
        case SRB_STATUS_ERROR:

@@ -911,6 +912,18 @@ static void storvsc_handle_error(struct vmscsi_request *vm_srb,
                }
                break;
        case SRB_STATUS_INVALID_LUN:
+               if (!scsi_host_get(host)) {
+                       set_host_byte(scmnd, DID_NO_CONNECT);
+                       break;
+               }
+

-----Original Message-----
From: Stephen Hemminger [mailto:[email protected]]
Sent: Monday, March 6, 2017 8:36 AM
To: KY Srinivasan <[email protected]>
Cc: James Bottomley <[email protected]>; Hannes Reinecke <[email protected]>; Christoph Hellwig <[email protected]>; James Bottomley <[email protected]>; Jens Axboe <[email protected]>; Linus Torvalds <[email protected]>; Martin K. Petersen <[email protected]>; Dexuan Cui <[email protected]>; Long Li <[email protected]>; Josh Poulson <[email protected]>; Adrian Suhov (Cloudbase Solutions SRL) <[email protected]>; linux- [email protected]; Haiyang Zhang <[email protected]>
Subject: Re: [RFC] hv_storvsc: error handling.

Quoted 10 lines · On Sat, 4 Mar 2017 21:03:41 +0000 KY Srinivasan <[email protected]> wrote:

Quoted 10 lines

-----Original Message-----
From: Stephen Hemminger [mailto:[email protected]]
Sent: Friday, March 3, 2017 4:50 PM
To: James Bottomley <[email protected]>
Cc: Hannes Reinecke <[email protected]>; Christoph Hellwig <[email protected]>; James Bottomley <[email protected]>; Jens Axboe <[email protected]>; Linus Torvalds <[email protected]>; Martin K. Petersen <[email protected]>; KY Srinivasan <[email protected]>; Dexuan Cui <[email protected]>; Long Li <[email protected]>; Josh Poulson <[email protected]>;

Adrian

Quoted 4 lines

Quoted 4 lines

Suhov (Cloudbase Solutions SRL) <[email protected]>; linux- [email protected]; Haiyang Zhang <[email protected]>
Subject: [RFC] hv_storvsc: error handling.

Quoted 8 lines

Quoted 8 lines

Needs more testing but this does fix the observed problem.

From: Stephen Hemminger <[email protected]>

Subject: [PATCH] hv_storvsc: fix error handling

The Hyper-V storvsc SCSI driver was hiding all errors in INQUIRY and MODE_SENSE commands. This caused the scan process to incorrectly

think

Quoted 37 lines

Quoted 37 lines

devices were present and online. Also invalid LUN errors were not being handled correctly.

This fixes problems booting a GEN2 VM on Hyper-V. It effectively reverts commit 4ed51a21c0f69 ("Staging: hv: storvsc: Fixup srb and scsi status for INQUIRY and MODE_SENSE")

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/scsi/storvsc_drv.c | 48 ++++------------------------------------------
 1 file changed, 4 insertions(+), 44 deletions(-)

drivers/scsi/storvsc_drv.c +0 -16

index 638e5f427c90..8cc241fc54b8 100644
@@ -543,28 +543,6 @@ static void storvsc_host_scan(struct work_struct
 *work)
 	kfree(wrk);
 }
 
-static void storvsc_remove_lun(struct work_struct *work)
-{
-	struct storvsc_scan_work *wrk;
-	struct scsi_device *sdev;
-
-	wrk = container_of(work, struct storvsc_scan_work, work);
-	if (!scsi_host_get(wrk->host))
-		goto done;
-
-	sdev = scsi_device_lookup(wrk->host, 0, wrk->tgt_id, wrk->lun);
-
-	if (sdev) {
-		scsi_remove_device(sdev);
-		scsi_device_put(sdev);
-	}
-	scsi_host_put(wrk->host);

Quoted 8 lines

drivers/scsi/storvsc_drv.c static void storvsc_host_scan(struct work_struct · line 548

Quoted 8 lines

drivers/scsi/storvsc_drv.c +0 -6

-
-done:
-	kfree(wrk);
-}
-
-
 /*
  * We can get incoming messages from the host that are not in response

to

Quoted 1 line

Quoted 1 line

* messages that we have sent out. An example of this would be

messages

Quoted 41 lines

Quoted 41 lines

drivers/scsi/storvsc_drv.c +1 -2

@@ -955,8 +933,7 @@ static void storvsc_handle_error(struct
 vmscsi_request *vm_srb,
 		}
 		break;
 	case SRB_STATUS_INVALID_LUN:
-		do_work = true;
-		process_err_fn = storvsc_remove_lun;
+		set_host_byte(scmnd, DID_NO_CONNECT);
 		break;
 	case SRB_STATUS_ABORTED:

if (vm_srb->srb_status & SRB_STATUS_AUTOSENSE_VALID
&&

drivers/scsi/storvsc_drv.c +0 -18

@@ -1050,32 +1027,15 @@ static void storvsc_on_io_completion(struct
 storvsc_device *stor_device,
 
 	stor_pkt = &request->vstor_packet;
 
-	/*
-	 * The current SCSI handling on the host side does
-	 * not correctly handle:
-	 * INQUIRY command with page code parameter set to 0x80
-	 * MODE_SENSE command with cmd[2] == 0x1c
-	 *
-	 * Setup srb and scsi status so this won't be fatal.
-	 * We do this so we can distinguish truly fatal failues
-	 * (srb status == 0x4) and off-line the device in that case.
-	 */
-
-	if ((stor_pkt->vm_srb.cdb[0] == INQUIRY) ||
-	   (stor_pkt->vm_srb.cdb[0] == MODE_SENSE)) {
-		vstor_packet->vm_srb.scsi_status = 0;
-		vstor_packet->vm_srb.srb_status = SRB_STATUS_SUCCESS;
-	}
-
-
 	/* Copy over the status...etc */
 	stor_pkt->vm_srb.scsi_status = vstor_packet->vm_srb.scsi_status;
 	stor_pkt->vm_srb.srb_status = vstor_packet->vm_srb.srb_status;
 	stor_pkt->vm_srb.sense_info_length =
 	vstor_packet->vm_srb.sense_info_length;
 

Quoted 11 lines

Quoted 9 lines

drivers/scsi/storvsc_drv.c +3 -2

-	if (vstor_packet->vm_srb.scsi_status != 0 ||
-	    vstor_packet->vm_srb.srb_status != SRB_STATUS_SUCCESS)
+	if (stor_pkt->vm_srb.cdb[0] != INQUIRY &&
+	    (vstor_packet->vm_srb.scsi_status != 0 ||
+	     vstor_packet->vm_srb.srb_status != SRB_STATUS_SUCCESS))
 		storvsc_log(device, STORVSC_LOGGING_WARN,
 			"cmd 0x%x scsi status 0x%x srb status 0x%x\n",

stor_pkt->vm_srb.cdb[0],
--

This patch gets rid of the ability to "hot remove" LUNs. I don't think that can

be part of any

Quoted 1 line

solution. The INQUIRY hack I put in a long time ago was to deal with host

bugs on prior versions of

Quoted 1 line

Windows server. WS2016 should not be trigerring this code. Stephen, could

you please test this patch -

Quoted 3 lines

a quick hack:

From b97f24f224a71a6e745c42e5640045a553eb407c Mon Sep 17 00:00:00

2001

drivers/scsi/storvsc_drv.c static void storvsc_handle_error(struct

Quoted 15 lines

From: K. Y. Srinivasan <[email protected]>
Date: Sat, 4 Mar 2017 14:00:46 -0700
Subject: [PATCH 1/1] scsi: storvsc: Fix a bug in LUN removal code
Reply-To: [email protected]

Signed-off-by K. Y. Srinivasan <[email protected]>

---
 drivers/scsi/storvsc_drv.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

drivers/scsi/storvsc_drv.c +0 -0

index 05526b7..27eb682 100644
@@ -885,6 +885,7 @@ static void storvsc_handle_error(struct

vmscsi_request *vm_srb,

drivers/scsi/storvsc_drv.c static void storvsc_handle_error(struct

Quoted 8 lines

drivers/scsi/storvsc_drv.c +1 -0

        struct storvsc_scan_work *wrk;
        void (*process_err_fn)(struct work_struct *work);
        bool do_work = false;
+       struct scsi_device *sdev;
 
        switch (SRB_STATUS(vm_srb->srb_status)) {
        case SRB_STATUS_ERROR:

@@ -911,6 +912,18 @@ static void storvsc_handle_error(struct

vmscsi_request *vm_srb,

Quoted 15 lines

drivers/scsi/storvsc_drv.c +12 -0

                }
                break;
        case SRB_STATUS_INVALID_LUN:
+               if (!scsi_host_get(host)) {
+                       set_host_byte(scmnd, DID_NO_CONNECT);
+                       break;
+               }
+
+               sdev = scsi_device_lookup(wrk->host, 0, wrk->tgt_id, wrk->lun);
+
+               if (!sdev) {
+                       set_host_byte(scmnd, DID_NO_CONNECT);
+                       break;
+               }
+

drivers/scsi/storvsc_drv.c static void storvsc_handle_error(struct vmscsi_request *vm_srb, · line 929

Quoted 3 lines

drivers/scsi/storvsc_drv.c +0 -0

                do_work = true;
                process_err_fn = storvsc_remove_lun;
                break;

I will try it, but it can't work for two reasons.
First, the INVALID_LUN error is masked off on INQUIRY in current code. Second, the scsi_device is instantiated already as part of scan probe process before it gets here.

The best solution so far is:
- remove old INQUIRY/SENSE error masking
+ add new workaround for INQUIRY of device id on LUN 0 which appears to be the reason for old masking
+ return errors on missing LUN
+ provide better transport services for hot remove (rather than detecting by failed I/O).

Quoted 5 lines

I will try it, but it can't work for two reasons.
First, the INVALID_LUN error is masked off on INQUIRY in current code. Second, the scsi_device is instantiated already as part of scan probe process before it gets here.

Was the invalid LUN in the LUN0 position. Inquiry of LUN0 support (when LUN0 is not populated) was added only recently to address host side issue.

Quoted 7 lines

The best solution so far is:
- remove old INQUIRY/SENSE error masking
+ add new workaround for INQUIRY of device id on LUN 0 which appears to be the reason for old masking
+ return errors on missing LUN
+ provide better transport services for hot remove (rather than detecting by failed I/O).

This the mechanism used by the host for notifying LUN removal - invalid LUN error code.

Was the invalid LUN in the LUN0 position. Inquiry of LUN0 support (when LUN0 is not populated) was added only recently to address host side issue.

-----Original Message-----
From: Christoph Hellwig [mailto:[email protected]]
Sent: Monday, March 6, 2017 9:06 PM
To: KY Srinivasan <[email protected]>
Cc: Stephen Hemminger <[email protected]>; James Bottomley <[email protected]>; Hannes Reinecke <[email protected]>; Christoph Hellwig <[email protected]>; James Bottomley <[email protected]>; Jens Axboe <[email protected]>; Linus Torvalds <[email protected]>; Martin K. Petersen <[email protected]>; Dexuan Cui <[email protected]>; Long Li <[email protected]>; Josh Poulson <[email protected]>; Adrian Suhov (Cloudbase Solutions SRL) <[email protected]>; linux- [email protected]; Haiyang Zhang <[email protected]>
Subject: Re: [RFC] hv_storvsc: error handling.

Quoted 1 line

Was the invalid LUN in the LUN0 position. Inquiry of LUN0 support (when

LUN0 is not populated)

Quoted 1 line

was added only recently to address host side issue.

How does HyperV expect device scanning to happen for a not populated LUN?

REPORT SUPPORTED LUNS but nothing else on LUN 0? Maybe a TEST UNIT READY
thrown? Or does it actually support the REPORT LUNS well known LU?

Quoted 3 lines · Stephen Hemminger

Quoted 1 line

	http://git.infradead.org/users/hch/block.git/commitdiff/148cff67b401e2229c076c0ea418712654be77e4  

It appears that is already in the code I am testing in linux-next...

It's in -next now, but it wasn't at the time you reported the bug.

And it would sortof explain the bug if the INQUIRY data is correct in the scatterlist, but we ignore it, given that scsi_probe_lun ignores the result based on sense data.

Can you check what happens with the horrible hack below:

drivers/scsi/scsi_scan.c +1 -1

index 6f7128f49c30..2f384ddb001a 100644
@@ -603,7 +603,7 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 		SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
 				"scsi scan: INQUIRY %s with code 0x%x\n",
 				result ? "failed" : "successful", result));
-
+#if 0
 		if (result) {
 			/*
 			 * not-ready to ready transition [asc/ascq=0x28/0x0]

@@ -628,6 +628,12 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 			if (resid == try_inquiry_len)
 				continue;

Quoted 12 lines · Stephen Hemminger

[    1.346023] hv_storvsc:  IO cmd 0x12 0x0 0x0 scsi status 0x0 srb
status 0x20 length 36
[    1.352913] inquiry data: 00000000: 00 aa be f1 5c 98 ff ff f0 64
02 89 ff ff ff ff
[    1.356543] inquiry data: 00000010: 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00
[    1.359996] inquiry data: 00000020: 00 00 00 00
[    1.361835] scsi host1: scsi scan: INQUIRY result too short (5),
using 36
[    1.361888] hv_storvsc:  IO cmd 0xa0 0x0 0x0 scsi status 0x0 srb
status 0x1 length 16
[    1.362307] hv_storvsc:  IO cmd 0x12 0x0 0x0 scsi status 0x0 srb

Quoted 8 lines

status 0x1 length 36

[    1.362308] inquiry data: 00000000: 00 23 34 f1 5c 98 ff ff f0 64
02 89 ff ff ff ff
[    1.362309] inquiry data: 00000010: 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00
[    1.362309] inquiry data: 00000020: 00 00 00 00
[    1.377423] scsi 1:0:0:1: Direct-Access                           
         PQ: 0 ANSI: 0  

Well, this pinpoints the fault to the block uncopy, I think. The Inquiry data is clearly correct in the page frame, so it's not getting copied to the scsi_execute() buffer for some reason.

James

Quoted 29 lines · On Tue, 28 Feb 2017 22:20:58 -0800 James Bottomley <[email protected]>…

Quoted 28 lines · Stephen Hemminger

[    1.346023] hv_storvsc:  IO cmd 0x12 0x0 0x0 scsi status 0x0
srb
status 0x20 length 36
[    1.352913] inquiry data: 00000000: 00 aa be f1 5c 98 ff ff f0
64
02 89 ff ff ff ff
[    1.356543] inquiry data: 00000010: 00 00 00 00 00 00 00 00 00
00
00 00 00 00 00 00
[    1.359996] inquiry data: 00000020: 00 00 00 00
[    1.361835] scsi host1: scsi scan: INQUIRY result too short
(5),
using 36
[    1.361888] hv_storvsc:  IO cmd 0xa0 0x0 0x0 scsi status 0x0
srb
status 0x1 length 16
[    1.362307] hv_storvsc:  IO cmd 0x12 0x0 0x0 scsi status 0x0
srb
status 0x1 length 36
[    1.362308] inquiry data: 00000000: 00 23 34 f1 5c 98 ff ff f0
64
02 89 ff ff ff ff
[    1.362309] inquiry data: 00000010: 00 00 00 00 00 00 00 00 00
00
00 00 00 00 00 00
[    1.362309] inquiry data: 00000020: 00 00 00 00
[    1.377423] scsi 1:0:0:1: Direct-Access                       

Quoted 9 lines

Quoted 1 line

         PQ: 0 ANSI: 0  

Well, this pinpoints the fault to the block uncopy, I think. The Inquiry data is clearly correct in the page frame, so it's not getting
copied to the scsi_execute() buffer for some reason.

James

Why do I see different sense data on good (4.10) versus bad (4.11)

Good 4.10 initial INQUIRY buffer

[    1.012413] data: 00000000: 00 2e 64 71 db 97 ff ff f0 94 62 96 ff
ff ff ff
[    1.012413] data: 00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00
[    1.012414] data: 00000020: 00 00 00 00

Bad 4.11 initial INQUIRY buffer

[    1.218159] data: 00000000: 00 00 05 02 1f 00 00 02 4d 73 66 74 20
20 20 20
[    1.225654] data: 00000010: 56 69 72 74 75 61 6c 20 44 69 73 6b 20
20 20 20
[    1.242930] data: 00000020: 31 2e 30 20

Is the kmap_atomic looking at the right place?

Quoted 67 lines · Stephen Hemminger

Quoted 34 lines · On Tue, 28 Feb 2017 22:20:58 -0800 James Bottomley <[email protected]>…

Quoted 33 lines · Stephen Hemminger

[    1.346023] hv_storvsc:  IO cmd 0x12 0x0 0x0 scsi status 0x0
srb
status 0x20 length 36
[    1.352913] inquiry data: 00000000: 00 aa be f1 5c 98 ff ff
f0
64
02 89 ff ff ff ff
[    1.356543] inquiry data: 00000010: 00 00 00 00 00 00 00 00
00
00
00 00 00 00 00 00
[    1.359996] inquiry data: 00000020: 00 00 00 00
[    1.361835] scsi host1: scsi scan: INQUIRY result too short
(5),
using 36
[    1.361888] hv_storvsc:  IO cmd 0xa0 0x0 0x0 scsi status 0x0
srb
status 0x1 length 16
[    1.362307] hv_storvsc:  IO cmd 0x12 0x0 0x0 scsi status 0x0
srb
status 0x1 length 36
[    1.362308] inquiry data: 00000000: 00 23 34 f1 5c 98 ff ff
f0
64
02 89 ff ff ff ff
[    1.362309] inquiry data: 00000010: 00 00 00 00 00 00 00 00
00
00
00 00 00 00 00 00
[    1.362309] inquiry data: 00000020: 00 00 00 00
[    1.377423] scsi 1:0:0:1: Direct-Access                     

Quoted 9 lines

Quoted 1 line

         PQ: 0 ANSI: 0  

Well, this pinpoints the fault to the block uncopy, I think. The Inquiry data is clearly correct in the page frame, so it's not getting
copied to the scsi_execute() buffer for some reason.

James

Why do I see different sense data on good (4.10) versus bad (4.11)

Good 4.10 initial INQUIRY buffer

[    1.012413] data: 00000000: 00 2e 64 71 db 97 ff ff f0 94 62 96
ff
ff ff ff
[    1.012413] data: 00000010: 00 00 00 00 00 00 00 00 00 00 00 00
00
00 00 00
[    1.012414] data: 00000020: 00 00 00 00

Bad 4.11 initial INQUIRY buffer

[    1.218159] data: 00000000: 00 00 05 02 1f 00 00 02 4d 73 66 74
20
20 20 20
[    1.225654] data: 00000010: 56 69 72 74 75 61 6c 20 44 69 73 6b
20
20 20 20
[    1.242930] data: 00000020: 31 2e 30 20

Quoted 1 line

Is the kmap_atomic looking at the right place?

Actually, the 4.11 data looks good. You can tell from the string at byte 8. It's rubbish in the 4.10 one and 'Msft ' in the 4.11 one (I assume you just reversed the cut and paste).

These should be the page physical addresses you sent down to the hypervisor, so kmap should work. Perhaps print out the physical page address so we see what we're getting.

Quoted 70 lines · James Bottomley

Quoted 67 lines · Stephen Hemminger

Quoted 34 lines · On Tue, 28 Feb 2017 22:20:58 -0800 James Bottomley <[email protected]>…

Quoted 33 lines · Stephen Hemminger

[    1.346023] hv_storvsc:  IO cmd 0x12 0x0 0x0 scsi status 0x0
srb
status 0x20 length 36
[    1.352913] inquiry data: 00000000: 00 aa be f1 5c 98 ff ff
f0
64
02 89 ff ff ff ff
[    1.356543] inquiry data: 00000010: 00 00 00 00 00 00 00 00
00
00
00 00 00 00 00 00
[    1.359996] inquiry data: 00000020: 00 00 00 00
[    1.361835] scsi host1: scsi scan: INQUIRY result too short
(5),
using 36
[    1.361888] hv_storvsc:  IO cmd 0xa0 0x0 0x0 scsi status 0x0
srb
status 0x1 length 16
[    1.362307] hv_storvsc:  IO cmd 0x12 0x0 0x0 scsi status 0x0
srb
status 0x1 length 36
[    1.362308] inquiry data: 00000000: 00 23 34 f1 5c 98 ff ff
f0
64
02 89 ff ff ff ff
[    1.362309] inquiry data: 00000010: 00 00 00 00 00 00 00 00
00
00
00 00 00 00 00 00
[    1.362309] inquiry data: 00000020: 00 00 00 00
[    1.377423] scsi 1:0:0:1: Direct-Access                     

Quoted 9 lines

Quoted 1 line

         PQ: 0 ANSI: 0    

Well, this pinpoints the fault to the block uncopy, I think. The Inquiry data is clearly correct in the page frame, so it's not getting
copied to the scsi_execute() buffer for some reason.

James

Why do I see different sense data on good (4.10) versus bad (4.11)

Good 4.10 initial INQUIRY buffer

[    1.012413] data: 00000000: 00 2e 64 71 db 97 ff ff f0 94 62 96
ff
ff ff ff
[    1.012413] data: 00000010: 00 00 00 00 00 00 00 00 00 00 00 00
00
00 00 00
[    1.012414] data: 00000020: 00 00 00 00

Bad 4.11 initial INQUIRY buffer

[    1.218159] data: 00000000: 00 00 05 02 1f 00 00 02 4d 73 66 74
20
20 20 20
[    1.225654] data: 00000010: 56 69 72 74 75 61 6c 20 44 69 73 6b
20
20 20 20
[    1.242930] data: 00000020: 31 2e 30 20

Quoted 1 line

Is the kmap_atomic looking at the right place?

Quoted 7 lines

Actually, the 4.11 data looks good. You can tell from the string at byte 8. It's rubbish in the 4.10 one and 'Msft ' in the 4.11 one (I assume you just reversed the cut and paste).

These should be the page physical addresses you sent down to the hypervisor, so kmap should work. Perhaps print out the physical page address so we see what we're getting.

Another possible explanation is non zero sg->offset, in which case you might not see the INQUIRY data because you start at the beginning of the page. This shouldn't happen because you specify no alignment override in the driver, so we should start the INQUIRY buffer on a new page and copy the data back to the real buffer, but perhaps that's what changed.

Ok here is much better data, wasn't accounting for the offset in the payload

Working 4.10

[    1.048920] hv_storvsc: INQUIRY cmd 0x12 0x0 0x0 scsi status 0x0
srb status 0x20 length 36
[    1.048920] hv_storvsc:  payload size 288 count 1 offset 1024 len
36 pfn 0x1f15d7
[    1.048921] hv_storvsc:  sg 0: phys 0x1f15d7000 virt
ffff9270b15d7000 offset 0x400 length 36
[    1.048922] data: 00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00  ................
[    1.048923] data: 00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00  ................
[    1.048923] data: 00000020: 00 00 00 00                           
           ....
[    1.048942] scsi host1: scsi scan: INQUIRY result too short (5),

using 36

Broken 4.11

[    1.812926] hv_storvsc: INQUIRY cmd 0x12 0x0 0x0 scsi status 0x0
srb status 0x20 length 36
[    1.812927] hv_storvsc:  payload size 288 count 1 offset 2816 len
36 pfn 0x1f198b
[    1.812928] hv_storvsc:  sg 0: phys 0x1f198b000 virt
ffff9a42f198b000 offset 0xb00 length 36
[    1.812953] data: 00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00  ................
[    1.812953] data: 00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00  ................
[    1.812954] data: 00000020: 00 00 00 00                           
           ....
[    1.813258] scsi host1: scsi scan: INQUIRY result too short (5),

using 36

Quoted 2 lines · Stephen Hemminger

Ok here is much better data, wasn't accounting for the offset in the payload

But now both responses are the same:

Quoted 1 line

Working 4.10

[...]

Quoted 17 lines

[    1.048920] hv_storvsc: INQUIRY cmd 0x12 0x0 0x0 scsi status 0x0
srb status 0x20 length 36
[    1.048920] hv_storvsc:  payload size 288 count 1 offset 1024 len
36 pfn 0x1f15d7
[    1.048921] hv_storvsc:  sg 0: phys 0x1f15d7000 virt
ffff9270b15d7000 offset 0x400 length 36
[    1.048922] data: 00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00  ................
[    1.048923] data: 00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00  ................
[    1.048923] data: 00000020: 00 00 00 00                           
           ....
[    1.048942] scsi host1: scsi scan: INQUIRY result too short (5),

using 36

Broken 4.11

[...]

Quoted 13 lines

[    1.812926] hv_storvsc: INQUIRY cmd 0x12 0x0 0x0 scsi status 0x0
srb status 0x20 length 36
[    1.812927] hv_storvsc:  payload size 288 count 1 offset 2816 len
36 pfn 0x1f198b
[    1.812928] hv_storvsc:  sg 0: phys 0x1f198b000 virt
ffff9a42f198b000 offset 0xb00 length 36
[    1.812953] data: 00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00  ................
[    1.812953] data: 00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00  ................
[    1.812954] data: 00000020: 00 00 00 00                           
           ....
[    1.813258] scsi host1: scsi scan: INQUIRY result too short (5),

Quoted 1 line

using 36

Hyper-v is returning a buffer of zeros for INQUIRY to the offline device in both cases ... that means the problem isn't where I thought it was.

James

Bad 4.11 initial INQUIRY buffer

[    1.218159] data: 00000000: 00 00 05 02 1f 00 00 02 4d 73 66 74 20 20 20 20
[    1.225654] data: 00000010: 56 69 72 74 75 61 6c 20 44 69 73 6b 20 20 20 20
[    1.242930] data: 00000020: 31 2e 30 20

Quoted 6 lines · Stephen Hemminger

Fixes ee5242360424 ("scsi: zero per-cmd driver data before each I/O")

but that is already in linux-next.

Noticed another place where memset(of the data was being done not the extra bits. Tried this, but didn't fix it either...

Are you using blk-mq or the legacy request code?