Re: [PATCH 2/3] virtio-scsi: Add FC transport class

[Hannes Reinecke]

On 02/02/2018 06:39 PM, Steffen Maier wrote:
> 
> On 02/02/2018 05:00 PM, Hannes Reinecke wrote:
>> On 01/26/2018 05:54 PM, Steffen Maier wrote:
>>> On 12/18/2017 09:31 AM, Hannes Reinecke wrote:
 On 12/15/2017 07:08 PM, Steffen Maier wrote:
> On 12/14/2017 11:11 AM, Hannes Reinecke wrote:
> 
>>> To me, this raises the question which properties of the host's FC
>>> (driver core) objects should be mirrored to the guest. Ideally all (and
>>> that's a lot).
>>> This in turn makes me wonder if mirroring is really desirable (e.g.
>>> considering the effort) or if only the guest should have its own FC
>>> object hierarchy which does _not_ exist on the KVM host in case an
>>> fc_host is passed through with virtio-(v)fc.
> 
>>> A few more thoughts on your presentation [1]:
>>>
>>> "Devices on the vport will not be visible on the host"
>>> I could not agree more to the design point that devices (or at least
>>> their descendant object subtree) passed through to a guest should not
>>> appear on the host!
>>> With virtio-blk or virtio-scsi, we have SCSI devices and thus disks
>>> visible in the host, which needlessly scans partitions, or even worse
>>> automatically scans for LVM and maybe even activates PVs/VGs/LVs. It's
>>> hard for a KVM host admin to suppress this (and not break the devices
>>> the host needs itself).
>>> If we mirror the host's scsi_transport_fc tree including fc_rports and
>>> thus SCSI devices etc., we would have the same problems?
>>> Even more so, dev_loss_tmo and fast_io_fail_tmo would run independently
>>> on the host and in the guest on the same mirrored scsi_transport_fc
>>> object tree. I can envision user confusion having configured timeouts on
>>> the "wrong" side (host vs. guest). Also we would still need a mechanism
>>> to mirror fc_rport (un)block from host to guest for proper transport
>>> recovery. In zfcp we try to recover on transport rather than scsi_eh
>>> whenever possible because it is so much smoother.
>>>
>> As similar thing can be achieved event today, by setting the
>> 'no_uld_attach' parameter when scanning the scsi device
>> (that's what some RAID HBAs do).
>> However, there currently is no way of modifying it from user-space, and
>> certainly not to change the behaviour for existing devices.
>> It should be relatively simple to set this flag whenever the host is
>> exposed to a VM; we would still see the scsi devices, but the 'sd'
>> driver won't be attached so nothing will scan the device on the host.
> 
> Ah, nice, didn't know that. It would solve the undesired I/O problem in
> the host.
> But it would not solve the so far somewhat unsynchronized state
> transitions of fc_rports on the host and their mirrors in the guest?
> 
> I would be very interested in how you intend to do transport recovery.
> 
So am I :-)

Current plan is to check fc-transport events; probably implementing a
listener which then will manage the interface to the guest.
Also I'm looking at implementing a new block device, which just gets a
WWPN as input and manages all associated scsi (sg) devices.
But that still needs some more work to be done.

>>> "Towards virtio-fc?"
[ .. ]
>> I'm not convinced that moving to full virtio-fc is something we want or
>> even can do.
>> Neither qla2xxx nor lpfc allow for direct FC frame access; so one would
>> need to reformat the FC frames into something the driver understands,
>> just so that the hardware can transform it back into FC frames.
> 
> I thought of a more high level para-virtualized FCP HBA interface, than
> FC frames (which did exist in kernel v2.4 under drivers/fc4/ but no
> longer as it seems). Just like large parts of today's FCP LLDDs handle
> scatter gather lists and framing is done by the hardware.
> 
But that would amount to yet another abstraction layer; not sure if it's
worth the trouble...

>> Another thing is xid management; some drivers have to do their own xid
>> management, based on hardware capabilities etc.
>> So the FC frames would need to re-write the xids, making it hard if not
>> impossible to match things up when the response comes in.
> 
> For such things, where the hardware exposes more details (than, say,
> zfcp sees) I thought the LLDD on the KVM host would handle such details
> internally and only expose the higher level interface to virtio-fc.
> 
Yes; but that's what I'm aiming for with my virtio-vfc thingie already :-)

