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 <h...@suse.de>; KY Srinivasan <k...@microsoft.com>;
> 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 2/4] scsi: storvsc: Properly support Fibre Channel devices

[Hannes Reinecke]

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(>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++) {
@@ -764,18 +788,14 @@ 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(>wait_event, 5*HZ);
-   if (t == 0) {
-   ret = -ETIMEDOUT;
-   goto cleanup;
-   }
+   if (t == 0)
+   return -ETIMEDOUT;

-   if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO) {
-   ret = -EINVAL;
-   goto cleanup;
-   }
+   if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO)
+   return -EINVAL;

if (vstor_packet->status == 0) {
vmstor_proto_version =
@@ -791,10 +811,8 @@ static int storvsc_channel_init(struct hv_device *device)
}
}

-   if (vstor_packet->status != 0) {
-   ret = -EINVAL;
-   goto cleanup;
-   }
+   if (vstor_packet->status != 0)
+   return -EINVAL;


memset(vstor_packet, 0, sizeof(struct vstor_packet));
@@ -809,19 +827,15 @@ static int 

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 <k...@microsoft.com>; 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 <k...@microsoft.com>
> > Reviewed-by: Long Li <lon...@microsoft.com>
> > Tested-by: Alex Ng <ale...@microsoft.com>
> > ---
> > V2: Fixed error paths - Dan Carpenter <dan.carpen...@oracle.com>
> > V3: Fixed build issues reported by kbuild test robot <l...@intel.com>
> >
> >   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(>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-&g

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

[James Bottomley]

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.

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