RE: [PATCH 1/1] scsi: storvsc: Set the SRB flags correctly when no data transfer is needed

[KY Srinivasan]

> -Original Message-
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Saturday, April 25, 2015 12:05 AM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> sta...@vger.kernel.org
> Subject: Re: [PATCH 1/1] scsi: storvsc: Set the SRB flags correctly when no
> data transfer is needed
> 
> On Fri, Apr 24, 2015 at 04:33:55PM -0700, K. Y. Srinivasan wrote:
> > Set the SRB flags correctly when there is no data transfer.
> >
> 
> What are the user visible effects of this bug?  We transfer bogus data?

No, we don't transfer any bogus data. However, some IHV drivers will
return an error on some valid commands because of these flags being set.

Regards,

K. Y
> 
> regards,
> dan carpenter

--
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 1/6] scsi: storvsc: Rather than look for sets of specific protocol versions, make decisions based on ranges.

[KY Srinivasan]

> -Original Message-
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Monday, June 1, 2015 3:57 AM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com; Keith
> Mange
> Subject: Re: [PATCH 1/6] scsi: storvsc: Rather than look for sets of specific
> protocol versions, make decisions based on ranges.
> 
> On Fri, May 29, 2015 at 01:29:14PM -0700, K. Y. Srinivasan wrote:
> > From: keith.ma...@microsoft.com 
> 
> Keith's name is wrong.

Thanks Dan; I will fix this and resend.

K. Y
> 
> regards,
> dan carpenter

--
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 3/6] hv:scsi:Untangle the storage protocol negotiation from the vmbus protocol negotiation.

[KY Srinivasan]

> -Original Message-
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Monday, June 1, 2015 3:43 AM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com; Keith
> Mange
> Subject: Re: [PATCH 3/6] hv:scsi:Untangle the storage protocol negotiation
> from the vmbus protocol negotiation.
> 
> On Fri, May 29, 2015 at 01:29:16PM -0700, K. Y. Srinivasan wrote:
> > -   if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO
> ||
> > -   vstor_packet->status != 0)
> > +   if (vstor_packet->status != 0) {
> > +   ret = -EINVAL;
> > goto cleanup;
> > +   }
> 
> There is not actually any cleanup, goto cleanup is just a do-nothing
> goto.
> 
> In the original code, we returned success here.  That always looked like
> a "forgot to set the error code" bug to me, but do-nothing labels always
> introduce ambiguous looking "forgot to set the error code" bugs so I can
> never be positive.
> 
> Could you take a look at the other "goto cleanup;" places in this
> function and maybe add a comment, change it to something more clear like
> "return 0;" or fix the error code?

Thanks Dan; will do.

K. Y
> 
> regards,
> dan carpenter

--
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

[KY Srinivasan]

> -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ář 
> Signed-off-by: Vitaly Kuznetsov 
Signed-off-by: K. Y. Srinivasan 
> ---
>  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

N�r��yb�X��ǧv�^�)޺{.n�+{���"�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

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

[KY Srinivasan]

> -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.

Regards,

K. Y
--
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

[KY Srinivasan]

> -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 

Signed-off-by: K. Y. Srinivasan 
> ---
>  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

--
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: Fix a bug in copy_from_bounce_buffer()

[KY Srinivasan]

> -Original Message-
> From: Ben Hutchings [mailto:b...@decadent.org.uk]
> Sent: Friday, August 7, 2015 12:05 PM
> To: KY Srinivasan 
> Cc: Long Li ; linux-scsi 
> Subject: Re: [PATCH] scsi: storvsc: Fix a bug in copy_from_bounce_buffer()
> 
> Commit 8de580742fee ("scsi: storvsc: Fix a bug in
> copy_from_bounce_buffer()"), actually modified the function
> copy_to_bounce_buffer().  Is the commit message wrong, or was the patch
> applied to the wrong function?
> 
Ben,

Most likely the message is "misleading". I currently don't have access to the 
source; I will confirm sometime tonight.

Thank you,

K. Y
> Ben.
> 
> --
> Ben Hutchings
> Computers are not intelligent.They only think they are.

N�r��yb�X��ǧv�^�)޺{.n�+{���"�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

RE: [PATCH] scsi: storvsc: Fix a bug in copy_from_bounce_buffer()

[KY Srinivasan]

> -Original Message-
> From: KY Srinivasan
> Sent: Saturday, August 8, 2015 7:55 AM
> To: 'Ben Hutchings' 
> Cc: Long Li ; linux-scsi 
> Subject: RE: [PATCH] scsi: storvsc: Fix a bug in copy_from_bounce_buffer()
> 
> > -Original Message-
> > From: Ben Hutchings [mailto:b...@decadent.org.uk]
> > Sent: Friday, August 7, 2015 12:05 PM
> > To: KY Srinivasan 
> > Cc: Long Li ; linux-scsi 
> > Subject: Re: [PATCH] scsi: storvsc: Fix a bug in copy_from_bounce_buffer()
> >
> > Commit 8de580742fee ("scsi: storvsc: Fix a bug in
> > copy_from_bounce_buffer()"), actually modified the function
> > copy_to_bounce_buffer().  Is the commit message wrong, or was the
> patch
> > applied to the wrong function?
> >
> Ben,
> 
> Most likely the message is "misleading". I currently don't have access to the
> source; I will confirm sometime tonight.

The commit message is a typo (and is misleading).

K. Y
> 
> Thank you,
> 
> K. Y
> > Ben.
> >
> > --
> > Ben Hutchings
> > Computers are not intelligent.  They only think they are.

N�r��yb�X��ǧv�^�)޺{.n�+{���"�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

RE: [PATCH RESEND V2 3/7] scsi: storvsc: Untangle the storage protocol negotiation from the vmbus protocol negotiation.

[KY Srinivasan]

> -Original Message-
> From: Johannes Thumshirn [mailto:jthumsh...@suse.de]
> Sent: Thursday, August 13, 2015 7:34 AM
> To: KY Srinivasan ; Keith Mange
> 
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com
> Subject: Re: [PATCH RESEND V2 3/7] scsi: storvsc: Untangle the storage
> protocol negotiation from the vmbus protocol negotiation.
> 
> "K. Y. Srinivasan"  writes:
> 
> > From: Keith Mange 
> >
> > Currently we are making decisions based on vmbus protocol versions
> > that have been negotiated; use storage potocol versions instead.
> >
> > Tested-by: Alex Ng 
> > Signed-off-by: Keith Mange 
> > Signed-off-by: K. Y. Srinivasan 
> > ---
> >  drivers/scsi/storvsc_drv.c |  109
> +++-
> >  1 files changed, 87 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > index 5f9d133..f29871e 100644
> > --- a/drivers/scsi/storvsc_drv.c
> > +++ b/drivers/scsi/storvsc_drv.c
> > @@ -56,14 +56,18 @@
> >   * V1 RC > 2008/1/31:  2.0
> >   * Win7: 4.2
> >   * Win8: 5.1
> > + * Win8.1: 6.0
> > + * Win10: 6.2
> >   */
> >
> >  #define VMSTOR_PROTO_VERSION(MAJOR_, MINOR_)   MAJOR_)
> & 0xff) << 8) | \
> > (((MINOR_) & 0xff)))
> >
> > +#define VMSTOR_PROTO_VERSION_WIN6
>   VMSTOR_PROTO_VERSION(2, 0)
> >  #define VMSTOR_PROTO_VERSION_WIN7
>   VMSTOR_PROTO_VERSION(4, 2)
> >  #define VMSTOR_PROTO_VERSION_WIN8
>   VMSTOR_PROTO_VERSION(5, 1)
> > -
> > +#define VMSTOR_PROTO_VERSION_WIN8_1
>   VMSTOR_PROTO_VERSION(6, 0)
> > +#define VMSTOR_PROTO_VERSION_WIN10
>   VMSTOR_PROTO_VERSION(6, 2)
> >
> >  /*  Packet structure describing virtual storage requests. */
> >  enum vstor_packet_operation {
> > @@ -205,6 +209,46 @@ struct vmscsi_request {
> >
> >
> >  /*
> > + * The list of storage protocols in order of preference.
> > + */
> > +struct vmstor_protocol {
> > +   int protocol_version;
> > +   int sense_buffer_size;
> > +   int vmscsi_size_delta;
> > +};
> > +
> > +#define VMSTOR_NUM_PROTOCOLS5
> 
> can't you just use ARRAY_SIZE() here, so you don't have to touch the
> constant every time a new protocol is appended to the list?

Certainly. These patches have been floating around for more than a month now 
and if it is ok
with you, I will submit a patch on top of this current series to address the 
concern you have raised.
James, please let me know.

Regards,

K. Y 
> 
> > +
> > +const struct vmstor_protocol
> vmstor_protocols[VMSTOR_NUM_PROTOCOLS] = {
> > +   {
> > +   VMSTOR_PROTO_VERSION_WIN10,
> > +   POST_WIN7_STORVSC_SENSE_BUFFER_SIZE,
> > +   0
> > +   },
> > +   {
> > +   VMSTOR_PROTO_VERSION_WIN8_1,
> > +   POST_WIN7_STORVSC_SENSE_BUFFER_SIZE,
> > +   0
> > +   },
> > +   {
> > +   VMSTOR_PROTO_VERSION_WIN8,
> > +   POST_WIN7_STORVSC_SENSE_BUFFER_SIZE,
> > +   0
> > +   },
> > +   {
> > +   VMSTOR_PROTO_VERSION_WIN7,
> > +   PRE_WIN8_STORVSC_SENSE_BUFFER_SIZE,
> > +   sizeof(struct vmscsi_win8_extension),
> > +   },
> > +   {
> > +   VMSTOR_PROTO_VERSION_WIN6,
> > +   PRE_WIN8_STORVSC_SENSE_BUFFER_SIZE,
> > +   sizeof(struct vmscsi_win8_extension),
> > +   }
> > +};
> > +
> 
> Thanks,
> Johannes
> --
> Johannes Thumshirn   Storage
> jthumsh...@suse.de +49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG Nürnberg)
> Key fingerprint = EC38 9CAB C2C4 F25D 8600  D0D0 0393 969D 2D76 0850

RE: [PATCH 1/1] Drivers: hv: vmbus: fix init_vp_index() for reloading hv_netvsc

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:james.bottom...@hansenpartnership.com]
> Sent: Thursday, August 13, 2015 4:09 PM
> To: KY Srinivasan 
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> Dexuan Cui 
> Subject: Re: [PATCH 1/1] Drivers: hv: vmbus: fix init_vp_index() for reloading
> hv_netvsc
> 
> On Thu, 2015-08-13 at 17:07 -0700, K. Y. Srinivasan wrote:
> > From: Dexuan Cui 
> >
> > This fixes the recent commit 3b71107d73b16074afa7658f3f0fcf837aabfe24:
> 
> Which tree is this in?  upstream linus is giving me bad object on that
> id.

Greg's char-misc tree.
> 
> 
> > Drivers: hv: vmbus: Further improve CPU affiliation logic
> >
> > Without the fix, reloading hv_netvsc hangs the guest.
> 
> The reason for looking for the commit id was to see if cc to stable was
> necessary, is it?
The offending patch was committed on August 5th and cc to stable was not 
necessary.

K. Y
> 
> James
> 
> > Signed-off-by: Dexuan Cui 
> > Signed-off-by: K. Y. Srinivasan 
> > ---
> >  drivers/hv/channel_mgmt.c |   17 +
> >  1 files changed, 17 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> > index 3ab4753..8a4105c 100644
> > --- a/drivers/hv/channel_mgmt.c
> > +++ b/drivers/hv/channel_mgmt.c
> > @@ -204,6 +204,8 @@ void hv_process_channel_removal(struct
> vmbus_channel *channel, u32 relid)
> > spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
> > list_del(&channel->listentry);
> > spin_unlock_irqrestore(&vmbus_connection.channel_lock,
> flags);
> > +
> > +   primary_channel = channel;
> > } else {
> > primary_channel = channel->primary_channel;
> > spin_lock_irqsave(&primary_channel->lock, flags);
> > @@ -211,6 +213,14 @@ void hv_process_channel_removal(struct
> vmbus_channel *channel, u32 relid)
> > primary_channel->num_sc--;
> > spin_unlock_irqrestore(&primary_channel->lock, flags);
> > }
> > +
> > +   /*
> > +* We need to free the bit for init_vp_index() to work in the case
> > +* of sub-channel, when we reload drivers like hv_netvsc.
> > +*/
> > +   cpumask_clear_cpu(channel->target_cpu,
> > + &primary_channel->alloced_cpus_in_node);
> > +
> > free_channel(channel);
> >  }
> >
> > @@ -457,6 +467,13 @@ static void init_vp_index(struct vmbus_channel
> *channel, const uuid_le *type_gui
> > continue;
> > }
> >
> > +   /*
> > +* NOTE: in the case of sub-channel, we clear the sub-channel
> > +* related bit(s) in primary->alloced_cpus_in_node in
> > +* hv_process_channel_removal(), so when we reload
> drivers
> > +* like hv_netvsc in SMP guest, here we're able to re-allocate
> > +* bit from primary->alloced_cpus_in_node.
> > +*/
> > if (!cpumask_test_cpu(cur_cpu,
> > &primary->alloced_cpus_in_node)) {
> > cpumask_set_cpu(cur_cpu,
> 
> 

RE: [PATCH RESEND V2 3/7] scsi: storvsc: Untangle the storage protocol negotiation from the vmbus protocol negotiation.

[KY Srinivasan]

> -Original Message-
> From: Johannes Thumshirn [mailto:jthumsh...@suse.de]
> Sent: Thursday, August 13, 2015 11:46 PM
> To: KY Srinivasan 
> Cc: Keith Mange ;
> gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com
> Subject: Re: [PATCH RESEND V2 3/7] scsi: storvsc: Untangle the storage
> protocol negotiation from the vmbus protocol negotiation.
> 
> KY Srinivasan  writes:
> 
> >> -Original Message-
> >> From: Johannes Thumshirn [mailto:jthumsh...@suse.de]
> >> Sent: Thursday, August 13, 2015 7:34 AM
> >> To: KY Srinivasan ; Keith Mange
> >> 
> >> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> >> de...@linuxdriverproject.org; oher...@suse.com;
> >> jbottom...@parallels.com; h...@infradead.org; linux-
> s...@vger.kernel.org;
> >> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com
> >> Subject: Re: [PATCH RESEND V2 3/7] scsi: storvsc: Untangle the storage
> >> protocol negotiation from the vmbus protocol negotiation.
> >>
> >> "K. Y. Srinivasan"  writes:
> >>
> >> > From: Keith Mange 
> >> >
> >> > Currently we are making decisions based on vmbus protocol versions
> >> > that have been negotiated; use storage potocol versions instead.
> >> >
> >> > Tested-by: Alex Ng 
> >> > Signed-off-by: Keith Mange 
> >> > Signed-off-by: K. Y. Srinivasan 
> >> > ---
> >> >  drivers/scsi/storvsc_drv.c |  109
> >> +++-
> >> >  1 files changed, 87 insertions(+), 22 deletions(-)
> >> >
> >> > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> >> > index 5f9d133..f29871e 100644
> >> > --- a/drivers/scsi/storvsc_drv.c
> >> > +++ b/drivers/scsi/storvsc_drv.c
> >> > @@ -56,14 +56,18 @@
> >> >   * V1 RC > 2008/1/31:  2.0
> >> >   * Win7: 4.2
> >> >   * Win8: 5.1
> >> > + * Win8.1: 6.0
> >> > + * Win10: 6.2
> >> >   */
> >> >
> >> >  #define VMSTOR_PROTO_VERSION(MAJOR_, MINOR_)MAJOR_)
> >> & 0xff) << 8) | \
> >> >  (((MINOR_) & 0xff)))
> >> >
> >> > +#define VMSTOR_PROTO_VERSION_WIN6
> >>VMSTOR_PROTO_VERSION(2, 0)
> >> >  #define VMSTOR_PROTO_VERSION_WIN7
> >>VMSTOR_PROTO_VERSION(4, 2)
> >> >  #define VMSTOR_PROTO_VERSION_WIN8
> >>VMSTOR_PROTO_VERSION(5, 1)
> >> > -
> >> > +#define VMSTOR_PROTO_VERSION_WIN8_1
> >>VMSTOR_PROTO_VERSION(6, 0)
> >> > +#define VMSTOR_PROTO_VERSION_WIN10
> >>VMSTOR_PROTO_VERSION(6, 2)
> >> >
> >> >  /*  Packet structure describing virtual storage requests. */
> >> >  enum vstor_packet_operation {
> >> > @@ -205,6 +209,46 @@ struct vmscsi_request {
> >> >
> >> >
> >> >  /*
> >> > + * The list of storage protocols in order of preference.
> >> > + */
> >> > +struct vmstor_protocol {
> >> > +int protocol_version;
> >> > +int sense_buffer_size;
> >> > +int vmscsi_size_delta;
> >> > +};
> >> > +
> >> > +#define VMSTOR_NUM_PROTOCOLS5
> >>
> >> can't you just use ARRAY_SIZE() here, so you don't have to touch the
> >> constant every time a new protocol is appended to the list?
> >
> > Certainly. These patches have been floating around for more than a month
> now and if it is ok
> > with you, I will submit a patch on top of this current series to address the
> concern you have raised.
> > James, please let me know.
> >
> 
> No objections from my side, but it's up to James to decide what and when
> he picks up patches.

James,

I have sent a separate patch on top of the set I sent yesterday that addresses 
Johannes' comments.

Regards,

K. Y
N�r��yb�X��ǧv�^�)޺{.n�+{���"�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

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

[KY Srinivasan]

> -Original Message-
> From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com]
> Sent: Monday, August 31, 2015 5:50 AM
> To: James E.J. Bottomley 
> Cc: linux-scsi@vger.kernel.org; linux-ker...@vger.kernel.org; KY Srinivasan
> ; Long Li ; Dexuan Cui
> 
> Subject: [PATCH] scsi_scan: move 'INQUIRY result too short' message to
> debug level
> 
> 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 

Signed-off-by: K. Y. Srinivasan 
> ---
>  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 1/1] storvsc: Don't set the SRB_FLAGS_QUEUE_ACTION_ENABLE flag

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:james.bottom...@hansenpartnership.com]
> Sent: Monday, August 31, 2015 7:02 AM
> To: KY Srinivasan 
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> sta...@vger.kernel.org
> Subject: Re: [PATCH 1/1] storvsc: Don't set the
> SRB_FLAGS_QUEUE_ACTION_ENABLE flag
> 
> On Mon, 2015-08-31 at 08:21 -0700, K. Y. Srinivasan wrote:
> > Don't set the SRB_FLAGS_QUEUE_ACTION_ENABLE flag since we are not
> specifying
> > tags.
> 
> What's the actual problem description this causes?
Qlogic driver does not work correctly if the SRB_FLAGS_QUEUE_ACTION_ENABLE
is set and no action tag is specified.

Regards,

K. Y

> 
> James
> 
> 
> > Signed-off-by: K. Y. Srinivasan 
> > Cc: sta...@vger.kernel.org
> > ---
> >  drivers/scsi/storvsc_drv.c |3 +--
> >  1 files changed, 1 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > index 40c43ae..ad8c4bc 100644
> > --- a/drivers/scsi/storvsc_drv.c
> > +++ b/drivers/scsi/storvsc_drv.c
> > @@ -1647,8 +1647,7 @@ static int storvsc_queuecommand(struct
> Scsi_Host *host, struct scsi_cmnd *scmnd)
> > vm_srb->win8_extension.time_out_value = 60;
> >
> > vm_srb->win8_extension.srb_flags |=
> > -   (SRB_FLAGS_QUEUE_ACTION_ENABLE |
> > -   SRB_FLAGS_DISABLE_SYNCH_TRANSFER);
> > +   SRB_FLAGS_DISABLE_SYNCH_TRANSFER;
> >
> > /* Build the SRB */
> > switch (scmnd->sc_data_direction) {
> 
> 

N�r��yb�X��ǧv�^�)޺{.n�+{���"�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

RE: [PATCH v2] [hv] storvsc: Payload buffer incorrectly sized for 32 bit kernels.

[KY Srinivasan]

> -Original Message-
> From: Cathy Avery [mailto:cav...@redhat.com]
> Sent: Wednesday, November 23, 2016 5:47 AM
> To: KY Srinivasan ; Haiyang Zhang
> ; j...@linux.vnet.ibm.com;
> martin.peter...@oracle.com
> Cc: de...@linuxdriverproject.org; linux-ker...@vger.kernel.org; linux-
> s...@vger.kernel.org
> Subject: [PATCH v2] [hv] storvsc: Payload buffer incorrectly sized for 32 bit
> kernels.
> 
> On a 32 bit kernel sizeof(void *) is not 64 bits as hv_mpb_array
> requires. Also the buffer needs to be cleared or the upper bytes
> will contain junk.
> 
> Suggested-by: Vitaly Kuznetsov 
> Signed-off-by: Cathy Avery 

Thanks Cathy.

Reviewed-by: K. Y. Srinivasan 
> 
> ChangeLog:
> 
> v1) Initial submission
> v2) Remove memset and replace kmalloc with kzalloc.
> ---
>  drivers/scsi/storvsc_drv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 8ccfc9e..05526b7 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -1495,9 +1495,9 @@ static int storvsc_queuecommand(struct Scsi_Host
> *host, struct scsi_cmnd *scmnd)
>   if (sg_count) {
>   if (sg_count > MAX_PAGE_BUFFER_COUNT) {
> 
> - payload_sz = (sg_count * sizeof(void *) +
> + payload_sz = (sg_count * sizeof(u64) +
> sizeof(struct vmbus_packet_mpb_array));
> - payload = kmalloc(payload_sz, GFP_ATOMIC);
> + payload = kzalloc(payload_sz, GFP_ATOMIC);
>   if (!payload)
>   return SCSI_MLQUEUE_DEVICE_BUSY;
>   }
> --
> 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 v2] [hv] storvsc: Payload buffer incorrectly sized for 32 bit kernels.

[KY Srinivasan]

> -Original Message-
> From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
> Sent: Tuesday, November 29, 2016 8:58 AM
> To: Cathy Avery 
> Cc: KY Srinivasan ; Haiyang Zhang
> ; j...@linux.vnet.ibm.com;
> martin.peter...@oracle.com; de...@linuxdriverproject.org; linux-
> ker...@vger.kernel.org; linux-scsi@vger.kernel.org
> Subject: Re: [PATCH v2] [hv] storvsc: Payload buffer incorrectly sized for 32
> bit kernels.
> 
> >>>>> "Cathy" == Cathy Avery  writes:
> 
> Cathy> On a 32 bit kernel sizeof(void *) is not 64 bits as hv_mpb_array
> Cathy> requires. Also the buffer needs to be cleared or the upper bytes
> Cathy> will contain junk.
> 
> K.Y.: Please review!

Done.

K. Y
> 
> --
> Martin K. PetersenOracle Linux Engineering
--
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 1/2] scsi: scsi_transport_fc: Provide a lightweight option for Virtual FC Hosts.

[KY Srinivasan]

> -Original Message-
> From: Christoph Hellwig [mailto:h...@infradead.org]
> Sent: Thursday, January 19, 2017 7:12 AM
> To: Cathy Avery 
> Cc: KY Srinivasan ; Haiyang Zhang
> ; j...@linux.vnet.ibm.com;
> martin.peter...@oracle.com; de...@linuxdriverproject.org; linux-
> ker...@vger.kernel.org; linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 1/2] scsi: scsi_transport_fc: Provide a lightweight option
> for Virtual FC Hosts.
> 
> On Wed, Jan 18, 2017 at 03:28:57PM -0500, Cathy Avery wrote:
> > The patch provides a means to offer a lightweight option to the
> > current FC transport class. The new option is selected by a
> > driver when it indicates it wants the lightweight
> > transport via fc_function_template.
> 
> This need some really good documentation in the code and changelog
> what "lightweight" means.  It's a pretty horrible term, btw.

This was a concept that James proposed almost a year ago. I agree, we need to 
document this
better.

K. Y

--
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 v2 0/2] scsi: storvsc: Add support for FC lightweight host.

[KY Srinivasan]

> -Original Message-
> From: Christoph Hellwig [mailto:h...@infradead.org]
> Sent: Thursday, January 26, 2017 6:52 AM
> To: Cathy Avery 
> Cc: KY Srinivasan ; h...@infradead.org; Haiyang Zhang
> ; j...@linux.vnet.ibm.com;
> martin.peter...@oracle.com; dan.carpen...@oracle.com;
> de...@linuxdriverproject.org; linux-ker...@vger.kernel.org; linux-
> s...@vger.kernel.org; f...@redhat.com
> Subject: Re: [PATCH v2 0/2] scsi: storvsc: Add support for FC lightweight 
> host.
> 
> On Thu, Jan 26, 2017 at 08:38:58AM -0500, Cathy Avery wrote:
> > Included in the current storvsc driver for Hyper-V is the ability
> > to access luns on an FC fabric via a virtualized fiber channel
> > adapter exposed by the Hyper-V host. This was done to provide an
> > interface for existing customer tools that was more consistent with
> > a conventional FC device. The driver attaches to the FC transport
> > to allow host and port names to be published under
> > /sys/class/fc_host/hostX.
> >
> > A problem arose when attaching to the FC transport. The scsi_scan code
> > attempts to call fc_user_scan which has basically become a no-op
> > due to the virtualized nature of the FC host
> > ( missing rports, vports, etc ). At this point you cannot refresh
> > the scsi bus after mapping or unmapping luns on the SAN without
> > a reboot.
> 
> I don't think a device without rports or vports is a FC device, plain and
> simple.  So as far as I'm concerned we should remove the code from storvsc
> that pretends to be FC, and not add it to virtio to start with.
> 
> And again I think leightweight is a very confusing name -
> what exactly is leight or heavy?   It's really fake or dummy
> in the current version.

Windows has chosen this model for virtualizing FC devices to the guest -
without rports (or vports). As I noted in my earlier email, James came up with
this notion of a lightweight template almost a year ago. We can certainly pick 
a 
more appropriate name and include better documentation. 
> 
> >
> > 2) Removes an original workaround dealing with replacing
> > the eh_timed_out function. Patch 1 will not set the
> > scsi_transport_template.eh_timed_out function directly during
> > lightweight fc_attach_transport(). It instead relies on
> > whatever was indicated as the scsi_host_template timeout handler
> > during scsi_times_out() scsi_error.c. So the workaround is
> > no longer necessary.
> 
> Can you send a patch that gets rid of the transport class timeout handler
> entirely?  I think it's simply the wrong layering we have here - the
> driver needs to be in control of timeouts, and if it wants it can
> optionally call into library code in the transport class.

We will address this concern.
> 
> 
> FYI, all the long-term relevant explanation need to go into the patches
> themselves (patch description or code comments), not in the cover
> letter.

We will address this.

Regards,

K. Y
--
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 v2 0/2] scsi: storvsc: Add support for FC lightweight host.

[KY Srinivasan]