> Maybe something roughly like the basic transport protocol part of
> ibmvfc/ibmvscsi (not the other end in the firmware and not the cross
> partition DMA part), if I understood its overall design correctly by
> quickly looking at the code.
> I somewhat had the impression that zfcp isn't too far from the overall
> operations style. As seem qla2xxx or lpfc to me, they just see and need
> to handle some more low-level FC details.
> Conceptually replace CRQ/RDMA or QDIO with virtio.
> (And for ibmvscsi also: SRP => FCP because it uses a different SCSI
> transport.)
> 
This is actually a different 

Re: [PATCH 2/3] virtio-scsi: Add FC transport class

[Steffen Maier]

On 02/02/2018 05:00 PM, Hannes Reinecke wrote:

On 01/26/2018 05:54 PM, Steffen Maier wrote:

On 12/18/2017 09:31 AM, Hannes Reinecke wrote:

On 12/15/2017 07:08 PM, Steffen Maier wrote:

On 12/14/2017 11:11 AM, Hannes Reinecke wrote:



To me, this raises the question which properties of the host's FC
(driver core) objects should be mirrored to the guest. Ideally all (and
that's a lot).
This in turn makes me wonder if mirroring is really desirable (e.g.
considering the effort) or if only the guest should have its own FC
object hierarchy which does _not_ exist on the KVM host in case an
fc_host is passed through with virtio-(v)fc.



A few more thoughts on your presentation [1]:

"Devices on the vport will not be visible on the host"
I could not agree more to the design point that devices (or at least
their descendant object subtree) passed through to a guest should not
appear on the host!
With virtio-blk or virtio-scsi, we have SCSI devices and thus disks
visible in the host, which needlessly scans partitions, or even worse
automatically scans for LVM and maybe even activates PVs/VGs/LVs. It's
hard for a KVM host admin to suppress this (and not break the devices
the host needs itself).
If we mirror the host's scsi_transport_fc tree including fc_rports and
thus SCSI devices etc., we would have the same problems?
Even more so, dev_loss_tmo and fast_io_fail_tmo would run independently
on the host and in the guest on the same mirrored scsi_transport_fc
object tree. I can envision user confusion having configured timeouts on
the "wrong" side (host vs. guest). Also we would still need a mechanism
to mirror fc_rport (un)block from host to guest for proper transport
recovery. In zfcp we try to recover on transport rather than scsi_eh
whenever possible because it is so much smoother.


As similar thing can be achieved event today, by setting the
'no_uld_attach' parameter when scanning the scsi device
(that's what some RAID HBAs do).
However, there currently is no way of modifying it from user-space, and
certainly not to change the behaviour for existing devices.
It should be relatively simple to set this flag whenever the host is
exposed to a VM; we would still see the scsi devices, but the 'sd'
driver won't be attached so nothing will scan the device on the host.


Ah, nice, didn't know that. It would solve the undesired I/O problem in 
the host.
But it would not solve the so far somewhat unsynchronized state 
transitions of fc_rports on the host and their mirrors in the guest?


I would be very interested in how you intend to do transport recovery.


"Towards virtio-fc?"
Using the FCP_CMND_IU (instead of just a plain SCB as with virtio-scsi)
sounds promising to me as starting point.
A listener from the audience asked if you would also do ELS/CT in the
guest and you replied that this would not be good. Why is that?
Based on above starting point, doing ELS/CT (and basic aborts and maybe
a few other functions such as open/close ports or metadata transfer
commands) in the guest is exactly what I would have expected. An HBA
LLDD on the KVM host would implement such API and for all fc_hosts,
passed through this way, it would *not* establish any scsi_transport_fc
tree on the host. Instead the one virtio-vfc implementation in the guest
would do this indendently of which HBA LLDD provides the passed through
fc_host in the KVM host.
ELS/CT pass through is maybe even for free via FC_BSG for those LLDDs
that already implement it.
Rport open/close is just the analogon of slave_alloc()/slave_destroy().


I'm not convinced that moving to full virtio-fc is something we want or
even can do.
Neither qla2xxx nor lpfc allow for direct FC frame access; so one would
need to reformat the FC frames into something the driver understands,
just so that the hardware can transform it back into FC frames.


I thought of a more high level para-virtualized FCP HBA interface, than 
FC frames (which did exist in kernel v2.4 under drivers/fc4/ but no 
longer as it seems). Just like large parts of today's FCP LLDDs handle 
scatter gather lists and framing is done by the hardware.



Another thing is xid management; some drivers have to do their own xid
management, based on hardware capabilities etc.
So the FC frames would need to re-write the xids, making it hard if not
impossible to match things up when the response comes in.


For such things, where the hardware exposes more details (than, say, 
zfcp sees) I thought the LLDD on the KVM host would handle such details 
internally and only expose the higher level interface to virtio-fc.


Maybe something roughly like the basic transport protocol part of 
ibmvfc/ibmvscsi (not the other end in the firmware and not the cross 
partition DMA part), if I understood its overall design correctly by 
quickly looking at the code.
I somewhat had the impression that zfcp isn't too far from the overall 
operations style. As seem qla2xxx or lpfc to me, they just see and need 
to handle some more low-level FC 

Re: [PATCH 2/3] virtio-scsi: Add FC transport class

[Hannes Reinecke]

On 01/26/2018 05:54 PM, Steffen Maier wrote:
> On 12/18/2017 09:31 AM, Hannes Reinecke wrote:
>> On 12/15/2017 07:08 PM, Steffen Maier wrote:
>>> On 12/14/2017 11:11 AM, Hannes Reinecke wrote:
 When a device announces an 'FC' protocol we should be pulling
 in the FC transport class to have the rports etc setup correctly.
>>>
>>> It took some time for me to understand what this does.
>>> It seems to mirror the topology of rports and sdevs that exist under the
>>> fc_host on the kvm host side to the virtio-scsi-fc side in the guest.
>>>
>>> I like the idea. This is also what I've been suggesting users to do if
>>> they back virtio-scsi with zfcp on the kvm host side. Primarily to not
>>> stall all virtio-scsi I/O on all paths if the guest ever gets into
>>> scsi_eh. But also to make it look like an HBA pass through so one can
>>> more easily migrate to this once we have FCP pass through.
> 
> On second thought, I like the idea for virtio-scsi.
> 
> For the future virtio-(v)fc case, see below.
> 
 @@ -755,19 +823,34 @@ static int virtscsi_abort(struct scsi_cmnd *sc)
>>>
 +    if (vscsi->protocol == SCSI_PROTOCOL_FCP) {
 +    struct fc_rport *rport =
 +    starget_to_rport(scsi_target(sc->device));
 +    if (rport && rport->dd_data ) {
 +    tgt = rport->dd_data;
 +    target_id = tgt->target_id;
 +    } else
 +    return FAST_IO_FAIL;
 +    } else {
 +    tgt = scsi_target(sc->device)->hostdata;
 +    if (!tgt || tgt->removed)
 +    return FAST_IO_FAIL;
 +    }
>>>
>>> dito
>>>
 @@ -857,27 +970,67 @@ static void virtscsi_rescan_work(struct
 work_struct *work)

    wait_for_completion();
>>>
>>> Waiting in work item .vs. having the response (IRQ) path trigger
>>> subsequent processing async ?
>>> Or do we need the call chain(s) getting here to be in our own process
>>> context via the workqueue anyway?
>>>
>> Can't see I can parse this sentence, but I'll be looking at the code
>> trying to come up with a clever explanation :-)
> 
> Sorry, meanwhile I have a hard time understanding my own words, too.
> 
> I think I wondered if the effort of a work item is really necessary,
> especially considering that it does block on the completion and thus
> could delay other queued work items (even though Concurrency Managed
> Workqueues can often hide this delay).
> 
> Couldn't we just return asynchronously after having sent the request.
> And then later on, simply have the response (IRQ) path trigger whatever
> processing is necessary (after the work item variant woke up from the
> wait_for_completion) in some asynchronuous fashion? Of course, this
> could also be a work item which just does necessary remaining processing
> after we got a response.
> Just a wild guess, without knowing the environmental requirements.
> 
The main point here was that we get off the irq-completion handler; if
we were to trigger this directly we would be running in an interrupt
context, which then will make things like spin_lock, mutex et al tricky.
Plus it's not really time critical; during rescanning all I/O should be
blocked, so we shouldn't have anything else going on.

 +    if (transport == SCSI_PROTOCOL_FCP) {
 +    struct fc_rport_identifiers rport_ids;
 +    struct fc_rport *rport;
 +
 +    rport_ids.node_name = wwn_to_u64(cmd->resp.rescan.node_wwn);
 +    rport_ids.port_name = wwn_to_u64(cmd->resp.rescan.port_wwn);
 +    rport_ids.port_id = (target_id >> 8);
>>>
>>> Why do you shift target_id by one byte to the right?
>>>
>> Because with the original setup virtio_scsi guest would pass in the
>> target_id, and the host would be selecting the device based on that
>> information.
>> With virtio-vfc we pass in the wwpn, but still require the target ID to
>> be compliant with things like event notification etc.
> 
> Don't we need the true N_Port-ID, then? That's what an fc_rport.port_id
> usually contains. It's also a simple way to lookup resources on a SAN
> switch for problem determination. Or did I misunderstand the
> content/semantics of the variable target_id, assuming it's a SCSI target
> ID, i.e. the 3rd part of a HCTL 4-tuple?
> 
Yes, that was the idea.
I've now modified the code so that we can pick up the port id for both,
target and host port. That should satisfy the needs here.

>> So I've shifted the target id onto the port ID (which is 24 bit anyway).
>> I could've used a bitfield here, but then I wasn't quite sure about the
>> endianness of which.
> 
 +    rport = fc_remote_port_add(sh, 0, _ids);
 +    if (rport) {
 +    tgt->rport = rport;
 +    rport->dd_data = tgt;
 +    fc_remote_port_rolechg(rport, FC_RPORT_ROLE_FCP_TARGET);
>>>
>>> Is the rolechg to get some event? Otherwise we could have
>>> rport_ids.roles = FC_RPORT_ROLE_FCP_TARGET before fc_remote_port_add().
>>>
>> That's 

Re: [PATCH 2/3] virtio-scsi: Add FC transport class

[Steffen Maier]

On 12/18/2017 09:31 AM, Hannes Reinecke wrote:

On 12/15/2017 07:08 PM, Steffen Maier wrote:

On 12/14/2017 11:11 AM, Hannes Reinecke wrote:

When a device announces an 'FC' protocol we should be pulling
in the FC transport class to have the rports etc setup correctly.


It took some time for me to understand what this does.
It seems to mirror the topology of rports and sdevs that exist under the
fc_host on the kvm host side to the virtio-scsi-fc side in the guest.

I like the idea. This is also what I've been suggesting users to do if
they back virtio-scsi with zfcp on the kvm host side. Primarily to not
stall all virtio-scsi I/O on all paths if the guest ever gets into
scsi_eh. But also to make it look like an HBA pass through so one can
more easily migrate to this once we have FCP pass through.


On second thought, I like the idea for virtio-scsi.

For the future virtio-(v)fc case, see below.


@@ -755,19 +823,34 @@ static int virtscsi_abort(struct scsi_cmnd *sc)



+    if (vscsi->protocol == SCSI_PROTOCOL_FCP) {
+    struct fc_rport *rport =
+    starget_to_rport(scsi_target(sc->device));
+    if (rport && rport->dd_data ) {
+    tgt = rport->dd_data;
+    target_id = tgt->target_id;
+    } else
+    return FAST_IO_FAIL;
+    } else {
+    tgt = scsi_target(sc->device)->hostdata;
+    if (!tgt || tgt->removed)
+    return FAST_IO_FAIL;
+    }


dito


@@ -857,27 +970,67 @@ static void virtscsi_rescan_work(struct
work_struct *work)

   wait_for_completion();


Waiting in work item .vs. having the response (IRQ) path trigger
subsequent processing async ?
Or do we need the call chain(s) getting here to be in our own process
context via the workqueue anyway?


Can't see I can parse this sentence, but I'll be looking at the code
trying to come up with a clever explanation :-)


Sorry, meanwhile I have a hard time understanding my own words, too.

I think I wondered if the effort of a work item is really necessary, 
especially considering that it does block on the completion and thus 
could delay other queued work items (even though Concurrency Managed 
Workqueues can often hide this delay).


Couldn't we just return asynchronously after having sent the request. 
And then later on, simply have the response (IRQ) path trigger whatever 
processing is necessary (after the work item variant woke up from the 
wait_for_completion) in some asynchronuous fashion? Of course, this 
could also be a work item which just does necessary remaining processing 
after we got a response.

Just a wild guess, without knowing the environmental requirements.


+    if (transport == SCSI_PROTOCOL_FCP) {
+    struct fc_rport_identifiers rport_ids;
+    struct fc_rport *rport;
+
+    rport_ids.node_name = wwn_to_u64(cmd->resp.rescan.node_wwn);
+    rport_ids.port_name = wwn_to_u64(cmd->resp.rescan.port_wwn);
+    rport_ids.port_id = (target_id >> 8);


Why do you shift target_id by one byte to the right?


Because with the original setup virtio_scsi guest would pass in the
target_id, and the host would be selecting the device based on that
information.
With virtio-vfc we pass in the wwpn, but still require the target ID to
be compliant with things like event notification etc.


Don't we need the true N_Port-ID, then? That's what an fc_rport.port_id 
usually contains. It's also a simple way to lookup resources on a SAN 
switch for problem determination. Or did I misunderstand the 
content/semantics of the variable target_id, assuming it's a SCSI target 
ID, i.e. the 3rd part of a HCTL 4-tuple?



So I've shifted the target id onto the port ID (which is 24 bit anyway).
I could've used a bitfield here, but then I wasn't quite sure about the
endianness of which.



+    rport = fc_remote_port_add(sh, 0, _ids);
+    if (rport) {
+    tgt->rport = rport;
+    rport->dd_data = tgt;
+    fc_remote_port_rolechg(rport, FC_RPORT_ROLE_FCP_TARGET);


Is the rolechg to get some event? Otherwise we could have
rport_ids.roles = FC_RPORT_ROLE_FCP_TARGET before fc_remote_port_add().


That's how the 'normal' transport classes do it; but I'll check if this
can be rolled into the call to fc_remote_port_add().


My idea was just based on how zfcp does it. Do you think I need to check 
if zfcp should do it via rolechg (even though zfcp never changes an 
rport role since it can only open targets)?



@@ -932,14 +1089,31 @@ static void virtscsi_scan_host(struct
virtio_scsi *vscsi)
   static void virtscsi_scan_start(struct Scsi_Host *sh)
   {



+    if (vscsi->protocol == SCSI_PROTOCOL_FCP) {
+    fc_host_node_name(sh) = vscsi->wwnn;
+    fc_host_port_name(sh) = vscsi->wwpn;
+    fc_host_port_id(sh) = 0x00ff00;
+    fc_host_port_type(sh) = FC_PORTTYPE_NPIV;


Why is this hardcoded?

At least with zfcp, we can have kvm host *v*HBAs without NPIV.


For the simple fact that I don't have fields to transfer this
information; plus 

Re: [PATCH 2/3] virtio-scsi: Add FC transport class

[Hannes Reinecke]

On 12/15/2017 07:08 PM, Steffen Maier wrote:
> 
> On 12/14/2017 11:11 AM, Hannes Reinecke wrote:
>> When a device announces an 'FC' protocol we should be pulling
>> in the FC transport class to have the rports etc setup correctly.
> 
> It took some time for me to understand what this does.
> It seems to mirror the topology of rports and sdevs that exist under the
> fc_host on the kvm host side to the virtio-scsi-fc side in the guest.
> 
> I like the idea. This is also what I've been suggesting users to do if
> they back virtio-scsi with zfcp on the kvm host side. Primarily to not
> stall all virtio-scsi I/O on all paths if the guest ever gets into
> scsi_eh. But also to make it look like an HBA pass through so one can
> more easily migrate to this once we have FCP pass through.
> 
Thanks for the review.

>>
>> Signed-off-by: Hannes Reinecke 
>> ---
>>   drivers/scsi/virtio_scsi.c | 323
>> ++---
>>   1 file changed, 277 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
>> index a561e90..f925fbd 100644
>> --- a/drivers/scsi/virtio_scsi.c
>> +++ b/drivers/scsi/virtio_scsi.c
> 
>> @@ -591,11 +616,25 @@ static int virtscsi_queuecommand_single(struct
>> Scsi_Host *sh,
>>   struct scsi_cmnd *sc)
> 
>> +    if (vscsi->protocol == SCSI_PROTOCOL_FCP) {
>> +    struct fc_rport *rport =
>> +    starget_to_rport(scsi_target(sc->device));
>> +    if (rport && rport->dd_data) {
>> +    tgt = rport->dd_data;
>> +    target_id = tgt->target_id;
>> +    }
>> +    } else
>> +    tgt = scsi_target(sc->device)->hostdata;
>> +    if (!tgt || tgt->removed) {
> 
>> @@ -648,16 +687,30 @@ static int virtscsi_queuecommand_multi(struct
>> Scsi_Host *sh,
>>  struct scsi_cmnd *sc)
> 
>> +    if (vscsi->protocol == SCSI_PROTOCOL_FCP) {
>> +    struct fc_rport *rport =
>> +    starget_to_rport(scsi_target(sc->device));
>> +    if (rport && rport->dd_data) {
>> +    tgt = rport->dd_data;
>> +    target_id = tgt->target_id;
>> +    }
>> +    } else
>> +    tgt = scsi_target(sc->device)->hostdata;
>> +    if (!tgt || tgt->removed) {
> 
> repeating pattern?
> 
Yeah, I could probably move that to a separate function.

>> @@ -696,12 +749,27 @@ static int virtscsi_device_reset(struct
>> scsi_cmnd *sc)
>>   {
>> +    if (vscsi->protocol == SCSI_PROTOCOL_FCP) {
>> +    struct fc_rport *rport =
>> +    starget_to_rport(scsi_target(sc->device));
>> +    if (rport && rport->dd_data ) {
>> +    tgt = rport->dd_data;
>> +    target_id = tgt->target_id;
>> +    } else
>> +    return FAST_IO_FAIL;
>> +    } else {
>> +    tgt = scsi_target(sc->device)->hostdata;
>> +    if (!tgt || tgt->removed)
> 
> The other patterns have the !tgt check outside of the if else condition.
> For consistency this might work here, too?
> 
Possibly. I'll check.

>> +    return FAST_IO_FAIL;
>> +    }
> 
> 
>> @@ -755,19 +823,34 @@ static int virtscsi_abort(struct scsi_cmnd *sc)
> 
>> +    if (vscsi->protocol == SCSI_PROTOCOL_FCP) {
>> +    struct fc_rport *rport =
>> +    starget_to_rport(scsi_target(sc->device));
>> +    if (rport && rport->dd_data ) {
>> +    tgt = rport->dd_data;
>> +    target_id = tgt->target_id;
>> +    } else
>> +    return FAST_IO_FAIL;
>> +    } else {
>> +    tgt = scsi_target(sc->device)->hostdata;
>> +    if (!tgt || tgt->removed)
>> +    return FAST_IO_FAIL;
>> +    }
> 
> dito
> 
>> @@ -857,27 +970,67 @@ static void virtscsi_rescan_work(struct
>> work_struct *work)
>>
>>   wait_for_completion();
> 
> Waiting in work item .vs. having the response (IRQ) path trigger
> subsequent processing async ?
> Or do we need the call chain(s) getting here to be in our own process
> context via the workqueue anyway?
> 
Can't see I can parse this sentence, but I'll be looking at the code
trying to come up with a clever explanation :-)

>>   target_id = virtio32_to_cpu(vscsi->vdev, cmd->resp.rescan.id);
>> -    if (target_id != -1) {
>> -    int transport = virtio32_to_cpu(vscsi->vdev,
>> -    cmd->resp.rescan.transport);
>> -    spin_lock_irq(>rescan_lock);
>> -    vscsi->next_target_id = target_id + 1;
>> -    spin_unlock_irq(>rescan_lock);
>> -    shost_printk(KERN_INFO, sh,
>> - "found %s target %d (WWN %*phN)\n",
>> - transport == SCSI_PROTOCOL_FCP ? "FC" : "SAS",
>> - target_id, 8,
>> - cmd->resp.rescan.port_wwn);
>> -    scsi_scan_target(>shost_gendev, 0, target_id,
>> - SCAN_WILD_CARD, SCSI_SCAN_INITIAL);
>> -    queue_work(system_freezable_wq, >rescan_work);
>> -    } else {
>> +    if (target_id == -1) {
> 
> This boolean if expression was introduced in the previous 

Re: [PATCH 2/3] virtio-scsi: Add FC transport class

[Steffen Maier]

On 12/14/2017 11:11 AM, Hannes Reinecke wrote:

When a device announces an 'FC' protocol we should be pulling
in the FC transport class to have the rports etc setup correctly.


It took some time for me to understand what this does.
It seems to mirror the topology of rports and sdevs that exist under the 
fc_host on the kvm host side to the virtio-scsi-fc side in the guest.


I like the idea. This is also what I've been suggesting users to do if 
they back virtio-scsi with zfcp on the kvm host side. Primarily to not 
stall all virtio-scsi I/O on all paths if the guest ever gets into 
scsi_eh. But also to make it look like an HBA pass through so one can 
more easily migrate to this once we have FCP pass through.




Signed-off-by: Hannes Reinecke 
---
  drivers/scsi/virtio_scsi.c | 323 ++---
  1 file changed, 277 insertions(+), 46 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index a561e90..f925fbd 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c



@@ -591,11 +616,25 @@ static int virtscsi_queuecommand_single(struct Scsi_Host 
*sh,
struct scsi_cmnd *sc)



+   if (vscsi->protocol == SCSI_PROTOCOL_FCP) {
+   struct fc_rport *rport =
+   starget_to_rport(scsi_target(sc->device));
+   if (rport && rport->dd_data) {
+   tgt = rport->dd_data;
+   target_id = tgt->target_id;
+   }
+   } else
+   tgt = scsi_target(sc->device)->hostdata;
+   if (!tgt || tgt->removed) {



@@ -648,16 +687,30 @@ static int virtscsi_queuecommand_multi(struct Scsi_Host 
*sh,
   struct scsi_cmnd *sc)



+   if (vscsi->protocol == SCSI_PROTOCOL_FCP) {
+   struct fc_rport *rport =
+   starget_to_rport(scsi_target(sc->device));
+   if (rport && rport->dd_data) {
+   tgt = rport->dd_data;
+   target_id = tgt->target_id;
+   }
+   } else
+   tgt = scsi_target(sc->device)->hostdata;
+   if (!tgt || tgt->removed) {


repeating pattern?


@@ -696,12 +749,27 @@ static int virtscsi_device_reset(struct scsi_cmnd *sc)
  {
+   if (vscsi->protocol == SCSI_PROTOCOL_FCP) {
+   struct fc_rport *rport =
+   starget_to_rport(scsi_target(sc->device));
+   if (rport && rport->dd_data ) {
+   tgt = rport->dd_data;
+   target_id = tgt->target_id;
+   } else
+   return FAST_IO_FAIL;
+   } else {
+   tgt = scsi_target(sc->device)->hostdata;
+   if (!tgt || tgt->removed)


The other patterns have the !tgt check outside of the if else condition.
For consistency this might work here, too?


+   return FAST_IO_FAIL;
+   }




@@ -755,19 +823,34 @@ static int virtscsi_abort(struct scsi_cmnd *sc)



+   if (vscsi->protocol == SCSI_PROTOCOL_FCP) {
+   struct fc_rport *rport =
+   starget_to_rport(scsi_target(sc->device));
+   if (rport && rport->dd_data ) {
+   tgt = rport->dd_data;
+   target_id = tgt->target_id;
+   } else
+   return FAST_IO_FAIL;
+   } else {
+   tgt = scsi_target(sc->device)->hostdata;
+   if (!tgt || tgt->removed)
+   return FAST_IO_FAIL;
+   }


dito


@@ -857,27 +970,67 @@ static void virtscsi_rescan_work(struct work_struct *work)

wait_for_completion();


Waiting in work item .vs. having the response (IRQ) path trigger 
subsequent processing async ?
Or do we need the call chain(s) getting here to be in our own process 
context via the workqueue anyway?



target_id = virtio32_to_cpu(vscsi->vdev, cmd->resp.rescan.id);
-   if (target_id != -1) {
-   int transport = virtio32_to_cpu(vscsi->vdev,
-   cmd->resp.rescan.transport);
-   spin_lock_irq(>rescan_lock);
-   vscsi->next_target_id = target_id + 1;
-   spin_unlock_irq(>rescan_lock);
-   shost_printk(KERN_INFO, sh,
-"found %s target %d (WWN %*phN)\n",
-transport == SCSI_PROTOCOL_FCP ? "FC" : "SAS",
-target_id, 8,
-cmd->resp.rescan.port_wwn);
-   scsi_scan_target(>shost_gendev, 0, target_id,
-SCAN_WILD_CARD, SCSI_SCAN_INITIAL);
-   queue_work(system_freezable_wq, >rescan_work);
-   } else {
+   if (target_id == -1) {


This boolean if expression was introduced in the previous patch.
Why negate it here?
It causes a number of potentially 

[PATCH 2/3] virtio-scsi: Add FC transport class

[Hannes Reinecke]

When a device announces an 'FC' protocol we should be pulling
in the FC transport class to have the rports etc setup correctly.

Signed-off-by: Hannes Reinecke 
---
 drivers/scsi/virtio_scsi.c | 323 ++---
 1 file changed, 277 insertions(+), 46 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index a561e90..f925fbd 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -25,11 +25,13 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -91,6 +93,12 @@ struct virtio_scsi_vq {
  * an atomic_t.
  */
 struct virtio_scsi_target_state {
+   struct list_head list;
+   struct fc_rport *rport;
+   struct virtio_scsi *vscsi;
+   int target_id;
+   bool removed;
+
seqcount_t tgt_seq;
 
/* Count of outstanding requests. */
@@ -117,8 +125,12 @@ struct virtio_scsi {
/* Protected by event_vq lock */
bool stop_events;
 
+   int protocol;
int next_target_id;
+   u64 wwnn;
+   u64 wwpn;
struct work_struct rescan_work;
+   struct list_head target_list;
spinlock_t rescan_lock;
 
struct virtio_scsi_vq ctrl_vq;
@@ -128,6 +140,7 @@ struct virtio_scsi {
 
 static struct kmem_cache *virtscsi_cmd_cache;
 static mempool_t *virtscsi_cmd_pool;
+static struct scsi_transport_template *virtio_transport_template;
 
 static inline struct Scsi_Host *virtio_scsi_host(struct virtio_device *vdev)
 {
@@ -156,15 +169,21 @@ static void virtscsi_compute_resid(struct scsi_cmnd *sc, 
u32 resid)
 static void virtscsi_complete_cmd(struct virtio_scsi *vscsi, void *buf)
 {
struct virtio_scsi_cmd *cmd = buf;
+   struct fc_rport *rport;
struct scsi_cmnd *sc = cmd->sc;
struct virtio_scsi_cmd_resp *resp = >resp.cmd;
-   struct virtio_scsi_target_state *tgt =
-   scsi_target(sc->device)->hostdata;
+   struct virtio_scsi_target_state *tgt;
 
dev_dbg(>device->sdev_gendev,
"cmd %p response %u status %#02x sense_len %u\n",
sc, resp->response, resp->status, resp->sense_len);
 
+   rport = starget_to_rport(scsi_target(sc->device));
+   if (!rport)
+   tgt = scsi_target(sc->device)->hostdata;
+   else
+   tgt = rport->dd_data;
+
sc->result = resp->status;
virtscsi_compute_resid(sc, virtio32_to_cpu(vscsi->vdev, resp->resid));
switch (resp->response) {
@@ -502,10 +521,11 @@ static int virtscsi_kick_cmd(struct virtio_scsi_vq *vq,
 
 static void virtio_scsi_init_hdr(struct virtio_device *vdev,
 struct virtio_scsi_cmd_req *cmd,
+int target_id,
 struct scsi_cmnd *sc)
 {
cmd->lun[0] = 1;
-   cmd->lun[1] = sc->device->id;
+   cmd->lun[1] = target_id;
cmd->lun[2] = (sc->device->lun >> 8) | 0x40;
cmd->lun[3] = sc->device->lun & 0xff;
cmd->tag = cpu_to_virtio64(vdev, (unsigned long)sc);
@@ -517,12 +537,14 @@ static void virtio_scsi_init_hdr(struct virtio_device 
*vdev,
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 static void virtio_scsi_init_hdr_pi(struct virtio_device *vdev,
struct virtio_scsi_cmd_req_pi *cmd_pi,
+   int target_id,
struct scsi_cmnd *sc)
 {
struct request *rq = sc->request;
struct blk_integrity *bi;
 
-   virtio_scsi_init_hdr(vdev, (struct virtio_scsi_cmd_req *)cmd_pi, sc);
+   virtio_scsi_init_hdr(vdev, (struct virtio_scsi_cmd_req *)cmd_pi,
+target_id, sc);
 
if (!rq || !scsi_prot_sg_count(sc))
return;
@@ -542,6 +564,7 @@ static void virtio_scsi_init_hdr_pi(struct virtio_device 
*vdev,
 
 static int virtscsi_queuecommand(struct virtio_scsi *vscsi,
 struct virtio_scsi_vq *req_vq,
+int target_id,
 struct scsi_cmnd *sc)
 {
struct Scsi_Host *shost = virtio_scsi_host(vscsi->vdev);
@@ -564,13 +587,15 @@ static int virtscsi_queuecommand(struct virtio_scsi 
*vscsi,
 
 #ifdef CONFIG_BLK_DEV_INTEGRITY
if (virtio_has_feature(vscsi->vdev, VIRTIO_SCSI_F_T10_PI)) {
-   virtio_scsi_init_hdr_pi(vscsi->vdev, >req.cmd_pi, sc);
+   virtio_scsi_init_hdr_pi(vscsi->vdev, >req.cmd_pi,
+   target_id, sc);
memcpy(cmd->req.cmd_pi.cdb, sc->cmnd, sc->cmd_len);
req_size = sizeof(cmd->req.cmd_pi);
} else
 #endif
{
-   virtio_scsi_init_hdr(vscsi->vdev, >req.cmd, sc);
+   virtio_scsi_init_hdr(vscsi->vdev, >req.cmd,
+target_id, sc);