[PATCH 1/1] libfc: fix queue command rport checks

[PATCH 1/1] libfc: fix queue command rport checks

[michaelc-hcNo3dDEHLuVc3sceRu5cw]

From: Mike Christie <michaelc-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org>

The rport->dd_data should be set by the time we get to the
queuecommand, but its data could be zerod out. This adds a check
like other drivers for this.

We also want to check fc_remote_port_chkready before the driver
bits, because the class could want to do something else.

Also some other review comments for this code path:

1. Do we need locking aruond the lp->state and status checks in
the queuecommand function?
2. Why do we fail the command with DID_NO_CONNECT if
FC_PAUSE is not supported. It seems like we want to retry
and then allow the class to fail or retry IO.
3. Same for the link_status DID_NO_CONNECT usage.
4. fc_lport_destroy, fc_fcp_destroy ordering in fcoe_destroy_interface does
not seem right. It looks like if we need to send a sync cache to the device
when it is removed it will always fail.

This patch was made over the last ones
http://www.open-fcoe.org/pipermail/devel/2008-July/000365.html
http://www.open-fcoe.org/pipermail/devel/2008-July/000368.html
http://www.open-fcoe.org/pipermail/devel/2008-July/000369.html

Signed-off-by: Mike Christie <michaelc-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org>
---
 drivers/scsi/libfc/fc_fcp.c |   28 +++++++++++++++-------------
 1 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/libfc/fc_fcp.c b/drivers/scsi/libfc/fc_fcp.c
index ea7c6bc..c2bbfa7 100644
--- a/drivers/scsi/libfc/fc_fcp.c
+++ b/drivers/scsi/libfc/fc_fcp.c
@@ -1649,6 +1649,21 @@ int fc_queuecommand(struct scsi_cmnd *sc_cmd, void (*done)(struct scsi_cmnd *))
 
 	lp = shost_priv(sc_cmd->device->host);
 
+	rval = fc_remote_port_chkready(rport);
+	if (rval) {
+		sc_cmd->result = rval;
+		done(sc_cmd);
+		goto out;
+	}
+
+	if (!*(struct fc_remote_port **)rport->dd_data) {
+		/* rport is transitioning */
+		sc_cmd->result = DID_IMM_RETRY << 16;
+		done(sc_cmd);
+		goto out;
+	}
+	rp = rport->dd_data;
+
 	if (lp->state != LPORT_ST_READY) {
 		if (lp->link_status & FC_PAUSE) {
 			rc = SCSI_MLQUEUE_HOST_BUSY;
@@ -1666,19 +1681,6 @@ int fc_queuecommand(struct scsi_cmnd *sc_cmd, void (*done)(struct scsi_cmnd *))
 		}
 	}
 