> -Original Message-
> From: Christoph Hellwig [mailto:h...@infradead.org]
> Sent: Sunday, January 29, 2017 12:56 AM
> To: KY Srinivasan 
> Cc: Christoph Hellwig ; Cathy Avery
> ; Haiyang Zhang ;
> j...@linux.vnet.ibm.com; martin.peter...@oracle.com;
> dan.carpen...@oracle.com; de...@linuxdriverproject.org; linux-
> ker...@vger.kernel.org; linux-scsi@vger.kernel.org; f...@redhat.com
> Subject: Re: [PATCH v2 0/2] scsi: storvsc: Add support for FC lightweight 
> host.
> 
> On Sun, Jan 29, 2017 at 12:35:32AM +, KY Srinivasan wrote:
> > Windows has chosen this model for virtualizing FC devices to the guest -
> > without rports (or vports). As I noted in my earlier email, James came
> > up with this notion of a lightweight template almost a year ago. We can
> > certainly pick a  more appropriate name and include better documentation.
> 
> Can we take a step back and figure out what you're trying to
> archive here.
> 
> storsvc is a paravirtualized device interface, and whatever underlies
> it should be of no relevance for the guest.
> 
> Despite that fact Microsoft apparently wants to expose a FC-like
> port_name and node_name to guests for some virtual disks.  Can you
> please explain what the guest is supposed to use them for?

My initial implementation of the virtual FC support on Hyper-V was exactly
what you are proposing here - FC disks were being treated exactly as SCSI
disks. We then had customers (HP Enterprise) that were building storage 
appliances based on FC disks on the host and they required port name and 
node name information to be published in sysfs so that their management tools
would work. They required the port and node ID information to be published
precisely the way the FC transport would publish them (in terms of location).

> 
> And second I'd like to understand what the fascination with the FC
> transport class is to expose these two attributes.  Given that
> your sysfs layout will be entirely different from real FC devices
> I simply don't see any need for that.
> 
> Why can't this whole thing simply be solved by adding sdev_attrs
> for the port_name and node_name to storsvc directly?

No fascination with the FC transport other than it already allowed for 
publishing the
port and node IDs. We can certainly look at other ways to publish this 
information -
the proposal by James to have this "light weight" FC transport was precisely 
for this reason.

Regards,

K. Y
--
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: remove bogus code to transfer struct scatterlist

[KY Srinivasan]

> -Original Message-
> From: Christoph Hellwig [mailto:h...@lst.de]
> Sent: Sunday, January 29, 2017 1:12 AM
> To: KY Srinivasan ; Haiyang Zhang
> ; Stephen Hemminger
> 
> Cc: linux-scsi@vger.kernel.org
> Subject: [PATCH] storvsc: remove bogus code to transfer struct scatterlist
> 
> Remove a piece of code in storvsc_queuecommand that tries to pass the
> physical address of the kernel struct scatterlist pointer to the host.
> 
> Fortunately the code can't ever be reached anyway.

Thanks Christoph.

Reviewed-by: K. Y. Srinivasan 

K. Y

> 
> Signed-off-by: Christoph Hellwig 
> ---
>  drivers/scsi/storvsc_drv.c | 7 ---
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 05526b7..62f4545 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -1511,13 +1511,6 @@ static int storvsc_queuecommand(struct Scsi_Host
> *host, struct scsi_cmnd *scmnd)
>   page_to_pfn(sg_page((cur_sgl)));
>   cur_sgl = sg_next(cur_sgl);
>   }
> -
> - } else if (scsi_sglist(scmnd)) {
> - payload->range.len = length;
> - payload->range.offset =
> - virt_to_phys(scsi_sglist(scmnd)) & (PAGE_SIZE-1);
> - payload->range.pfn_array[0] =
> - virt_to_phys(scsi_sglist(scmnd)) >> PAGE_SHIFT;
>   }
> 
>   cmd_request->payload = payload;
> --
> 2.1.4

--
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: [RFC] hv_storvsc: error handling.

[KY Srinivasan]

> -Original Message-
> From: Stephen Hemminger [mailto:step...@networkplumber.org]
> Sent: Friday, March 3, 2017 4:50 PM
> To: James Bottomley 
> Cc: Hannes Reinecke ; Christoph Hellwig ;
> James Bottomley ; Jens Axboe
> ; Linus Torvalds ;
> Martin K. Petersen ; KY Srinivasan
> ; Dexuan Cui ; Long Li
> ; Josh Poulson ; Adrian
> Suhov (Cloudbase Solutions SRL) ; linux-
> s...@vger.kernel.org; Haiyang Zhang 
> Subject: [RFC] hv_storvsc: error handling.
> 
> Needs more testing but this does fix the observed problem.
> 
> From: Stephen Hemminger 
> 
> 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 
> ---
>  drivers/scsi/storvsc_drv.c | 48 
> --
>  1 file changed, 4 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 638e5f427c90..8cc241fc54b8 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -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 =
>   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],
> --

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. Steph

RE: [RFC] hv_storvsc: error handling.

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:j...@linux.vnet.ibm.com]
> Sent: Saturday, March 4, 2017 1:37 PM
> To: KY Srinivasan ; Stephen Hemminger
> 
> Cc: Hannes Reinecke ; Christoph Hellwig ; Jens
> Axboe ; Linus Torvalds  foundation.org>; Martin K. Petersen ;
> Dexuan Cui ; Long Li ; Josh
> Poulson ; Adrian Suhov (Cloudbase Solutions SRL)
> ; linux-scsi@vger.kernel.org; Haiyang Zhang
> 
> Subject: Re: [RFC] hv_storvsc: error handling.
> 
> On Sat, 2017-03-04 at 21:03 +, KY Srinivasan wrote:
> >
> > > -Original Message-
> > > From: Stephen Hemminger [mailto:step...@networkplumber.org]
> > > Sent: Friday, March 3, 2017 4:50 PM
> > > To: James Bottomley 
> > > Cc: Hannes Reinecke ; Christoph Hellwig ;
> > > James Bottomley ; Jens Axboe
> > > ; Linus Torvalds ;
> > > Martin K. Petersen ; KY Srinivasan
> > > ; Dexuan Cui ; Long Li
> > > ; Josh Poulson ;
> > > Adrian
> > > Suhov (Cloudbase Solutions SRL) ; linux-
> > > s...@vger.kernel.org; Haiyang Zhang 
> > > Subject: [RFC] hv_storvsc: error handling.
> > >
> > > Needs more testing but this does fix the observed problem.
> > >
> > > From: Stephen Hemminger 
> > >
> > > 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 
> > > ---
> > >  drivers/scsi/storvsc_drv.c | 48 --
> > > 
> > >  1 file changed, 4 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/drivers/scsi/storvsc_drv.c
> > > b/drivers/scsi/storvsc_drv.c
> > > index 638e5f427c90..8cc241fc54b8 100644
> > > --- a/drivers/scsi/storvsc_drv.c
> > > +++ b/drivers/scsi/storvsc_drv.c
> > > @@ -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 

RE: [RFC] hv_storvsc: error handling.

[KY Srinivasan]

> -Original Message-
> From: KY Srinivasan
> Sent: Saturday, March 4, 2017 1:40 PM
> To: 'James Bottomley' ; Stephen Hemminger
> 
> Cc: Hannes Reinecke ; Christoph Hellwig ; Jens
> Axboe ; Linus Torvalds  foundation.org>; Martin K. Petersen ;
> Dexuan Cui ; Long Li ; Josh
> Poulson ; Adrian Suhov (Cloudbase Solutions SRL)
> ; linux-scsi@vger.kernel.org; Haiyang Zhang
> 
> Subject: RE: [RFC] hv_storvsc: error handling.
> 
> 
> 
> > -Original Message-
> > From: James Bottomley [mailto:j...@linux.vnet.ibm.com]
> > Sent: Saturday, March 4, 2017 1:37 PM
> > To: KY Srinivasan ; Stephen Hemminger
> > 
> > Cc: Hannes Reinecke ; Christoph Hellwig ;
> Jens
> > Axboe ; Linus Torvalds  > foundation.org>; Martin K. Petersen ;
> > Dexuan Cui ; Long Li ; Josh
> > Poulson ; Adrian Suhov (Cloudbase Solutions
> SRL)
> > ; linux-scsi@vger.kernel.org; Haiyang Zhang
> > 
> > Subject: Re: [RFC] hv_storvsc: error handling.
> >
> > On Sat, 2017-03-04 at 21:03 +, KY Srinivasan wrote:
> > >
> > > > -Original Message-
> > > > From: Stephen Hemminger [mailto:step...@networkplumber.org]
> > > > Sent: Friday, March 3, 2017 4:50 PM
> > > > To: James Bottomley 
> > > > Cc: Hannes Reinecke ; Christoph Hellwig
> ;
> > > > James Bottomley ; Jens Axboe
> > > > ; Linus Torvalds ;
> > > > Martin K. Petersen ; KY Srinivasan
> > > > ; Dexuan Cui ; Long Li
> > > > ; Josh Poulson ;
> > > > Adrian
> > > > Suhov (Cloudbase Solutions SRL) ; linux-
> > > > s...@vger.kernel.org; Haiyang Zhang 
> > > > Subject: [RFC] hv_storvsc: error handling.
> > > >
> > > > Needs more testing but this does fix the observed problem.
> > > >
> > > > From: Stephen Hemminger 
> > > >
> > > > 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 
> > > > ---
> > > >  drivers/scsi/storvsc_drv.c | 48 --
> > > > 
> > > >  1 file changed, 4 insertions(+), 44 deletions(-)
> > > >
> > > > diff --git a/drivers/scsi/storvsc_drv.c
> > > > b/drivers/scsi/storvsc_drv.c
> > > > index 638e5f427c90..8cc241fc54b8 100644
> > > > --- a/drivers/scsi/storvsc_drv.c
> > > > +++ b/drivers/scsi/storvsc_drv.c
> > > > @@ -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,
> > > >   

RE: [RFC] hv_storvsc: error handling.

[KY Srinivasan]

> -Original Message-
> From: Stephen Hemminger [mailto:step...@networkplumber.org]
> Sent: Monday, March 6, 2017 8:36 AM
> To: KY Srinivasan 
> Cc: James Bottomley ; Hannes
> Reinecke ; Christoph Hellwig ; James
> Bottomley ; Jens Axboe ;
> Linus Torvalds ; Martin K. Petersen
> ; Dexuan Cui ; Long
> Li ; Josh Poulson ; Adrian
> Suhov (Cloudbase Solutions SRL) ; linux-
> s...@vger.kernel.org; Haiyang Zhang 
> Subject: Re: [RFC] hv_storvsc: error handling.
> 
> On Sat, 4 Mar 2017 21:03:41 +
> KY Srinivasan  wrote:
> 
> > > -Original Message-
> > > From: Stephen Hemminger [mailto:step...@networkplumber.org]
> > > Sent: Friday, March 3, 2017 4:50 PM
> > > To: James Bottomley 
> > > Cc: Hannes Reinecke ; Christoph Hellwig ;
> > > James Bottomley ; Jens Axboe
> > > ; Linus Torvalds ;
> > > Martin K. Petersen ; KY Srinivasan
> > > ; Dexuan Cui ; Long Li
> > > ; Josh Poulson ;
> Adrian
> > > Suhov (Cloudbase Solutions SRL) ; linux-
> > > s...@vger.kernel.org; Haiyang Zhang 
> > > Subject: [RFC] hv_storvsc: error handling.
> > >
> > > Needs more testing but this does fix the observed problem.
> > >
> > > From: Stephen Hemminger 
> > >
> > > 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 
> > > ---
> > >  drivers/scsi/storvsc_drv.c | 48 
> > > --
> > >  1 file changed, 4 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > > index 638e5f427c90..8cc241fc54b8 100644
> > > --- a/drivers/scsi/storvsc_drv.c
> > > +++ b/drivers/scsi/storvsc_drv.c
> > > @@ -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.
> >

RE: [RFC] hv_storvsc: error handling.

[KY Srinivasan]

> -Original Message-
> From: Christoph Hellwig [mailto:h...@lst.de]
> Sent: Monday, March 6, 2017 9:06 PM
> To: KY Srinivasan 
> Cc: Stephen Hemminger ; James
> Bottomley ; Hannes Reinecke
> ; Christoph Hellwig ; James Bottomley
> ; Jens Axboe ; Linus Torvalds
> ; Martin K. Petersen
> ; Dexuan Cui ; Long
> Li ; Josh Poulson ; Adrian
> Suhov (Cloudbase Solutions SRL) ; linux-
> s...@vger.kernel.org; Haiyang Zhang 
> Subject: Re: [RFC] hv_storvsc: error handling.
> 
> > 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.
> 
> 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?

LUN0 on every target is supposed at least return enough data to support
report luns  scan for the target. It looks like if LUN0 on a target is empty DVD
device, the INQUIRY data that is returned from the host is bogus. Stephen can 
give
additional information on this.

K. Y  

RE: [PATCH] storvsc: workaround for virtual DVD SCSI version

[KY Srinivasan]

> -Original Message-
> From: Stephen Hemminger [mailto:step...@networkplumber.org]
> Sent: Tuesday, March 7, 2017 9:16 AM
> To: KY Srinivasan ; Haiyang Zhang
> ; Long Li ;
> martin.peter...@oracle.com; h...@lst.de; h...@suse.de
> Cc: linux-scsi@vger.kernel.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; Stephen Hemminger
> 
> Subject: [PATCH] storvsc: workaround for virtual DVD SCSI version
> 
> Hyper-V host emulation of SCSI for virtual DVD device reports SCSI
> version 0 (UNKNOWN) but is still capable of supporting REPORTLUN.
> 
> Without this patch, a GEN2 Linux guest on Hyper-V will not boot 4.11
> successfully with virtual DVD ROM device. What happens is that the
> SCSI scan process falls back to doing sequential probing by INQUIRY.
> But the storvsc driver has a previous workaround that masks/blocks all
> errors reports from INQUIRY (or MODE_SENSE) commands.  This workaround
> causes the scan to then populate a full set of bogus LUN's on the
> target and then sends kernel spinning off into a death spiral doing
> block reads on the non-existent LUNs.
> 
> By setting the correct blacklist flags, the target with the
> DVD device is scanned with REPORTLUN and that works correctly.
> 
> Patch needs to go in current 4.11, it is safe but not necessary
> in older kernels.
> 
> Signed-off-by: Stephen Hemminger 

Reviewed-by: K. Y. Srinivasan 
> ---
>  drivers/scsi/storvsc_drv.c | 27 +--
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> PS: The error handling does need to be fixed (have patches pending)
> but that is interrelated with hotplug and can wait.
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 638e5f427c90..19973e874830 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -400,8 +400,6 @@
> MODULE_PARM_DESC(storvsc_vcpus_per_sub_channel, "Ratio of VCPUs to
> subchannels")
>   */
>  static int storvsc_timeout = 180;
> 
> -static int msft_blist_flags = BLIST_TRY_VPD_PAGES;
> -
>  #if IS_ENABLED(CONFIG_SCSI_FC_ATTRS)
>  static struct scsi_transport_template *fc_transport_template;
>  #endif
> @@ -1383,6 +1381,22 @@ static int storvsc_do_io(struct hv_device *device,
>   return ret;
>  }
> 
> +static int storvsc_device_alloc(struct scsi_device *sdevice)
> +{
> + /*
> +  * Set blist flag to permit the reading of the VPD pages even when
> +  * the target may claim SPC-2 compliance. MSFT targets currently
> +  * claim SPC-2 compliance while they implement post SPC-2 features.
> +  * With this flag we can correctly handle WRITE_SAME_16 issues.
> +  *
> +  * Hypervisor reports SCSI_UNKNOWN type for DVD ROM device but
> +  * still supports REPORT LUN.
> +  */
> + sdevice->sdev_bflags = BLIST_REPORTLUN2 |
> BLIST_TRY_VPD_PAGES;
> +
> + return 0;
> +}
> +
>  static int storvsc_device_configure(struct scsi_device *sdevice)
>  {
> 
> @@ -1396,14 +1410,6 @@ static int storvsc_device_configure(struct
> scsi_device *sdevice)
>   sdevice->no_write_same = 1;
> 
>   /*
> -  * Add blist flags to permit the reading of the VPD pages even when
> -  * the target may claim SPC-2 compliance. MSFT targets currently
> -  * claim SPC-2 compliance while they implement post SPC-2 features.
> -  * With this patch we can correctly handle WRITE_SAME_16 issues.
> -  */
> - sdevice->sdev_bflags |= msft_blist_flags;
> -
> - /*
>* If the host is WIN8 or WIN8 R2, claim conformance to SPC-3
>* if the device is a MSFT virtual device.  If the host is
>* WIN10 or newer, allow write_same.
> @@ -1661,6 +1667,7 @@ static struct scsi_host_template scsi_driver = {
>   .eh_host_reset_handler =storvsc_host_reset_handler,
>   .proc_name ="storvsc_host",
>   .eh_timed_out = storvsc_eh_timed_out,
> + .slave_alloc =  storvsc_device_alloc,
>   .slave_configure =  storvsc_device_configure,
>   .cmd_per_lun =  255,
>   .this_id =  -1,
> --
> 2.11.0

RE: [PATCH] SCSI: Add a blacklist flag which enables VPD page inquiries

[KY Srinivasan]

> -Original Message-
> From: Christoph Hellwig [mailto:h...@infradead.org]
> Sent: Friday, July 18, 2014 8:04 AM
> To: Martin K. Petersen
> Cc: linux-scsi@vger.kernel.org; KY Srinivasan
> Subject: Re: [PATCH] SCSI: Add a blacklist flag which enables VPD page
> inquiries
> 
> On Tue, Jul 15, 2014 at 12:49:17PM -0400, Martin K. Petersen wrote:
> >
> > Despite supporting modern SCSI features some storage devices continue
> > to claim conformance to an older version of the SPC spec. This is done
> > for compatibility with legacy operating systems.
> >
> > Linux by default will not attempt to read VPD pages on devices that
> > claim SPC-2 or older. Introduce a blacklist flag that can be used to
> > trigger VPD page inquiries on devices that are known to support them.
> >
> > Reported-by: KY Srinivasan 
> > Tested-by: KY Srinivasan 
> > Signed-off-by: Martin K. Petersen 
> > CC: 
> 
> Looks good,
> 
> Reviewed-by: Christoph Hellwig 
> 
> KY, can you send a hyperv patch that sets it to go along with this one?
Will do. Thanks Christoph.

K. Y
--
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 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT from the basic I/O timeout

[KY Srinivasan]

> -Original Message-
> From: Christoph Hellwig (h...@infradead.org) [mailto:h...@infradead.org]
> Sent: Friday, July 18, 2014 8:11 AM
> To: KY Srinivasan
> Cc: Jens Axboe; James Bottomley; micha...@cs.wisc.edu; Christoph Hellwig
> (h...@infradead.org); linux-scsi@vger.kernel.org;
> gre...@linuxfoundation.org; jasow...@redhat.com; linux-
> ker...@vger.kernel.org; oher...@suse.com; a...@canonical.com;
> de...@linuxdriverproject.org
> Subject: Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT
> from the basic I/O timeout
> 
> On Thu, Jul 17, 2014 at 11:53:33PM +, KY Srinivasan wrote:
> > I still see this problem. There was talk of fixing it elsewhere.
> 
> Well, what we have right not is entirely broken, given that the block layer
> doesn't initialize ->timeout on TYPE_FS requeuests.
> 
> We either need to revert that initial commit or apply something like the
> attached patch as a quick fix.
I had sent this exact patch sometime back:

http://www.spinics.net/lists/linux-scsi/msg75385.html

K. Y


--
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 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT from the basic I/O timeout

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:jbottom...@parallels.com]
> Sent: Friday, July 18, 2014 9:57 AM
> To: KY Srinivasan
> Cc: linux-ker...@vger.kernel.org; h...@infradead.org; a...@canonical.com;
> de...@linuxdriverproject.org; micha...@cs.wisc.edu; ax...@kernel.dk;
> linux-scsi@vger.kernel.org; oher...@suse.com;
> gre...@linuxfoundation.org; jasow...@redhat.com
> Subject: Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT
> from the basic I/O timeout
> 
> On Fri, 2014-07-18 at 16:44 +, KY Srinivasan wrote:
> >
> > > -Original Message-
> > > From: Christoph Hellwig (h...@infradead.org)
> > > [mailto:h...@infradead.org]
> > > Sent: Friday, July 18, 2014 8:11 AM
> > > To: KY Srinivasan
> > > Cc: Jens Axboe; James Bottomley; micha...@cs.wisc.edu; Christoph
> > > Hellwig (h...@infradead.org); linux-scsi@vger.kernel.org;
> > > gre...@linuxfoundation.org; jasow...@redhat.com; linux-
> > > ker...@vger.kernel.org; oher...@suse.com; a...@canonical.com;
> > > de...@linuxdriverproject.org
> > > Subject: Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the
> > > FLUSH_TIMEOUT from the basic I/O timeout
> > >
> > > On Thu, Jul 17, 2014 at 11:53:33PM +, KY Srinivasan wrote:
> > > > I still see this problem. There was talk of fixing it elsewhere.
> > >
> > > Well, what we have right not is entirely broken, given that the
> > > block layer doesn't initialize ->timeout on TYPE_FS requeuests.
> > >
> > > We either need to revert that initial commit or apply something like
> > > the attached patch as a quick fix.
> > I had sent this exact patch sometime back:
> >
> > http://www.spinics.net/lists/linux-scsi/msg75385.html
> 
> Actually, no you didn't.  The difference is in the derivation of the timeout.
> Christoph's patch is absolute in terms of SD_TIMEOUT; yours is relative to the
> queue timeout setting ... I thought there was a reason for preferring the
> relative version.

You are right; sorry about that. I think my version is better since we do want 
to base the
flush timeout relative to the basic timeout.

K. Y
> 
> James

--
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 1/1] Drivers: scsi: storvsc: Add blist flags

[KY Srinivasan]

> -Original Message-
> From: Christoph Hellwig [mailto:h...@infradead.org]
> Sent: Monday, July 21, 2014 4:27 AM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; a...@canonical.com;
> jasow...@redhat.com; jbottom...@parallels.com; h...@infradead.org;
> linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 1/1] Drivers: scsi: storvsc: Add blist flags
> 
> On Sun, Jul 20, 2014 at 08:33:42PM -0700, K. Y. Srinivasan wrote:
> > Add blist flags to permit the reading of the VPD pages even when the
> > target may claim SPC-2 compliance. MSFT targets currently claim SPC-2
> > compliance while they implement post SPC-2 features.
> > With this patch we can correctly handle WRITE_SAME_16 issues.
> >
> > Signed-off-by: K. Y. Srinivasan 
> 
> This looks way to complicated - should be a single line added to your
> slave_configure function, maybe plus a comment stating what you do in your
> commit message:
> 
> 
>   sdev->sdev_bflags |= BLIST_TRY_VPD_PAGES;

Thanks Christoph. We can go with this. I will re-send the patch.


K. Y
--
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: Add a blacklist flag which enables VPD page inquiries

[KY Srinivasan]

> -Original Message-
> From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
> Sent: Tuesday, July 15, 2014 9:49 AM
> To: linux-scsi@vger.kernel.org
> Cc: KY Srinivasan
> Subject: [PATCH] SCSI: Add a blacklist flag which enables VPD page inquiries
> 
> 
> Despite supporting modern SCSI features some storage devices continue to
> claim conformance to an older version of the SPC spec. This is done for
> compatibility with legacy operating systems.
> 
> Linux by default will not attempt to read VPD pages on devices that claim
> SPC-2 or older. Introduce a blacklist flag that can be used to trigger VPD 
> page
> inquiries on devices that are known to support them.
> 
> Reported-by: KY Srinivasan 
> Tested-by: KY Srinivasan 

Reviewed-by: KY Srinivasan 

> Signed-off-by: Martin K. Petersen 
> CC: 
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index
> 4a6e4ba5a400..a5b1a224628a 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -949,7 +949,9 @@ static int scsi_add_lun(struct scsi_device *sdev,
> unsigned char *inq_result,
> 
>   sdev->eh_timeout = SCSI_DEFAULT_EH_TIMEOUT;
> 
> - if (*bflags & BLIST_SKIP_VPD_PAGES)
> + if (*bflags & BLIST_TRY_VPD_PAGES)
> + sdev->try_vpd_pages = 1;
> + else if (*bflags & BLIST_SKIP_VPD_PAGES)
>   sdev->skip_vpd_pages = 1;
> 
>   transport_configure_device(&sdev->sdev_gendev);
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index
> 87566b51fcf7..31d32b9077ca 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2701,6 +2701,11 @@ static void sd_read_write_same(struct scsi_disk
> *sdkp, unsigned char *buffer)
> 
>  static int sd_try_extended_inquiry(struct scsi_device *sdp)  {
> + /* Attempt VPD inquiry if the device blacklist explicitly calls
> +  * for it.
> +  */
> + if (sdp->try_vpd_pages)
> + return 1;
>   /*
>* Although VPD inquiries can go to SCSI-2 type devices,
>* some USB ones crash on receiving them, and the pages diff --git
> a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index
> 9aa38f7b303b..f579408620f0 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -155,6 +155,7 @@ struct scsi_device {
>   unsigned skip_ms_page_8:1;  /* do not use MODE SENSE page 0x08
> */
>   unsigned skip_ms_page_3f:1; /* do not use MODE SENSE page 0x3f
> */
>   unsigned skip_vpd_pages:1;  /* do not read VPD pages */
> + unsigned try_vpd_pages:1;   /* attempt to read VPD pages */
>   unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page
> 0x3f */
>   unsigned no_start_on_add:1; /* do not issue start on add */
>   unsigned allow_restart:1; /* issue START_UNIT in error handler */
> diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h index
> 8670c04e199e..1fdd6fc5492b 100644
> --- a/include/scsi/scsi_devinfo.h
> +++ b/include/scsi/scsi_devinfo.h
> @@ -34,4 +34,5 @@
>  #define BLIST_SKIP_VPD_PAGES 0x400 /* Ignore SBC-3 VPD pages
> */
>  #define BLIST_SCSI3LUN   0x800 /* Scan more than 256
> LUNs
>for sequential scan */
> +#define BLIST_TRY_VPD_PAGES  0x1000 /* Attempt to read VPD
> pages */
>  #endif
--
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 1/1] Drivers: scsi: storvsc: Add blist flags

[KY Srinivasan]

> -Original Message-
> From: Sitsofe Wheeler [mailto:sits...@gmail.com]
> Sent: Wednesday, July 23, 2014 3:05 AM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; a...@canonical.com;
> jasow...@redhat.com; jbottom...@parallels.com; h...@infradead.org;
> linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 1/1] Drivers: scsi: storvsc: Add blist flags
> 
> On Mon, Jul 21, 2014 at 04:06:01PM -0700, K. Y. Srinivasan wrote:
> > Add blist flags to permit the reading of the VPD pages even when the
> > target may claim SPC-2 compliance. MSFT targets currently claim SPC-2
> > compliance while they implement post SPC-2 features.
> > With this patch we can correctly handle WRITE_SAME_16 issues.
> 
> OK I've just seen this as I was about to post a similar patch to get discard
> going on Hyper-V. Will your patches handle Hyper-V pass through devices
> that support discard? The SSD I have here reports the following in the Linux

It should. Hyper-V does support pass through devices.

