[PATCH] scsi: storvsc: fix SRB_STATUS_ABORTED handling

[Vitaly Kuznetsov]

Commit 3209f9d780d1 ("scsi: storvsc: Fix a bug in the handling of SRB
status flags") filtered SRB_STATUS_AUTOSENSE_VALID out effectively making
the (SRB_STATUS_ABORTED | SRB_STATUS_AUTOSENSE_VALID) case a dead code. The
logic from this branch (e.g. storvsc_device_scan() call) is still required,
fix the check.

Fixes: 3209f9d780d1 ("scsi: storvsc: Fix a bug in the handling of SRB status 
flags")
Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
---
 drivers/scsi/storvsc_drv.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 292c04e..3ddcabb 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -914,8 +914,9 @@ static void storvsc_handle_error(struct vmscsi_request 
*vm_srb,
do_work = true;
process_err_fn = storvsc_remove_lun;
break;
-   case (SRB_STATUS_ABORTED | SRB_STATUS_AUTOSENSE_VALID):
-   if ((asc == 0x2a) && (ascq == 0x9)) {
+   case SRB_STATUS_ABORTED:
+   if (vm_srb->srb_status & SRB_STATUS_AUTOSENSE_VALID &&
+   (asc == 0x2a) && (ascq == 0x9)) {
do_work = true;
process_err_fn = storvsc_device_scan;
/*
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] storvsc: add more logging for error and warning messages

[Vitaly Kuznetsov]

Long Li  writes:

> Introduce a logging level for storvsc to log certain error/warning
> messages. Those messages are helpful in some environments,
> e.g. Microsoft Azure, for customer support and troubleshooting
> purposes.

I have an alternative suggestion: let's use dynamic debug! Basically, we
need to convert all non-error logging to using dev_dbg() and this can be
enabled dynamically when needed, even reboot won't be required.

>
> Signed-off-by: Long Li 
> ---
>  drivers/scsi/storvsc_drv.c | 30 +-
>  1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 40c43ae..afa1647 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -164,6 +164,21 @@ static int sense_buffer_size = 
> PRE_WIN8_STORVSC_SENSE_BUFFER_SIZE;
>  */
>  static int vmstor_proto_version;
>
> +#define STORVSC_LOGGING_NONE 0
> +#define STORVSC_LOGGING_ERROR1
> +#define STORVSC_LOGGING_WARN 2
> +
> +static int logging_level = STORVSC_LOGGING_ERROR;
> +module_param(logging_level, int, S_IRUGO|S_IWUSR);
> +MODULE_PARM_DESC(logging_level,
> + "Logging level, 0 - None, 1 - Error (default), 2 - Warning.");
> +
> +static inline bool do_logging(int level)
> +{
> + return (logging_level >= level) ? true : false;
> +}
> +
> +
>  struct vmscsi_win8_extension {
>   /*
>* The following were added in Windows 8
> @@ -1183,7 +1198,7 @@ static void storvsc_command_completion(struct 
> storvsc_cmd_request *cmd_request)
>
>   scmnd->result = vm_srb->scsi_status;
>
> - if (scmnd->result) {
> + if (scmnd->result && do_logging(STORVSC_LOGGING_ERROR)) {
>   if (scsi_normalize_sense(scmnd->sense_buffer,
>   SCSI_SENSE_BUFFERSIZE, _hdr))
>   scsi_print_sense_hdr(scmnd->device, "storvsc",
> @@ -1239,12 +1254,25 @@ static void storvsc_on_io_completion(struct hv_device 
> *device,
>   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 (do_logging(STORVSC_LOGGING_WARN))
> + dev_warn(>device,
> + "cmd 0x%x scsi status 0x%x srb status 0x%x\n",
> + stor_pkt->vm_srb.cdb[0],
> + vstor_packet->vm_srb.scsi_status,
> + vstor_packet->vm_srb.srb_status);
>
>   if ((vstor_packet->vm_srb.scsi_status & 0xFF) == 0x02) {
>   /* CHECK_CONDITION */
>   if (vstor_packet->vm_srb.srb_status &
>   SRB_STATUS_AUTOSENSE_VALID) {
>   /* autosense data available */
> + if (do_logging(STORVSC_LOGGING_WARN))
> + dev_warn(>device,
> + "stor pkt %p autosense data valid - len 
> %d\n",
> + request,
> + vstor_packet->vm_srb.sense_info_length);
>
>   memcpy(request->cmd->sense_buffer,
>  vstor_packet->vm_srb.sense_data,

-- 
  Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 RESEND_2] scsi: report 'INQUIRY result too short' once per host

[Vitaly Kuznetsov]

Some host adapters (e.g. Hyper-V storvsc) are known for not respecting the
SPC-2/3/4 requirement for 'INQUIRY data (see table ...) shall contain at
least 36 bytes'. As a result we get tons on 'scsi 0:7:1:1: scsi scan:
INQUIRY result too short (5), using 36' messages on console. This can be
problematic for slow consoles. Introduce short_inquiry flag in struct
Scsi_Host to print the message once per host.

Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
---
Changes since v3 RESEND:
- No changes, this is just a RESEND.

Changes since v3:
- No changes, this is just a RESEND.

Changes since v2:
- This is a successor of previously sent (and still not merged) "scsi:
  introduce short_inquiry flag for broken host adapters" patch. I'm not
  particularly sure which solution is better but I'm leaning towards this
  one as it doesn't require changes to adapter drivers.
---
 drivers/scsi/scsi_scan.c | 9 ++---
 include/scsi/scsi_host.h | 3 +++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 8324539..054923e 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -701,9 +701,12 @@ static int scsi_probe_lun(struct scsi_device *sdev, 
unsigned char *inq_result,
 * strings.
 */
if (sdev->inquiry_len < 36) {
-   sdev_printk(KERN_INFO, sdev,
-   "scsi scan: INQUIRY result too short (%d),"
-   " using 36\n", sdev->inquiry_len);
+   if (!sdev->host->short_inquiry) {
+   shost_printk(KERN_INFO, sdev->host,
+   "scsi scan: INQUIRY result too short (%d),"
+   " using 36\n", sdev->inquiry_len);
+   sdev->host->short_inquiry = 1;
+   }
sdev->inquiry_len = 36;
}
 
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index ed52712..fcfa3d7 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -668,6 +668,9 @@ struct Scsi_Host {
unsigned use_blk_mq:1;
unsigned use_cmd_list:1;
 
+   /* Host responded with short (<36 bytes) INQUIRY result */
+   unsigned short_inquiry:1;
+
/*
 * Optional work queue to be utilized by the transport
 */
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RESEND_2] scsi_sysfs: protect against double execution of __scsi_remove_device()

[Vitaly Kuznetsov]

On some host errors storvsc module tries to remove sdev by scheduling a job
which does the following:

   sdev = scsi_device_lookup(wrk->host, 0, 0, wrk->lun);
   if (sdev) {
   scsi_remove_device(sdev);
   scsi_device_put(sdev);
   }

While this code seems correct the following crash is observed:

 general protection fault:  [#1] SMP DEBUG_PAGEALLOC
 RIP: 0010:[]  [] bdi_destroy+0x39/0x220
 ...
 [] ? _raw_spin_unlock_irq+0x2c/0x40
 [] blk_cleanup_queue+0x17b/0x270
 [] __scsi_remove_device+0x54/0xd0 [scsi_mod]
 [] scsi_remove_device+0x2b/0x40 [scsi_mod]
 [] storvsc_remove_lun+0x3d/0x60 [hv_storvsc]
 [] process_one_work+0x1b1/0x530
 ...

The problem comes with the fact that many such jobs (for the same device)
are being scheduled simultaneously. While scsi_remove_device() uses
shost->scan_mutex and scsi_device_lookup() will fail for a device in
SDEV_DEL state there is no protection against someone who did
scsi_device_lookup() before we actually entered __scsi_remove_device(). So
the whole scenario looks like that: two callers do simultaneous (or
preemption happens) calls to scsi_device_lookup() ant these calls succeed
for both of them, after that they try doing scsi_remove_device().
shost->scan_mutex only serializes their calls to __scsi_remove_device()
and we end up doing the cleanup path twice.

Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
---
 drivers/scsi/scsi_sysfs.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 8d23122..905dd1c 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1102,6 +1102,14 @@ void __scsi_remove_device(struct scsi_device *sdev)
 {
struct device *dev = >sdev_gendev;
 
+   /*
+* This cleanup path is not reentrant and while it is impossible
+* to get a new reference with scsi_device_get() someone can still
+* hold a previously acquired one.
+*/
+   if (sdev->sdev_state == SDEV_DEL)
+   return;
+
if (sdev->is_visible) {
if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
return;
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RESEND] scsi_sysfs: protect against double execution of __scsi_remove_device()

[Vitaly Kuznetsov]

On some host errors storvsc module tries to remove sdev by scheduling a job
which does the following:

   sdev = scsi_device_lookup(wrk->host, 0, 0, wrk->lun);
   if (sdev) {
   scsi_remove_device(sdev);
   scsi_device_put(sdev);
   }

While this code seems correct the following crash is observed:

 general protection fault:  [#1] SMP DEBUG_PAGEALLOC
 RIP: 0010:[]  [] bdi_destroy+0x39/0x220
 ...
 [] ? _raw_spin_unlock_irq+0x2c/0x40
 [] blk_cleanup_queue+0x17b/0x270
 [] __scsi_remove_device+0x54/0xd0 [scsi_mod]
 [] scsi_remove_device+0x2b/0x40 [scsi_mod]
 [] storvsc_remove_lun+0x3d/0x60 [hv_storvsc]
 [] process_one_work+0x1b1/0x530
 ...

The problem comes with the fact that many such jobs (for the same device)
are being scheduled simultaneously. While scsi_remove_device() uses
shost->scan_mutex and scsi_device_lookup() will fail for a device in
SDEV_DEL state there is no protection against someone who did
scsi_device_lookup() before we actually entered __scsi_remove_device(). So
the whole scenario looks like that: two callers do simultaneous (or
preemption happens) calls to scsi_device_lookup() ant these calls succeed
for both of them, after that they try doing scsi_remove_device().
shost->scan_mutex only serializes their calls to __scsi_remove_device()
and we end up doing the cleanup path twice.

Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
---
 drivers/scsi/scsi_sysfs.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index dff8faf..3b7e2bb 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1078,6 +1078,14 @@ void __scsi_remove_device(struct scsi_device *sdev)
 {
struct device *dev = >sdev_gendev;
 
+   /*
+* This cleanup path is not reentrant and while it is impossible
+* to get a new reference with scsi_device_get() someone can still
+* hold a previously acquired one.
+*/
+   if (sdev->sdev_state == SDEV_DEL)
+   return;
+
if (sdev->is_visible) {
if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
return;
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RESEND] scsi_scan: don't dump trace when scsi_prep_async_scan() is called twice

[Vitaly Kuznetsov]

The only user of scsi_prep_async_scan() is scsi_scan_host() and it handles
the situation correctly. Move 'called twice' reporting to debug level as
well.

The issue is observed on Hyper-V: on any device add/remove event storvsc
driver calls scsi_scan_host() and in case previous scan is still running
we get the message and stack dump on console.

Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
Reviewed-by: K. Y. Srinivasan <k...@microsoft.com>
Tested-by: Alex Ng <ale...@microsoft.com>
Signed-off-by: K. Y. Srinivasan <k...@microsoft.com>
---
Changes since v1:
- No changes, this is just a resend.
---
 drivers/scsi/scsi_scan.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index f9f3f82..01ad016 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1712,8 +1712,7 @@ static struct async_scan_data 
*scsi_prep_async_scan(struct Scsi_Host *shost)
return NULL;
 
if (shost->async_scan) {
-   shost_printk(KERN_INFO, shost, "%s called twice\n", __func__);
-   dump_stack();
+   shost_printk(KERN_DEBUG, shost, "%s called twice\n", __func__);
return NULL;
}
 
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 RESEND] scsi: report 'INQUIRY result too short' once per host

[Vitaly Kuznetsov]

Some host adapters (e.g. Hyper-V storvsc) are known for not respecting the
SPC-2/3/4 requirement for 'INQUIRY data (see table ...) shall contain at
least 36 bytes'. As a result we get tons on 'scsi 0:7:1:1: scsi scan:
INQUIRY result too short (5), using 36' messages on console. This can be
problematic for slow consoles. Introduce short_inquiry flag in struct
Scsi_Host to print the message once per host.

Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
---
Changes since v3 RESEND:
- Sorry, I screwed up James address when sending 'v3 RESEND'.

Changes since v3:
- no changes, this is just a RESEND.

Changes since v2:
- This is a successor of previously sent (and still not merged) "scsi:
  introduce short_inquiry flag for broken host adapters" patch. I'm not
  particularly sure which solution is better but I'm leaning towards this
  one as it doesn't require changes to adapter drivers.
---
 drivers/scsi/scsi_scan.c | 9 ++---
 include/scsi/scsi_host.h | 3 +++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index f9f3f82..cd347e4 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -701,9 +701,12 @@ static int scsi_probe_lun(struct scsi_device *sdev, 
unsigned char *inq_result,
 * strings.
 */
if (sdev->inquiry_len < 36) {
-   sdev_printk(KERN_INFO, sdev,
-   "scsi scan: INQUIRY result too short (%d),"
-   " using 36\n", sdev->inquiry_len);
+   if (!sdev->host->short_inquiry) {
+   shost_printk(KERN_INFO, sdev->host,
+   "scsi scan: INQUIRY result too short (%d),"
+   " using 36\n", sdev->inquiry_len);
+   sdev->host->short_inquiry = 1;
+   }
sdev->inquiry_len = 36;
}
 
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index e113c75..3a22da7 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -673,6 +673,9 @@ struct Scsi_Host {
unsigned use_blk_mq:1;
unsigned use_cmd_list:1;
 
+   /* Host responded with short (<36 bytes) INQUIRY result */
+   unsigned short_inquiry:1;
+
/*
 * Optional work queue to be utilized by the transport
 */
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 RESEND] scsi: report 'INQUIRY result too short' once per host

[Vitaly Kuznetsov]

Some host adapters (e.g. Hyper-V storvsc) are known for not respecting the
SPC-2/3/4 requirement for 'INQUIRY data (see table ...) shall contain at
least 36 bytes'. As a result we get tons on 'scsi 0:7:1:1: scsi scan:
INQUIRY result too short (5), using 36' messages on console. This can be
problematic for slow consoles. Introduce short_inquiry flag in struct
Scsi_Host to print the message once per host.

Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
---
Changes since v3:
- no changes, this is just a RESEND.

Changes since v2:
- This is a successor of previously sent (and still not merged) "scsi:
  introduce short_inquiry flag for broken host adapters" patch. I'm not
  particularly sure which solution is better but I'm leaning towards this
  one as it doesn't require changes to adapter drivers.
---
 drivers/scsi/scsi_scan.c | 9 ++---
 include/scsi/scsi_host.h | 3 +++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index f9f3f82..cd347e4 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -701,9 +701,12 @@ static int scsi_probe_lun(struct scsi_device *sdev, 
unsigned char *inq_result,
 * strings.
 */
if (sdev->inquiry_len < 36) {
-   sdev_printk(KERN_INFO, sdev,
-   "scsi scan: INQUIRY result too short (%d),"
-   " using 36\n", sdev->inquiry_len);
+   if (!sdev->host->short_inquiry) {
+   shost_printk(KERN_INFO, sdev->host,
+   "scsi scan: INQUIRY result too short (%d),"
+   " using 36\n", sdev->inquiry_len);
+   sdev->host->short_inquiry = 1;
+   }
sdev->inquiry_len = 36;
}
 
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index e113c75..3a22da7 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -673,6 +673,9 @@ struct Scsi_Host {
unsigned use_blk_mq:1;
unsigned use_cmd_list:1;
 
+   /* Host responded with short (<36 bytes) INQUIRY result */
+   unsigned short_inquiry:1;
+
/*
 * Optional work queue to be utilized by the transport
 */
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] scsi_sysfs: protect against double execution of __scsi_remove_device()

[Vitaly Kuznetsov]

Bart Van Assche <bart.vanass...@sandisk.com> writes:

> On 10/22/2015 10:12 AM, Vitaly Kuznetsov wrote:
>> On some host errors storvsc module tries to remove sdev by scheduling a job
>> which does the following:
>>
>> sdev = scsi_device_lookup(wrk->host, 0, 0, wrk->lun);
>> if (sdev) {
>> scsi_remove_device(sdev);
>> scsi_device_put(sdev);
>> }
>>
>> While this code seems correct the following crash is observed:
>>
>>   general protection fault:  [#1] SMP DEBUG_PAGEALLOC
>>   RIP: 0010:[]  [] bdi_destroy+0x39/0x220
>>   ...
>>   [] ? _raw_spin_unlock_irq+0x2c/0x40
>>   [] blk_cleanup_queue+0x17b/0x270
>>   [] __scsi_remove_device+0x54/0xd0 [scsi_mod]
>>   [] scsi_remove_device+0x2b/0x40 [scsi_mod]
>>   [] storvsc_remove_lun+0x3d/0x60 [hv_storvsc]
>>   [] process_one_work+0x1b1/0x530
>>   ...
>>
>> The problem comes with the fact that many such jobs (for the same device)
>> are being scheduled simultaneously. While scsi_remove_device() uses
>> shost->scan_mutex and scsi_device_lookup() will fail for a device in
>> SDEV_DEL state there is no protection against someone who did
>> scsi_device_lookup() before we actually entered __scsi_remove_device(). So
>> the whole scenario looks like that: two callers do simultaneous (or
>> preemption happens) calls to scsi_device_lookup() ant these calls succeed
>> for all of them, after that both callers try doing scsi_remove_device().
>> shost->scan_mutex only serializes their calls to __scsi_remove_device()
>> and we end up doing the cleanup path twice.
>>
>> Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
>> ---
>>   drivers/scsi/scsi_sysfs.c | 8 
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>> index b89..e0d2707 100644
>> --- a/drivers/scsi/scsi_sysfs.c
>> +++ b/drivers/scsi/scsi_sysfs.c
>> @@ -1076,6 +1076,14 @@ void __scsi_remove_device(struct scsi_device *sdev)
>>   {
>>  struct device *dev = >sdev_gendev;
>>
>> +/*
>> + * This cleanup path is not reentrant and while it is impossible
>> + * to get a new reference with scsi_device_get() someone can still
>> + * hold a previously acquired one.
>> + */
>> +if (sdev->sdev_state == SDEV_DEL)
>> +return;
>> +
>>  if (sdev->is_visible) {
>>  if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
>>  return;
>
> Hello Vitaly,
>
> Sorry but I don't see how the above patch could be a proper fix. If
> two calls to __scsi_remove_device() occur concurrently the crash
> explained above can still occur. The storsvc driver should be modified
> such that concurrent __scsi_remove_device() calls do not occur. How
> about preventing concurrent calls via a mutex ?

Nobody is supposed to call __scsi_remove_device() without holding
shost->scan_mutex and scsi_remove_device() does that. Here I'm trying to
protect against two *consequent* calls to the __scsi_remove_device(). As
we set sdev_state to SDEV_DEL on the cleanup path checking it should be
enough.

> Another possible
> approach is to use the workqueue mechanism. An example can be found in
> the SRP initiator driver (ib_srp).

Yes, but I think the existent approach is good enough:
1) Every caller is supposed to get a reference to the device with
scsi_device_get() (scsi_device_lookup() does that).
2) shost->scan_mutex is suppose to be held by all __scsi_remove_device()
callers.

-- 
  Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] scsi_sysfs: protect against double execution of __scsi_remove_device()

[Vitaly Kuznetsov]

On some host errors storvsc module tries to remove sdev by scheduling a job
which does the following:

   sdev = scsi_device_lookup(wrk->host, 0, 0, wrk->lun);
   if (sdev) {
   scsi_remove_device(sdev);
   scsi_device_put(sdev);
   }

While this code seems correct the following crash is observed:

 general protection fault:  [#1] SMP DEBUG_PAGEALLOC
 RIP: 0010:[]  [] bdi_destroy+0x39/0x220
 ...
 [] ? _raw_spin_unlock_irq+0x2c/0x40
 [] blk_cleanup_queue+0x17b/0x270
 [] __scsi_remove_device+0x54/0xd0 [scsi_mod]
 [] scsi_remove_device+0x2b/0x40 [scsi_mod]
 [] storvsc_remove_lun+0x3d/0x60 [hv_storvsc]
 [] process_one_work+0x1b1/0x530
 ...

The problem comes with the fact that many such jobs (for the same device)
are being scheduled simultaneously. While scsi_remove_device() uses
shost->scan_mutex and scsi_device_lookup() will fail for a device in
SDEV_DEL state there is no protection against someone who did
scsi_device_lookup() before we actually entered __scsi_remove_device(). So
the whole scenario looks like that: two callers do simultaneous (or
preemption happens) calls to scsi_device_lookup() ant these calls succeed
for all of them, after that both callers try doing scsi_remove_device().
shost->scan_mutex only serializes their calls to __scsi_remove_device()
and we end up doing the cleanup path twice.

Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
---
 drivers/scsi/scsi_sysfs.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index b89..e0d2707 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1076,6 +1076,14 @@ void __scsi_remove_device(struct scsi_device *sdev)
 {
struct device *dev = >sdev_gendev;
 
+   /*
+* This cleanup path is not reentrant and while it is impossible
+* to get a new reference with scsi_device_get() someone can still
+* hold a previously acquired one.
+*/
+   if (sdev->sdev_state == SDEV_DEL)
+   return;
+
if (sdev->is_visible) {
if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
return;
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] scsi: report 'INQUIRY result too short' once per host

[Vitaly Kuznetsov]

Hannes Reinecke <h...@suse.de> writes:

> On 10/12/2015 05:16 PM, Vitaly Kuznetsov wrote:
>> Hannes Reinecke <h...@suse.de> writes:
>> 
>>> On 10/08/2015 06:54 PM, Vitaly Kuznetsov wrote:
>>>> Some host adapters (e.g. Hyper-V storvsc) are known for not respecting the
>>>> SPC-2/3/4 requirement for 'INQUIRY data (see table ...) shall contain at
>>>> least 36 bytes'. As a result we get tons on 'scsi 0:7:1:1: scsi scan:
>>>> INQUIRY result too short (5), using 36' messages on console. This can be
>>>> problematic for slow consoles. Introduce short_inquiry flag in struct
>>>> Scsi_Host to print the message once per host.
>>>>
>>>> Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
>>>> ---
>>>> Changes since v2:
>>>> - This is a successor of previously sent (and still not merged) "scsi:
>>>>   introduce short_inquiry flag for broken host adapters" patch. I'm not
>>>>   particularly sure which solution is better but I'm leaning towards this
>>>>   one as it doesn't require changes to adapter drivers.
>>>> ---
>>>>  drivers/scsi/scsi_scan.c | 9 ++---
>>>>  include/scsi/scsi_host.h | 3 +++
>>>>  2 files changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
>>>> index f9f3f82..cd347e4 100644
>>>> --- a/drivers/scsi/scsi_scan.c
>>>> +++ b/drivers/scsi/scsi_scan.c
>>>> @@ -701,9 +701,12 @@ static int scsi_probe_lun(struct scsi_device *sdev, 
>>>> unsigned char *inq_result,
>>>> * strings.
>>>> */
>>>>if (sdev->inquiry_len < 36) {
>>>> -  sdev_printk(KERN_INFO, sdev,
>>>> -  "scsi scan: INQUIRY result too short (%d),"
>>>> -  " using 36\n", sdev->inquiry_len);
>>>> +  if (!sdev->host->short_inquiry) {
>>>> +  shost_printk(KERN_INFO, sdev->host,
>>>> +  "scsi scan: INQUIRY result too short (%d),"
>>>> +  " using 36\n", sdev->inquiry_len);
>>>> +  sdev->host->short_inquiry = 1;
>>>> +  }
>>>>sdev->inquiry_len = 36;
>>>>}
>>>>  
>>> At least you need to check if you've received any valid data here;
>>> 'INQUIRY result too short' is also a common error if the interrupt
>>> is hosed when trying to access the device.
>>> So please check for 'inquiry_len > 4' before setting 'short_inquiry'.
>> 
>> Currently we proceed even with a shorter reply... Should we abort the
>> scan (and return -EOI?) in case we got inquiry_len <= 4?
>> 
> Yes please. We need to ensure that we actually received some data,
> and not running into an error scenario here.
> So we need to read at least five bytes of data, as byte 4 carries
> the response length. If we read less than that we have no way of
> figuring out if the response data is even remotely sane.
>

I just checked and it seems such check would be redundant. inquiry_len
is always >=5 as we have the following code:

response_len = inq_result[4] + 5;

where inq_result is unsigned char *

After that we do:
sdev->inquiry_len = min(try_inquiry_len, response_len);

As far as I can see try_inquiry_len can be lower than 36 only in case
some driver sets it manually. I don't see a '>=5' check for it but I'm
not sure it is needed.

-- 
  Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] scsi: report 'INQUIRY result too short' once per host

[Vitaly Kuznetsov]

Hannes Reinecke <h...@suse.de> writes:

> On 10/08/2015 06:54 PM, Vitaly Kuznetsov wrote:
>> Some host adapters (e.g. Hyper-V storvsc) are known for not respecting the
>> SPC-2/3/4 requirement for 'INQUIRY data (see table ...) shall contain at
>> least 36 bytes'. As a result we get tons on 'scsi 0:7:1:1: scsi scan:
>> INQUIRY result too short (5), using 36' messages on console. This can be
>> problematic for slow consoles. Introduce short_inquiry flag in struct
>> Scsi_Host to print the message once per host.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
>> ---
>> Changes since v2:
>> - This is a successor of previously sent (and still not merged) "scsi:
>>   introduce short_inquiry flag for broken host adapters" patch. I'm not
>>   particularly sure which solution is better but I'm leaning towards this
>>   one as it doesn't require changes to adapter drivers.
>> ---
>>  drivers/scsi/scsi_scan.c | 9 ++---
>>  include/scsi/scsi_host.h | 3 +++
>>  2 files changed, 9 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
>> index f9f3f82..cd347e4 100644
>> --- a/drivers/scsi/scsi_scan.c
>> +++ b/drivers/scsi/scsi_scan.c
>> @@ -701,9 +701,12 @@ static int scsi_probe_lun(struct scsi_device *sdev, 
>> unsigned char *inq_result,
>>   * strings.
>>   */
>>  if (sdev->inquiry_len < 36) {
>> -sdev_printk(KERN_INFO, sdev,
>> -"scsi scan: INQUIRY result too short (%d),"
>> -" using 36\n", sdev->inquiry_len);
>> +if (!sdev->host->short_inquiry) {
>> +shost_printk(KERN_INFO, sdev->host,
>> +"scsi scan: INQUIRY result too short (%d),"
>> +" using 36\n", sdev->inquiry_len);
>> +sdev->host->short_inquiry = 1;
>> +}
>>  sdev->inquiry_len = 36;
>>  }
>>  
> At least you need to check if you've received any valid data here;
> 'INQUIRY result too short' is also a common error if the interrupt
> is hosed when trying to access the device.
> So please check for 'inquiry_len > 4' before setting 'short_inquiry'.

Currently we proceed even with a shorter reply... Should we abort the
scan (and return -EOI?) in case we got inquiry_len <= 4?

Thanks,

-- 
  Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] scsi: report 'INQUIRY result too short' once per host

[Vitaly Kuznetsov]

Some host adapters (e.g. Hyper-V storvsc) are known for not respecting the
SPC-2/3/4 requirement for 'INQUIRY data (see table ...) shall contain at
least 36 bytes'. As a result we get tons on 'scsi 0:7:1:1: scsi scan:
INQUIRY result too short (5), using 36' messages on console. This can be
problematic for slow consoles. Introduce short_inquiry flag in struct
Scsi_Host to print the message once per host.

Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
---
Changes since v2:
- This is a successor of previously sent (and still not merged) "scsi:
  introduce short_inquiry flag for broken host adapters" patch. I'm not
  particularly sure which solution is better but I'm leaning towards this
  one as it doesn't require changes to adapter drivers.
---
 drivers/scsi/scsi_scan.c | 9 ++---
 include/scsi/scsi_host.h | 3 +++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index f9f3f82..cd347e4 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -701,9 +701,12 @@ static int scsi_probe_lun(struct scsi_device *sdev, 
unsigned char *inq_result,
 * strings.
 */
if (sdev->inquiry_len < 36) {
-   sdev_printk(KERN_INFO, sdev,
-   "scsi scan: INQUIRY result too short (%d),"
-   " using 36\n", sdev->inquiry_len);
+   if (!sdev->host->short_inquiry) {
+   shost_printk(KERN_INFO, sdev->host,
+   "scsi scan: INQUIRY result too short (%d),"
+   " using 36\n", sdev->inquiry_len);
+   sdev->host->short_inquiry = 1;
+   }
sdev->inquiry_len = 36;
}
 
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index e113c75..3a22da7 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -673,6 +673,9 @@ struct Scsi_Host {
unsigned use_blk_mq:1;
unsigned use_cmd_list:1;
 
+   /* Host responded with short (<36 bytes) INQUIRY result */
+   unsigned short_inquiry:1;
+
/*
 * Optional work queue to be utilized by the transport
 */
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] scsi: Some miscellaneous fixes

[Vitaly Kuznetsov]

"K. Y. Srinivasan" <k...@microsoft.com> writes:

[...]

>
> Vitaly Kuznetsov (2):
>   scsi_scan: don't dump trace when scsi_prep_async_scan() is called
> twice
>   scsi: introduce short_inquiry flag for broken host adapters

James,

I'm sorry for the annoyance but when I asked about these patches last
time you said we don't have them reviewed. Is it OK now when we have
signed-off-by: from K. Y. or do we need to ask someone else?

Thanks,

-- 
  Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] storvsc: get rid of homegrown copy_{to,from}_bounce_buffer()

[Vitaly Kuznetsov]

Christoph Hellwig <h...@infradead.org> writes:

> On Tue, Sep 22, 2015 at 06:27:50PM +0200, Vitaly Kuznetsov wrote:
>> Storvsc driver needs to ensure there are no 'holes' in the presented
>> sg list (all segments in the middle of the list need to be of PAGE_SIZE).
>
> I think it should instead set a virt_boundary.  That's what we added for
> the NVMe driver which has the same requirements, and Sagi recently also
> switched iSER to it after we ensured that flag is handled correctly by
> the SG_IO ioctl.

Wow,

I checked and blk_queue_virt_boundary(sdevice->request_queue, PAGE_SIZE
- 1) seems to be solving the issue completely, no bounce buffer
required. I'll test more and send v2 with removing the rest.

-- 
  Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] storvsc: get rid of bounce buffer

[Vitaly Kuznetsov]

Storvsc driver needs to ensure there are no 'holes' in the presented
sg list (all segments in the middle of the list need to be of PAGE_SIZE).
When a hole is detected storvsc driver creates a 'bounce sgl' without
holes and copies data over with copy_{to,from}_bounce_buffer() functions.
Setting virt_boundary_mask to PAGE_SIZE - 1 guarantees we'll never see
such holes so we can significantly simplify the driver. This is also
supposed to bring us some performance improvement for certain workloads
as we eliminate copying.

Reported-by: Radim Krčmář <rkrc...@redhat.com>
Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
---
Changes since v1:
- This patch is a successor of 'storvsc: get rid of homegrown
 copy_{to,from}_bounce_buffer()'. Use virt_boundary instead to
 eliminate the need for bounce buffer completely [Christoph Hellwig].
---
 drivers/scsi/storvsc_drv.c | 286 +
 1 file changed, 5 insertions(+), 281 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 40c43ae..eeade40 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -393,9 +393,6 @@ static void storvsc_on_channel_callback(void *context);
 struct storvsc_cmd_request {
struct scsi_cmnd *cmd;
 
-   unsigned int bounce_sgl_count;
-   struct scatterlist *bounce_sgl;
-
struct hv_device *device;
 
/* Synchronize the request/response if needed */
@@ -586,241 +583,6 @@ get_in_err:
 
 }
 
-static void destroy_bounce_buffer(struct scatterlist *sgl,
- unsigned int sg_count)
-{
-   int i;
-   struct page *page_buf;
-
-   for (i = 0; i < sg_count; i++) {
-   page_buf = sg_page(([i]));
-   if (page_buf != NULL)
-   __free_page(page_buf);
-   }
-
-   kfree(sgl);
-}
-
-static int do_bounce_buffer(struct scatterlist *sgl, unsigned int sg_count)
-{
-   int i;
-
-   /* No need to check */
-   if (sg_count < 2)
-   return -1;
-
-   /* We have at least 2 sg entries */
-   for (i = 0; i < sg_count; i++) {
-   if (i == 0) {
-   /* make sure 1st one does not have hole */
-   if (sgl[i].offset + sgl[i].length != PAGE_SIZE)
-   return i;
-   } else if (i == sg_count - 1) {
-   /* make sure last one does not have hole */
-   if (sgl[i].offset != 0)
-   return i;
-   } else {
-   /* make sure no hole in the middle */
-   if (sgl[i].length != PAGE_SIZE || sgl[i].offset != 0)
-   return i;
-   }
-   }
-   return -1;
-}
-
-static struct scatterlist *create_bounce_buffer(struct scatterlist *sgl,
-   unsigned int sg_count,
-   unsigned int len,
-   int write)
-{
-   int i;
-   int num_pages;
-   struct scatterlist *bounce_sgl;
-   struct page *page_buf;
-   unsigned int buf_len = ((write == WRITE_TYPE) ? 0 : PAGE_SIZE);
-
-   num_pages = ALIGN(len, PAGE_SIZE) >> PAGE_SHIFT;
-
-   bounce_sgl = kcalloc(num_pages, sizeof(struct scatterlist), GFP_ATOMIC);
-   if (!bounce_sgl)
-   return NULL;
-
-   sg_init_table(bounce_sgl, num_pages);
-   for (i = 0; i < num_pages; i++) {
-   page_buf = alloc_page(GFP_ATOMIC);
-   if (!page_buf)
-   goto cleanup;
-   sg_set_page(_sgl[i], page_buf, buf_len, 0);
-   }
-
-   return bounce_sgl;
-
-cleanup:
-   destroy_bounce_buffer(bounce_sgl, num_pages);
-   return NULL;
-}
-
-/* Assume the original sgl has enough room */
-static unsigned int copy_from_bounce_buffer(struct scatterlist *orig_sgl,
-   struct scatterlist *bounce_sgl,
-   unsigned int orig_sgl_count,
-   unsigned int bounce_sgl_count)
-{
-   int i;
-   int j = 0;
-   unsigned long src, dest;
-   unsigned int srclen, destlen, copylen;
-   unsigned int total_copied = 0;
-   unsigned long bounce_addr = 0;
-   unsigned long dest_addr = 0;
-   unsigned long flags;
-   struct scatterlist *cur_dest_sgl;
-   struct scatterlist *cur_src_sgl;
-
-   local_irq_save(flags);
-   cur_dest_sgl = orig_sgl;
-   cur_src_sgl = bounce_sgl;
-   for (i = 0; i < orig_sgl_count; i++) {
-   dest_addr = (unsigned long)
-   kmap_atomic(sg_page(cur_dest_sgl)) +
-   cur_dest_sgl->offset;
-   dest = dest_addr;
-   destlen = cur_dest_sgl->length;

[PATCH] storvsc: get rid of homegrown copy_{to,from}_bounce_buffer()

[Vitaly Kuznetsov]

Storvsc driver needs to ensure there are no 'holes' in the presented
sg list (all segments in the middle of the list need to be of PAGE_SIZE).
When a hole is detected storvsc driver creates a 'bounce sgl' without
holes and copies data over with its own copy_{to,from}_bounce_buffer()
functions. Scsi layer already has scsi_sg_copy_{to,from}_buffer()
functions to linearize a list, the only difference is that already
existent functions create a flat buffer instead of a new list but we can
cope.

Reported-by: Radim Krčmář <rkrc...@redhat.com>
Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
---
 drivers/scsi/storvsc_drv.c | 281 ++---
 1 file changed, 36 insertions(+), 245 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 40c43ae..31e8c67 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -393,8 +393,8 @@ static void storvsc_on_channel_callback(void *context);
 struct storvsc_cmd_request {
struct scsi_cmnd *cmd;
 
-   unsigned int bounce_sgl_count;
-   struct scatterlist *bounce_sgl;
+   unsigned int bounce_len;
+   void *bounce_buf;
 
struct hv_device *device;
 
@@ -586,21 +586,6 @@ get_in_err:
 
 }
 
-static void destroy_bounce_buffer(struct scatterlist *sgl,
- unsigned int sg_count)
-{
-   int i;
-   struct page *page_buf;
-
-   for (i = 0; i < sg_count; i++) {
-   page_buf = sg_page(([i]));
-   if (page_buf != NULL)
-   __free_page(page_buf);
-   }
-
-   kfree(sgl);
-}
-
 static int do_bounce_buffer(struct scatterlist *sgl, unsigned int sg_count)
 {
int i;
@@ -628,199 +613,6 @@ static int do_bounce_buffer(struct scatterlist *sgl, 
unsigned int sg_count)
return -1;
 }
 
-static struct scatterlist *create_bounce_buffer(struct scatterlist *sgl,
-   unsigned int sg_count,
-   unsigned int len,
-   int write)
-{
-   int i;
-   int num_pages;
-   struct scatterlist *bounce_sgl;
-   struct page *page_buf;
-   unsigned int buf_len = ((write == WRITE_TYPE) ? 0 : PAGE_SIZE);
-
-   num_pages = ALIGN(len, PAGE_SIZE) >> PAGE_SHIFT;
-
-   bounce_sgl = kcalloc(num_pages, sizeof(struct scatterlist), GFP_ATOMIC);
-   if (!bounce_sgl)
-   return NULL;
-
-   sg_init_table(bounce_sgl, num_pages);
-   for (i = 0; i < num_pages; i++) {
-   page_buf = alloc_page(GFP_ATOMIC);
-   if (!page_buf)
-   goto cleanup;
-   sg_set_page(_sgl[i], page_buf, buf_len, 0);
-   }
-
-   return bounce_sgl;
-
-cleanup:
-   destroy_bounce_buffer(bounce_sgl, num_pages);
-   return NULL;
-}
-
-/* Assume the original sgl has enough room */
-static unsigned int copy_from_bounce_buffer(struct scatterlist *orig_sgl,
-   struct scatterlist *bounce_sgl,
-   unsigned int orig_sgl_count,
-   unsigned int bounce_sgl_count)
-{
-   int i;
-   int j = 0;
-   unsigned long src, dest;
-   unsigned int srclen, destlen, copylen;
-   unsigned int total_copied = 0;
-   unsigned long bounce_addr = 0;
-   unsigned long dest_addr = 0;
-   unsigned long flags;
-   struct scatterlist *cur_dest_sgl;
-   struct scatterlist *cur_src_sgl;
-
-   local_irq_save(flags);
-   cur_dest_sgl = orig_sgl;
-   cur_src_sgl = bounce_sgl;
-   for (i = 0; i < orig_sgl_count; i++) {
-   dest_addr = (unsigned long)
-   kmap_atomic(sg_page(cur_dest_sgl)) +
-   cur_dest_sgl->offset;
-   dest = dest_addr;
-   destlen = cur_dest_sgl->length;
-
-   if (bounce_addr == 0)
-   bounce_addr = (unsigned long)kmap_atomic(
-   sg_page(cur_src_sgl));
-
-   while (destlen) {
-   src = bounce_addr + cur_src_sgl->offset;
-   srclen = cur_src_sgl->length - cur_src_sgl->offset;
-
-   copylen = min(srclen, destlen);
-   memcpy((void *)dest, (void *)src, copylen);
-
-   total_copied += copylen;
-   cur_src_sgl->offset += copylen;
-   destlen -= copylen;
-   dest += copylen;
-
-   if (cur_src_sgl->offset == cur_src_sgl->length) {
-   /* full */
-   kunmap_atomic((void *)bounce_addr);
-   j++;
-
-   /*
- 

Re: [PATCH v2] scsi: introduce short_inquiry flag for broken host adapters

[Vitaly Kuznetsov]

Vitaly Kuznetsov <vkuzn...@redhat.com> writes:

> Some host adapters (e.g. Hyper-V storvsc) are known for not respecting the
> SPC-2/3/4 requirement for 'INQUIRY data (see table ...) shall contain at
> least 36 bytes'. As a result we get tons on 'scsi 0:7:1:1: scsi scan:
> INQUIRY result too short (5), using 36' messages on console. This can be
> problematic for slow consoles. Introduce short_inquiry host template flag
> to avoid printing error messages for such adapters.
>
> Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
> ---
> Changes since v1:
> - This is a successor of previously sent "scsi_scan: move 'INQUIRY result
>   too short' message to debug level" patch. Instead of moving the message
>   to debug level for all adapters introduce a special 'short_inquiry' flag
>   for host template [inspired by James Bottomley].

James,

sorry for the ping but can you please let me know your opinion? This is
not a 'cosmetic fix', serial port on Hyper-V is extremely slow and users
get softlockups just because we output too much. Here is a freshly
booted guest with SCSI and FC adapters connected:

# dmesg | grep -c INQUIRY
2076

(my other pernding '[PATCH] scsi_scan: don't dump trace when
scsi_prep_async_scan() is called twice' is related to the same issue).

See also: https://lkml.org/lkml/2015/9/6/119

Thanks,

[...]

-- 
  Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] scsi: introduce short_inquiry flag for broken host adapters

[Vitaly Kuznetsov]

Some host adapters (e.g. Hyper-V storvsc) are known for not respecting the
SPC-2/3/4 requirement for 'INQUIRY data (see table ...) shall contain at
least 36 bytes'. As a result we get tons on 'scsi 0:7:1:1: scsi scan:
INQUIRY result too short (5), using 36' messages on console. This can be
problematic for slow consoles. Introduce short_inquiry host template flag
to avoid printing error messages for such adapters.

Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
---
Changes since v1:
- This is a successor of previously sent "scsi_scan: move 'INQUIRY result
  too short' message to debug level" patch. Instead of moving the message
  to debug level for all adapters introduce a special 'short_inquiry' flag
  for host template [inspired by James Bottomley].
---
 drivers/scsi/scsi_scan.c   | 7 ---
 drivers/scsi/storvsc_drv.c | 1 +
 include/scsi/scsi_host.h   | 6 ++
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index f9f3f82..f1d00a0 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -701,9 +701,10 @@ static int scsi_probe_lun(struct scsi_device *sdev, 
unsigned char *inq_result,
 * strings.
 */
if (sdev->inquiry_len < 36) {
-   sdev_printk(KERN_INFO, sdev,
-   "scsi scan: INQUIRY result too short (%d),"
-   " using 36\n", sdev->inquiry_len);
+   if (!sdev->host->hostt->short_inquiry)
+   sdev_printk(KERN_INFO, sdev,
+   "scsi scan: INQUIRY result too short (%d),"
+   " using 36\n", sdev->inquiry_len);
sdev->inquiry_len = 36;
}
 
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 3c6584f..f3b4d0f 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1711,6 +1711,7 @@ static struct scsi_host_template scsi_driver = {
/* Make sure we dont get a sg segment crosses a page boundary */
.dma_boundary = PAGE_SIZE-1,
.no_write_same =1,
+   .short_inquiry =1,
 };
 
 enum {
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index e113c75..7b022af 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -454,6 +454,12 @@ struct scsi_host_template {
unsigned no_async_abort:1;
 
/*
+* True if this host adapter returns short (<36 bytes) responses to
+* some INQUIRY requests.
+*/
+   unsigned short_inquiry:1;
+
+   /*
 * Countdown for host blocking with no commands outstanding.
 */
unsigned int max_host_blocked;
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] scsi_scan: move 'INQUIRY result too short' message to debug level

[Vitaly Kuznetsov]

James Bottomley <james.bottom...@hansenpartnership.com> writes:

> On Mon, 2015-08-31 at 14:50 +0200, Vitaly Kuznetsov wrote:
>> Some Hyper-V hosts are known for ignoring SPC-2/3/4 requirement
>> for 'INQUIRY data (see table ...) shall contain at least 36 bytes'. As a
>> result we get tons on 'scsi 0:7:1:1: scsi scan: INQUIRY result too short
>> (5), using 36' messages on console. As Hyper-V is also known for its
>> serial port being extremely slow multi-VCPU guests we get CPU blocked
>> putting these (useless) messages on console (e.g. happens when we add
>> multiple disks). Move them to debug level.
>
> This isn't an ignorable debug message.  It means the inquiry information
> the system relies on will be false, so it's fairly essential for bug
> reports.  It could be made a once per device print, but I don't think we
> can eliminate it.

I see. What if we introduce a special flag to mark such known-as-broken
hosts? E.g.:

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index f9f3f82..c6ffb5c 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -701,9 +701,10 @@ static int scsi_probe_lun(struct scsi_device *sdev, 
unsigned char *inq_result,
 * strings.
 */
if (sdev->inquiry_len < 36) {
-   sdev_printk(KERN_INFO, sdev,
-   "scsi scan: INQUIRY result too short (%d),"
-   " using 36\n", sdev->inquiry_len);
+   if (!sdev->host->hostt->short_inquiry)
+   sdev_printk(KERN_INFO, sdev,
+   "scsi scan: INQUIRY result too short (%d),"
+   " using 36\n", sdev->inquiry_len);
sdev->inquiry_len = 36;
}
 
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 3c6584f..f3b4d0f 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1711,6 +1711,7 @@ static struct scsi_host_template scsi_driver = {
/* Make sure we dont get a sg segment crosses a page boundary */
.dma_boundary = PAGE_SIZE-1,
.no_write_same =1,
+   .short_inquiry =1,
 };
 
 enum {
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index e113c75..04aefad 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -454,6 +454,12 @@ struct scsi_host_template {
unsigned no_async_abort:1;
 
/*
+* True if this host adapter returns short (<36 bytes) resposes to
+* INQUIRY requests.
+*/
+   unsigned short_inquiry:1;
+
+   /*
 * Countdown for host blocking with no commands outstanding.
 */
unsigned int max_host_blocked;

-- 
  Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] scsi_scan: don't dump trace when scsi_prep_async_scan() is called twice

[Vitaly Kuznetsov]

The only user of scsi_prep_async_scan() is scsi_scan_host() and it handles
the situation correctly. Move 'called twice' reporting to debug level as
well.

The issue is observed on Hyper-V: on any device add/remove event storvsc
driver calls scsi_scan_host() and in case previous scan is still running
we get the message and stack dump on console.

Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
---
 drivers/scsi/scsi_scan.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index f9f3f82..01ad016 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1712,8 +1712,7 @@ static struct async_scan_data 
*scsi_prep_async_scan(struct Scsi_Host *shost)
return NULL;
 
if (shost->async_scan) {
-   shost_printk(KERN_INFO, shost, "%s called twice\n", __func__);
-   dump_stack();
+   shost_printk(KERN_DEBUG, shost, "%s called twice\n", __func__);
return NULL;
}
 
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] scsi_scan: move 'INQUIRY result too short' message to debug level

[Vitaly Kuznetsov]

Some Hyper-V hosts are known for ignoring SPC-2/3/4 requirement
for 'INQUIRY data (see table ...) shall contain at least 36 bytes'. As a
result we get tons on 'scsi 0:7:1:1: scsi scan: INQUIRY result too short
(5), using 36' messages on console. As Hyper-V is also known for its
serial port being extremely slow multi-VCPU guests we get CPU blocked
putting these (useless) messages on console (e.g. happens when we add
multiple disks). Move them to debug level.

Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
---
 drivers/scsi/scsi_scan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index f9f3f82..cb5c50a 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -701,7 +701,7 @@ static int scsi_probe_lun(struct scsi_device *sdev, 
unsigned char *inq_result,
 * strings.
 */
if (sdev->inquiry_len < 36) {
-   sdev_printk(KERN_INFO, sdev,
+   sdev_printk(KERN_DEBUG, sdev,
"scsi scan: INQUIRY result too short (%d),"
" using 36\n", sdev->inquiry_len);
sdev->inquiry_len = 36;
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] scsi: storvsc: be more picky about scmnd-sc_data_direction

[Vitaly Kuznetsov]

KY Srinivasan k...@microsoft.com writes:

 -Original Message-
 From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com]
 Sent: Thursday, June 25, 2015 9:12 AM
 To: linux-scsi@vger.kernel.org
 Cc: Long Li; KY Srinivasan; Haiyang Zhang; James E.J. Bottomley;
 de...@linuxdriverproject.org; linux-ker...@vger.kernel.org; Radim Krčmář
 Subject: [PATCH] scsi: storvsc: be more picky about scmnd-
 sc_data_direction
 
 Under the 'default' case in scmnd-sc_data_direction we have 3 options:
 - DMA_NONE which we handle correctly.
 - DMA_BIDIRECTIONAL which is never supposed to be set by SCSI stack.
 - Garbage value.
 
 Do WARN() and return -EINVAL in the last two cases. virtio_scsi does
 BUG_ON() here but it looks like an overkill.
 
 Reported-by: Radim Krčmář rkrc...@redhat.com
 Signed-off-by: Vitaly Kuznetsov vkuzn...@redhat.com
 Signed-off-by: K. Y. Srinivasan k...@microsoft.com

Sorry for the ping but it seems the fix never made it to scsi tree...

 ---
  drivers/scsi/storvsc_drv.c | 10 +-
  1 file changed, 9 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
 index 3c6584f..61f4855 100644
 --- a/drivers/scsi/storvsc_drv.c
 +++ b/drivers/scsi/storvsc_drv.c
 @@ -1598,10 +1598,18 @@ static int storvsc_queuecommand(struct
 Scsi_Host *host, struct scsi_cmnd *scmnd)
  vm_srb-data_in = READ_TYPE;
  vm_srb-win8_extension.srb_flags |=
 SRB_FLAGS_DATA_IN;
  break;
 -default:
 +case DMA_NONE:
  vm_srb-data_in = UNKNOWN_TYPE;
  vm_srb-win8_extension.srb_flags |=
 SRB_FLAGS_NO_DATA_TRANSFER;
  break;
 +default:
 +/*
 + * This is DMA_BIDIRECTIONAL or something else we are
 never
 + * supposed to see here.
 + */
 +WARN(1, Unexpected data direction: %d\n,
 + scmnd-sc_data_direction);
 +return -EINVAL;
  }
 
 
 --
 2.4.3

-- 
  Vitaly
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] scsi: storvsc: use shost_for_each_device() instead of open coding

[Vitaly Kuznetsov]

Long Li lon...@microsoft.com writes:

 -Original Message-
 From: KY Srinivasan
 Sent: Friday, July 03, 2015 11:35 AM
 To: Vitaly Kuznetsov; linux-scsi@vger.kernel.org
 Cc: Long Li; Haiyang Zhang; James E.J. Bottomley; 
 de...@linuxdriverproject.org;
 linux-ker...@vger.kernel.org
 Subject: RE: [PATCH] scsi: storvsc: use shost_for_each_device() instead of 
 open
 coding
 
 
 
  -Original Message-
  From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com]
  Sent: Wednesday, July 1, 2015 2:31 AM
  To: linux-scsi@vger.kernel.org
  Cc: Long Li; KY Srinivasan; Haiyang Zhang; James E.J. Bottomley;
  de...@linuxdriverproject.org; linux-ker...@vger.kernel.org
  Subject: [PATCH] scsi: storvsc: use shost_for_each_device() instead of
  open coding
 
  Comment in struct Scsi_Host says that drivers are not supposed to
  access __devices directly. storvsc_host_scan() doesn't happen in irq
  context so we can just use shost_for_each_device().
 
  Signed-off-by: Vitaly Kuznetsov vkuzn...@redhat.com
 
 Signed-off-by: K. Y. Srinivasan k...@microsoft.com
 Reviewed-by: Long Li lon...@microsoft.com

Sorry for the ping but it seems the fix never made it to scsi tree...

  ---
   drivers/scsi/storvsc_drv.c | 9 +
   1 file changed, 1 insertion(+), 8 deletions(-)
 
  diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
  index 3c6584f..9ea912b 100644
  --- a/drivers/scsi/storvsc_drv.c
  +++ b/drivers/scsi/storvsc_drv.c
  @@ -426,7 +426,6 @@ static void storvsc_host_scan(struct work_struct
  *work)
 struct storvsc_scan_work *wrk;
 struct Scsi_Host *host;
 struct scsi_device *sdev;
  -  unsigned long flags;
 
 wrk = container_of(work, struct storvsc_scan_work, work);
 host = wrk-host;
  @@ -443,14 +442,8 @@ static void storvsc_host_scan(struct work_struct
  *work)
  * may have been removed this way.
  */
 mutex_lock(host-scan_mutex);
  -  spin_lock_irqsave(host-host_lock, flags);
  -  list_for_each_entry(sdev, host-__devices, siblings) {
  -  spin_unlock_irqrestore(host-host_lock, flags);
  +  shost_for_each_device(sdev, host)
 scsi_test_unit_ready(sdev, 1, 1, NULL);
  -  spin_lock_irqsave(host-host_lock, flags);
  -  continue;
  -  }
  -  spin_unlock_irqrestore(host-host_lock, flags);
 mutex_unlock(host-scan_mutex);
 /*
  * Now scan the host to discover LUNs that may have been added.
  --
  2.4.3

-- 
  Vitaly
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] scsi: storvsc: make INQUIRY response SPC-compliant

[Vitaly Kuznetsov]

KY Srinivasan k...@microsoft.com writes:

 -Original Message-
 From: Christoph Hellwig [mailto:h...@infradead.org]
 Sent: Friday, July 3, 2015 9:19 AM
 To: Vitaly Kuznetsov
 Cc: linux-scsi@vger.kernel.org; Long Li; KY Srinivasan; Haiyang Zhang; James
 E.J. Bottomley; de...@linuxdriverproject.org; linux-ker...@vger.kernel.org
 Subject: Re: [PATCH] scsi: storvsc: make INQUIRY response SPC-compliant
 
 On Wed, Jul 01, 2015 at 11:04:08AM +0200, Vitaly Kuznetsov wrote:
  SPC-2/3/4 specs state that The standard INQUIRY data (see table ...)
  shall contain at least 36 bytes. Hyper-V host doesn't always honor this
  requirement, e.g. when there is no physical device present at a particular
  LUN host sets Peripheral qualifier to 011b and Additional length to 0
  (thus making the reply 5-bytes long). Upper level SCSI stack complains
  with 'INQUIRY result too short (5), using 36'. Fix the issue by mangling
  Additional length field in host's reply at the driver level.
 
 This looks like a big mess, and usage of phys_to_virt is not generally
 safe to start with.
 
 If HyperV really is that broken the warning seems correct, but if you
 really have to get rid of it we could add a blist flag to not issue
 the warning in the core code instead of hacking around it in the driver.

 Agreed. We have fixed this issue in win10 and I am trying to get the fix 
 backported.

In case this is fixed in future Hyper-V versions introducing new blist
flags looks like an overkill, let's leave things as they are.

Thanks,

-- 
  Vitaly
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] scsi: storvsc: be more picky about scmnd-sc_data_direction

[Vitaly Kuznetsov]

Under the 'default' case in scmnd-sc_data_direction we have 3 options:
- DMA_NONE which we handle correctly.
- DMA_BIDIRECTIONAL which is never supposed to be set by SCSI stack.
- Garbage value.

Do WARN() and return -EINVAL in the last two cases. virtio_scsi does
BUG_ON() here but it looks like an overkill.

Reported-by: Radim Krčmář rkrc...@redhat.com
Signed-off-by: Vitaly Kuznetsov vkuzn...@redhat.com
---
 drivers/scsi/storvsc_drv.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 3c6584f..61f4855 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1598,10 +1598,18 @@ static int storvsc_queuecommand(struct Scsi_Host *host, 
struct scsi_cmnd *scmnd)
vm_srb-data_in = READ_TYPE;
vm_srb-win8_extension.srb_flags |= SRB_FLAGS_DATA_IN;
break;
-   default:
+   case DMA_NONE:
vm_srb-data_in = UNKNOWN_TYPE;
vm_srb-win8_extension.srb_flags |= SRB_FLAGS_NO_DATA_TRANSFER;
break;
+   default:
+   /*
+* This is DMA_BIDIRECTIONAL or something else we are never
+* supposed to see here.
+*/
+   WARN(1, Unexpected data direction: %d\n,
+scmnd-sc_data_direction);
+   return -EINVAL;
}
 
 
-- 
2.4.3

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html