-	rval = fc_remote_port_chkready(rport);
-	if (rval) {
-		sc_cmd->result = rval;
-		done(sc_cmd);
-		goto out;
-	}
-	rp = rport->dd_data;
-	if (!rp) {
-		sc_cmd->result = DID_NO_CONNECT << 16;
-		done(sc_cmd);
-		goto out;
-	}
-
 	if (!(rp->rp_fcp_parm & FCP_SPPF_TARG_FCN)) {
 		sc_cmd->result = DID_NO_CONNECT << 16;
 		done(sc_cmd);
-- 
1.5.4.1

Re: [PATCH 1/1] libfc: fix queue command rport checks

[Mike Christie]

michaelc-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org wrote:
> 4. fc_lport_destroy, fc_fcp_destroy ordering in fcoe_destroy_interface does
> not seem right. It looks like if we need to send a sync cache to the device
> when it is removed it will always fail.
>

Oh yeah, I ccd linux-scsi for this patch and not others, because JamesS 
and them are not on fcoe devel list, and it seems like all fc drivers 
fail the sync cache command.

What happens is that we remove the rport which sets the rport state, 
then the fc class removes the target. This causes the target and its 
devices to be removed, which can cause a sync cache to be sent. But 
because the the rport is not in the online state it will fail the command.

RE: [PATCH 1/1] libfc: fix queue command rport checks

[James.Smart]

 

> michaelc@cs.wisc.edu wrote:
> Oh yeah, I ccd linux-scsi for this patch and not others, 
> because JamesS 
> and them are not on fcoe devel list, and it seems like all fc drivers 
> fail the sync cache command.
> 
> What happens is that we remove the rport which sets the rport state, 
> then the fc class removes the target. This causes the target and its 
> devices to be removed, which can cause a sync cache to be sent. But 
> because the the rport is not in the online state it will fail 
> the command.

Well, excepting detachment paths, if we're tearing the rport down, it's
because
we've already lost connectivity to the target, so there's no way a sync
cache would ever succeed. It's a suprise-removal scenario.

-- james

Re: [PATCH 1/1] libfc: fix queue command rport checks

[Mike Christie]

James.Smart-iH1Dq9VlAzfQT0dZR+AlfA@public.gmane.org wrote:
>  
> 
>> michaelc-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org wrote:
>> Oh yeah, I ccd linux-scsi for this patch and not others, 
>> because JamesS 
>> and them are not on fcoe devel list, and it seems like all fc drivers 
>> fail the sync cache command.
>>
>> What happens is that we remove the rport which sets the rport state, 
>> then the fc class removes the target. This causes the target and its 
>> devices to be removed, which can cause a sync cache to be sent. But 
>> because the the rport is not in the online state it will fail 
>> the command.
> 
> Well, excepting detachment paths, if we're tearing the rport down, it's
> because
> we've already lost connectivity to the target, so there's no way a sync
> cache would ever succeed. It's a suprise-removal scenario.
> 

Yeah, I am only talking about the paths where we want a clean shutdown 
like if for fcoe we did echo ethX > /sys/module/fcoe/destroy. For normal 
fc do we want to be able to remove rpots too? Did you guys do that with 
the ioctl modules before? Then there is the module removal case. I think 
for module removal we want a clean shutdown too, but I am not sure if 
other people feel that for module removal there are gaurantees like that.

So during a clean shutdown are drivers supposed to remove the targets by 
calling scsi_remove_target to remove the devices, then remove the rports 
through the class?

Re: [Open-FCoE] [PATCH 1/1] libfc: fix queue command rport checks

[Mike Christie]

Mike Christie wrote:
> James.Smart@Emulex.Com wrote:
>>  
>>
>>> michaelc@cs.wisc.edu wrote:
>>> Oh yeah, I ccd linux-scsi for this patch and not others, 
>>> because JamesS 
>>> and them are not on fcoe devel list, and it seems like all fc drivers 
>>> fail the sync cache command.
>>>
>>> What happens is that we remove the rport which sets the rport state, 
>>> then the fc class removes the target. This causes the target and its 
>>> devices to be removed, which can cause a sync cache to be sent. But 
>>> because the the rport is not in the online state it will fail 
>>> the command.
>> Well, excepting detachment paths, if we're tearing the rport down, it's
>> because
>> we've already lost connectivity to the target, so there's no way a sync
>> cache would ever succeed. It's a suprise-removal scenario.
>>
> 
> Yeah, I am only talking about the paths where we want a clean shutdown 
> like if for fcoe we did echo ethX > /sys/module/fcoe/destroy. For normal 
> fc do we want to be able to remove rpots too? Did you guys do that with 
> the ioctl modules before? Then there is the module removal case. I think 
> for module removal we want a clean shutdown too, but I am not sure if 
> other people feel that for module removal there are gaurantees like that.
> 
> So during a clean shutdown are drivers supposed to remove the targets by 
> calling scsi_remove_target to remove the devices, then remove the rports 
> through the class?

Or for the case where we are stopping a host (rmmod or single host stop 
like with fcoe), should drivers call

1. fc_remove_host()
	This could be modified to cleanup shutdown targets then remove rports. 
We could then have a rport shutdown callback which the class could call 
and drivers could cleanup and shutdown the rport here before it is freed.

2. scsi_remove_host()

3. cleanup internal host resources.

4. scsi_put_host().

Re: [Open-FCoE] [PATCH 1/1] libfc: fix queue command rport checks

[Mike Christie]

Mike Christie wrote:
> Mike Christie wrote:
>> James.Smart@Emulex.Com wrote:
>>>  
>>>
>>>> michaelc@cs.wisc.edu wrote:
>>>> Oh yeah, I ccd linux-scsi for this patch and not others, because 
>>>> JamesS and them are not on fcoe devel list, and it seems like all fc 
>>>> drivers fail the sync cache command.
>>>>
>>>> What happens is that we remove the rport which sets the rport state, 
>>>> then the fc class removes the target. This causes the target and its 
>>>> devices to be removed, which can cause a sync cache to be sent. But 
>>>> because the the rport is not in the online state it will fail the 
>>>> command.
>>> Well, excepting detachment paths, if we're tearing the rport down, it's
>>> because
>>> we've already lost connectivity to the target, so there's no way a sync
>>> cache would ever succeed. It's a suprise-removal scenario.
>>>
>>
>> Yeah, I am only talking about the paths where we want a clean shutdown 
>> like if for fcoe we did echo ethX > /sys/module/fcoe/destroy. For 
>> normal fc do we want to be able to remove rpots too? Did you guys do 
>> that with the ioctl modules before? Then there is the module removal 
>> case. I think for module removal we want a clean shutdown too, but I 
>> am not sure if other people feel that for module removal there are 
>> gaurantees like that.
>>
>> So during a clean shutdown are drivers supposed to remove the targets 
>> by calling scsi_remove_target to remove the devices, then remove the 
>> rports through the class?
> 
> Or for the case where we are stopping a host (rmmod or single host stop 
> like with fcoe), should drivers call
> 
> 1. fc_remove_host()
>     This could be modified to cleanup shutdown targets then remove 
> rports. We could then have a rport shutdown callback which the class 
> could call and drivers could cleanup and shutdown the rport here before

I guess we have the terminate_rport_io callback already. So we could do:


fc_remove_host()
for each target
	scsi_remove_target();
for each rport
	fc_queue_work(shost, &rport->rport_delete_work);

.....


fc_rport_final_delete()
	i->f->terminate_rport_io

.......

then in the fcoe termniate_rport_io function we could stop the port?

Re: [PATCH 1/1] libfc: fix queue command rport checks

[James.Smart-iH1Dq9VlAzfQT0dZR+AlfA]

 

> -----Original Message-----
> From: Mike Christie [mailto:michaelc-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org] 
> 
> > So during a clean shutdown are drivers supposed to remove 
> the targets by 
> > calling scsi_remove_target to remove the devices, then 
> remove the rports 
> > through the class?
> 
> Or for the case where we are stopping a host (rmmod or single 
> host stop 
> like with fcoe), should drivers call

Yes - the steps below is what the drivers do today.  And this does all
work without failing the cache sync (unless something's changed recently
in the midlayer above us).

> 
> 1. fc_remove_host()
> 	This could be modified to cleanup shutdown targets then 
> remove rports. 
> We could then have a rport shutdown callback which the class 
> could call 
> and drivers could cleanup and shutdown the rport here before 
> it is freed.

Is this related to your new scsi-target block code ?
Yes - agree with your comment - we can change it so that it cleansup
the blocked target, then terminates the rport.
> 
> 2. scsi_remove_host()
> 
> 3. cleanup internal host resources.
> 
> 4. scsi_put_host().
> 


-- james

Re: [Open-FCoE] [PATCH 1/1] libfc: fix queue command rport checks

[Mike Christie]

James.Smart@Emulex.Com wrote:
>  
> 
>> -----Original Message-----
>> From: Mike Christie [mailto:michaelc@cs.wisc.edu] 
>>
>>> So during a clean shutdown are drivers supposed to remove 
>> the targets by 
>>> calling scsi_remove_target to remove the devices, then 
>> remove the rports 
>>> through the class?
>> Or for the case where we are stopping a host (rmmod or single 
>> host stop 
>> like with fcoe), should drivers call
> 
> Yes - the steps below is what the drivers do today.  And this does all
> work without failing the cache sync (unless something's changed recently
> in the midlayer above us).

You mean if I do rmmod lpfc it should work today? I do not think it 
works anymore because in fc_remove_host we do this:

fc_remove_host()

         /* Remove any remote ports */
         list_for_each_entry_safe(rport, next_rport,
                         &fc_host->rports, peers) {
                 list_del(&rport->peers);
                 rport->port_state = FC_PORTSTATE_DELETED;
                 fc_queue_work(shost, &rport->rport_delete_work);
         }


We set the rport->port_state to deleted before removing the target, so 
when the cache sync is sent later as a result of fc_rport_final_delete 
calling scsi_remove_target, the fc_remote_port_chkready checks in lpfc 
or qla2xxx or mpt's queeucommand will fail the command with 
DID_NO_CONNECT (fc_remote_port_chkready will

> 
>> 1. fc_remove_host()
>> 	This could be modified to cleanup shutdown targets then 
>> remove rports. 
>> We could then have a rport shutdown callback which the class 
>> could call 
>> and drivers could cleanup and shutdown the rport here before 
>> it is freed.
> 
> Is this related to your new scsi-target block code ?

No. It is unrelated. The block code patch is just to handle the case 
where when fc_timeout_fail_rport_io calls terminate_rport_io, and we 
will fail IO in the driver, but if there is IO in the blocked request 
queues we will just queue that back up until dev loss tmo fires. The 
problem there is because the fc_remote_port_chkready calls in the LLDs 
queuecommand will see the rport is blocked with a dev loss pending they 
will return DID_IMM_RETRY.

So if you have fast io fail tmo at 3 seconds and dev loss tmo at 120 
seconds, we could get initial IO errors after 3 seconds, but IO in the 
blocked queue will not be sent upwards until 120 seconds later. With the 
block patches all the IO will be sent upwards after 3 seconds.


> Yes - agree with your comment - we can change it so that it cleansup
> the blocked target, then terminates the rport.
>> 2. scsi_remove_host()
>>
>> 3. cleanup internal host resources.
>>
>> 4. scsi_put_host().
>>
> 
> 
> -- james
> 
> 
> 

Re: [PATCH 1/1] libfc: fix queue command rport checks

[Mike Christie]

Mike Christie wrote:
> James.Smart-iH1Dq9VlAzfQT0dZR+AlfA@public.gmane.org wrote:
>>  
>>
>>> -----Original Message-----
>>> From: Mike Christie [mailto:michaelc-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org] 
>>>
>>>> So during a clean shutdown are drivers supposed to remove 
>>> the targets by 
>>>> calling scsi_remove_target to remove the devices, then 
>>> remove the rports 
>>>> through the class?
>>> Or for the case where we are stopping a host (rmmod or single 
>>> host stop 
>>> like with fcoe), should drivers call
>> Yes - the steps below is what the drivers do today.  And this does all
>> work without failing the cache sync (unless something's changed recently
>> in the midlayer above us).
> 
> You mean if I do rmmod lpfc it should work today? I do not think it 
> works anymore because in fc_remove_host we do this:
> 
> fc_remove_host()
> 
>          /* Remove any remote ports */
>          list_for_each_entry_safe(rport, next_rport,
>                          &fc_host->rports, peers) {
>                  list_del(&rport->peers);
>                  rport->port_state = FC_PORTSTATE_DELETED;
>                  fc_queue_work(shost, &rport->rport_delete_work);
>          }
> 
> 
> We set the rport->port_state to deleted before removing the target, so 
> when the cache sync is sent later as a result of fc_rport_final_delete 
> calling scsi_remove_target, the fc_remote_port_chkready checks in lpfc 
> or qla2xxx or mpt's queeucommand will fail the command with 
> DID_NO_CONNECT (fc_remote_port_chkready will
>

oops that got cut off. I mean to write fc_remote_port_chkready will do this:

fc_remote_port_chkready(struct fc_rport *rport)
{
         int result;

         switch (rport->port_state) {
         case FC_PORTSTATE_ONLINE:
                 if (rport->roles & FC_PORT_ROLE_FCP_TARGET)
                         result = 0;
                 else if (rport->flags & FC_RPORT_DEVLOSS_PENDING)
                         result = DID_IMM_RETRY << 16;
                 else
                         result = DID_NO_CONNECT << 16;
                 break;
         case FC_PORTSTATE_BLOCKED:
                 result = DID_IMM_RETRY << 16;
                 break;
         default:
                 result = DID_NO_CONNECT << 16;
                 break;


So because port_state == FC_PORTSTATE_DELETED we reutrn DID_NO_CONNECT 
on any IO like the cache sync that gets sent.

Re: [PATCH 1/1] libfc: fix queue command rport checks

[James.Smart-iH1Dq9VlAzfQT0dZR+AlfA]

Yeah, I realize the scenario. What's confusing is, it's has been a
while since I've seen any cache failure message get spit out. But your
logic is pretty solid.

Anyway, your idea of converting fc_remove_host() to call
scsi_remove_target()
for all the targets prior to deleting the rports is a good one.  And the
terminate_rport_io call is already in fc_rport_final_delete().

This doesn't make sense to me though:
> then in the fcoe termniate_rport_io function we could stop the port?
If "port" means the fcoe port, and if this was the last rport, then I
guess
it kinda makes sense. But I would have assumed that the outer thread
that is
calling fc_remove_host() and scsi_remove_host() would have actually have
been the one to stop/terminate the fcoe port after making these calls.

-- james s

> Mike Christie wrote:
> I guess we have the terminate_rport_io callback already. So 
> we could do:
> 
> 
> fc_remove_host()
> for each target
> 	scsi_remove_target();
> for each rport
> 	fc_queue_work(shost, &rport->rport_delete_work);
> 
> .....
> 
> 
> fc_rport_final_delete()
> 	i->f->terminate_rport_io
> 
> .......
> 
> then in the fcoe termniate_rport_io function we could stop the port?
> 

Re: [Open-FCoE] [PATCH 1/1] libfc: fix queue command rport checks

[Mike Christie]

James.Smart@Emulex.Com wrote:
> Yeah, I realize the scenario. What's confusing is, it's has been a
> while since I've seen any cache failure message get spit out. But your
> logic is pretty solid.
> 
> Anyway, your idea of converting fc_remove_host() to call
> scsi_remove_target()
> for all the targets prior to deleting the rports is a good one.  And the
> terminate_rport_io call is already in fc_rport_final_delete().
> 
> This doesn't make sense to me though:
>> then in the fcoe termniate_rport_io function we could stop the port?
> If "port" means the fcoe port, and if this was the last rport, then I
> guess
> it kinda makes sense. But I would have assumed that the outer thread
> that is
> calling fc_remove_host() and scsi_remove_host() would have actually have
> been the one to stop/terminate the fcoe port after making these calls.
> 

The problem I was thinking about was if we had a rport that we were 
logged into and needed to cleanup some resources for it before it got 
freed. If we do the cleanup after calling 
fc_remove_host()/scsi_remove_host(), the LLD would need to get a ref to 
it, so that when fc_remove_host() deletes it, the rport and the dd_data 
would not get freed.

So I thought it if we could do the cleanup in some callout after the 
target is removed but before the rport is freed. I think I meant to do 
this in the dev_loss_tmo_callbk(). qla2xxx was allocating its own rport 
struct and is freeing it in dev_loss_tmo_callbk(). For fcoe we are just 
using the scsi_transport_fc rport it the dd_data, but I was thinking if 
we needed to cleanup some other rport resrouces we could do it from the 
dev_loss_tmo_callbk().

Re: [PATCH 1/1] libfc: fix queue command rport checks

[James.Smart-iH1Dq9VlAzfQT0dZR+AlfA]

 

Mike Christie wrote:
> So I thought it if we could do the cleanup in some callout after the 
> target is removed but before the rport is freed. I think I 
> meant to do 
> this in the dev_loss_tmo_callbk(). qla2xxx was allocating its 
> own rport 
> struct and is freeing it in dev_loss_tmo_callbk(). For fcoe 
> we are just 
> using the scsi_transport_fc rport it the dd_data, but I was 
> thinking if 
> we needed to cleanup some other rport resrouces we could do 
> it from the 
> dev_loss_tmo_callbk().

yep - the callback is when the rport struct is really done with, so
this seems right.

-- james s