K. Y
> VM:
> 
> # sg_vpd -p lbpv  /dev/sdc
> Logical block provisioning VPD page (SBC):
>   Unmap command supported (LBPU): 1
>   Write same (16) with unmap bit supported (LBWS): 0
>   Write same (10) with unmap bit supported (LBWS10): 0
>   Logical block provisioning read zeros (LBPRZ): 0
>   Anchored LBAs supported (ANC_SUP): 1
>   Threshold exponent: 0
>   Descriptor present (DP): 0
>   Provisioning type: 0
> 
> # sg_readcap -l /dev/sdc
> Read Capacity results:
>Protection: prot_en=0, p_type=0, p_i_exponent=0
>Logical block provisioning: lbpme=0, lbprz=0
>Last logical block address=234441647 (0xdf94baf), Number of logical
> blocks=234441648
>Logical block length=512 bytes
>Logical blocks per physical block exponent=0
>Lowest aligned logical block address=0
> Hence:
>Device size: 120034123776 bytes, 114473.5 MiB, 120.03 GB
> 
> --
> Sitsofe | http://sucs.org/~sits/
--
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 1/1] Drivers: scsi: storvsc: Add blist flags

[KY Srinivasan]

> -Original Message-
> From: Christoph Hellwig [mailto:h...@infradead.org]
> Sent: Wednesday, July 23, 2014 7:10 AM
> To: Sitsofe Wheeler
> Cc: KY Srinivasan; gre...@linuxfoundation.org; linux-
> ker...@vger.kernel.org; de...@linuxdriverproject.org; oher...@suse.com;
> a...@canonical.com; jasow...@redhat.com; jbottom...@parallels.com;
> linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 1/1] Drivers: scsi: storvsc: Add blist flags
> 
> On Wed, Jul 23, 2014 at 01:54:43PM +0100, Sitsofe Wheeler wrote:
> > That's good to know (I was worried the device would not be detected as
> > supporting discard because it doesn't report lbpme and doesn't declare
> > a conformance version (see below)).
> 
> Ok, that makes things worse - you might be able to force it through sysfs,
> though.
> 
> > Is there a tree with all these updates in or do they need to be pieced
> > together from this email thread?
> 
> I'm picking this up for my scsi-queue tree.  Still need a review for the patch
> we're replying to before that one can go in, though.

Hannes, could you please review this patch. Christoph, other than the review of 
this patch,
Is there something you want done before these can be committed.

Regards,

K. Y
> 
> > Additionally is it worth quirking sd_try_rc16_first on when
> > BLIST_TRY_VPD_PAGES is present?
> 
> Sounds reasonable to me.
--
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 v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[KY Srinivasan]

> -Original Message-
> From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
> Sent: Thursday, July 24, 2014 8:54 AM
> To: Sitsofe Wheeler
> Cc: Martin K. Petersen; Christoph Hellwig; KY Srinivasan;
> gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; a...@canonical.com;
> jasow...@redhat.com; jbottom...@parallels.com; linux-
> s...@vger.kernel.org
> Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
> 
> >>>>> "Sitsofe" == Sitsofe Wheeler  writes:
> 
> Sitsofe> So we can see it is really a SATA device that announces discard
> Sitsofe> correctly and supports discard through WRITE_SAME(16).
> 
> No, that's the SATA device that announces support for DSM TRIM, and as a
> result the Linux SATL reports support for WRITE SAME(16) w. the UNMAP bit
> set and LBPME.
> 
> Sitsofe> It is the act of passing it through Hyper-V that turned it into
> Sitsofe> a SCSI device that supports UNMAP (but not WRITE_SAME(16)),
> Sitsofe> doesn't announce its SCSI conformance number and doesn't
> Sitsofe> correctly announce which features it supports. Surely in this
> Sitsofe> case it's reasonable to quirk our way around the problem?
> 
> No. That's an issue in Hyper-V that'll you'll have to take up with Microsoft. 
> I
> don't know what their passthrough limitations are for SCSI-ATA translation.
> Maybe K. Y. has some insight into this?

For the pass through case, the host validates the request and passes the 
request to the device.
However, not all scsi commands are passed through even though the device it is 
being passed through
may support the command. WRITE_SAME is one such command. Consequently, in the 
EVPD page,
we will set state indicating that WRITE_SAME is not supported (even if the 
device supports it).

Hope this helps.

K. Y 
> 
> There must be a reason why the VPD page was added and yet the device not
> flagged as LBPME=1.
> 
> Many vendors do not support UNMAP/WRITE SAME to DSM TRIM
> translation.
> Additionally, many vendors explicitly only whitelist drives that are known to
> be working correctly. Your drive is an ADATA and therefore very likely to be
> blacklisted by default by a vendor SATL.
> 
> --
> Martin K. PetersenOracle Linux Engineering
--
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 v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:jbottom...@parallels.com]
> Sent: Friday, July 25, 2014 10:10 AM
> To: KY Srinivasan
> Cc: linux-ker...@vger.kernel.org; h...@infradead.org; sits...@gmail.com;
> de...@linuxdriverproject.org; a...@canonical.com;
> martin.peter...@oracle.com; linux-scsi@vger.kernel.org;
> oher...@suse.com; gre...@linuxfoundation.org; jasow...@redhat.com
> Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
> 
> On Fri, 2014-07-25 at 16:47 +, KY Srinivasan wrote:
> >
> > > -Original Message-
> > > From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
> > > Sent: Thursday, July 24, 2014 8:54 AM
> > > To: Sitsofe Wheeler
> > > Cc: Martin K. Petersen; Christoph Hellwig; KY Srinivasan;
> > > gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> > > de...@linuxdriverproject.org; oher...@suse.com; a...@canonical.com;
> > > jasow...@redhat.com; jbottom...@parallels.com; linux-
> > > s...@vger.kernel.org
> > > Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks
> > > tests
> > >
> > > >>>>> "Sitsofe" == Sitsofe Wheeler  writes:
> > >
> > > Sitsofe> So we can see it is really a SATA device that announces
> > > Sitsofe> discard correctly and supports discard through WRITE_SAME(16).
> > >
> > > No, that's the SATA device that announces support for DSM TRIM, and
> > > as a result the Linux SATL reports support for WRITE SAME(16) w. the
> > > UNMAP bit set and LBPME.
> > >
> > > Sitsofe> It is the act of passing it through Hyper-V that turned it
> > > Sitsofe> into a SCSI device that supports UNMAP (but not
> > > Sitsofe> WRITE_SAME(16)), doesn't announce its SCSI conformance
> > > Sitsofe> number and doesn't correctly announce which features it
> > > Sitsofe> supports. Surely in this case it's reasonable to quirk our way
> around the problem?
> > >
> > > No. That's an issue in Hyper-V that'll you'll have to take up with
> > > Microsoft. I don't know what their passthrough limitations are for SCSI-
> ATA translation.
> > > Maybe K. Y. has some insight into this?
> >
> > For the pass through case, the host validates the request and passes
> > the request to the device.
> > However, not all scsi commands are passed through even though the
> > device it is being passed through may support the command. WRITE_SAME
> > is one such command. Consequently, in the EVPD page, we will set state
> > indicating that WRITE_SAME is not supported (even if the device
> > supports it).
> 
> I think you haven't appreciated the problem: He's passing a SATA SSD via the
> SCSI hyper-v interface.  That means that the windows host is doing SCSI<-
> >ATA translation.  The problem is that the Windows translation layer (SATL)
> looks to be incomplete and it's not correctly translating the IDENTIFY bit 
> that
> corresponds to TRIM to the correct VPD pages so consequently, Linux  won't
> send UNMAP commands to the device (to be translated back to TRIM).
> 
> We already know this is a bug in the Windows SATL which needs fixing (if you
> could report it and get a fix, that would be great) and that we're not going 
> to
> be able to work around this automatically in Linux because the proposed
> patch would have us unconditionally try UNMAP for all Hyper-V devices.  The
> current proposed fix is to enable UNMAP manually via sysfs in the guest boot
> scripts, but obviously that means that Hyper-V guests with direct pass
> through of SSDs need operator intervention to turn on TRIM.

James,

Thanks for the clarification. I am talking to the folks in MSFT that develop 
the native scsi
stack on Windows. Hyper-V's back-end driver is not involved in SATL. I will 
keep you guys
posted.

Regards,

K. Y
> 
> James

RE: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[KY Srinivasan]

> -Original Message-
> From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
> Sent: Friday, July 25, 2014 9:57 AM
> To: KY Srinivasan
> Cc: Martin K. Petersen; Sitsofe Wheeler; Christoph Hellwig;
> gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; a...@canonical.com;
> jasow...@redhat.com; jbottom...@parallels.com; linux-
> s...@vger.kernel.org
> Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
> 
> >>>>> "KY" == KY Srinivasan  writes:
> 
> KY> For the pass through case, the host validates the request and passes
> KY> the request to the device.  However, not all scsi commands are
> KY> passed through even though the device it is being passed through may
> KY> support the command. WRITE_SAME is one such command.
> Consequently,
> KY> in the EVPD page, we will set state indicating that WRITE_SAME is
> KY> not supported (even if the device supports it).
> 
> The LBP VPD page flags UNMAP as being supported. Do you actually support
> UNMAP to DSM TRIM SCSI-ATA translation?

Martin,

I have been told by the Windows folks that this is done. I am trying to get
additional details.

K. Y
> 
> One challenge in that department is that a single UNMAP command may turn
> into many, many, many DSM TRIM commands on the underlying SATA
> device.
> That's why we went with WRITE SAME for the internal Linux SATL, capping
> the maximum number of blocks to what we can fit in a single DSM TRIM
> command.
> 
> --
> Martin K. PetersenOracle Linux Engineering
--
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 v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[KY Srinivasan]

> -Original Message-
> From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
> Sent: Saturday, July 26, 2014 9:55 AM
> To: KY Srinivasan
> Cc: Martin K. Petersen; Sitsofe Wheeler; Christoph Hellwig;
> gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; a...@canonical.com;
> jasow...@redhat.com; jbottom...@parallels.com; linux-
> s...@vger.kernel.org
> Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
> 
> >>>>> "KY" == KY Srinivasan  writes:
> 
> >> The LBP VPD page flags UNMAP as being supported. Do you actually
> >> support UNMAP to DSM TRIM SCSI-ATA translation?
> 
> KY> I have been told by the Windows folks that this is done. I am trying
> KY> to get additional details.
> 
> Great! I'd just like to have a reasonable level of confidence in what's
> happening down the stack before I entertain turning something on that's not
> being properly advertised.

As I look at the output of inquiry between Linux on Hyper-V and native Linux, 
is not specifying
conformance level the main issue?

K. Y
> 
> --
> Martin K. PetersenOracle Linux Engineering
--
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 v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[KY Srinivasan]

> -Original Message-
> From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
> Sent: Saturday, July 26, 2014 12:25 PM
> To: KY Srinivasan
> Cc: Martin K. Petersen; Sitsofe Wheeler; Christoph Hellwig;
> gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; a...@canonical.com;
> jasow...@redhat.com; jbottom...@parallels.com; linux-
> s...@vger.kernel.org
> Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
> 
> >>>>> "KY" == KY Srinivasan  writes:
> 
> >> Great! I'd just like to have a reasonable level of confidence in
> >> what's happening down the stack before I entertain turning something
> >> on that's not being properly advertised.
> 
> KY> As I look at the output of inquiry between Linux on Hyper-V and
> KY> native Linux, is not specifying conformance level the main issue?
> 
> The main problem for this particular use case (aside from the issue we've
> already addressed) is that the passthrough device (SATA SSD) has
> LBPME=0 in the READ CAPACITY(16) response. The LBP VPD is correctly
> provided with LBPU flag set but because LBPME is reported as disabled we
> will not attempt to issue UNMAP commands to the device.

Oh; ok. I missed the read_capacity response.

Thanks,

K. Y
> 
> --
> Martin K. PetersenOracle Linux Engineering
--
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 v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[KY Srinivasan]

> -Original Message-
> From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
> Sent: Saturday, July 26, 2014 12:25 PM
> To: KY Srinivasan
> Cc: Martin K. Petersen; Sitsofe Wheeler; Christoph Hellwig;
> gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; a...@canonical.com;
> jasow...@redhat.com; jbottom...@parallels.com; linux-
> s...@vger.kernel.org
> Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
> 
> >>>>> "KY" == KY Srinivasan  writes:
> 
> >> Great! I'd just like to have a reasonable level of confidence in
> >> what's happening down the stack before I entertain turning something
> >> on that's not being properly advertised.
> 
> KY> As I look at the output of inquiry between Linux on Hyper-V and
> KY> native Linux, is not specifying conformance level the main issue?
> 
> The main problem for this particular use case (aside from the issue we've
> already addressed) is that the passthrough device (SATA SSD) has
> LBPME=0 in the READ CAPACITY(16) response. The LBP VPD is correctly
> provided with LBPU flag set but because LBPME is reported as disabled we
> will not attempt to issue UNMAP commands to the device.

Had a chance to chat with the Windows sscsi folks. Here is their response:

"At the time  thin-provisioning was defined, the discovery information was 
first proposed in READ CAPACITY 16 command. And then moved into the new 
dedicated VPD page - B2h. You can see the information reported in this VPD page 
is richer than READ CAPACITY 16 command. As this transition happened during we 
added the feature, Windows uses the newer method that based on VPD page B2h. It 
looks Linux tries to use both new and old method which is weird to me."

Would it make sense for us to work  around this issue for Linux on Hyper-V 
(just use the VPD page instead of looking at the output of READ CAPACITY)?

Regards,

K. Y
> 
> --
> Martin K. PetersenOracle Linux Engineering
--
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 v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[KY Srinivasan]

> -Original Message-
> From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
> Sent: Monday, July 28, 2014 12:03 PM
> To: KY Srinivasan
> Cc: Martin K. Petersen; Sitsofe Wheeler; Christoph Hellwig;
> gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; a...@canonical.com;
> jasow...@redhat.com; jbottom...@parallels.com; linux-
> s...@vger.kernel.org
> Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
> 
> >>>>> "KY" == KY Srinivasan  writes:
> 
> KY,
> 
> KY> "At the time thin-provisioning was defined, the discovery
> KY> information was first proposed in READ CAPACITY 16 command. And
> then
> KY> moved into the new dedicated VPD page - B2h. You can see the
> KY> information reported in this VPD page is richer than READ CAPACITY
> KY> 16 command. As this transition happened during we added the feature,
> KY> Windows uses the newer method that based on VPD page B2h. It looks
> KY> Linux tries to use both new and old method which is weird to me."
> 
> The READ CAPACITY(16) response is not optional.

Ok; that settles the issue then. I will attempt to get it fixed on Windows.

K. Y

> 
> --
> Martin K. PetersenOracle Linux Engineering
--
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 v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:jbottom...@parallels.com]
> Sent: Monday, July 28, 2014 1:03 PM
> To: KY Srinivasan
> Cc: linux-ker...@vger.kernel.org; h...@infradead.org; sits...@gmail.com;
> de...@linuxdriverproject.org; a...@canonical.com;
> martin.peter...@oracle.com; linux-scsi@vger.kernel.org;
> oher...@suse.com; gre...@linuxfoundation.org; jasow...@redhat.com
> Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
> 
> On Mon, 2014-07-28 at 19:05 +, KY Srinivasan wrote:
> >
> > > -Original Message-
> > > From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
> > > Sent: Monday, July 28, 2014 12:03 PM
> > > To: KY Srinivasan
> > > Cc: Martin K. Petersen; Sitsofe Wheeler; Christoph Hellwig;
> > > gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> > > de...@linuxdriverproject.org; oher...@suse.com; a...@canonical.com;
> > > jasow...@redhat.com; jbottom...@parallels.com; linux-
> > > s...@vger.kernel.org
> > > Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks
> > > tests
> > >
> > > >>>>> "KY" == KY Srinivasan  writes:
> > >
> > > KY,
> > >
> > > KY> "At the time thin-provisioning was defined, the discovery
> > > KY> information was first proposed in READ CAPACITY 16 command. And
> > > then
> > > KY> moved into the new dedicated VPD page - B2h. You can see the
> > > KY> information reported in this VPD page is richer than READ
> > > KY> CAPACITY
> > > KY> 16 command. As this transition happened during we added the
> > > KY> feature, Windows uses the newer method that based on VPD page
> > > KY> B2h. It looks Linux tries to use both new and old method which is
> weird to me."
> > >
> > > The READ CAPACITY(16) response is not optional.
> >
> > Ok; that settles the issue then. I will attempt to get it fixed on Windows.
> 
> Like Martin says, this isn't optional either/or; it's mandatory to support 
> the RC
> 16 bits.  If you don't want to get into playing the messenger between us and
> the windows guys on SCSI standards, we'd be happy to communicate
> directly, either by email or a phone meeting.

Thanks James. I will take up your offer if needed.

Regards,

K. Y
N�r��yb�X��ǧv�^�)޺{.n�+{���"�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

RE: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:jbottom...@parallels.com]
> Sent: Monday, July 28, 2014 1:03 PM
> To: KY Srinivasan
> Cc: linux-ker...@vger.kernel.org; h...@infradead.org; sits...@gmail.com;
> de...@linuxdriverproject.org; a...@canonical.com;
> martin.peter...@oracle.com; linux-scsi@vger.kernel.org;
> oher...@suse.com; gre...@linuxfoundation.org; jasow...@redhat.com
> Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
> 
> On Mon, 2014-07-28 at 19:05 +, KY Srinivasan wrote:
> >
> > > -Original Message-
> > > From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
> > > Sent: Monday, July 28, 2014 12:03 PM
> > > To: KY Srinivasan
> > > Cc: Martin K. Petersen; Sitsofe Wheeler; Christoph Hellwig;
> > > gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> > > de...@linuxdriverproject.org; oher...@suse.com; a...@canonical.com;
> > > jasow...@redhat.com; jbottom...@parallels.com; linux-
> > > s...@vger.kernel.org
> > > Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks
> > > tests
> > >
> > > >>>>> "KY" == KY Srinivasan  writes:
> > >
> > > KY,
> > >
> > > KY> "At the time thin-provisioning was defined, the discovery
> > > KY> information was first proposed in READ CAPACITY 16 command. And
> > > then
> > > KY> moved into the new dedicated VPD page - B2h. You can see the
> > > KY> information reported in this VPD page is richer than READ
> > > KY> CAPACITY
> > > KY> 16 command. As this transition happened during we added the
> > > KY> feature, Windows uses the newer method that based on VPD page
> > > KY> B2h. It looks Linux tries to use both new and old method which is
> weird to me."
> > >
> > > The READ CAPACITY(16) response is not optional.
> >
> > Ok; that settles the issue then. I will attempt to get it fixed on Windows.
> 
> Like Martin says, this isn't optional either/or; it's mandatory to support 
> the RC
> 16 bits.  If you don't want to get into playing the messenger between us and
> the windows guys on SCSI standards, we'd be happy to communicate
> directly, either by email or a phone meeting.

We will fix this bug in the next release of Windows; we are also looking at 
backporting the fix to prior versions of Windows.

Thanks,

K. Y

RE: [PATCH 2/2] Drivers: scsi: storvsc: Force discovery of LUNs that may have been removed.

[KY Srinivasan]

> -Original Message-
> From: Christoph Hellwig [mailto:h...@infradead.org]
> Sent: Tuesday, August 19, 2014 10:55 AM
> To: KY Srinivasan
> Cc: h...@suse.de; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; linux-scsi@vger.kernel.org; Martin K. Petersen
> Subject: Re: [PATCH 2/2] Drivers: scsi: storvsc: Force discovery of LUNs that
> may have been removed.
> 
> On Sat, Aug 16, 2014 at 08:09:48PM -0700, K. Y. Srinivasan wrote:
> > The host asks the guest to scan when a LUN is removed or added.
> > The only way a guest can identify the removed LUN is when an I/O is
> > attempted on a removed LUN - the SRB status code indicates that the
> > LUN is invalid. We currently handle this SRB status and remove the device.
> >
> > Rather than waiting for an I/O to remove the device, force the
> > discovery of LUNs that may have been removed prior to discovering LUNs
> > that may have been added.
> 
> This looks pretty reasonable to me, but I wonder if we should move this up
> to common code so that it happens for any host rescan triggered by sysfs or
> other drivers as well.
Ping. Any decision on if/when this patch may be checked in.

Regards,

K. Y

--
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 1/1] Drivers: scsi: storvsc: Get rid of warning messages

[KY Srinivasan]

> -Original Message-
> From: Christoph Hellwig [mailto:h...@infradead.org]
> Sent: Thursday, September 4, 2014 10:40 PM
> To: KY Srinivasan
> Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> oher...@suse.com; jbottom...@parallels.com; linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 1/1] Drivers: scsi: storvsc: Get rid of warning messages
> 
> Looks good to me.
> 
> Olaf, Hannes - can I get another review for this (and the older hyperv
> scanning patch set)?

Olaf, Hannes,

Ping.

K. Y
--
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 1/1] Drivers: scsi: storvsc: Get rid of warning messages

[KY Srinivasan]

> -Original Message-
> From: Christoph Hellwig [mailto:h...@infradead.org]
> Sent: Thursday, September 18, 2014 9:43 AM
> To: Olaf Hering
> Cc: KY Srinivasan; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; jbottom...@parallels.com; linux-
> s...@vger.kernel.org
> Subject: Re: [PATCH 1/1] Drivers: scsi: storvsc: Get rid of warning messages
> 
> On Thu, Sep 18, 2014 at 01:50:17PM +0200, Olaf Hering wrote:
> > On Wed, Sep 17, Christoph Hellwig wrote:
> >
> > > Also the two patches at
> > > http://thread.gmane.org/gmane.linux.drivers.driver-project.devel/562
> > > 42/
> >
> > Wasnt the outcome that this was a bad idea? Or at least doing it
> > globally is bad. In any case, its not something I can help with.
> 
> The outcome was that we don't want to do it globally, but it does make sense
> for the hyperv case.

Yes; it has already solved a few open issues. Please apply.

K. Y
--
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/2] Drivers: scsi: storvsc: Fix issues with hot-add/remove of LUNs

[KY Srinivasan]

> -Original Message-
> From: Christoph Hellwig [mailto:h...@infradead.org]
> Sent: Thursday, September 25, 2014 6:47 AM
> To: KY Srinivasan
> Cc: h...@suse.de; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 0/2] Drivers: scsi: storvsc: Fix issues with hot-
> add/remove of LUNs
> 
> So can anywayone give me a review for those two patches?

Olaf,

Could you review these patches.

K. Y
> 
> On Sat, Aug 16, 2014 at 08:09:14PM -0700, K. Y. Srinivasan wrote:
> > This patch-set addresses issues with LUN hot-add and remove. When the
> > host notifies the guest that a scan is needed, scan the host. Also,
> > prior to discovering new devices that may have been added, ensure we
> > handle the LUN remove case first.
> >
> > K. Y. Srinivasan (2):
> >   Drivers: scsi: storvsc: In responce to a scan event, scan the host
> >   Drivers: scsi: storvsc: Force discovery of LUNs that may have been
> > removed.
> >
> >  drivers/scsi/storvsc_drv.c |   43 -
> --
> >  1 files changed, 32 insertions(+), 11 deletions(-)
> >
> > --
> > 1.7.4.1
> >
> > --
> > 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
> ---end quoted text---
--
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/2] Drivers: scsi: storvsc: Fix issues with hot-add/remove of LUNs

[KY Srinivasan]

> -Original Message-
> From: Christoph Hellwig [mailto:h...@infradead.org]
> Sent: Thursday, September 25, 2014 6:47 AM
> To: KY Srinivasan
> Cc: h...@suse.de; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 0/2] Drivers: scsi: storvsc: Fix issues with hot-
> add/remove of LUNs
> 
> So can anywayone give me a review for those two patches?

Did you get the reviews?

K. Y
> 
> On Sat, Aug 16, 2014 at 08:09:14PM -0700, K. Y. Srinivasan wrote:
> > This patch-set addresses issues with LUN hot-add and remove. When the
> > host notifies the guest that a scan is needed, scan the host. Also,
> > prior to discovering new devices that may have been added, ensure we
> > handle the LUN remove case first.
> >
> > K. Y. Srinivasan (2):
> >   Drivers: scsi: storvsc: In responce to a scan event, scan the host
> >   Drivers: scsi: storvsc: Force discovery of LUNs that may have been
> > removed.
> >
> >  drivers/scsi/storvsc_drv.c |   43 -
> --
> >  1 files changed, 32 insertions(+), 11 deletions(-)
> >
> > --
> > 1.7.4.1
> >
> > --
> > 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
> ---end quoted text---
--
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: Add Hyper-V logical block provisioning quirks

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:james.bottom...@hansenpartnership.com]
> Sent: Saturday, October 11, 2014 10:42 AM
> To: Christoph Hellwig
> Cc: Sitsofe Wheeler; KY Srinivasan; Haiyang Zhang; Christoph Hellwig; Hannes
> Reinecke; linux-scsi@vger.kernel.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org
> Subject: Re: [PATCH 0/3] scsi: Add Hyper-V logical block provisioning quirks
> 
> On Sat, 2014-10-11 at 10:39 -0700, Christoph Hellwig wrote:
> > On Fri, Oct 10, 2014 at 08:49:01AM +0100, Sitsofe Wheeler wrote:
> > > Microsoft Hyper-V virtual disks currently only claim SPC-2
> > > compliance even though they implement post SPC-2 features (such as
> > > thin
> > > provisioning) which means the Linux kernel does not go on to test
> > > for those features even though they are advertised.
> > >
> > > A previous patch attempted to add a quirk to workaround this but the
> > > quirk was only enabled after the features had been scanned for,
> > > wouldn't work for "small" disks and would quirk on  all Hyper-V SCSI
> > > devices (e.g. passthrough disks).
> > >
> > > The new patches partially revert the previous effort, add the quirk
> > > in a more traditional manner to only Hyper-V virtual disks and work
> > > on small virtual disks.
> >
> > This seems like might want a quirk to simply "force" a SPC3 compliance
> > level?
> 
> This was initially suggested, but rejected by Microsoft because of other
> problems advertising SPC-3 compliance brings.  Perhaps the hyper-v
> emulator has matured sufficiently that it will now work OK?

James,

On the current release of Windows (windows 10), we are advertising SPC3 
compliance.
We are ok with declaring compliance to SPC3 in our drivers. 

Regards,

K. Y

RE: [PATCH 0/3] scsi: Add Hyper-V logical block provisioning quirks

[KY Srinivasan]

> -Original Message-
> From: Jeff Leung [mailto:jle...@v10networks.ca]
> Sent: Saturday, October 11, 2014 1:22 PM
> To: KY Srinivasan; James Bottomley; Christoph Hellwig
> Cc: Sitsofe Wheeler; Haiyang Zhang; Christoph Hellwig; Hannes Reinecke;
> linux-scsi@vger.kernel.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org
> Subject: RE: [PATCH 0/3] scsi: Add Hyper-V logical block provisioning quirks
> 
> > On the current release of Windows (windows 10), we are advertising SPC3
> compliance.
> > We are ok with declaring compliance to SPC3 in our drivers.
> If you are going to declare SPC3 compliance in the drivers, are you going to
> put in checks to ensure that SPC-3 compliance doesn't get accidentally
> enabled for hypervisors below Win10?
> 
> I do know for a fact that Ubuntu's kernels already force SPC3 compliance to
> enable specific features such as TRIM on earlier versions of Hyper-V (Namely
> Hyper-V 2012 and 2012 R2).

You are right; Ubuntu has been carrying a patch  that was doing just this and 
this has been
working without any issues on many earlier versions of Windows. (2012 and 2012 
R2).
On windows 10 we don't need any changes in the Linux driver as the host itself 
is
advertising SPC3 compliance. Based on the testing we have done with Ubuntu, we 
are
comfortable picking up that patch.

Regards,

K. Y
> 
> --Jeff
> 
> > Regards,
> >
> > K. Y
> >
> >
> > NryزXvؖ){nlj{zX}zj:v zZzf~zwڢ)jyA
> >
> > i
N�r��yb�X��ǧv�^�)޺{.n�+{���"�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

RE: [PATCH 1/1] scsi: storvsc: Fix a build issue reported by kbuild test robot

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:james.bottom...@hansenpartnership.com]
> Sent: Friday, February 26, 2016 2:25 PM
> To: KY Srinivasan ; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> martin.peter...@oracle.com; h...@suse.de
> Subject: Re: [PATCH 1/1] scsi: storvsc: Fix a build issue reported by kbuild 
> test
> robot
> 
> On Fri, 2016-02-26 at 15:45 -0800, K. Y. Srinivasan wrote:
> > tree:   https://na01.safelinks.protection.outlook.com/?url=https%3a%2
> > f%2fgit.kernel.org%2fpub%2fscm%2flinux%2fkernel%2fgit%2ftorvalds%2fli
> >
> nux.git&data=01%7c01%7ckys%40microsoft.com%7ce2e0622715844b79ad71
> 08d3
> >
> 2796ec3c%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=ubr4GbBaNS
> %2ftO
> > z%2buJBk0CL9N0UNG9x2TidLgy6Yovg4%3d master
> > head:   03c21cb775a313f1ff19be59c5d02df3e3526471
> > commit: dac582417bc449b1f7f572d3f1dd9d23eec15cc9 storvsc: Properly
> > support Fibre Channel devices
> > date:   3 weeks ago
> > config: x86_64-randconfig-s3-01281016 (attached as .config)
> > reproduce:
> > git checkout dac582417bc449b1f7f572d3f1dd9d23eec15cc9
> > # save the attached .config to linux build tree
> > make ARCH=x86_64
> >
> > All errors (new ones prefixed by >>):
> >
> >drivers/built-in.o: In function `storvsc_remove':
> > > > storvsc_drv.c:(.text+0x213af7): undefined reference to
> > > > `fc_remove_host'
> >drivers/built-in.o: In function `storvsc_drv_init':
> > > > storvsc_drv.c:(.init.text+0xcbcc): undefined reference to
> > > > `fc_attach_transport'
> > > > storvsc_drv.c:(.init.text+0xcc06): undefined reference to
> > > > `fc_release_transport'
> >drivers/built-in.o: In function `storvsc_drv_exit':
> > > > storvsc_drv.c:(.exit.text+0x123c): undefined reference to
> > > > `fc_release_transport'
> >
> > With this commit, the storvsc driver depends on FC atttributes. Make
> > this
> > dependency explicit.
> >
> > Signed-off-by: K. Y. Srinivasan 
> > Reported-by: Fengguang Wu 
> > ---
> >  drivers/scsi/Kconfig |1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> > index 64eed87..24365c3 100644
> > --- a/drivers/scsi/Kconfig
> > +++ b/drivers/scsi/Kconfig
> > @@ -594,6 +594,7 @@ config XEN_SCSI_FRONTEND
> >  config HYPERV_STORAGE
> > tristate "Microsoft Hyper-V virtual storage driver"
> > depends on SCSI && HYPERV
> > +   depends on SCSI_FC_ATTRS
> 
> Well, I suppose continually sending the wrong patch until I get annoyed
> enough to send the right one is one way of doing it.  This patch is
> wrong. what you want is below.

James, that was not my intent; although it is a tempting strategy!
Here is an excerpt of your comments on v2 of this patch:
(dated Jan 29 of this year):

"No ... if you want to depend on the FC_ATTRS then a simple depend
works.  If you want to be able to build without them or with them, then
that line must read

depends on m || SCSI_FC_ATTRS != m"

Since all I wanted was to depend on FC_ATTRS I chose to go with your 
recommendation -
which also happened to be what I had initially sent (v1 of this patch).

> 
> You want HYPERV_STORAGE to be built in if the FC attributes are,
> otherwise you don't care because if they're N the FC code will be
> compiled out.

If FC attributes are built in, I have no issue - I want to be able to build the 
storvsc
driver both as a module as well as built in. However, if storvsc is built as 
part of the kernel,
I want to make sure that FC attributes are also built as part of the kernel. 
The build test failure
that was reported was this case. With this patch, the build issue reported by 
kbuild cannot
happen since if the FC_ATTRS are built as a module, storvsc cannot be builtin.

Regards,

K. Y 
> 
> James
> 
> ---
> 
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index e2f31c9..5ecabdb 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -596,6 +596,7 @@ config XEN_SCSI_FRONTEND
>  config HYPERV_STORAGE
>   tristate "Microsoft Hyper-V virtual storage driver"
>   depends on SCSI && HYPERV
> + depends on m || SCSI_FC_ATTRS
>   default HYPERV
>   help
> Select this option to enable the Hyper-V virtual storage driver.

RE: [PATCH] storvsc: use small sg_tablesize on x86

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:james.bottom...@hansenpartnership.com]
> Sent: Thursday, January 28, 2016 7:37 AM
> To: Olaf Hering 
> Cc: KY Srinivasan ; Haiyang Zhang
> ; linux-ker...@vger.kernel.org; linux-
> s...@vger.kernel.org
> Subject: Re: [PATCH] storvsc: use small sg_tablesize on x86
> 
> On Thu, 2016-01-28 at 07:48 +0100, Olaf Hering wrote:
> > On Wed, Jan 27, James Bottomley wrote:
> >
> > > It's not really architecture independent, is it?  Just use the bit
> > > width config.
> >
> > Again: which one? This driver is not for mips|powerpc|score|sh.
> 
> zgrep CONFIG_.*BIT /proc/config.gz
> [...]
> CONFIG_64BIT=y
> 
> James

Olaf,

Would you be resubmitting this patch?

Thanks,

K. Y

RE: [PATCH 1/1] scsi: storvsc: Fix a build issue reported by kbuild test robot

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:james.bottom...@hansenpartnership.com]
> Sent: Friday, February 26, 2016 3:33 PM
> To: KY Srinivasan ; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> martin.peter...@oracle.com; h...@suse.de
> Subject: Re: [PATCH 1/1] scsi: storvsc: Fix a build issue reported by kbuild 
> test
> robot
> 
> On Fri, 2016-02-26 at 23:22 +, KY Srinivasan wrote:
> >
> > > -Original Message-
> > > From: James Bottomley
> [mailto:james.bottom...@hansenpartnership.com
> > > ]
> > > Sent: Friday, February 26, 2016 2:25 PM
> > > To: KY Srinivasan ; linux-ker...@vger.kernel.org
> > > ;
> > > de...@linuxdriverproject.org; oher...@suse.com;
> > > jbottom...@parallels.com; h...@infradead.org;
> > > linux-scsi@vger.kernel.org;
> > > a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> > > martin.peter...@oracle.com; h...@suse.de
> > > Subject: Re: [PATCH 1/1] scsi: storvsc: Fix a build issue reported
> > > by kbuild test
> > > robot
> > >
> > > On Fri, 2016-02-26 at 15:45 -0800, K. Y. Srinivasan wrote:
> > > > tree:
> > > > https://na01.safelinks.protection.outlook.com/?url=https%3a%2
> > > >
> f%2fgit.kernel.org%2fpub%2fscm%2flinux%2fkernel%2fgit%2ftorvalds%
> > > > 2fli
> > > >
> > >
> nux.git&data=01%7c01%7ckys%40microsoft.com%7ce2e0622715844b79ad71
> > > 08d3
> > > >
> > >
> 2796ec3c%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=ubr4GbBaNS
> > > %2ftO
> > > > z%2buJBk0CL9N0UNG9x2TidLgy6Yovg4%3d master
> > > > head:   03c21cb775a313f1ff19be59c5d02df3e3526471
> > > > commit: dac582417bc449b1f7f572d3f1dd9d23eec15cc9 storvsc:
> > > > Properly
> > > > support Fibre Channel devices
> > > > date:   3 weeks ago
> > > > config: x86_64-randconfig-s3-01281016 (attached as .config)
> > > > reproduce:
> > > > git checkout dac582417bc449b1f7f572d3f1dd9d23eec15cc9
> > > > # save the attached .config to linux build tree
> > > > make ARCH=x86_64
> > > >
> > > > All errors (new ones prefixed by >>):
> > > >
> > > >drivers/built-in.o: In function `storvsc_remove':
> > > > > > storvsc_drv.c:(.text+0x213af7): undefined reference to
> > > > > > `fc_remove_host'
> > > >drivers/built-in.o: In function `storvsc_drv_init':
> > > > > > storvsc_drv.c:(.init.text+0xcbcc): undefined reference to
> > > > > > `fc_attach_transport'
> > > > > > storvsc_drv.c:(.init.text+0xcc06): undefined reference to
> > > > > > `fc_release_transport'
> > > >drivers/built-in.o: In function `storvsc_drv_exit':
> > > > > > storvsc_drv.c:(.exit.text+0x123c): undefined reference to
> > > > > > `fc_release_transport'
> > > >
> > > > With this commit, the storvsc driver depends on FC atttributes.
> > > > Make
> > > > this
> > > > dependency explicit.
> > > >
> > > > Signed-off-by: K. Y. Srinivasan 
> > > > Reported-by: Fengguang Wu 
> > > > ---
> > > >  drivers/scsi/Kconfig |1 +
> > > >  1 files changed, 1 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> > > > index 64eed87..24365c3 100644
> > > > --- a/drivers/scsi/Kconfig
> > > > +++ b/drivers/scsi/Kconfig
> > > > @@ -594,6 +594,7 @@ config XEN_SCSI_FRONTEND
> > > >  config HYPERV_STORAGE
> > > > tristate "Microsoft Hyper-V virtual storage driver"
> > > > depends on SCSI && HYPERV
> > > > +   depends on SCSI_FC_ATTRS
> > >
> > > Well, I suppose continually sending the wrong patch until I get
> > > annoyed
> > > enough to send the right one is one way of doing it.  This patch is
> > > wrong. what you want is below.
> >
> > James, that was not my intent; although it is a tempting strategy!
> > Here is an excerpt of your comments on v2 of this patch:
> > (dated Jan 29 of this year):
> >
> > "No ... if you want to depend on the 

RE: [PATCH] scsi: storvsc: fix SRB_STATUS_ABORTED handling

[KY Srinivasan]

> -Original Message-
> From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com]
> Sent: Monday, March 7, 2016 3:00 AM
> To: linux-scsi@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; KY
> Srinivasan ; Haiyang Zhang
> ; Cathy Avery ; James E.J.
> Bottomley ; Martin K. Petersen"
> 
> Subject: [PATCH] scsi: storvsc: fix SRB_STATUS_ABORTED handling
> 
> 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 

Thanks Vitaly.

Acked-by: K. Y. Srinivasan 
> ---
>  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 1/1] scsi: storvsc: Support manual scan of FC hosts on Hyper-V

[KY Srinivasan]

> -Original Message-
> From: Christoph Hellwig [mailto:h...@infradead.org]
> Sent: Tuesday, March 15, 2016 6:40 AM
> To: KY Srinivasan 
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> martin.peter...@oracle.com; h...@suse.de
> Subject: Re: [PATCH 1/1] scsi: storvsc: Support manual scan of FC hosts on
> Hyper-V
> 
> On Sat, Mar 12, 2016 at 01:52:48PM -0800, K. Y. Srinivasan wrote:
> > The default user scan function associated with FC (fc_user_scan)
> > is not suitable for FC hosts on Hyper-V since we don't have
> > an rport associated with FC host on Hyper-V . Set it to NULL so we can
> > support manual scan of FC targets on Hyper-V.
> 
> This isn't really how the FC transport class in intended to work, but
> neither is the eh_timed_out (which I haven't seen in my tree yet).
> 
> It sounds like storvsc simply shouldn't attach to the FC transport
> if it doesn't actually look like a FC HBA.

Till recently I had not. However, we do support publishing wwn in the guest and
some customers wanted this. That is the reason I am attaching FC transport and 
working
through the issues. With this change, I now have wwn names published in the 
guest and I can
also issue manual scan.


Regards,

K. Y 
--
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 1/1] scsi: storvsc: Support manual scan of FC hosts on Hyper-V

[KY Srinivasan]

> -Original Message-
> From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
> Sent: Tuesday, March 15, 2016 2:25 PM
> To: KY Srinivasan 
> Cc: Christoph Hellwig ; gre...@linuxfoundation.org;
> linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> oher...@suse.com; jbottom...@parallels.com; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> martin.peter...@oracle.com; h...@suse.de
> Subject: Re: [PATCH 1/1] scsi: storvsc: Support manual scan of FC hosts on
> Hyper-V
> 
> >>>>> "KY" == KY Srinivasan  writes:
> 
> KY> Till recently I had not. However, we do support publishing wwn in
> KY> the guest and some customers wanted this. That is the reason I am
> KY> attaching FC transport and working through the issues. With this
> KY> change, I now have wwn names published in the guest and I can also
> KY> issue manual scan.
> 
> Why does it have to look like FC? Will a device identification VPD page
> not do the trick?

How would I get the sysfs files under fc_host if I don't use the FC transport.
The customer scripts expect these sysfs files.

Regards,

K. Y
> 
> --
> Martin K. PetersenOracle Linux Engineering
--
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 1/1] scsi: storvsc: Support manual scan of FC hosts on Hyper-V

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:james.bottom...@hansenpartnership.com]
> Sent: Wednesday, March 16, 2016 4:08 PM
> To: Martin K. Petersen ; KY Srinivasan
> 
> Cc: Christoph Hellwig ; gre...@linuxfoundation.org;
> linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> oher...@suse.com; jbottom...@parallels.com; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> h...@suse.de
> Subject: Re: [PATCH 1/1] scsi: storvsc: Support manual scan of FC hosts on
> Hyper-V
> 
> On Wed, 2016-03-16 at 18:34 -0400, Martin K. Petersen wrote:
> > > > > > > "KY" == KY Srinivasan  writes:
> >
> > KY> How would I get the sysfs files under fc_host if I don't use the
> > FC
> > KY> transport.  The customer scripts expect these sysfs files.
> >
> > Right, but I was interested in finding out why they need those
> > files. And whether an alternative to the FC transport would be a
> > better solution.
> 
> If it's just the wwn file (or a set of other values), we might be able
> to separate that bit out of the FC transport class so you can use it
> independently ... do you have a full list of the files being used?

Wwn files are what we can support on Hyper-V and that is what I want to support
(to address customer requirements).

Regards,

K. Y 

RE: [PATCH 1/1] scsi: storvsc: Support manual scan of FC hosts on Hyper-V

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:james.bottom...@hansenpartnership.com]
> Sent: Wednesday, March 16, 2016 4:41 PM
> To: KY Srinivasan ; Martin K. Petersen
> 
> Cc: Christoph Hellwig ; gre...@linuxfoundation.org;
> linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> oher...@suse.com; jbottom...@parallels.com; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> h...@suse.de
> Subject: Re: [PATCH 1/1] scsi: storvsc: Support manual scan of FC hosts on
> Hyper-V
> 
> On Wed, 2016-03-16 at 23:15 +, KY Srinivasan wrote:
> >
> > > -Original Message-
> > > From: James Bottomley
> [mailto:james.bottom...@hansenpartnership.com
> > > ]
> > > Sent: Wednesday, March 16, 2016 4:08 PM
> > > To: Martin K. Petersen ; KY Srinivasan
> > > 
> > > Cc: Christoph Hellwig ;
> > > gre...@linuxfoundation.org;
> > > linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> > > oher...@suse.com; jbottom...@parallels.com;
> > > linux-scsi@vger.kernel.org;
> > > a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> > > h...@suse.de
> > > Subject: Re: [PATCH 1/1] scsi: storvsc: Support manual scan of FC
> > > hosts on
> > > Hyper-V
> > >
> > > On Wed, 2016-03-16 at 18:34 -0400, Martin K. Petersen wrote:
> > > > > > > > > "KY" == KY Srinivasan  writes:
> > > >
> > > > KY> How would I get the sysfs files under fc_host if I don't use
> > > > the
> > > > FC
> > > > KY> transport.  The customer scripts expect these sysfs files.
> > > >
> > > > Right, but I was interested in finding out why they need those
> > > > files. And whether an alternative to the FC transport would be a
> > > > better solution.
> > >
> > > If it's just the wwn file (or a set of other values), we might be
> > > able
> > > to separate that bit out of the FC transport class so you can use
> > > it
> > > independently ... do you have a full list of the files being used?
> >
> > Wwn files are what we can support on Hyper-V and that is what I want
> > to support (to address customer requirements).
> 
> There is no wwn file.  These are all the possible attributes they could
> use; which one(s) do you want:
> 
>   /*
>* Setup SCSI Host Attributes.
>*/
>   SETUP_HOST_ATTRIBUTE_RD(node_name);
>   SETUP_HOST_ATTRIBUTE_RD(port_name);
>   SETUP_HOST_ATTRIBUTE_RD(permanent_port_name);
>   SETUP_HOST_ATTRIBUTE_RD(supported_classes);
>   SETUP_HOST_ATTRIBUTE_RD(supported_fc4s);
>   SETUP_HOST_ATTRIBUTE_RD(supported_speeds);
>   SETUP_HOST_ATTRIBUTE_RD(maxframe_size);
>   if (ft->vport_create) {
>   SETUP_HOST_ATTRIBUTE_RD_NS(max_npiv_vports);
>   SETUP_HOST_ATTRIBUTE_RD_NS(npiv_vports_inuse);
>   }
>   SETUP_HOST_ATTRIBUTE_RD(serial_number);
>   SETUP_HOST_ATTRIBUTE_RD(manufacturer);
>   SETUP_HOST_ATTRIBUTE_RD(model);
>   SETUP_HOST_ATTRIBUTE_RD(model_description);
>   SETUP_HOST_ATTRIBUTE_RD(hardware_version);
>   SETUP_HOST_ATTRIBUTE_RD(driver_version);
>   SETUP_HOST_ATTRIBUTE_RD(firmware_version);
>   SETUP_HOST_ATTRIBUTE_RD(optionrom_version);
> 
>   SETUP_HOST_ATTRIBUTE_RD(port_id);
>   SETUP_HOST_ATTRIBUTE_RD(port_type);
>   SETUP_HOST_ATTRIBUTE_RD(port_state);
>   SETUP_HOST_ATTRIBUTE_RD(active_fc4s);
>   SETUP_HOST_ATTRIBUTE_RD(speed);
>   SETUP_HOST_ATTRIBUTE_RD(fabric_name);
>   SETUP_HOST_ATTRIBUTE_RD(symbolic_name);
>   SETUP_HOST_ATTRIBUTE_RW(system_hostname);
> 
>   /* Transport-managed attributes */
>   SETUP_PRIVATE_HOST_ATTRIBUTE_RW(dev_loss_tmo);
>   SETUP_PRIVATE_HOST_ATTRIBUTE_RW(tgtid_bind_type);
>   if (ft->issue_fc_host_lip)
>   SETUP_PRIVATE_HOST_ATTRIBUTE_RW(issue_lip);
>   if (ft->vport_create)
>   SETUP_PRIVATE_HOST_ATTRIBUTE_RW(vport_create);
>   if (ft->vport_delete)
>   SETUP_PRIVATE_HOST_ATTRIBUTE_RW(vport_delete);
>   /*
>* Setup Remote Port Attributes.
>*/
>   count=0;
>   SETUP_RPORT_ATTRIBUTE_RD(maxframe_size);
>   SETUP_RPORT_ATTRIBUTE_RD(supported_classes);
>   SETUP_RPORT_ATTRIBUTE_RW(dev_loss_tmo);
>   SETUP_PRIVATE_RPORT_ATTRIBUTE_RD(node_name);
>   SETUP_PRIVATE_RPORT_ATTRIBUTE_RD(port_name);
>   SETUP_PRIVATE_RPORT_ATTRIBUTE_RD(port_id);
>   SETUP_PRIVATE_RPORT_ATTRI

RE: [PATCH 1/1] scsi: storvsc: Support manual scan of FC hosts on Hyper-V

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:james.bottom...@hansenpartnership.com]
> Sent: Friday, March 18, 2016 3:41 PM
> To: KY Srinivasan ; Martin K. Petersen
> 
> Cc: Christoph Hellwig ; gre...@linuxfoundation.org;
> linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> oher...@suse.com; jbottom...@parallels.com; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> h...@suse.de
> Subject: Re: [PATCH 1/1] scsi: storvsc: Support manual scan of FC hosts on
> Hyper-V
> 
> On Thu, 2016-03-17 at 00:01 +, KY Srinivasan wrote:
> > The only attributes I would be interested are:
> > 1) node name
> > 2) port name
> >
> > Ideally, if this can show under /sys/class/fc_host/hostx/port_name
> > and node_name,
> > it will be ideal since all user scripts can work.
> 
> OK, like this?

Yes; thank you very much James. Looking at the patch though, it may be an 
overkill considering how much of the code is duplicated. The current fc 
transport
class does give us the flexibility to control the attributes we want to surface
(fc_function_template). In any case I will test this code and get back to you 
soon.

Regards,

K. Y

 
> 
> From 7af7c428e7e04ddcc87fda12d6571e3dff8ae024 Mon Sep 17 00:00:00
> 2001
> From: James Bottomley 
> Date: Fri, 18 Mar 2016 15:35:45 -0700
> Subject: scsi_transport_fc: introduce lightweight class for virtualization
>  systems
> 
> The FC transport class is very heavily tilted towards helping things
> which operate a fabric (as it should be).  However, there seems to be
> a need for a lightweight version for use in virtual systems that
> simply want to show pass through FC information without making any use
> of the heavyweight functions.  This is an attempt to give them what
> they want: the lightweight class has no vports or rports and only two
> host attributes.  Essentially, it's designed for the HV storvsc
> driver, but if other virtualizataion systems have similar problems, we
> can add more attributes.
> 
> Signed-off-by: James Bottomley 
> ---
>  drivers/scsi/scsi_transport_fc.c | 94
> 
>  include/scsi/scsi_transport_fc.h |  3 ++
>  2 files changed, 97 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_transport_fc.c 
> b/drivers/scsi/scsi_transport_fc.c
> index 8a88226..a9fcb4d 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -351,6 +351,27 @@ struct fc_internal {
> 
>  #define to_fc_internal(tmpl) container_of(tmpl, struct fc_internal, t)
> 
> +#define FC_LW_HOST_NUM_ATTRS 2
> +struct fc_lw_internal {
> + struct scsi_transport_template t;
> + struct fc_function_template *f;
> +
> + /*
> +  * For attributes : each object has :
> +  *   An array of the actual attributes structures
> +  *   An array of null-terminated pointers to the attribute
> +  * structures - used for mid-layer interaction.
> +  *
> +  * The attribute containers for the starget and host are are
> +  * part of the midlayer. As the remote port is specific to the
> +  * fc transport, we must provide the attribute container.
> +  */
> + struct device_attribute
> private_host_attrs[FC_LW_HOST_NUM_ATTRS];
> + struct device_attribute *host_attrs[FC_LW_HOST_NUM_ATTRS + 1];
> +};
> +
> +#define to_fc_lw_internal(tmpl)  container_of(tmpl, struct
> fc_lw_internal, t)
> +
>  static int fc_target_setup(struct transport_container *tc, struct device 
> *dev,
>  struct device *cdev)
>  {
> @@ -472,6 +493,12 @@ static int fc_host_remove(struct transport_container
> *tc, struct device *dev,
>   return 0;
>  }
> 
> +static DECLARE_TRANSPORT_CLASS(fc_lw_host_class,
> +"fc_host",
> +NULL,
> +NULL,
> +NULL);
> +
>  static DECLARE_TRANSPORT_CLASS(fc_host_class,
>  "fc_host",
>  fc_host_setup,
> @@ -1968,6 +1995,25 @@ static int fc_host_match(struct
> attribute_container *cont,
>   return &i->t.host_attrs.ac == cont;
>  }
> 
> +static int fc_lw_host_match(struct attribute_container *cont,
> +   struct device *dev)
> +{
> + struct Scsi_Host *shost;
> + struct fc_lw_internal *i;
> +
> + if (!scsi_is_host_device(dev))
> + return 0;
> +
> + shost = dev_to_shost(dev);
> + if (!shost->transportt  || shost->transportt->host_attrs.ac.class
> + != &fc_lw_host_class.c

RE: [PATCH 1/1] scsi: storvsc: Support manual scan of FC hosts on Hyper-V

[KY Srinivasan]

> -Original Message-
> From: KY Srinivasan
> Sent: Sunday, March 20, 2016 11:59 AM
> To: 'James Bottomley' ; Martin
> K. Petersen 
> Cc: Christoph Hellwig ; gre...@linuxfoundation.org;
> linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> oher...@suse.com; jbottom...@parallels.com; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> h...@suse.de
> Subject: RE: [PATCH 1/1] scsi: storvsc: Support manual scan of FC hosts on
> Hyper-V
> 
> 
> 
> > -Original Message-
> > From: James Bottomley
> [mailto:james.bottom...@hansenpartnership.com]
> > Sent: Friday, March 18, 2016 3:41 PM
> > To: KY Srinivasan ; Martin K. Petersen
> > 
> > Cc: Christoph Hellwig ; gre...@linuxfoundation.org;
> > linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> > oher...@suse.com; jbottom...@parallels.com; linux-scsi@vger.kernel.org;
> > a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> > h...@suse.de
> > Subject: Re: [PATCH 1/1] scsi: storvsc: Support manual scan of FC hosts on
> > Hyper-V
> >
> > On Thu, 2016-03-17 at 00:01 +, KY Srinivasan wrote:
> > > The only attributes I would be interested are:
> > > 1) node name
> > > 2) port name
> > >
> > > Ideally, if this can show under /sys/class/fc_host/hostx/port_name
> > > and node_name,
> > > it will be ideal since all user scripts can work.
> >
> > OK, like this?
> 
> Yes; thank you very much James. Looking at the patch though, it may be an
> overkill considering how much of the code is duplicated. The current fc
> transport
> class does give us the flexibility to control the attributes we want to 
> surface
> (fc_function_template). In any case I will test this code and get back to you
> soon.

Today I got a chance to test this patch. Looks like all the state is not getting
properly set in this new transport class. I am hitting this NULL pointer 
reference fault in
get_device_parent(). Looks like the device class is not properly setup for
this transport class. class_dir_create_and_add() is not called for this class
and so the glue_dirs is NULL.   I fixed the issue:

1) You will need to call the transport_class_register() for this new transport 
class in
fc_transport_init()
2) We cannot use fc_host as the name in this new class since the standard fc 
transport already
Uses that name. I changed the name to get this to work. This will create a new 
directory under /sys/class.
So my original goal of  being compatible with existing scripts that expect to 
find the information under
/sys/class/fc_host will not be met.

Regards,

K. Y


> 
> Regards,
> 
> K. Y
> 
> 
> >
> > From 7af7c428e7e04ddcc87fda12d6571e3dff8ae024 Mon Sep 17 00:00:00
> > 2001
> > From: James Bottomley 
> > Date: Fri, 18 Mar 2016 15:35:45 -0700
> > Subject: scsi_transport_fc: introduce lightweight class for virtualization
> >  systems
> >
> > The FC transport class is very heavily tilted towards helping things
> > which operate a fabric (as it should be).  However, there seems to be
> > a need for a lightweight version for use in virtual systems that
> > simply want to show pass through FC information without making any use
> > of the heavyweight functions.  This is an attempt to give them what
> > they want: the lightweight class has no vports or rports and only two
> > host attributes.  Essentially, it's designed for the HV storvsc
> > driver, but if other virtualizataion systems have similar problems, we
> > can add more attributes.
> >
> > Signed-off-by: James Bottomley 
> > ---
> >  drivers/scsi/scsi_transport_fc.c | 94
> > 
> >  include/scsi/scsi_transport_fc.h |  3 ++
> >  2 files changed, 97 insertions(+)
> >
> > diff --git a/drivers/scsi/scsi_transport_fc.c
> b/drivers/scsi/scsi_transport_fc.c
> > index 8a88226..a9fcb4d 100644
> > --- a/drivers/scsi/scsi_transport_fc.c
> > +++ b/drivers/scsi/scsi_transport_fc.c
> > @@ -351,6 +351,27 @@ struct fc_internal {
> >
> >  #define to_fc_internal(tmpl)   container_of(tmpl, struct fc_internal, 
> > t)
> >
> > +#define FC_LW_HOST_NUM_ATTRS   2
> > +struct fc_lw_internal {
> > +   struct scsi_transport_template t;
> > +   struct fc_function_template *f;
> > +
> > +   /*
> > +* For attributes : each object has :
> > +*   An array of the actual attributes structures
> > +*   An array of null-terminated pointers to the attribute
> > +* structures - used for mid-layer interac

RE: [PATCH 1/1] scsi: storvsc: Support manual scan of FC hosts on Hyper-V

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:james.bottom...@hansenpartnership.com]
> Sent: Friday, March 18, 2016 3:41 PM
> To: KY Srinivasan ; Martin K. Petersen
> 
> Cc: Christoph Hellwig ; gre...@linuxfoundation.org;
> linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> oher...@suse.com; jbottom...@parallels.com; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> h...@suse.de
> Subject: Re: [PATCH 1/1] scsi: storvsc: Support manual scan of FC hosts on
> Hyper-V
> 
> On Thu, 2016-03-17 at 00:01 +, KY Srinivasan wrote:
> > The only attributes I would be interested are:
> > 1) node name
> > 2) port name
> >
> > Ideally, if this can show under /sys/class/fc_host/hostx/port_name
> > and node_name,
> > it will be ideal since all user scripts can work.
> 
> OK, like this?
> 
> From 7af7c428e7e04ddcc87fda12d6571e3dff8ae024 Mon Sep 17 00:00:00
> 2001
> From: James Bottomley 
> Date: Fri, 18 Mar 2016 15:35:45 -0700
> Subject: scsi_transport_fc: introduce lightweight class for virtualization
>  systems
> 
> The FC transport class is very heavily tilted towards helping things
> which operate a fabric (as it should be).  However, there seems to be
> a need for a lightweight version for use in virtual systems that
> simply want to show pass through FC information without making any use
> of the heavyweight functions.  This is an attempt to give them what
> they want: the lightweight class has no vports or rports and only two
> host attributes.  Essentially, it's designed for the HV storvsc
> driver, but if other virtualizataion systems have similar problems, we
> can add more attributes.
> 
> Signed-off-by: James Bottomley 
> ---
>  drivers/scsi/scsi_transport_fc.c | 94
> 
>  include/scsi/scsi_transport_fc.h |  3 ++
>  2 files changed, 97 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_transport_fc.c 
> b/drivers/scsi/scsi_transport_fc.c
> index 8a88226..a9fcb4d 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -351,6 +351,27 @@ struct fc_internal {
> 
>  #define to_fc_internal(tmpl) container_of(tmpl, struct fc_internal, t)
> 
> +#define FC_LW_HOST_NUM_ATTRS 2
> +struct fc_lw_internal {
> + struct scsi_transport_template t;
> + struct fc_function_template *f;
> +
> + /*
> +  * For attributes : each object has :
> +  *   An array of the actual attributes structures
> +  *   An array of null-terminated pointers to the attribute
> +  * structures - used for mid-layer interaction.
> +  *
> +  * The attribute containers for the starget and host are are
> +  * part of the midlayer. As the remote port is specific to the
> +  * fc transport, we must provide the attribute container.
> +  */
> + struct device_attribute
> private_host_attrs[FC_LW_HOST_NUM_ATTRS];
> + struct device_attribute *host_attrs[FC_LW_HOST_NUM_ATTRS + 1];
> +};
> +
> +#define to_fc_lw_internal(tmpl)  container_of(tmpl, struct
> fc_lw_internal, t)
> +
>  static int fc_target_setup(struct transport_container *tc, struct device 
> *dev,
>  struct device *cdev)
>  {
> @@ -472,6 +493,12 @@ static int fc_host_remove(struct transport_container
> *tc, struct device *dev,
>   return 0;
>  }
> 
> +static DECLARE_TRANSPORT_CLASS(fc_lw_host_class,
> +"fc_host",
> +NULL,
> +NULL,
> +NULL);
> +
>  static DECLARE_TRANSPORT_CLASS(fc_host_class,
>  "fc_host",
>  fc_host_setup,
> @@ -1968,6 +1995,25 @@ static int fc_host_match(struct
> attribute_container *cont,
>   return &i->t.host_attrs.ac == cont;
>  }
> 
> +static int fc_lw_host_match(struct attribute_container *cont,
> +   struct device *dev)
> +{
> + struct Scsi_Host *shost;
> + struct fc_lw_internal *i;
> +
> + if (!scsi_is_host_device(dev))
> + return 0;
> +
> + shost = dev_to_shost(dev);
> + if (!shost->transportt  || shost->transportt->host_attrs.ac.class
> + != &fc_lw_host_class.class)
> + return 0;
> +
> + i = to_fc_lw_internal(shost->transportt);
> +
> + return &i->t.host_attrs.ac == cont;
> +}
> +
>  static int fc_target_match(struct attribute_container *cont,
>   struct device *dev)
>  {
> @@ -2171,6 +2217,54 @@ static int fc_

RE: [PATCH 1/1] scsi: storvsc: Filter out storvsc messages CD-ROM medium not present

[KY Srinivasan]

> -Original Message-
> From: Cathy Avery [mailto:cav...@redhat.com]
> Sent: Monday, May 23, 2016 7:29 AM
> To: linux-scsi@vger.kernel.org
> Cc: KY Srinivasan ; Haiyang Zhang
> ; martin.peter...@oracle.com;
> j...@linux.vnet.ibm.com; de...@linuxdriverproject.org; linux-
> ker...@vger.kernel.org
> Subject: [PATCH 1/1] scsi: storvsc: Filter out storvsc messages CD-ROM medium
> not present
> 
> When a virtual scsi DVD device is present with no image file
> attached the storvsc driver logs all resulting unnecessary sense errors
> whenever IO is issued to the device.
> 
> [storvsc] Sense Key : Not Ready [current]
> [storvsc] Add. Sense: Medium not present - tray closed
> 
> Signed-off-by: Cathy Avery 

Thanks Cathy.

Reviewed-by: K. Y. Srinivasan 

> ---
>  drivers/scsi/storvsc_drv.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 3ddcabb..f0efa07 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -966,7 +966,9 @@ static void storvsc_command_completion(struct
> storvsc_cmd_request *cmd_request,
>   if (scmnd->result) {
>   if (scsi_normalize_sense(scmnd->sense_buffer,
>   SCSI_SENSE_BUFFERSIZE, &sense_hdr) &&
> - do_logging(STORVSC_LOGGING_ERROR))
> + !(sense_hdr.sense_key == NOT_READY &&
> +  sense_hdr.asc == 0x03A) &&
> + do_logging(STORVSC_LOGGING_ERROR))
>   scsi_print_sense_hdr(scmnd->device, "storvsc",
>&sense_hdr);
>   }
> --
> 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 V1 1/7] Drivers: hv: vmbus: Implement multi-channel support

[KY Srinivasan]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Thursday, May 16, 2013 10:01 AM
> To: KY Srinivasan
> Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> oher...@suse.com; jbottom...@parallels.com; h...@infradead.org; linux-
> s...@vger.kernel.org; a...@canonical.com; jasow...@redhat.com
> Subject: Re: [PATCH V1 1/7] Drivers: hv: vmbus: Implement multi-channel
> support
> 
> On Thu, May 16, 2013 at 05:21:13AM -0700, K. Y. Srinivasan wrote:
> > Starting with Win8, the host supports multiple sub-channels for a given
> > device. As in the past, the initial channel offer specifies the device and
> > is associated with both the type and the instance GUIDs. For performance
> > critical devices, the host may support multiple sub-channels. The 
> > sub-channels
> > share the same type and instance GUID as the primary channel. The number of
> > sub-channels offerrred to the guest depends on the number of virtual CPUs
> > assigned to the guest. The guest can request the creation of these sub-
> channels
> > and once created and opened, the guest can distribute the traffic across all
> > the channels (the primary and the sub-channels). A request sent on a sub-
> channel
> > will have the response delivered on the same sub-channel.
> >
> > At channel (sub-channel) creation we bind the channel interrupt to a CPU and
> > with this sub-channel support we will be able to spread the interrupt load
> > of a given device across all available CPUs.
> >
> > Signed-off-by: K. Y. Srinivasan 
> > Reviewed-by: Haiyang Zhang 
> 
> Acked-by: Greg Kroah-Hartman 
> 

James,

Please drop this patch-set. I fixed a couple of bugs in the vmbus support for 
multi-channel and I would need to resubmit this. Greg, I think it may make 
sense to first get the vmbus support checked in before I submit the storage 
related patches. There are networking related patches for multi-channel support 
that we will be submitting shortly. Getting the vmbus support in first would 
allow us to decouple network and storage patches. Greg, I will shortly submit 
the next version of the vmbus related patches for your consideration.

Regards,

K. Y

--
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 1/5] Drivers: scsi: storvsc: Make the scsi timeout a module parameter

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:jbottom...@parallels.com]
> Sent: Monday, June 03, 2013 7:03 PM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; h...@infradead.org; linux-
> s...@vger.kernel.org
> Subject: Re: [PATCH 1/5] Drivers: scsi: storvsc: Make the scsi timeout a 
> module
> parameter
> 
> On Mon, 2013-06-03 at 16:21 -0700, K. Y. Srinivasan wrote:
> > The standard scsi timeout is not appropriate in some of the environments
> where
> > Hyper-V is deployed. Set this timeout appropriately for all devices managed
> > by this driver. Further make this a module parameter.
> >
> > Signed-off-by: K. Y. Srinivasan 
> > Reviewed-by: Haiyang Zhang 
> > ---
> >  drivers/scsi/storvsc_drv.c |9 +
> >  1 files changed, 9 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > index 16a3a0c..8d29a95 100644
> > --- a/drivers/scsi/storvsc_drv.c
> > +++ b/drivers/scsi/storvsc_drv.c
> > @@ -221,6 +221,13 @@ static int storvsc_ringbuffer_size = (20 * PAGE_SIZE);
> >  module_param(storvsc_ringbuffer_size, int, S_IRUGO);
> >  MODULE_PARM_DESC(storvsc_ringbuffer_size, "Ring buffer size (bytes)");
> >
> > +/*
> > + * Timeout in seconds for all devices managed by this driver.
> > + */
> > +static int storvsc_timeout = 180;
> > +module_param(storvsc_timeout, uint, (S_IRUGO | S_IWUSR));
> > +MODULE_PARM_DESC(storvsc_timeout, "Device timeout (seconds)");
> > +
> >  #define STORVSC_MAX_IO_REQUESTS128
> >
> >  /*
> > @@ -1204,6 +1211,8 @@ static int storvsc_device_configure(struct scsi_device
> *sdevice)
> >
> > blk_queue_bounce_limit(sdevice->request_queue, BLK_BOUNCE_ANY);
> >
> > +   blk_queue_rq_timeout(sdevice->request_queue, (storvsc_timeout *
> HZ));
> 
> Why does this need to be a module parameter?  It's already a sysfs one
> in the scsi_device class?  Three minutes is also a bit large.  The
> default is 30s with huge cache arrays recommending upping this to
> 60s ... you're three times this.

James,
This number was arrived at based on some testing that was done on the cloud. On 
our cloud, we have a  120 second
timeouts that trigger broader VM level recovery  and in cases where there is 
storage access issues
(which is when we would hit this timeout), it will be better to defer to the 
fabric level recovery than attempt
Scsi level recovery/retry.  The default value chosen for devices managed by 
storvsc should be just fine, 
I made it a module parameter to have more flexibility.

Regards,

K. Y  
> 
> James
> 
> 


--
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 1/5] Drivers: scsi: storvsc: Make the scsi timeout a module parameter

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:jbottom...@parallels.com]
> Sent: Monday, June 03, 2013 7:47 PM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; h...@infradead.org; linux-
> s...@vger.kernel.org
> Subject: Re: [PATCH 1/5] Drivers: scsi: storvsc: Make the scsi timeout a 
> module
> parameter
> 
> On Mon, 2013-06-03 at 23:25 +, KY Srinivasan wrote:
> >
> > > -Original Message-
> > > From: James Bottomley [mailto:jbottom...@parallels.com]
> > > Sent: Monday, June 03, 2013 7:03 PM
> > > To: KY Srinivasan
> > > Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> > > de...@linuxdriverproject.org; oher...@suse.com; h...@infradead.org; linux-
> > > s...@vger.kernel.org
> > > Subject: Re: [PATCH 1/5] Drivers: scsi: storvsc: Make the scsi timeout a
> module
> > > parameter
> > >
> > > On Mon, 2013-06-03 at 16:21 -0700, K. Y. Srinivasan wrote:
> > > > The standard scsi timeout is not appropriate in some of the environments
> > > where
> > > > Hyper-V is deployed. Set this timeout appropriately for all devices 
> > > > managed
> > > > by this driver. Further make this a module parameter.
> > > >
> > > > Signed-off-by: K. Y. Srinivasan 
> > > > Reviewed-by: Haiyang Zhang 
> > > > ---
> > > >  drivers/scsi/storvsc_drv.c |9 +
> > > >  1 files changed, 9 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > > > index 16a3a0c..8d29a95 100644
> > > > --- a/drivers/scsi/storvsc_drv.c
> > > > +++ b/drivers/scsi/storvsc_drv.c
> > > > @@ -221,6 +221,13 @@ static int storvsc_ringbuffer_size = (20 *
> PAGE_SIZE);
> > > >  module_param(storvsc_ringbuffer_size, int, S_IRUGO);
> > > >  MODULE_PARM_DESC(storvsc_ringbuffer_size, "Ring buffer size (bytes)");
> > > >
> > > > +/*
> > > > + * Timeout in seconds for all devices managed by this driver.
> > > > + */
> > > > +static int storvsc_timeout = 180;
> > > > +module_param(storvsc_timeout, uint, (S_IRUGO | S_IWUSR));
> > > > +MODULE_PARM_DESC(storvsc_timeout, "Device timeout (seconds)");
> > > > +
> > > >  #define STORVSC_MAX_IO_REQUESTS128
> > > >
> > > >  /*
> > > > @@ -1204,6 +1211,8 @@ static int storvsc_device_configure(struct
> scsi_device
> > > *sdevice)
> > > >
> > > > blk_queue_bounce_limit(sdevice->request_queue, BLK_BOUNCE_ANY);
> > > >
> > > > +   blk_queue_rq_timeout(sdevice->request_queue, (storvsc_timeout *
> > > HZ));
> > >
> > > Why does this need to be a module parameter?  It's already a sysfs one
> > > in the scsi_device class?  Three minutes is also a bit large.  The
> > > default is 30s with huge cache arrays recommending upping this to
> > > 60s ... you're three times this.
> >
> > James,
> > This number was arrived at based on some testing that was done on the
> > cloud. On our cloud, we have a  120 second
> > timeouts that trigger broader VM level recovery  and in cases where
> > there is storage access issues
> > (which is when we would hit this timeout), it will be better to defer
> > to the fabric level recovery than attempt
> > Scsi level recovery/retry.  The default value chosen for devices
> > managed by storvsc should be just fine,
> 
> So are you sure you want to set the command timeout to 3 minutes? ...
> it's an incredibly high value.  The actual complete timeout is this
> value multiplied by the number of retries, which is 5 for disk devices,
> so you'll be waiting up to 15 minutes before we signal a failure in some
> circumstances.  It sounds like you want the actual path length of error
> recovery to be on average 3 minutes.
>
> The value of the timeout should be a compromise between the longest time
> you want the user to wait for a failure and the longest time a device
> should take to respond.

This should be fine. Note that all error recovery/retry is happening on the 
host side and beyond
a certain delay, we will do a VM level recovery at the fabric level.  On a 
slightly different note,
we have the same issue with the SCSI FLUSH timeout. Would you consider changing 
this.
> 
> > I made it a module parameter to have more flexibility.
> 
> It's *already* a sysfs parameter ... why do you want an additional
> module parameter?  Multiple parameters for the same quantity, especially
> ones which can't be altered at runtime like module parameters, end up
> confusing users.

Agreed. I can send you a patch that would remove this parameter. Or, if you 
prefer
I could resend this set with the change to this patch (removing the module 
parameter).

Regards,

K. Y
> 
> James
> 
> 


--
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 0/5] Drivers: scsi: storvsc

[KY Srinivasan]

> -Original Message-
> From: K. Y. Srinivasan [mailto:k...@microsoft.com]
> Sent: Tuesday, June 04, 2013 3:05 PM
> To: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; jbottom...@parallels.com;
> h...@infradead.org; linux-scsi@vger.kernel.org
> Cc: KY Srinivasan
> Subject: [PATCH V3 0/5] Drivers: scsi: storvsc
> 
> This set adds multi-channel support as well synthetic Fibre Channel support
> to storvsc. The multi-channel support depends on infrastructure in the VMBUS
> driver. Greg has already checked in the relevant patches to VMBUS.
> 
> I had posted an earlier version of this patch-set that included the VMBUS
> related changes. I have since separated the VMBUS chages and these have
> already been
> checked in.
> 
> In this version, based on comments from James, the timeout is no longer a
> module
> parameter.

James,

I think I have addressed all the comments that you had; if not, please let me 
know.

Regards,

K. Y
> 
> K. Y. Srinivasan (5):
>   Drivers: scsi: storvsc: Increase the value of scsi timeout for
> storvsc devices
>   Drivers: scsi: storvsc: Update the storage protocol to win8 level
>   Drivers: scsi: storvsc: Implement multi-channel support
>   Drivers: scsi: storvsc: Support FC devices
>   Drivers: scsi: storvsc: Increase the value of STORVSC_MAX_IO_REQUESTS
> 
>  drivers/scsi/storvsc_drv.c |  347
> +++-
>  1 files changed, 308 insertions(+), 39 deletions(-)
> 
> --
> 1.7.4.1
> 
> 


--
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 0/5] Drivers: scsi: storvsc

[KY Srinivasan]

> -Original Message-
> From: KY Srinivasan
> Sent: Tuesday, June 11, 2013 9:02 AM
> To: KY Srinivasan; gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; jbottom...@parallels.com;
> h...@infradead.org; linux-scsi@vger.kernel.org
> Subject: RE: [PATCH V3 0/5] Drivers: scsi: storvsc
> 
> 
> 
> > -Original Message-
> > From: K. Y. Srinivasan [mailto:k...@microsoft.com]
> > Sent: Tuesday, June 04, 2013 3:05 PM
> > To: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> > de...@linuxdriverproject.org; oher...@suse.com; jbottom...@parallels.com;
> > h...@infradead.org; linux-scsi@vger.kernel.org
> > Cc: KY Srinivasan
> > Subject: [PATCH V3 0/5] Drivers: scsi: storvsc
> >
> > This set adds multi-channel support as well synthetic Fibre Channel support
> > to storvsc. The multi-channel support depends on infrastructure in the VMBUS
> > driver. Greg has already checked in the relevant patches to VMBUS.
> >
> > I had posted an earlier version of this patch-set that included the VMBUS
> > related changes. I have since separated the VMBUS chages and these have
> > already been
> > checked in.
> >
> > In this version, based on comments from James, the timeout is no longer a
> > module
> > parameter.
> 
> James,
> 
> I think I have addressed all the comments that you had; if not, please let me
> know.

Ping.

Regards,

K. Y

--
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 0/5] Drivers: scsi: storvsc

[KY Srinivasan]

> -Original Message-
> From: KY Srinivasan
> Sent: Monday, June 17, 2013 9:32 AM
> To: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; jbottom...@parallels.com;
> h...@infradead.org; linux-scsi@vger.kernel.org
> Subject: RE: [PATCH V3 0/5] Drivers: scsi: storvsc
> 
> 
> 
> > -----Original Message-
> > From: KY Srinivasan
> > Sent: Tuesday, June 11, 2013 9:02 AM
> > To: KY Srinivasan; gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> > de...@linuxdriverproject.org; oher...@suse.com; jbottom...@parallels.com;
> > h...@infradead.org; linux-scsi@vger.kernel.org
> > Subject: RE: [PATCH V3 0/5] Drivers: scsi: storvsc
> >
> >
> >
> > > -Original Message-
> > > From: K. Y. Srinivasan [mailto:k...@microsoft.com]
> > > Sent: Tuesday, June 04, 2013 3:05 PM
> > > To: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> > > de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com;
> > > h...@infradead.org; linux-scsi@vger.kernel.org
> > > Cc: KY Srinivasan
> > > Subject: [PATCH V3 0/5] Drivers: scsi: storvsc
> > >
> > > This set adds multi-channel support as well synthetic Fibre Channel 
> > > support
> > > to storvsc. The multi-channel support depends on infrastructure in the
> VMBUS
> > > driver. Greg has already checked in the relevant patches to VMBUS.
> > >
> > > I had posted an earlier version of this patch-set that included the VMBUS
> > > related changes. I have since separated the VMBUS chages and these have
> > > already been
> > > checked in.
> > >
> > > In this version, based on comments from James, the timeout is no longer a
> > > module
> > > parameter.
> >
> > James,
> >
> > I think I have addressed all the comments that you had; if not, please let 
> > me
> > know.
> 
> Ping.

James,

Let me know if I should re-send the patches.

Thanks,

K. Y

--
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 3/5] Drivers: scsi: storvsc: Implement multi-channel support

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:jbottom...@parallels.com]
> Sent: Wednesday, June 26, 2013 11:22 PM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; h...@infradead.org; linux-
> s...@vger.kernel.org
> Subject: Re: [PATCH 3/5] Drivers: scsi: storvsc: Implement multi-channel 
> support
> 
> On Tue, 2013-06-04 at 12:05 -0700, K. Y. Srinivasan wrote:
> > Implement multi-channel support for the storage devices.
> 
> This doesn't compile:
> 
>   CC [M]  drivers/scsi/storvsc_drv.o
> drivers/scsi/storvsc_drv.c: In function ‘handle_sc_creation’:
> drivers/scsi/storvsc_drv.c:763:35: error: ‘struct vmbus_channel’ has no
> member named ‘primary_channel’
> drivers/scsi/storvsc_drv.c: In function ‘handle_multichannel_storage’:
> drivers/scsi/storvsc_drv.c:805:2: error: implicit declaration of
> function
> ‘vmbus_set_sc_create_callback’ [-Werror=implicit-function-declaration]
> drivers/scsi/storvsc_drv.c:812:2: error: implicit declaration of
> function
> ‘vmbus_are_subchannels_present’ [-Werror=implicit-function-declaration]
> drivers/scsi/storvsc_drv.c: In function ‘storvsc_on_channel_callback’:
> drivers/scsi/storvsc_drv.c:1223:13: error: ‘struct vmbus_channel’ has no
> member named ‘primary_channel’
> drivers/scsi/storvsc_drv.c:1224:19: error: ‘struct vmbus_channel’ has no
> member named ‘primary_channel’
> drivers/scsi/storvsc_drv.c: In function ‘storvsc_do_io’:
> drivers/scsi/storvsc_drv.c:1341:2: error: implicit declaration of
> function
> ‘vmbus_get_outgoing_channel’ [-Werror=implicit-function-declaration]
> drivers/scsi/storvsc_drv.c:1341:19: warning: assignment makes pointer
> from integer without a cast [enabled by default]
> cc1: some warnings being treated as errors
> make[2]: *** [drivers/scsi/storvsc_drv.o] Error 1
> 
> I assume this is a cross tree dependency?  What's the relevant branch I
> need?

You are right; Greg checked in the relevant patch sometime back:

git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
in the char-misc-next branch.


Regards,

K. Y

> 
> James

N�r��yb�X��ǧv�^�)޺{.n�+{���"�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

RE: Drivers: scsi: FLUSH timeout

[KY Srinivasan]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Friday, September 20, 2013 1:32 PM
> To: KY Srinivasan
> Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> oher...@suse.com; jbottom...@parallels.com; h...@infradead.org; linux-
> s...@vger.kernel.org
> Subject: Re: Drivers: scsi: FLUSH timeout
> 
> On Fri, Sep 20, 2013 at 12:32:27PM -0700, K. Y. Srinivasan wrote:
> > The SD_FLUSH_TIMEOUT value is currently hardcoded.
> 
> Hardcoded where?  Please, more context.

This is defined in scsi/sd.h:

#define SD_FLUSH_TIMEOUT(60 * HZ)
> 
> > On our cloud, we sometimes hit this timeout. I was wondering if we
> > could make this a module parameter. If this is acceptable, I can send
> > you a patch for this.
> 
> A module parameter don't make sense for a per-device value, does it?
Currently, the 60 second timeout is applied across devices. Ideally, I want to 
be
able to control the FLUSH TIMEOUT as we currently do I/O timeout. If this is 
acceptable, I can work on a patch for that as well.

Regards,

K. Y
> 
> greg k-h
--
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: Drivers: scsi: FLUSH timeout

[KY Srinivasan]

> -Original Message-
> From: Jack Wang [mailto:xjtu...@gmail.com]
> Sent: Tuesday, September 24, 2013 5:08 AM
> To: KY Srinivasan
> Cc: Greg KH; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> oher...@suse.com; jbottom...@parallels.com; h...@infradead.org; linux-
> s...@vger.kernel.org; Mike Christie
> Subject: Re: Drivers: scsi: FLUSH timeout
> 
> On 09/21/2013 07:24 AM, KY Srinivasan wrote:
> >
> >
> >> -Original Message-
> >> From: Greg KH [mailto:gre...@linuxfoundation.org]
> >> Sent: Friday, September 20, 2013 1:32 PM
> >> To: KY Srinivasan
> >> Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> >> oher...@suse.com; jbottom...@parallels.com; h...@infradead.org; linux-
> >> s...@vger.kernel.org
> >> Subject: Re: Drivers: scsi: FLUSH timeout
> >>
> >> On Fri, Sep 20, 2013 at 12:32:27PM -0700, K. Y. Srinivasan wrote:
> >>> The SD_FLUSH_TIMEOUT value is currently hardcoded.
> >>
> >> Hardcoded where?  Please, more context.
> >
> > This is defined in scsi/sd.h:
> >
> > #define SD_FLUSH_TIMEOUT(60 * HZ)
> >>
> >>> On our cloud, we sometimes hit this timeout. I was wondering if we
> >>> could make this a module parameter. If this is acceptable, I can send
> >>> you a patch for this.
> >>
> >> A module parameter don't make sense for a per-device value, does it?
> > Currently, the 60 second timeout is applied across devices. Ideally, I want 
> > to be
> > able to control the FLUSH TIMEOUT as we currently do I/O timeout. If this is
> > acceptable, I can work on a patch for that as well.
> >
> > Regards,
> >
> > K. Y
> >>
> >> greg k-h
> > --
> > 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
> >
> Hi,
> 
> Back to 2010, Mike(cc-ed) try to add a flush time out interface, similar
> to what you want here, no idea why it's just ignored?
> http://www.spinics.net/lists/linux-scsi/msg45017.html

Thanks Jack. Mike, do you know what the concerns were as to why this
patch was not accepted?

Regards,

K. Y 
--
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: Drivers: scsi: FLUSH timeout

[KY Srinivasan]

> -Original Message-
> From: Mike Christie [mailto:micha...@cs.wisc.edu]
> Sent: Tuesday, September 24, 2013 10:21 AM
> To: KY Srinivasan
> Cc: Jack Wang; Greg KH; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; jbottom...@parallels.com;
> h...@infradead.org; linux-scsi@vger.kernel.org
> Subject: Re: Drivers: scsi: FLUSH timeout
> 
> On 09/24/2013 07:35 AM, KY Srinivasan wrote:
> >
> >
> >> -Original Message-
> >> From: Jack Wang [mailto:xjtu...@gmail.com]
> >> Sent: Tuesday, September 24, 2013 5:08 AM
> >> To: KY Srinivasan
> >> Cc: Greg KH; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> >> oher...@suse.com; jbottom...@parallels.com; h...@infradead.org; linux-
> >> s...@vger.kernel.org; Mike Christie
> >> Subject: Re: Drivers: scsi: FLUSH timeout
> >>
> >> On 09/21/2013 07:24 AM, KY Srinivasan wrote:
> >>>
> >>>
> >>>> -Original Message-
> >>>> From: Greg KH [mailto:gre...@linuxfoundation.org]
> >>>> Sent: Friday, September 20, 2013 1:32 PM
> >>>> To: KY Srinivasan
> >>>> Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> >>>> oher...@suse.com; jbottom...@parallels.com; h...@infradead.org; linux-
> >>>> s...@vger.kernel.org
> >>>> Subject: Re: Drivers: scsi: FLUSH timeout
> >>>>
> >>>> On Fri, Sep 20, 2013 at 12:32:27PM -0700, K. Y. Srinivasan wrote:
> >>>>> The SD_FLUSH_TIMEOUT value is currently hardcoded.
> >>>>
> >>>> Hardcoded where?  Please, more context.
> >>>
> >>> This is defined in scsi/sd.h:
> >>>
> >>> #define SD_FLUSH_TIMEOUT(60 * HZ)
> >>>>
> >>>>> On our cloud, we sometimes hit this timeout. I was wondering if we
> >>>>> could make this a module parameter. If this is acceptable, I can send
> >>>>> you a patch for this.
> >>>>
> >>>> A module parameter don't make sense for a per-device value, does it?
> >>> Currently, the 60 second timeout is applied across devices. Ideally, I 
> >>> want to
> be
> >>> able to control the FLUSH TIMEOUT as we currently do I/O timeout. If this 
> >>> is
> >>> acceptable, I can work on a patch for that as well.
> >>>
> >>> Regards,
> >>>
> >>> K. Y
> >>>>
> >>>> greg k-h
> >>> --
> >>> 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
> >>>
> >> Hi,
> >>
> >> Back to 2010, Mike(cc-ed) try to add a flush time out interface, similar
> >> to what you want here, no idea why it's just ignored?
> >> http://www.spinics.net/lists/linux-scsi/msg45017.html
> >
> > Thanks Jack. Mike, do you know what the concerns were as to why this
> > patch was not accepted?
> >
> 
> I do not remember the exact concerns. We ended up just increasing the
> hard coded value in:
> 
> commit e3b3e6246726cd05950677ed843010b8e8c5884c
> Author: Mike Christie 
> Date:   Wed Aug 11 11:06:25 2010 -0500
> 
> [SCSI] scsi/block: increase flush/sync timeout
> 
> 
> In the git commit message there is a comment about people thinking
> making it configurable for users was troublesome.

I am not sure how that magic number was arrived at (the 60HZ number). We want 
this to be little higher -
would there be any issues raising this to say  180 seconds. This is the value 
we currently have for I/O
timeout.

Regards,

K. Y

--
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: Drivers: scsi: FLUSH timeout

[KY Srinivasan]

> -Original Message-
> From: geert.uytterhoe...@gmail.com [mailto:geert.uytterhoe...@gmail.com]
> On Behalf Of Geert Uytterhoeven
> Sent: Wednesday, September 25, 2013 1:40 AM
> To: KY Srinivasan
> Cc: Mike Christie; Jack Wang; Greg KH; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; jbottom...@parallels.com;
> h...@infradead.org; linux-scsi@vger.kernel.org
> Subject: Re: Drivers: scsi: FLUSH timeout
> 
> On Tue, Sep 24, 2013 at 11:53 PM, KY Srinivasan  wrote:
> > I am not sure how that magic number was arrived at (the 60HZ number). We
> want this to be little higher -
> 
> "60 * HZ" means "60 seconds".
> 
> > would there be any issues raising this to say  180 seconds. This is the 
> > value we
> currently have for I/O
> > timeout.
> 
> So you want to replace it by "180 * HZ", which is ... another magic number?

Ideally, I want this to be adjustable like the way we can change the I/O 
timeout.
Since that has been attempted earlier and rejected (not clear what the reasons 
were),
I was suggesting that we pick a larger number. James, let me know how I should 
proceed here.

Regards,

K. Y
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds
N�r��yb�X��ǧv�^�)޺{.n�+{���"�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

RE: Drivers: scsi: FLUSH timeout

[KY Srinivasan]

> -Original Message-
> From: Nicholas A. Bellinger [mailto:n...@linux-iscsi.org]
> Sent: Thursday, October 03, 2013 5:09 AM
> To: KY Srinivasan
> Cc: Geert Uytterhoeven; Mike Christie; Jack Wang; Greg KH; linux-
> ker...@vger.kernel.org; de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org
> Subject: Re: Drivers: scsi: FLUSH timeout
> 
> On Wed, 2013-10-02 at 18:29 +, KY Srinivasan wrote:
> >
> > > -Original Message-
> > > From: geert.uytterhoe...@gmail.com
> [mailto:geert.uytterhoe...@gmail.com]
> > > On Behalf Of Geert Uytterhoeven
> > > Sent: Wednesday, September 25, 2013 1:40 AM
> > > To: KY Srinivasan
> > > Cc: Mike Christie; Jack Wang; Greg KH; linux-ker...@vger.kernel.org;
> > > de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com;
> > > h...@infradead.org; linux-scsi@vger.kernel.org
> > > Subject: Re: Drivers: scsi: FLUSH timeout
> > >
> > > On Tue, Sep 24, 2013 at 11:53 PM, KY Srinivasan  
> > > wrote:
> > > > I am not sure how that magic number was arrived at (the 60HZ number). We
> > > want this to be little higher -
> > >
> > > "60 * HZ" means "60 seconds".
> > >
> > > > would there be any issues raising this to say  180 seconds. This is the 
> > > > value
> we
> > > currently have for I/O
> > > > timeout.
> > >
> > > So you want to replace it by "180 * HZ", which is ... another magic 
> > > number?
> >
> > Ideally, I want this to be adjustable like the way we can change the I/O 
> > timeout.
> > Since that has been attempted earlier and rejected (not clear what the 
> > reasons
> were),
> > I was suggesting that we pick a larger number. James, let me know how I 
> > should
> proceed here.
> >
> 
> I think the objection was to making a module parameter for doing this
> globally for all struct scsi_disk, and not the idea of making it
> adjustable on an individual basis per-say..
> 
> What about adding a /sys/class/scsi_disk/$HCTL/flush_timeout..?
> 
> Here's a quick (untested) patch to that effect.

Thanks Nicholas. This is precisely what I was hoping we could agree on. 
James, if this is acceptable, we will go ahead and test this patch.


Regards,

K. Y
> 
> --nab
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index e62d17d..f7a8c5b 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -460,6 +460,38 @@ max_write_same_blocks_store(struct device *dev,
> struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RW(max_write_same_blocks);
> 
> +static ssize_t
> +flush_timeout_show(struct device *dev,
> +struct device_attribute *attr, char *buf)
> +{
> + struct scsi_disk *sdkp = to_scsi_disk(dev);
> +
> + return snprintf(buf, 20, "%u\n", sdkp->flush_timeout);
> +}
> +
> +static ssize_t
> +flush_timeout_store(struct device *dev,
> + struct device_attribute *attr, const char *buf,
> + size_t count)
> +{
> + struct scsi_disk *sdkp = to_scsi_disk(dev);
> + unsigned int flush_timeout;
> + int err;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EACCES;
> +
> + err = kstrtouint(buf, 10, &flush_timeout);
> +
> + if (flush_timeout > SD_FLUSH_TIMEOUT_MAX)
> + return -EINVAL;
> +
> + sdkp->flush_timeout = flush_timeout;
> +
> + return err ? err : count;
> +}
> +static DEVICE_ATTR_RW(flush_timeout);
> +
>  static struct attribute *sd_disk_attrs[] = {
>   &dev_attr_cache_type.attr,
>   &dev_attr_FUA.attr,
> @@ -472,6 +504,7 @@ static struct attribute *sd_disk_attrs[] = {
>   &dev_attr_provisioning_mode.attr,
>   &dev_attr_max_write_same_blocks.attr,
>   &dev_attr_max_medium_access_timeouts.attr,
> + &dev_attr_flush_timeout.attr,
>   NULL,
>  };
>  ATTRIBUTE_GROUPS(sd_disk);
> @@ -829,7 +862,9 @@ static int sd_setup_write_same_cmnd(struct scsi_device
> *sdp, struct request *rq)
> 
>  static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq)
>  {
> - rq->timeout = SD_FLUSH_TIMEOUT;
> + struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
> +
> + rq->timeout = sdkp->flush_timeout;
>   rq->retries = SD_MAX_RETRIES;
>   rq->cmd[0] = SYNCHRONIZE_CACHE;
>   rq->cmd_len = 10;
> @@ -2841,6 +2876,7 @@ static void sd_probe_async(void *data, async_cookie_t
> co

RE: Drivers: scsi: FLUSH timeout

[KY Srinivasan]

> -Original Message-
> From: Eric Seppanen [mailto:e...@purestorage.com]
> Sent: Thursday, October 03, 2013 1:49 PM
> To: Nicholas A. Bellinger
> Cc: KY Srinivasan; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> linux-scsi@vger.kernel.org
> Subject: Re: Drivers: scsi: FLUSH timeout
> 
> On Thu, Oct 3, 2013 at 5:09 AM, Nicholas A. Bellinger
>  wrote:
> >
> > On Wed, 2013-10-02 at 18:29 +, KY Srinivasan wrote:
> > > Ideally, I want this to be adjustable like the way we can change the I/O
> timeout.
> > > Since that has been attempted earlier and rejected (not clear what the
> reasons were),
> > > I was suggesting that we pick a larger number. James, let me know how I
> should proceed here.
> > >
> >
> > I think the objection was to making a module parameter for doing this
> > globally for all struct scsi_disk, and not the idea of making it
> > adjustable on an individual basis per-say..
> >
> > What about adding a /sys/class/scsi_disk/$HCTL/flush_timeout..?
> 
> Do I/O timeouts and flush timeouts need to be independently adjusted?
> If you're having trouble with slow operations, it seems likely to be
> across the board.
> 
> Flush timeout could be defined as 2x the read/write timeout.  Any
> other command-specific timeouts could be scaled the same way.

I like this idea and would result in minimal changes. James, if it ok with you,
I could send you the patch.

Regards,

K. Y
--
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 1/1] Drivers: scsi: Derive the FLUSH_TIMEOUT from the basic I/O timeout

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:jbottom...@parallels.com]
> Sent: Friday, October 04, 2013 2:42 PM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; h...@infradead.org;
> e...@purestorage.com; n...@linux-iscsi.org; linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 1/1] Drivers: scsi: Derive the FLUSH_TIMEOUT from the 
> basic
> I/O timeout
> 
> On Fri, 2013-10-04 at 12:38 -0700, K. Y. Srinivasan wrote:
> > Rather than having a separate constant for specifying the timeout on FLUSH
> > operations, use the basic I/O timeout value that is already configurable
> > on a per target basis to derive the FLUSH timeout. Looking at the current
> > definitions of these timeout values, the FLUSH operation is supposed to have
> > a value that is twice the normal timeout value. This patch preserves this
> > relationship while leveraging the flexibility of specifying the I/O timeout.
> >
> > I would like to thank Eric Seppanen  and
> > Nicholas A. Bellinger  for their help in resolving
> > this issue.
> >
> > Signed-off-by: K. Y. Srinivasan 
> > ---
> >  drivers/scsi/sd.c |5 +++--
> >  1 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index e62d17d..8aff306 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -829,7 +829,7 @@ static int sd_setup_write_same_cmnd(struct
> scsi_device *sdp, struct request *rq)
> >
> >  static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request 
> > *rq)
> >  {
> > -   rq->timeout = SD_FLUSH_TIMEOUT;
> > +   rq->timeout *= 2;
> > rq->retries = SD_MAX_RETRIES;
> > rq->cmd[0] = SYNCHRONIZE_CACHE;
> > rq->cmd_len = 10;
> > @@ -1433,6 +1433,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> >  {
> > int retries, res;
> > struct scsi_device *sdp = sdkp->device;
> > +   unsigned int timeout = sdp->request_queue->rq_timeout;
> 
> The timeout is signed in the function prototype
> 
> > struct scsi_sense_hdr sshdr;
> >
> > if (!scsi_device_online(sdp))
> > @@ -1448,7 +1449,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> >  * flush everything.
> >  */
> > res = scsi_execute_req_flags(sdp, cmd, DMA_NONE, NULL, 0,
> > -&sshdr, SD_FLUSH_TIMEOUT,
> > +&sshdr, timeout * 2,
> >  SD_MAX_RETRIES, NULL, REQ_PM);
> > if (res == 0)
> > break;
> 
> Not like this, please: you now leave us with a dangling #define whose
> name makes you think it should be related to flushing and a couple of
> curious magic constants.  It's almost hand crafted to confuse people
> reading the code.
> 
> Please do it like this instead: with a comment explaining what we're
> doing above the #define and a name that clearly relates to the actions.
> 
> James
> 
> ---
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index e62d17d..5c9496d 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -829,7 +829,7 @@ static int sd_setup_write_same_cmnd(struct scsi_device
> *sdp, struct request *rq)
> 
>  static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq)
>  {
> - rq->timeout = SD_FLUSH_TIMEOUT;
> + rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER;
>   rq->retries = SD_MAX_RETRIES;
>   rq->cmd[0] = SYNCHRONIZE_CACHE;
>   rq->cmd_len = 10;
> @@ -1433,6 +1433,8 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
>  {
>   int retries, res;
>   struct scsi_device *sdp = sdkp->device;
> + const int timeout = sdp->request_queue->rq_timeout
> + * SD_FLUSH_TIMEOUT_MULTIPLIER;
>   struct scsi_sense_hdr sshdr;
> 
>   if (!scsi_device_online(sdp))
> @@ -1448,8 +1450,8 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
>* flush everything.
>*/
>   res = scsi_execute_req_flags(sdp, cmd, DMA_NONE, NULL, 0,
> -  &sshdr, SD_FLUSH_TIMEOUT,
> -  SD_MAX_RETRIES, NULL, REQ_PM);
> +  &sshdr, timeout, SD_MAX_RETRIES,
> +  NULL, REQ_PM);
>   if (res == 0)
>   break;
>   }

RE: [PATCH 0/3] scsi: storvsc: Increase the tablesize based on host's capabilities

[KY Srinivasan]

> -Original Message-
> From: K. Y. Srinivasan [mailto:k...@microsoft.com]
> Sent: Monday, March 9, 2015 8:42 PM
> To: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com
> Cc: KY Srinivasan
> Subject: [PATCH 0/3] scsi: storvsc: Increase the tablesize based on host's
> capabilities
> 
> Presently, storvsc limits the I/O size arbitrarily. Make this configurable 
> based
> on what the host advertises.
> 
> K. Y. Srinivasan (3):
>   scsi: storvsc: Retrieve information about the capability of the
> target
>   scsi: storvsc: Set the tablesize based on the information given by
> the host
>   scsi: storvsc: Enable clustering
> 
>  drivers/scsi/storvsc_drv.c |   78
> +++-
>  1 files changed, 62 insertions(+), 16 deletions(-)

Christoph,

Let me know if I should resend these patches.

Regards,

K. Y
> 
> --
> 1.7.4.1

--
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: storvsc: Increase the tablesize based on host's capabilities

[KY Srinivasan]

> -Original Message-
> From: KY Srinivasan
> Sent: Monday, March 16, 2015 4:45 PM
> To: 'K. Y. Srinivasan'; gre...@linuxfoundation.org; linux-
> ker...@vger.kernel.org; de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com
> Subject: RE: [PATCH 0/3] scsi: storvsc: Increase the tablesize based on host's
> capabilities
> 
> 
> 
> > -Original Message-
> > From: K. Y. Srinivasan [mailto:k...@microsoft.com]
> > Sent: Monday, March 9, 2015 8:42 PM
> > To: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> > de...@linuxdriverproject.org; oher...@suse.com;
> > jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> > a...@canonical.com; vkuzn...@redhat.com
> > Cc: KY Srinivasan
> > Subject: [PATCH 0/3] scsi: storvsc: Increase the tablesize based on host's
> > capabilities
> >
> > Presently, storvsc limits the I/O size arbitrarily. Make this configurable
> based
> > on what the host advertises.
> >
> > K. Y. Srinivasan (3):
> >   scsi: storvsc: Retrieve information about the capability of the
> > target
> >   scsi: storvsc: Set the tablesize based on the information given by
> > the host
> >   scsi: storvsc: Enable clustering
> >
> >  drivers/scsi/storvsc_drv.c |   78
> > +++-
> >  1 files changed, 62 insertions(+), 16 deletions(-)
> 
> Christoph,
> 
> Let me know if I should resend these patches.
> 
> Regards,

Christoph,

Please drop this set. I will resend the patches with a couple of additional 
patches
that need to go in before I can enable clustering.

Regards,

K. Y 

--
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 RESEND 2/7] scsi: storvsc: Size the queue depth based on the ringbuffer size

[KY Srinivasan]

> -Original Message-
> From: Venkatesh Srinivas [mailto:venkate...@google.com]
> Sent: Monday, March 23, 2015 5:23 PM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; Linux Kernel Developers List;
> de...@linuxdriverproject.org; oher...@suse.com; James E.J. Bottomley;
> Christoph Hellwig; linux-scsi@vger.kernel.org; a...@canonical.com;
> vkuzn...@redhat.com; jasow...@redhat.com
> Subject: Re: [PATCH RESEND 2/7] scsi: storvsc: Size the queue depth based
> on the ringbuffer size
> 
> On Mon, Mar 23, 2015 at 2:06 PM, K. Y. Srinivasan 
> wrote:
> > Size the queue depth based on the ringbuffer size. Also accomodate for
> the
> > fact that we could have multiple channels (ringbuffers) per adaptor.
> >
> > Signed-off-by: K. Y. Srinivasan 
> > Reviewed-by: Long Li 
> > ---
> >  drivers/scsi/storvsc_drv.c |   27 ---
> >  1 files changed, 16 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > index 27fe850..5a12897 100644
> > --- a/drivers/scsi/storvsc_drv.c
> > +++ b/drivers/scsi/storvsc_drv.c
> > @@ -309,10 +309,15 @@ enum storvsc_request_type {
> >   */
> >
> >  static int storvsc_ringbuffer_size = (256 * PAGE_SIZE);
> > +static u32 max_outstanding_req_per_channel;
> > +
> > +static int storvsc_vcpus_per_sub_channel = 4;
> >
> >  module_param(storvsc_ringbuffer_size, int, S_IRUGO);
> >  MODULE_PARM_DESC(storvsc_ringbuffer_size, "Ring buffer size
> (bytes)");
> >
> > +module_param(storvsc_vcpus_per_sub_channel, int, S_IRUGO);
> > +MODULE_PARM_DESC(vcpus_per_sub_channel, "Ratio of VCPUs to
> subchannels");
> >  /*
> >   * Timeout in seconds for all devices managed by this driver.
> >   */
> > @@ -320,7 +325,6 @@ static int storvsc_timeout = 180;
> >
> >  static int msft_blist_flags = BLIST_TRY_VPD_PAGES;
> >
> > -#define STORVSC_MAX_IO_REQUESTS200
> >
> >  static void storvsc_on_channel_callback(void *context);
> >
> > @@ -1376,7 +1380,6 @@ static int storvsc_do_io(struct hv_device *device,
> >
> >  static int storvsc_device_configure(struct scsi_device *sdevice)
> >  {
> > -   scsi_change_queue_depth(sdevice, STORVSC_MAX_IO_REQUESTS);
> >
> > blk_queue_max_segment_size(sdevice->request_queue,
> PAGE_SIZE);
> >
> > @@ -1646,7 +1649,6 @@ static struct scsi_host_template scsi_driver = {
> > .eh_timed_out = storvsc_eh_timed_out,
> > .slave_configure =  storvsc_device_configure,
> > .cmd_per_lun =  255,
> > -   .can_queue =
> STORVSC_MAX_IO_REQUESTS*STORVSC_MAX_TARGETS,
> > .this_id =  -1,
> > /* no use setting to 0 since ll_blk_rw reset it to 1 */
> > /* currently 32 */
> > @@ -1686,6 +1688,7 @@ static int storvsc_probe(struct hv_device *device,
> > const struct hv_vmbus_device_id *dev_id)
> >  {
> > int ret;
> > +   int num_cpus = num_online_cpus();
> > struct Scsi_Host *host;
> > struct hv_host_device *host_dev;
> > bool dev_is_ide = ((dev_id->driver_data == IDE_GUID) ? true : 
> > false);
> > @@ -1694,6 +1697,7 @@ static int storvsc_probe(struct hv_device *device,
> > int max_luns_per_target;
> > int max_targets;
> > int max_channels;
> > +   int max_sub_channels = 0;
> >
> > /*
> >  * Based on the windows host we are running on,
> > @@ -1719,12 +1723,18 @@ static int storvsc_probe(struct hv_device
> *device,
> > max_luns_per_target = STORVSC_MAX_LUNS_PER_TARGET;
> > max_targets = STORVSC_MAX_TARGETS;
> > max_channels = STORVSC_MAX_CHANNELS;
> > +   /*
> > +* On Windows8 and above, we support sub-channels for 
> > storage.
> > +* The number of sub-channels offerred is based on the 
> > number
> of
> > +* VCPUs in the guest.
> > +*/
> > +   max_sub_channels = (num_cpus /
> storvsc_vcpus_per_sub_channel);
> > break;
> > }
> >
> > -   if (dev_id->driver_data == SFC_GUID)
> > -   scsi_driver.can_queue = (STORVSC_MAX_IO_REQUESTS *
> > -STORVSC_FC_MAX_TARGETS);
> > +   scsi_driver.can_queue = (max_outstanding_req_per_channel *
> > +max_sub_channels + 1);
> > +
> 
> If num_online_cpus() returned 1 - 3, can_queue will be set to 1 I
> think. Is that desired?

can_ queue will be set  max_outstanding_req_per_channel in
this case. That is what is expected. We will always have the primary channel;
Additionally, if the guest has more than 4 VCPUs, the host will offer
Additional subchannels for each 4 VCPus in the guest. So for less than
4 VCPus in the guest, we will only have the primary channel.


Regards,

K. Y
N�r��yb�X��ǧv�^�)޺{.n�+{���"�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

RE: [PATCH 6/7] scsi: storvsc: Don't assume that the scatterlist is not chained

[KY Srinivasan]

> -Original Message-
> From: Olaf Hering [mailto:o...@aepfle.de]
> Sent: Tuesday, March 24, 2015 1:56 AM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; jbottom...@parallels.com;
> h...@infradead.org; linux-scsi@vger.kernel.org; a...@canonical.com;
> vkuzn...@redhat.com; jasow...@redhat.com
> Subject: Re: [PATCH 6/7] scsi: storvsc: Don't assume that the scatterlist is 
> not
> chained
> 
> On Mon, Mar 23, K. Y. Srinivasan wrote:
> 
> > @@ -653,32 +640,39 @@ static unsigned int
> copy_from_bounce_buffer(struct scatterlist *orig_sgl,
> > 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 = sg_kmap_atomic(orig_sgl,i) + orig_sgl[i].offset;
> > +   dest_addr = (unsigned long)
> > +   kmap_atomic(sg_page(cur_dest_sgl)) +
> > +   cur_dest_sgl->offset;
> > dest = dest_addr;
> > destlen = orig_sgl[i].length;
> > +   destlen = cur_dest_sgl->length;
> 
> Double assignment.

Thanks Olaf; I will fix this and resubmit.

K. Y

> 
> Olaf

RE: [scsi:misc 14/106] drivers/scsi/storvsc_drv.c:1658 storvsc_queuecommand() warn: curly braces intended?

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:jbottom...@odin.com]
> Sent: Monday, April 13, 2015 7:26 AM
> To: dan.carpen...@oracle.com
> Cc: linux-scsi@vger.kernel.org; kbu...@01.org; KY Srinivasan
> Subject: Re: [scsi:misc 14/106] drivers/scsi/storvsc_drv.c:1658
> storvsc_queuecommand() warn: curly braces intended?
> 
> On Mon, 2015-04-13 at 14:56 +0300, Dan Carpenter wrote:
> > tree:   git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git misc
> > head:   0351b8f81392c6d036e5c8f73ceff68726e9
> > commit: be0cf6ca301c61458dc4aa1a37acf4f58d2ed3d6 [14/106] scsi:
> storvsc: Set the tablesize based on the information given by the host
> >
> > drivers/scsi/storvsc_drv.c:1658 storvsc_queuecommand() warn: curly
> braces intended?
> >
> > git remote add scsi
> git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git
> > git remote update scsi
> > git checkout be0cf6ca301c61458dc4aa1a37acf4f58d2ed3d6
> > vim +1658 drivers/scsi/storvsc_drv.c
> >
> > c5b463ae drivers/staging/hv/storvsc_drv.c K. Y. Srinivasan 2011-05-10  1642
>   sgl = cmd_request->bounce_sgl;
> > c5b463ae drivers/staging/hv/storvsc_drv.c K. Y. Srinivasan 2011-05-10  1643
>   sg_count = cmd_request->bounce_sgl_count;
> > c5b463ae drivers/staging/hv/storvsc_drv.c K. Y. Srinivasan 2011-05-10  1644
>   }
> > c5b463ae drivers/staging/hv/storvsc_drv.c K. Y. Srinivasan 2011-05-10  1645
> > be0cf6ca drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27  1646
> > be0cf6ca drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27  1647
>   if (sg_count > MAX_PAGE_BUFFER_COUNT) {
> > be0cf6ca drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27  1648
> > be0cf6ca drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27  1649
>   payload_sz = (sg_count * sizeof(void *) +
> > be0cf6ca drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27  1650
> sizeof(struct vmbus_packet_mpb_array));
> > be0cf6ca drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27  1651
>   payload = kmalloc(payload_sz, GFP_ATOMIC);
> > be0cf6ca drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27  1652
>   if (!payload) {
> > be0cf6ca drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27  1653
>   if (cmd_request->bounce_sgl_count)
> >
> > Start block here.
> >
> > be0cf6ca drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27  1654
>   destroy_bounce_buffer(
> > be0cf6ca drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27  1655
>   cmd_request->bounce_sgl,
> > be0cf6ca drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27  1656
>   cmd_request->bounce_sgl_count);
> > be0cf6ca drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27  1657
> > be0cf6ca drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27 @1658
>   return
> SCSI_MLQUEUE_DEVICE_BUSY;
> >
> > End block here.
> >
> > be0cf6ca drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27  1659
>   }
> > be0cf6ca drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27  1660
>   }
> > be0cf6ca drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27  1661
> > be0cf6ca drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27  1662
>   payload->range.len = length;
> > be0cf6ca drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27  1663
>   payload->range.offset = sgl[0].offset;
> > be0cf6ca drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27  1664
> > aaced993 drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27  1665
>   cur_sgl = sgl;
> > aaced993 drivers/scsi/storvsc_drv.c   K. Y. Srinivasan 2015-03-27  1666
>   for (i = 0; i < sg_count; i++) {
> 
> I think this is just screwed up indentation.  You have to return
> DEVICE_BUSY for the kmalloc failure.  The if is just seeing if cleanup
> is needed.

Yes; bad indentation. I will fix it.

K. Y
> 
> James

N�r��yb�X��ǧv�^�)޺{.n�+{���"�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

RE: storvsc loops with No Sense messages

[KY Srinivasan]

> -Original Message-
> From: Olaf Hering [mailto:o...@aepfle.de]
> Sent: Tuesday, January 29, 2013 10:28 AM
> To: KY Srinivasan
> Cc: linux-scsi@vger.kernel.org
> Subject: storvsc loops with No Sense messages
> 
> 
> KY,
> 
> while attempting to install the upcoming openSuSE 12.3 in a hyper-v
> guest (1 vcpu, 512MB) hosted on Windows Server 2012 I see a hanging
> guest after formating the root partition, when the installer starts to
> write files to it.  After booting the install kernel with
> 'ignore_loglevel' I see a flood of the "No Sense" messages shown below.
> I can reproduce it every time with 12.3.
> 
> The sglist init patch does not fix it for me.
> 
> Hannes did many storage related backports for SLES11 SP3, and sometimes
> the installer hangs at similar points during a fresh SP3 install.
> 
> The same procedure works fine on Windows Server 2008.
> 
> Any ideas what 2012 does differently? It happens with a plain 3.7.n
> kernel. I will try 3.8-rc now, and see if it happens as well.
> 
> Olaf
> 
> ...
> [0.00] Linux version 3.7.5-5.home_olh_12_3-vanilla (geeko@buildhost)
> (gcc version 4.7.2 20130108 [gcc-4_7-branch revision 195012] (SUSE Linux) ) #1
> SMP Mon Jan 28 12:39:46 UTC 2013 (d9a9e6d)
> [0.00] Command line: quiet panic=9 sysrq=yes start_shell splash=silent
> vga=0x317 video=1024x768 console=ttyS0,115200 console=tty1 ignore_loglevel
> initrd=sl/12.3/initrd-x86_64 BOOT_IMAGE=sl/12.3/linux-x86_64
> ...
> [7.938806] scsi2 : storvsc_host_t
> [7.940709] scsi 2:0:0:0: Direct-Access Msft Virtual Disk 1.0  
> PQ: 0 ANSI: 4
> [7.943952] sd 2:0:0:0: Attached scsi generic sg1 type 0
> 
> [7.946440] sd 2:0:0:0: [sda] 20971520 512-byte logical blocks: (10.7 
> GB/10.0 GiB)
> [7.949485] sd 2:0:0:0: [sda] Write Protect is off
> [7.951231] sd 2:0:0:0: [sda] Mode Sense: 0f 00 00 00
> [7.953105] sd 2:0:0:0: [sda] Write cache: enabled, read cache: enabled, 
> doesn't
> support DPO or FUA
> [7.955269] scsi3 : storvsc_host_t
> [7.957027] scsi scan: INQUIRY result too short (5), using 36
> [7.958786] scsi scan: INQUIRY result too short (5), using 36
> [7.961292] input: Microsoft Vmbus HID-compliant Mouse as
> /devices/virtual/input/input3
> [7.965400] hid-generic 0006:045E:0621.0001: input:  HID v0.01
> Mouse [Microsoft Vmbus HID-compliant Mouse] on
> [7.971872] hv_netvsc: hv_netvsc channel opened successfully
> [7.974574] hv_netvsc vmbus_0_11: Device MAC 00:15:5d:02:51:02 link state 
> up
> [7.985086]  sda: sda1 sda2
> [7.986725] sd 2:0:0:0: [sda] Attached SCSI disk
> ...
> [  156.205185] Adding 746492k swap on /dev/sda1.  Priority:-1 extents:1
> across:746492k
> ...
> [  156.478186] EXT4-fs (sda2): mounted filesystem with ordered data mode. 
> Opts:
> acl,user_xattr
> [  161.459523] hv_storvsc vmbus_0_1: cmd 0x41 scsi status 0x2 srb status 0x6
> [  161.462157] sd 2:0:0:0: [sda]
> [  161.463135] Sense Key : No Sense [current]
> [  161.464983] sd 2:0:0:0: [sda]
> [  161.465899] Add. Sense: No additional sense information
> [  161.468211] hv_storvsc vmbus_0_1: cmd 0x41 scsi status 0x2 srb status 0x6
> [  161.475766] sd 2:0:0:0: [sda]
> [  161.476728] Sense Key : No Sense [current]
> [  161.478284] sd 2:0:0:0: [sda]
> [  161.479441] Add. Sense: No additional sense information
> [  161.481311] hv_storvsc vmbus_0_1: cmd 0x41 scsi status 0x2 srb status 0x6
> [  161.484477] sd 2:0:0:0: [sda]
> [  161.485422] Sense Key : No Sense [current]
> [  161.486977] sd 2:0:0:0: [sda]
> [  161.487920] Add. Sense: No additional sense information
> 
> ... and so it goes on until the VM is powered off.

Olaf,

I have not seen this. The SRB status indicates it is an illegal request; the 
command is WRITE_SAME. I will take a look.

K. Y
> 
> 

N�r��yb�X��ǧv�^�)޺{.n�+{���"�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

RE: storvsc loops with No Sense messages

[KY Srinivasan]

Was the installation done on a vhd or VHDX. I checked with Hyper-V guys and 
they say they never supported WRITE_SAME. I am wondering how it worked on 
ws2008.

K. Y

> -Original Message-
> From: Olaf Hering [mailto:o...@aepfle.de]
> Sent: Tuesday, January 29, 2013 1:56 PM
> To: KY Srinivasan
> Cc: linux-scsi@vger.kernel.org
> Subject: Re: storvsc loops with No Sense messages
> 
> On Tue, Jan 29, KY Srinivasan wrote:
> 
> > I have not seen this. The SRB status indicates it is an illegal request; the
> command is WRITE_SAME. I will take a look.
> 
> I can ignore it like SET_WINDOW, and the installation proceeds.
> Just a few messages in dmesg:
> 
> ...
> [  107.448281] EXT4-fs (sda2): mounted filesystem with ordered data mode. 
> Opts:
> acl,user_xattr
> [  111.020383] sd 2:0:0:0: [sda] Unhandled error code
> [  111.020913] sd 2:0:0:0: [sda]
> [  111.021264] Result: hostbyte=DID_ABORT driverbyte=DRIVER_OK
> [  111.021791] sd 2:0:0:0: [sda] CDB:
> [  111.022186] Write Same(10): 41 00 00 16 e3 a8 00 0f d8 00
> [  111.024400] end_request: I/O error, dev sda, sector 1500072
> [  111.025790] sda2: WRITE SAME failed. Manually zeroing.
> [  112.936342] sd 2:0:0:0: [sda] Unhandled error code
> [  112.937588] sd 2:0:0:0: [sda]
> [  112.938429] Result: hostbyte=DID_ABORT driverbyte=DRIVER_OK
> [  112.939642] sd 2:0:0:0: [sda] CDB:
> [  112.940550] Write Same(10): 41 00 00 16 f3 80 00 0f e0 00
> [  112.945081] end_request: I/O error, dev sda, sector 1504128
> [  112.946627] sda2: WRITE SAME failed. Manually zeroing.
> [  113.495880] ISO 9660 Extensions: Microsoft Joliet Level 3
> [  113.498959] ISO 9660 Extensions: RRIP_1991A
> [  114.728274] sd 2:0:0:0: [sda] Unhandled error code
> [  114.729540] sd 2:0:0:0: [sda]
> [  114.730419] Result: hostbyte=DID_ABORT driverbyte=DRIVER_OK
> [  114.731670] sd 2:0:0:0: [sda] CDB:
> [  114.732644] Write Same(10): 41 00 00 17 03 60 00 0f e0 00
> [  114.737219] end_request: I/O error, dev sda, sector 1508192
> [  114.739027] sda2: WRITE SAME failed. Manually zeroing.
> [  116.512217] sd 2:0:0:0: [sda] Unhandled error code
> [  116.513206] sd 2:0:0:0: [sda]
> [  116.513947] Result: hostbyte=DID_ABORT driverbyte=DRIVER_OK
> [  116.515187] sd 2:0:0:0: [sda] CDB:
> [  116.515983] Write Same(10): 41 00 00 17 13 40 00 0f e0 00
> [  116.519547] end_request: I/O error, dev sda, sector 1512256
> [  116.521011] sda2: WRITE SAME failed. Manually zeroing.
> ...
> 
> Olaf
> 

N�r��yb�X��ǧv�^�)޺{.n�+{���"�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

RE: storvsc loops with No Sense messages

[KY Srinivasan]

> -Original Message-
> From: Olaf Hering [mailto:o...@aepfle.de]
> Sent: Wednesday, January 30, 2013 5:00 AM
> To: KY Srinivasan
> Cc: linux-scsi@vger.kernel.org
> Subject: Re: storvsc loops with No Sense messages
> 
> On Tue, Jan 29, Olaf Hering wrote:
> 
> > On Tue, Jan 29, KY Srinivasan wrote:
> >
> > > Was the installation done on a vhd or VHDX. I checked with Hyper-V
> > > guys and they say they never supported WRITE_SAME. I am wondering how
> > > it worked on ws2008.
> >
> > Its a vhdx image, both fixed and dynamically size have the same issue.
> > I will see what happens with ws2008, at least the install proceeds
> > without issues.
> 
> I see that WRITE_SAME is also used when running on ws2008, but
> appearently it does not cause any issues.
> This triggers only when the default ext4 is used, not when I force ext3
> for the root filesystem.

Talking to the MSFT storage guys, WRITE_SAME has never been supported - not
on ws2008 or on ws2012. So I am not sure why or how this works on ws2008.

K. Y
> So to me it looks like ws2012 has issues with WRITE_SAME handling.
> 
> 
> Is there a way to not use WRITE_SAME at all? While browsing the code its
> not clear if there is a conditional for this command.
> 
> 
> Olaf
> 

RE: storvsc loops with No Sense messages

[KY Srinivasan]

> -Original Message-
> From: Olaf Hering [mailto:o...@aepfle.de]
> Sent: Wednesday, January 30, 2013 8:35 AM
> To: KY Srinivasan
> Cc: linux-scsi@vger.kernel.org
> Subject: Re: storvsc loops with No Sense messages
> 
> On Wed, Jan 30, Olaf Hering wrote:
> 
> > Is there a way to not use WRITE_SAME at all? While browsing the code its
> > not clear if there is a conditional for this command.
> 
> It seems scsi_device->no_write_same may avoid this command, I will
> test this patch:
> 
> # Subject: [PATCH] scsi: storvsc: avoid usage of WRITE_SAME
> 
> Set scsi_device->no_write_same because the host does not support it.
> Also blacklist WRITE_SAME to avoid (and log) accident usage.
>
Olaf,

Thanks for looking into this.

Acked-by:  K. Y. Srinivasan 
 
> Signed-off-by: Olaf Hering 
> ---
>  drivers/scsi/storvsc_drv.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 0144078..21f788a 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -1155,6 +1155,8 @@ static int storvsc_device_configure(struct scsi_device
> *sdevice)
> 
>   blk_queue_bounce_limit(sdevice->request_queue, BLK_BOUNCE_ANY);
> 
> + sdevice->no_write_same = 1;
> +
>   return 0;
>  }
> 
> @@ -1241,6 +1243,7 @@ static bool storvsc_scsi_cmd_ok(struct scsi_cmnd
> *scmnd)
>* smartd sends this command and the host does not handle
>* this. So, don't send it.
>*/
> + case WRITE_SAME:
>   case SET_WINDOW:
>   scmnd->result = ILLEGAL_REQUEST << 16;
>   allowed = false;
> --
> 1.8.1.1
> 
> 

RE: [PATCH] scsi: storvsc: avoid usage of WRITE_SAME

[KY Srinivasan]

> -Original Message-
> From: Olaf Hering [mailto:o...@aepfle.de]
> Sent: Monday, February 11, 2013 11:07 AM
> To: linux-scsi@vger.kernel.org; James Bottomley
> Cc: KY Srinivasan
> Subject: Re: [PATCH] scsi: storvsc: avoid usage of WRITE_SAME
> 
> James,
> 
> this patch fixes a regression in 3.8. Please try to include it in final 3.8.
> There is also another outstanding fix regarding sglist init from KY.
> 
> Olaf
> 
> On Wed, Jan 30, Olaf Hering wrote:
> 
> > Set scsi_device->no_write_same because the host does not support it.
> > Also blacklist WRITE_SAME to avoid (and log) accident usage.
> >
> > If the guest uses the ext4 filesystem, storvsc hangs while it prints
> > these messages in an endless loop:
> 
> 
> >  drivers/scsi/storvsc_drv.c | 4 
> >  1 file changed, 4 insertions(+)
> 
> > +   sdevice->no_write_same = 1;
> 
> 

James,

Ping. Can these make the 3.8 release.


Regards,

K. Y

RE: [PATCH] scsi: storvsc: avoid usage of WRITE_SAME

[KY Srinivasan]

> -Original Message-
> From: Olaf Hering [mailto:o...@aepfle.de]
> Sent: Monday, February 11, 2013 11:07 AM
> To: linux-scsi@vger.kernel.org; James Bottomley
> Cc: KY Srinivasan
> Subject: Re: [PATCH] scsi: storvsc: avoid usage of WRITE_SAME
> 
> James,
> 
> this patch fixes a regression in 3.8. Please try to include it in final 3.8.
> There is also another outstanding fix regarding sglist init from KY.
> 
> Olaf

Ping.

K. Y
> 
> On Wed, Jan 30, Olaf Hering wrote:
> 
> > Set scsi_device->no_write_same because the host does not support it.
> > Also blacklist WRITE_SAME to avoid (and log) accident usage.
> >
> > If the guest uses the ext4 filesystem, storvsc hangs while it prints
> > these messages in an endless loop:
> 
> 
> >  drivers/scsi/storvsc_drv.c | 4 
> >  1 file changed, 4 insertions(+)
> 
> > +   sdevice->no_write_same = 1;
> 
> 

RE: scanning for LUNs

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:jbottom...@parallels.com]
> Sent: Thursday, April 04, 2013 11:15 AM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; h...@infradead.org; linux-
> s...@vger.kernel.org
> Subject: Re: scanning for LUNs
> 
> On Thu, 2013-04-04 at 08:12 -0700, K. Y. Srinivasan wrote:
> > Here is the code snippet for scanning LUNS (drivers/scsi/scsi_scan.c in 
> > function
> > __scsi_scan_target()):
> >
> > /*
> >  * Scan LUN 0, if there is some response, scan further. Ideally, we
> >  * would not configure LUN 0 until all LUNs are scanned.
> >  */
> > res = scsi_probe_and_add_lun(starget, 0, &bflags, NULL, rescan, 
> > NULL);
> > if (res == SCSI_SCAN_LUN_PRESENT || res ==
> SCSI_SCAN_TARGET_PRESENT) {
> > if (scsi_report_lun_scan(starget, bflags, rescan) != 0)
> >
> >
> > So, if we don't get a response while scanning LUN0, we will not use
> > scsi_report_lun_scan().
> > On Hyper-V, the scsi emulation on the host does not treat LUN0 as
> > anything special and we
> > could have situations where the only device under a scsi controller is
> > at a location other than 0
> > or 1. In this case the standard LUN scanning code in Linux fails to
> > detect this device. Is this
> > behaviour expected? Why is LUN0 treated differently here. Looking at
> > the scsi spec, I am not sure
> > if this is what is specified. Any help/guidance will be greatly
> > appreciated.
> 
> Why don't you describe the problem.  We can't scan randomly a bunch of
> LUNs hoping for a response (the space is 10^19).  SAM thinks you use
> LUNW for this, but that's not well supported.  We can't annoy USB
> devices by probing with REPORT LUNS, so conventionally most arrays
> return something for LUN0 even if they don't actually have one (That's
> what the peripheral qualifier codes are supposed to be about).  We
> translate PQ1 and PQ2 to SCSI_SCAN_TARGET_PRESENT, which means no LUN,
> but there is a target to scan here.
> 
> If you're sending back an error to an INQUIRY to LUN0, then you're out
> of spec.  The SCSI standards say:
> 
> SPC3 6.4.1: In response to an INQUIRY command received by an
> incorrect logical unit, the SCSI target device shall return the
> INQUIRY data with the peripheral qualifier set to the value
> defined in 6.4.2. The INQUIRY command shall return CHECK
> CONDITION status only when the device server is unable to return
> the requested INQUIRY data

Thanks James. I will further investigate the issue on our platform.

Regards,

K. Y
> 
> James
> 
> 
> James
> 
> 


--
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: scanning for LUNs

[KY Srinivasan]

> -Original Message-
> From: Hannes Reinecke [mailto:h...@suse.de]
> Sent: Monday, April 08, 2013 10:42 AM
> To: KY Srinivasan
> Cc: James Bottomley; gre...@linuxfoundation.org; linux-
> ker...@vger.kernel.org; de...@linuxdriverproject.org; oher...@suse.com;
> h...@infradead.org; linux-scsi@vger.kernel.org
> Subject: Re: scanning for LUNs
> 
> On 04/04/2013 07:12 PM, KY Srinivasan wrote:
> >
> >
> >> -Original Message-
> >> From: James Bottomley [mailto:jbottom...@parallels.com]
> >> Sent: Thursday, April 04, 2013 11:15 AM
> >> To: KY Srinivasan
> >> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> >> de...@linuxdriverproject.org; oher...@suse.com; h...@infradead.org; linux-
> >> s...@vger.kernel.org
> >> Subject: Re: scanning for LUNs
> >>
> >> On Thu, 2013-04-04 at 08:12 -0700, K. Y. Srinivasan wrote:
> >>> Here is the code snippet for scanning LUNS (drivers/scsi/scsi_scan.c in
> function
> >>> __scsi_scan_target()):
> >>>
> >>> /*
> >>>  * Scan LUN 0, if there is some response, scan further. Ideally, 
> >>> we
> >>>  * would not configure LUN 0 until all LUNs are scanned.
> >>>  */
> >>> res = scsi_probe_and_add_lun(starget, 0, &bflags, NULL, rescan, 
> >>> NULL);
> >>> if (res == SCSI_SCAN_LUN_PRESENT || res ==
> >> SCSI_SCAN_TARGET_PRESENT) {
> >>> if (scsi_report_lun_scan(starget, bflags, rescan) != 0)
> >>>
> >>>
> >>> So, if we don't get a response while scanning LUN0, we will not use
> >>> scsi_report_lun_scan().
> >>> On Hyper-V, the scsi emulation on the host does not treat LUN0 as
> >>> anything special and we
> >>> could have situations where the only device under a scsi controller is
> >>> at a location other than 0
> >>> or 1. In this case the standard LUN scanning code in Linux fails to
> >>> detect this device. Is this
> >>> behaviour expected? Why is LUN0 treated differently here. Looking at
> >>> the scsi spec, I am not sure
> >>> if this is what is specified. Any help/guidance will be greatly
> >>> appreciated.
> >>
> >> Why don't you describe the problem.  We can't scan randomly a bunch of
> >> LUNs hoping for a response (the space is 10^19).  SAM thinks you use
> >> LUNW for this, but that's not well supported.  We can't annoy USB
> >> devices by probing with REPORT LUNS, so conventionally most arrays
> >> return something for LUN0 even if they don't actually have one (That's
> >> what the peripheral qualifier codes are supposed to be about).  We
> >> translate PQ1 and PQ2 to SCSI_SCAN_TARGET_PRESENT, which means no
> LUN,
> >> but there is a target to scan here.
> >>
> >> If you're sending back an error to an INQUIRY to LUN0, then you're out
> >> of spec.  The SCSI standards say:
> >>
> >> SPC3 6.4.1: In response to an INQUIRY command received by an
> >> incorrect logical unit, the SCSI target device shall return the
> >> INQUIRY data with the peripheral qualifier set to the value
> >> defined in 6.4.2. The INQUIRY command shall return CHECK
> >> CONDITION status only when the device server is unable to return
> >> the requested INQUIRY data
> >
> > Thanks James. I will further investigate the issue on our platform.
> >
> Or check if you can use W_LUN for scanning.
> I've done a patchset for this (check the mailing list).
> 
> Using W_LUN is precisely for this type of setup.
> 
> (And would provide me with another scenario for using W_LUNs :-)

Thanks Hannes. What is the status on this patch; is it planned for the next 
upstream release?

Regards,

K. Y
> 
> Cheers,
> 
> Hannes
> --
> Dr. Hannes Reinecke zSeries & Storage
> h...@suse.de+49 911 74053 688
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
> 


--
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/6] Drivers: hv/scsi: Implement multi-channel support

[KY Srinivasan]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Thursday, May 16, 2013 12:00 AM
> To: KY Srinivasan
> Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> oher...@suse.com; jbottom...@parallels.com; h...@infradead.org; linux-
> s...@vger.kernel.org; a...@canonical.com; jasow...@redhat.com
> Subject: Re: [PATCH 0/6] Drivers: hv/scsi: Implement multi-channel support
> 
> On Wed, May 15, 2013 at 03:02:00PM -0700, K. Y. Srinivasan wrote:
> > This patch-set implements multi-channel support for Hyper-V devices. Also
> > support for synthetic Fiber Channel device is included. The first two
> > patches in the series are the foundational pieces for the remaining patches.
> >
> > James, once Greg accepts the first two patches, you can consider the scsi
> > patches.
> 
> I have no objection for these first two patches to go through James's
> tree, I'll go add my ack to them.  That should make it easier for
> everyone involved.

Thanks Greg.

Regards,

K. Y
 


--
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 1/6] Drivers: hv: vmbus: Implement multi-channel support

[KY Srinivasan]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Thursday, May 16, 2013 12:02 AM
> To: KY Srinivasan
> Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> oher...@suse.com; jbottom...@parallels.com; h...@infradead.org; linux-
> s...@vger.kernel.org; a...@canonical.com; jasow...@redhat.com
> Subject: Re: [PATCH 1/6] Drivers: hv: vmbus: Implement multi-channel support
> 
> On Wed, May 15, 2013 at 03:02:29PM -0700, K. Y. Srinivasan wrote:
> > +/*
> > + * Retrieve the (sub) channel on which to send an outgoing request.
> > + * When a primary channel has multiple sub-channels, we choose a
> > + * channel whose VCPU binding is closest to the VCPU on which
> > + * this call is being made.
> > + */
> > +struct vmbus_channel *get_outgoing_channel(struct vmbus_channel
> *primary)
> 
> That's a _very_ vague global symbol name you are adding to the kernel.
> Same goes for the other functions you are adding here, please fix that,
> and make them have the vmbus_ prefix, like everything else in this
> patch.


Will do.

K. Y


--
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 V1 7/7] Drivers: scsi: storvsc: Increase the value of STORVSC_MAX_IO_REQUESTS

[KY Srinivasan]

> -Original Message-
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Thursday, May 16, 2013 8:02 AM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; jbottom...@parallels.com;
> h...@infradead.org; linux-scsi@vger.kernel.org; a...@canonical.com;
> jasow...@redhat.com
> Subject: Re: [PATCH V1 7/7] Drivers: scsi: storvsc: Increase the value of
> STORVSC_MAX_IO_REQUESTS
> 
> On Thu, May 16, 2013 at 05:21:19AM -0700, K. Y. Srinivasan wrote:
> > Increase the value of STORVSC_MAX_IO_REQUESTS to 200 requests. The
> current
> > ringbuffer size can support this higher value.
> >
> 
> The ringbuffer size is a module parameter so it's odd to talk about
> the "current" size.

While the ringbuffer size is a module parameter; there is a default value. The 
current size refers to the default. 
Your comment applies to the current value (of 128) as well in that it is 
possible for somebody to load this
driver  with a ringbuffer size that could not support  the value of 128. If 
this  is the case, we fail the load.
This safety check continues to exist. 

Regards,

K. Y
> 
> regards,
> dan carpenter
> 
> 


--
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 V1 7/7] Drivers: scsi: storvsc: Increase the value of STORVSC_MAX_IO_REQUESTS

[KY Srinivasan]

> -Original Message-
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Thursday, May 16, 2013 9:55 AM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; jbottom...@parallels.com;
> h...@infradead.org; linux-scsi@vger.kernel.org; a...@canonical.com;
> jasow...@redhat.com
> Subject: Re: [PATCH V1 7/7] Drivers: scsi: storvsc: Increase the value of
> STORVSC_MAX_IO_REQUESTS
> 
> On Thu, May 16, 2013 at 01:37:41PM +, KY Srinivasan wrote:
> >
> >
> > > -Original Message-
> > > From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> > > Sent: Thursday, May 16, 2013 8:02 AM
> > > To: KY Srinivasan
> > > Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> > > de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com;
> > > h...@infradead.org; linux-scsi@vger.kernel.org; a...@canonical.com;
> > > jasow...@redhat.com
> > > Subject: Re: [PATCH V1 7/7] Drivers: scsi: storvsc: Increase the value of
> > > STORVSC_MAX_IO_REQUESTS
> > >
> > > On Thu, May 16, 2013 at 05:21:19AM -0700, K. Y. Srinivasan wrote:
> > > > Increase the value of STORVSC_MAX_IO_REQUESTS to 200 requests. The
> > > current
> > > > ringbuffer size can support this higher value.
> > > >
> > >
> > > The ringbuffer size is a module parameter so it's odd to talk about
> > > the "current" size.
> >
> > While the ringbuffer size is a module parameter; there is a default value. 
> > The
> current size refers to the default.
> > Your comment applies to the current value (of 128) as well in that it is 
> > possible
> for somebody to load this
> > driver  with a ringbuffer size that could not support  the value of 128. If 
> > this  is
> the case, we fail the load.
> > This safety check continues to exist.
> 
> The issue is there in the original code, true.
> 
> Would the right fix be to add some sanity checks in module_init()?

The check is already there (as I noted above). Look at the function:
storvsc_drv_init(). If the ring size is picked incorrectly, the load is failed.

Regards,

K. Y
> 
> regards,
> dan carpenter
> 
> 


--
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 v2] storvsc: add logging for error/warning messages

[KY Srinivasan]

> -Original Message-
> From: Long Li [mailto:lon...@microsoft.com]
> Sent: Friday, December 4, 2015 12:07 AM
> To: KY Srinivasan ; Haiyang Zhang
> ; James E.J. Bottomley 
> Cc: de...@linuxdriverproject.org; linux-scsi@vger.kernel.org; linux-
> ker...@vger.kernel.org; Long Li 
> Subject: [PATCH v2] storvsc: add logging for error/warning messages
> 
> 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.
> 
> Signed-off-by: Long Li 
Reviewed-by : K. Y. Srinivasan 
Signed-off-by: K. Y. Srinivasan 

> ---
>  drivers/scsi/storvsc_drv.c | 34 +-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 40c43ae..f46ed2c 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -164,6 +164,26 @@ 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;
> +}
> +
> +#define storvsc_log(dev, level, fmt, ...)\
> +do { \
> + if (do_logging(level))  \
> + dev_warn(&(dev)->device, fmt, ##__VA_ARGS__);   \
> +} while (0)
> +
>  struct vmscsi_win8_extension {
>   /*
>* The following were added in Windows 8
> @@ -1185,7 +1205,8 @@ static void storvsc_command_completion(struct
> storvsc_cmd_request *cmd_request)
> 
>   if (scmnd->result) {
>   if (scsi_normalize_sense(scmnd->sense_buffer,
> - SCSI_SENSE_BUFFERSIZE, &sense_hdr))
> + SCSI_SENSE_BUFFERSIZE, &sense_hdr) &&
> + do_logging(STORVSC_LOGGING_ERROR))
>   scsi_print_sense_hdr(scmnd->device, "storvsc",
>&sense_hdr);
>   }
> @@ -1239,6 +1260,13 @@ 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)
> + storvsc_log(device, STORVSC_LOGGING_WARN,
> + "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 */
> @@ -1246,6 +1274,10 @@ static void storvsc_on_io_completion(struct
> hv_device *device,
>   SRB_STATUS_AUTOSENSE_VALID) {
>   /* autosense data available */
> 
> + storvsc_log(device, STORVSC_LOGGING_WARN,
> + "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,
>  vstor_packet->vm_srb.sense_info_length);
> --
> 1.8.5.6

--
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 3/4] scsi: storvsc: Refactor the code in storvsc_channel_init()

[KY Srinivasan]

> -Original Message-
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Friday, December 11, 2015 2:41 AM
> To: KY Srinivasan 
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> martin.peter...@oracle.com
> Subject: Re: [PATCH 3/4] scsi: storvsc: Refactor the code in
> storvsc_channel_init()
> 
> On Thu, Dec 10, 2015 at 04:14:19PM -0800, K. Y. Srinivasan wrote:
> > @@ -753,27 +740,62 @@ static int storvsc_channel_init(struct hv_device
> *device, bool is_fc)
> >VM_PKT_DATA_INBAND,
> >
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> > if (ret != 0)
> > -   goto cleanup;
> > +   goto done;
> >
> > t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> > if (t == 0) {
> > ret = -ETIMEDOUT;
> > -   goto cleanup;
> > +   goto done;
> > }
> >
> > +   if (!status_check)
> > +   goto done;
> 
> See?  This goto looks exactly the same as the earlier buggy goto but
> it's actually correct.  Meanwhile if you just used an explicit
> "return 0;" then it would be easy to understand.
> 
> I rant about this all the time but it's because it's bad deliberately.
> It's normal to have bugs, but this deliberate stuff really I can't
> understand it...
> 
> > +
> > if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO
> ||
> > vstor_packet->status != 0) {
> > ret = -EINVAL;
> > -   goto cleanup;
> > +   goto done;
> > }
> >
> > +done:
> > +   return ret;
> > +}
> > +
> > +static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> > +{
> > +   struct storvsc_device *stor_device;
> > +   struct storvsc_cmd_request *request;
> > +   struct vstor_packet *vstor_packet;
> > +   int ret, i;
> > +   int max_chns;
> > +   bool process_sub_channels = false;
> > +
> > +   stor_device = get_out_stor_device(device);
> > +   if (!stor_device)
> > +   return -ENODEV;
> > +
> > +   request = &stor_device->init_request;
> > +   vstor_packet = &request->vstor_packet;
> > +
> > +   /*
> > +* Now, initiate the vsc/vsp initialization protocol on the open
> > +* channel
> > +*/
> > +   memset(request, 0, sizeof(struct storvsc_cmd_request));
> > +   vstor_packet->operation =
> VSTOR_OPERATION_BEGIN_INITIALIZATION;
> > +   ret = storvsc_execute_vstor_op(device, request, true);
> > +   if (ret)
> > +   goto cleanup;
> 
> 10 lines earlier there is an explicit "return -ENODEV" so it's not as if
> writing explicit returns will kill you.

Thanks Dan; I will cleanup the code and resend.

Regards,

K. Y



--
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 2/4] scsi: storvsc: Properly support Fibre Channel devices

[KY Srinivasan]

> -Original Message-
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Friday, December 11, 2015 2:25 AM
> To: KY Srinivasan 
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> martin.peter...@oracle.com
> Subject: Re: [PATCH 2/4] scsi: storvsc: Properly support Fibre Channel devices
> 
> On Thu, Dec 10, 2015 at 04:14:18PM -0800, K. Y. Srinivasan wrote:
> > +   ret = vmbus_sendpacket(device->channel, vstor_packet,
> > +  (sizeof(struct vstor_packet) -
> > +  vmscsi_size_delta),
> > +  (unsigned long)request,
> > +  VM_PKT_DATA_INBAND,
> > +
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> > +
> > +   if (ret != 0)
> > +   goto cleanup;
> > +
> > +   t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> > +   if (t == 0) {
> > +   ret = -ETIMEDOUT;
> > +   goto cleanup;
> > +   }
> > +
> > +   if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO
> ||
> > +   vstor_packet->status != 0)
> > +   goto cleanup;
> 
> "cleanup" is a misleading name because it doesn't clean up anything.
> Do nothing gotos are a pain in the butt and they always introduce bugs.
> For example, you appear to have forgotten to set the error code.  But
> because it's a do-nothing goto it's ambiguous so perhaps returning
> success was intended.
> 
> Empirically this style of coding causes bugs.  It does not prevent them.
> It is a bad style if you believe in measuring, evidence and science.

Thanks Dan. I will fix this and resend.

K. Y

--
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 1/4] scsi: storvsc: Fix a bug in the layout of the hv_fc_wwn_packet

[KY Srinivasan]

> -Original Message-
> From: Johannes Thumshirn [mailto:jthumsh...@suse.de]
> Sent: Friday, December 11, 2015 12:48 AM
> To: KY Srinivasan 
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> martin.peter...@oracle.com
> Subject: Re: [PATCH 1/4] scsi: storvsc: Fix a bug in the layout of the
> hv_fc_wwn_packet
> 
> On Thu, Dec 10, 2015 at 04:14:17PM -0800, K. Y. Srinivasan wrote:
> > The hv_fc_wwn_packet is exchanged over vmbus. Make the definition in
> Linux match
> > the Window's definition.
> >
> > Signed-off-by: K. Y. Srinivasan 
> > Reviewed-by: Long Li 
> > Tested-by: Alex Ng 
> > ---
> >  drivers/scsi/storvsc_drv.c |5 ++---
> >  1 files changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > index c41f674..00bb4bd 100644
> > --- a/drivers/scsi/storvsc_drv.c
> > +++ b/drivers/scsi/storvsc_drv.c
> > @@ -92,9 +92,8 @@ enum vstor_packet_operation {
> >   */
> >
> >  struct hv_fc_wwn_packet {
> > -   boolprimary_active;
> > -   u8  reserved1;
> > -   u8  reserved2;
> > +   u8  primary_active;
> > +   u8  reserved1[3];
> > u8  primary_port_wwn[8];
> > u8  primary_node_wwn[8];
> > u8  secondary_port_wwn[8];
> > --
> > 1.7.4.1
> >
> > --
> > 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
> https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fvger.ker
> nel.org%2fmajordomo-
> info.html&data=01%7c01%7ckys%40microsoft.com%7cb6c3be0d1b474ac1374
> 908d30207c170%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=7hfMcn
> qW32KiJgr7JkXsC4P0CnnQvAearwu7LzyWaqM%3d
> 
> Should this go through stable as well?
> If yes:
> Fixes: 8b612fa23 '[SCSI] storvsc: Update the storage protocol to win8 level'
> Cc: sta...@vger.kernel.org # v3.11+
This is the first time we are using this data structure and so I don't think we 
need
to port it back to stable.

Regards,

K. Y

> Reviewed-by: Johannes Thumshirn 
> 
> --
> Johannes Thumshirn  Storage
> jthumsh...@suse.de+49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
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 4/4] scsi: storvsc: Tighten up the interrupt path

[KY Srinivasan]

> -Original Message-
> From: Hannes Reinecke [mailto:h...@suse.de]
> Sent: Friday, December 18, 2015 12:52 AM
> To: KY Srinivasan ; gre...@linuxfoundation.org; linux-
> ker...@vger.kernel.org; de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> martin.peter...@oracle.com
> Subject: Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path
> 
> On 12/13/2015 09:28 PM, K. Y. Srinivasan wrote:
> > On the interrupt path, we repeatedly establish the pointer to the
> > storvsc_device. Fix this.
> >
> > Signed-off-by: K. Y. Srinivasan 
> > Reviewed-by: Long Li 
> > Reviewed-by: Johannes Thumshirn 
> > Tested-by: Alex Ng 
> > ---
> >   drivers/scsi/storvsc_drv.c |   23 ---
> >   1 files changed, 8 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > index d6ca4f2..b68aebe 100644
> > --- a/drivers/scsi/storvsc_drv.c
> > +++ b/drivers/scsi/storvsc_drv.c
> > @@ -945,19 +945,16 @@ static void storvsc_handle_error(struct
> vmscsi_request *vm_srb,
> >   }
> >
> >
> > -static void storvsc_command_completion(struct storvsc_cmd_request
> *cmd_request)
> > +static void storvsc_command_completion(struct storvsc_cmd_request
> *cmd_request,
> > +  struct storvsc_device *stor_dev)
> >   {
> > struct scsi_cmnd *scmnd = cmd_request->cmd;
> > -   struct hv_host_device *host_dev = shost_priv(scmnd->device-
> >host);
> > struct scsi_sense_hdr sense_hdr;
> > struct vmscsi_request *vm_srb;
> > struct Scsi_Host *host;
> > -   struct storvsc_device *stor_dev;
> > -   struct hv_device *dev = host_dev->dev;
> > u32 payload_sz = cmd_request->payload_sz;
> > void *payload = cmd_request->payload;
> >
> > -   stor_dev = get_in_stor_device(dev);
> > host = stor_dev->host;
> >
> > vm_srb = &cmd_request->vstor_packet.vm_srb;
> > @@ -987,14 +984,13 @@ static void storvsc_command_completion(struct
> storvsc_cmd_request *cmd_request)
> > kfree(payload);
> >   }
> >
> > -static void storvsc_on_io_completion(struct hv_device *device,
> > +static void storvsc_on_io_completion(struct storvsc_device *stor_device,
> >   struct vstor_packet *vstor_packet,
> >   struct storvsc_cmd_request *request)
> >   {
> > -   struct storvsc_device *stor_device;
> > struct vstor_packet *stor_pkt;
> > +   struct hv_device *device = stor_device->device;
> >
> > -   stor_device = hv_get_drvdata(device);
> > stor_pkt = &request->vstor_packet;
> >
> > /*
> > @@ -1049,7 +1045,7 @@ static void storvsc_on_io_completion(struct
> hv_device *device,
> > stor_pkt->vm_srb.data_transfer_length =
> > vstor_packet->vm_srb.data_transfer_length;
> >
> > -   storvsc_command_completion(request);
> > +   storvsc_command_completion(request, stor_device);
> >
> > if (atomic_dec_and_test(&stor_device->num_outstanding_req) &&
> > stor_device->drain_notify)
> > @@ -1058,21 +1054,19 @@ static void storvsc_on_io_completion(struct
> hv_device *device,
> >
> >   }
> >
> > -static void storvsc_on_receive(struct hv_device *device,
> > +static void storvsc_on_receive(struct storvsc_device *stor_device,
> >  struct vstor_packet *vstor_packet,
> >  struct storvsc_cmd_request *request)
> >   {
> > struct storvsc_scan_work *work;
> > -   struct storvsc_device *stor_device;
> >
> > switch (vstor_packet->operation) {
> > case VSTOR_OPERATION_COMPLETE_IO:
> > -   storvsc_on_io_completion(device, vstor_packet, request);
> > +   storvsc_on_io_completion(stor_device, vstor_packet,
> request);
> > break;
> >
> > case VSTOR_OPERATION_REMOVE_DEVICE:
> > case VSTOR_OPERATION_ENUMERATE_BUS:
> > -   stor_device = get_in_stor_device(device);
> > work = kmalloc(sizeof(struct storvsc_scan_work),
> GFP_ATOMIC);
> > if (!work)
> > return;
> > @@ -1083,7 +1077,6 @@ static void storvsc_on_receive(struct hv_device
> *device,
> > break;
> >
> > case VSTOR_OPERATION_FCHBA_DAT

RE: [PATCH V3 2/4] scsi: storvsc: Properly support Fibre Channel devices

[KY Srinivasan]

> -Original Message-
> From: Hannes Reinecke [mailto:h...@suse.de]
> Sent: Friday, December 18, 2015 12:49 AM
> To: KY Srinivasan ; gre...@linuxfoundation.org; linux-
> ker...@vger.kernel.org; de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> martin.peter...@oracle.com
> Subject: Re: [PATCH V3 2/4] scsi: storvsc: Properly support Fibre Channel
> devices
> 
> On 12/13/2015 09:28 PM, K. Y. Srinivasan wrote:
> > For FC devices managed by this driver, atttach the appropriate transport
> > template. This will allow us to create the appropriate sysfs files for
> > these devices. With this we can publish the wwn for both the port and the
> node.
> >
> > Signed-off-by: K. Y. Srinivasan 
> > Reviewed-by: Long Li 
> > Tested-by: Alex Ng 
> > ---
> > V2: Fixed error paths - Dan Carpenter 
> > V3: Fixed build issues reported by kbuild test robot 
> >
> >   drivers/scsi/storvsc_drv.c |  181
> ---
> >   1 files changed, 134 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > index 00bb4bd..220b794 100644
> > --- a/drivers/scsi/storvsc_drv.c
> > +++ b/drivers/scsi/storvsc_drv.c
> > @@ -41,6 +41,7 @@
> >   #include 
> >   #include 
> >   #include 
> > +#include 
> >
> >   /*
> >* All wire protocol details (storage protocol between the guest and the
> host)
> > @@ -397,6 +398,9 @@ static int storvsc_timeout = 180;
> >
> >   static int msft_blist_flags = BLIST_TRY_VPD_PAGES;
> >
> > +#ifdef CONFIG_SCSI_FC_ATTRS
> > +static struct scsi_transport_template *fc_transport_template;
> > +#endif
> >
> >   static void storvsc_on_channel_callback(void *context);
> >
> > @@ -456,6 +460,11 @@ struct storvsc_device {
> > /* Used for vsc/vsp channel reset process */
> > struct storvsc_cmd_request init_request;
> > struct storvsc_cmd_request reset_request;
> > +   /*
> > +* Currently active port and node names for FC devices.
> > +*/
> > +   u64 node_name;
> > +   u64 port_name;
> >   };
> >
> >   struct hv_host_device {
> > @@ -695,7 +704,26 @@ static void  handle_multichannel_storage(struct
> hv_device *device, int max_chns)
> > vmbus_are_subchannels_present(device->channel);
> >   }
> >
> > -static int storvsc_channel_init(struct hv_device *device)
> > +static void cache_wwn(struct storvsc_device *stor_device,
> > + struct vstor_packet *vstor_packet)
> > +{
> > +   /*
> > +* Cache the currently active port and node ww names.
> > +*/
> > +   if (vstor_packet->wwn_packet.primary_active) {
> > +   stor_device->node_name =
> > +   wwn_to_u64(vstor_packet-
> >wwn_packet.primary_node_wwn);
> > +   stor_device->port_name =
> > +   wwn_to_u64(vstor_packet-
> >wwn_packet.primary_port_wwn);
> > +   } else {
> > +   stor_device->node_name =
> > +   wwn_to_u64(vstor_packet-
> >wwn_packet.secondary_node_wwn);
> > +   stor_device->port_name =
> > +   wwn_to_u64(vstor_packet-
> >wwn_packet.secondary_port_wwn);
> > +   }
> > +}
> > +
> > +static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> >   {
> > struct storvsc_device *stor_device;
> > struct storvsc_cmd_request *request;
> > @@ -727,19 +755,15 @@ static int storvsc_channel_init(struct hv_device
> *device)
> >VM_PKT_DATA_INBAND,
> >
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> > if (ret != 0)
> > -   goto cleanup;
> > +   return ret;
> >
> > t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> > -   if (t == 0) {
> > -   ret = -ETIMEDOUT;
> > -   goto cleanup;
> > -   }
> > +   if (t == 0)
> > +   return -ETIMEDOUT;
> >
> > if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO
> ||
> > -   vstor_packet->status != 0) {
> > -   ret = -EINVAL;
> > -   goto cleanup;
> > -   }
> > +   vstor_packet->status != 0)
> > +   return -EINVAL;
> >
> >
> > for (i = 0; i < ARRAY_SIZE(vmstor_protocols); i++) {
> > 

RE: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:james.bottom...@hansenpartnership.com]
> Sent: Friday, December 18, 2015 8:48 AM
> To: KY Srinivasan ; Hannes Reinecke ;
> gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> martin.peter...@oracle.com
> Subject: Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path
> 
> On Fri, 2015-12-18 at 16:20 +, KY Srinivasan wrote:
> >
> > > -Original Message-
> > > From: Hannes Reinecke [mailto:h...@suse.de]
> > > Sent: Friday, December 18, 2015 12:52 AM
> > > To: KY Srinivasan ; gre...@linuxfoundation.org;
> > > linux-
> > > ker...@vger.kernel.org; de...@linuxdriverproject.org;
> > > oher...@suse.com;
> > > jbottom...@parallels.com; h...@infradead.org;
> > > linux-scsi@vger.kernel.org;
> > > a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> > > martin.peter...@oracle.com
> > > Subject: Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt
> > > path
> > >
> > > On 12/13/2015 09:28 PM, K. Y. Srinivasan wrote:
> > > > On the interrupt path, we repeatedly establish the pointer to the
> > > > storvsc_device. Fix this.
> > > >
> > > > Signed-off-by: K. Y. Srinivasan 
> > > > Reviewed-by: Long Li 
> > > > Reviewed-by: Johannes Thumshirn 
> > > > Tested-by: Alex Ng 
> > > > ---
> > > >   drivers/scsi/storvsc_drv.c |   23 ---
> > > >   1 files changed, 8 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/drivers/scsi/storvsc_drv.c
> > > > b/drivers/scsi/storvsc_drv.c
> > > > index d6ca4f2..b68aebe 100644
> > > > --- a/drivers/scsi/storvsc_drv.c
> > > > +++ b/drivers/scsi/storvsc_drv.c
> > > > @@ -945,19 +945,16 @@ static void storvsc_handle_error(struct
> > > vmscsi_request *vm_srb,
> > > >   }
> > > >
> > > >
> > > > -static void storvsc_command_completion(struct
> > > > storvsc_cmd_request
> > > *cmd_request)
> > > > +static void storvsc_command_completion(struct
> > > > storvsc_cmd_request
> > > *cmd_request,
> > > > +  struct storvsc_device
> > > > *stor_dev)
> > > >   {
> > > > struct scsi_cmnd *scmnd = cmd_request->cmd;
> > > > -   struct hv_host_device *host_dev = shost_priv(scmnd
> > > > ->device-
> > > > host);
> > > > struct scsi_sense_hdr sense_hdr;
> > > > struct vmscsi_request *vm_srb;
> > > > struct Scsi_Host *host;
> > > > -   struct storvsc_device *stor_dev;
> > > > -   struct hv_device *dev = host_dev->dev;
> > > > u32 payload_sz = cmd_request->payload_sz;
> > > > void *payload = cmd_request->payload;
> > > >
> > > > -   stor_dev = get_in_stor_device(dev);
> > > > host = stor_dev->host;
> > > >
> > > > vm_srb = &cmd_request->vstor_packet.vm_srb;
> > > > @@ -987,14 +984,13 @@ static void
> > > > storvsc_command_completion(struct
> > > storvsc_cmd_request *cmd_request)
> > > > kfree(payload);
> > > >   }
> > > >
> > > > -static void storvsc_on_io_completion(struct hv_device *device,
> > > > +static void storvsc_on_io_completion(struct storvsc_device
> > > > *stor_device,
> > > >   struct vstor_packet
> > > > *vstor_packet,
> > > >   struct storvsc_cmd_request
> > > > *request)
> > > >   {
> > > > -   struct storvsc_device *stor_device;
> > > > struct vstor_packet *stor_pkt;
> > > > +   struct hv_device *device = stor_device->device;
> > > >
> > > > -   stor_device = hv_get_drvdata(device);
> > > > stor_pkt = &request->vstor_packet;
> > > >
> > > > /*
> > > > @@ -1049,7 +1045,7 @@ static void storvsc_on_io_completion(struct
> > > hv_device *device,
> > > > stor_pkt->vm_srb.data_transfer_length =
> > &

RE: [PATCH V3 2/4] scsi: storvsc: Properly support Fibre Channel devices

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:james.bottom...@hansenpartnership.com]
> Sent: Friday, December 18, 2015 9:14 AM
> To: Hannes Reinecke ; KY Srinivasan ;
> gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> martin.peter...@oracle.com
> Subject: Re: [PATCH V3 2/4] scsi: storvsc: Properly support Fibre Channel
> devices
> 
> On Fri, 2015-12-18 at 09:49 +0100, Hannes Reinecke wrote:
> > What I would like to see is a clear separation here:
> > - Disable FC disk handling if FC attributes are not configured
> > - Add a module parameter allowing to disable FC attributes even if
> > they are compiled in. Remember: this is a virtualized guest, and
> > people might want so save kernel memory wherever they can. So always
> > attaching to the fc transport template will make them very unhappy.
> > Alternatively you could split out FC device handling into a separate
> > driver, but seeing the diff that's probably overkill.
> 
> I don't quite see how this can be a module parameter: the
> fc_transport_class is pulled in by symbol references.  They won't go
> away whether a module parameter is zero or one.  The only way to get
> the module not to link with a transport class is to have it not use the
> symbols at compile time (either because they're surrounded by an #ifdef
> or with an if() which the compiler evaluates at compile time to zero).
>  In userspace you get around this with introspection and dlopen, but I
> don't think we have that functionality in the kernel.

Hannes,
Perhaps I misunderstood your comment when I first responded to this suggestion
from you - I thought you were concerned about unconditionally allocating FC 
transport
template and I had proposed a work around that. Now looking at James comment, 
it looks
like you were concerned about FC transport module dependency on the storvsc 
module.
Do you still want me to work on my proposal.

Thanks,

K. Y
> 
> James

RE: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path

[KY Srinivasan]

> -Original Message-
> From: James Bottomley [mailto:james.bottom...@hansenpartnership.com]
> Sent: Monday, December 21, 2015 8:28 AM
> To: KY Srinivasan ; Hannes Reinecke ;
> gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> martin.peter...@oracle.com
> Subject: Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path
> 
> On Sat, 2015-12-19 at 02:28 +, KY Srinivasan wrote:
> >
> > > -Original Message-
> > > From: James Bottomley
> [mailto:james.bottom...@hansenpartnership.com
> > > ]
> > > Sent: Friday, December 18, 2015 8:48 AM
> > > To: KY Srinivasan ; Hannes Reinecke <
> > > h...@suse.de>;
> > > gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> > > de...@linuxdriverproject.org; oher...@suse.com;
> > > jbottom...@parallels.com; h...@infradead.org;
> > > linux-scsi@vger.kernel.org;
> > > a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> > > martin.peter...@oracle.com
> > > Subject: Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt
> > > path
> > >
> > > On Fri, 2015-12-18 at 16:20 +, KY Srinivasan wrote:
> > > >
> > > > > -Original Message-
> > > > > From: Hannes Reinecke [mailto:h...@suse.de]
> > > > > Sent: Friday, December 18, 2015 12:52 AM
> > > > > To: KY Srinivasan ;
> > > > > gre...@linuxfoundation.org;
> > > > > linux-
> > > > > ker...@vger.kernel.org; de...@linuxdriverproject.org;
> > > > > oher...@suse.com;
> > > > > jbottom...@parallels.com; h...@infradead.org;
> > > > > linux-scsi@vger.kernel.org;
> > > > > a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> > > > > martin.peter...@oracle.com
> > > > > Subject: Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the
> > > > > interrupt
> > > > > path
> > > > >
> > > > > On 12/13/2015 09:28 PM, K. Y. Srinivasan wrote:
> > > > > > On the interrupt path, we repeatedly establish the pointer to
> > > > > > the
> > > > > > storvsc_device. Fix this.
> > > > > >
> > > > > > Signed-off-by: K. Y. Srinivasan 
> > > > > > Reviewed-by: Long Li 
> > > > > > Reviewed-by: Johannes Thumshirn 
> > > > > > Tested-by: Alex Ng 
> > > > > > ---
> > > > > >   drivers/scsi/storvsc_drv.c |   23 ---
> > > > > >   1 files changed, 8 insertions(+), 15 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/scsi/storvsc_drv.c
> > > > > > b/drivers/scsi/storvsc_drv.c
> > > > > > index d6ca4f2..b68aebe 100644
> > > > > > --- a/drivers/scsi/storvsc_drv.c
> > > > > > +++ b/drivers/scsi/storvsc_drv.c
> > > > > > @@ -945,19 +945,16 @@ static void storvsc_handle_error(struct
> > > > > vmscsi_request *vm_srb,
> > > > > >   }
> > > > > >
> > > > > >
> > > > > > -static void storvsc_command_completion(struct
> > > > > > storvsc_cmd_request
> > > > > *cmd_request)
> > > > > > +static void storvsc_command_completion(struct
> > > > > > storvsc_cmd_request
> > > > > *cmd_request,
> > > > > > +  struct storvsc_device
> > > > > > *stor_dev)
> > > > > >   {
> > > > > > struct scsi_cmnd *scmnd = cmd_request->cmd;
> > > > > > -   struct hv_host_device *host_dev = shost_priv(scmnd
> > > > > > ->device-
> > > > > > host);
> > > > > > struct scsi_sense_hdr sense_hdr;
> > > > > > struct vmscsi_request *vm_srb;
> > > > > > struct Scsi_Host *host;
> > > > > > -   struct storvsc_device *stor_dev;
> > > > > > -   struct hv_device *dev = host_dev->dev;
> > > > > > u32 payload_sz = cmd_request->payload_sz;
> > > > > > void *payload = cmd_request->payload;
> > > > > >
> > > > > > -   stor_dev = get_in_stor_devic