fc_remote_port_delete and returning SCSI commands from LLD

fc_remote_port_delete and returning SCSI commands from LLD

[Christof Schmitt]

I am looking again at how and when a FC LLD should call
fc_remote_port_delete. Some help would be welcome to cover all
requirements and to plug the holes...

One scenario i am looking at: The connection to the HBA has been
temporarily lost and the LLD has to return all pending I/O requests to
the upper layers, so they can be retried later. Now with the SCSI
device being part of a multipath device, the first failed I/O request
triggers path failover:

multipath_end_io
do_end_io
fail_path
queue_work(kmultipathd, &pgpath->deactivate_path);

which then marks the following returned requests as timed out:

deactivate_path
blk_abort_queue
blk_abort_request
blk_rq_timed_out
scsi_times_out
fc_timed_out

If the remote_port status is not BLOCKED, this will trigger the SCSI
midlayer error handling which cannot do much during the interruption
to the hardware and will mark the SCSI devices 'offline'. In order to
prevent this, the rule would be: First call fc_remote_port_delete to
set the remote port (or in the case of an HBA interruption all remote
ports) to BLOCKED, and only after this step call scsi_done to pass the
SCSI commands back to the upper layers.

This means, if the HBA problem is detected in interrupt context,
fc_remote_port_delete has to be called before calling scsi_done.

But the description for fc_remote_port_delete states:

 * Called from normal process context only - cannot be called from
 * interrupt.
 *
 * Notes:
 *	This routine assumes no locks are held on entry.
 */

Looking at the functions called from fc_remote_port_delete, i don't
see a problem in calling fc_remote_port_delete from interrupt context
or with locks held. Does this mean the description should be fixed or
am i missing something?

fc_remote_port_add on the other hand can wait during flushes and has
to be called from process context. To summarize:
- A LLD has to call fc_remote_port_delete before returning SCSI
  commands from a failed port or failed HBA.
- fc_remote_port_delete can be called from interrupt context before
  calling scsi_done if necessary
- fc_remote_port_add has to be called from process context
- The LLD has to serialize the fc_remote_port_add and
  fc_remote_port_delete calls to guarantee the add->delete->...
  sequence.

--
Christof

Re: fc_remote_port_delete and returning SCSI commands from LLD

[Christof Schmitt]

On Tue, Oct 20, 2009 at 04:40:27PM +0200, Christof Schmitt wrote:
> If the remote_port status is not BLOCKED, this will trigger the SCSI
> midlayer error handling which cannot do much during the interruption
> to the hardware and will mark the SCSI devices 'offline'. In order to
> prevent this, the rule would be: First call fc_remote_port_delete to
> set the remote port (or in the case of an HBA interruption all remote
> ports) to BLOCKED, and only after this step call scsi_done to pass the
> SCSI commands back to the upper layers.

I just stumbled across a loop that blocks the SCSI error handling
thread:

	spin_lock_irqsave(shost->host_lock, flags);
	while (rport->port_state == FC_PORTSTATE_BLOCKED) {
		spin_unlock_irqrestore(shost->host_lock, flags);
		msleep(1000);
		spin_lock_irqsave(shost->host_lock, flags);
	}
	spin_unlock_irqrestore(shost->host_lock, flags);

This seems to be popular among FC drivers. Is this the preferred way
to synchronize the FC transport class state changes with the SCSI
midlayer error recovery?

Christof

Re: fc_remote_port_delete and returning SCSI commands from LLD

[James Smart]

Christof Schmitt wrote:
> I am looking again at how and when a FC LLD should call
> fc_remote_port_delete. Some help would be welcome to cover all
> requirements and to plug the holes...

It's pretty simple, as far as the FC transport is concerned.
Call fc_remote_port_add() once connectivity is established.
Call fc_remote_port_delete() once connectivity is lost.
It is expected that there is a clear add->delete->add->delete->... sequence.

Timing is considered "immediate", but there's always a window of delay.

In general, the Transport ignores what happens to outstanding i/o, letting the 
LLDD do something based on its policy, or let natural i/o timers or the fast 
fail timer to fire.  The transport, at the delete call, will start the fast 
fail timer if it is set.


> One scenario i am looking at: The connection to the HBA has been
> temporarily lost and the LLD has to return all pending I/O requests to
> the upper layers, so they can be retried later. Now with the SCSI
> device being part of a multipath device, the first failed I/O request
> triggers path failover:

You are now asking a different question - how to make the upper layers play
nice with the different responses from queuecommand, the LLDD's interaction 
with the transport and midlayer, etc.

> 
> multipath_end_io
> do_end_io
> fail_path
> queue_work(kmultipathd, &pgpath->deactivate_path);
> 
> which then marks the following returned requests as timed out:
> 
> deactivate_path
> blk_abort_queue
> blk_abort_request
> blk_rq_timed_out
> scsi_times_out
> fc_timed_out
> 
> If the remote_port status is not BLOCKED, this will trigger the SCSI
> midlayer error handling which cannot do much during the interruption
> to the hardware and will mark the SCSI devices 'offline'.

Well - this isn't absolute, but is pretty much true. We expect, when 
connectivity is lost, for the block state to be temporarily entered. The 
blocked state holds off further i/o and the eh handler as well, to postpone 
the normal i/o failure cases which do lead to offline conditions in most 
scenarios.

But - this process is a coordinated effort between the driver and the upper 
layers, and where the driver doesn't get helped by the transport (the blocked 
state) it had better mimic the return codes at the different points, and 
perhaps more, so that bad things don't happen.


> In order to
> prevent this, the rule would be: First call fc_remote_port_delete to
> set the remote port (or in the case of an HBA interruption all remote
> ports) to BLOCKED, and only after this step call scsi_done to pass the
> SCSI commands back to the upper layers.

True, although as mentioned, i/o termination is considered independent from 
the rport/transport. But, you're best off if the target is blocked due to the 
rport delete as we've prepped the upper layers to behave best with this behavior.

There will always be a few i/o's that sneak in or complete (timeout ?) in 
between when the LLDD detects connectivity loss and when the 
fc_remote_port_delete has been called. It's up to the LLDD to handle this window.

Completions, including i/o timeouts, are typically not a big deal and should 
just return via scsi_done as they normally would. The caveat is when those 
i/o's are from the eh thread.  Granted - if you are actively aborting/failing 
i/o at the connectivity loss, and doing so before the block is in place, 
you're causing more headaches for yourself in getting the upper layers to play 
right with the LLDD - with the recommendation being "don't do that".

New i/o needs to be caught in queuecommand with the LLDD emulating the 
transport status that would normally get returned.  E.g. the call to 
fc_remote_port_chkready() won't catch it as the fc_remote_port_delete() call 
hasn't completed yet - so the LLDD needs a 2nd check against it's own 
structures, and if it detects the state, it should fail the i/o with the same 
codes that chkready would. In reality, if you wanted to accept the command, 
but never issue it and just leave it outstanding - waiting for i/o timeout, or 
fast fail i/o timout, or devloss_tmo, I guess you could.


> This means, if the HBA problem is detected in interrupt context,
> fc_remote_port_delete has to be called before calling scsi_done.

Well - execution context is somewhat unrelated, as it depends on how the LLDD 
is implemented, and what else its doing when connectivity is lost.

> 
> But the description for fc_remote_port_delete states:
> 
>  * Called from normal process context only - cannot be called from
>  * interrupt.
>  *
>  * Notes:
>  *	This routine assumes no locks are held on entry.
>  */
> 
> Looking at the functions called from fc_remote_port_delete, i don't
> see a problem in calling fc_remote_port_delete from interrupt context
> or with locks held. Does this mean the description should be fixed or
> am i missing something?

That's probably true. underlying routines have changed a bit over time and it 
may be better now. I'd still hesitate with fc_tgt_it_nexus_destroy() (although 
it shouldn't be applicable to you), and scsi_target_block().  Creating 
additional lock hierachies between LLDD locks and the locks in these paths 
(which the LLDD rarely sees/knows about) isn't good.   Thus, we've mostly 
pushed LLDDs to use a pristine context when calling the transport (such as a 
workq context) so that we can disassociate low-level LLDD design from midlayer 
design.


> fc_remote_port_add on the other hand can wait during flushes and has
> to be called from process context. To summarize:
> - A LLD has to call fc_remote_port_delete before returning SCSI
>   commands from a failed port or failed HBA.

not true, but best behavior.

> - fc_remote_port_delete can be called from interrupt context before
>   calling scsi_done if necessary

part a (called from interrupt context) - do so at your own risk.  These other 
paths can change at any time and its not fair for those developers to know 
your driver dependencies.

part b (before calling scsi_done) - recommended approach.

> - fc_remote_port_add has to be called from process context

True.

> - The LLD has to serialize the fc_remote_port_add and
>   fc_remote_port_delete calls to guarantee the add->delete->...
>   sequence.

True.

-- james s


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

Re: fc_remote_port_delete and returning SCSI commands from LLD

[James Smart]

Here's what I remember about this from the past:

- This was originally added when dealing with older kernels that didn't have 
the eh patch that bounced the timeout handler when the rport was blocked (see 
fc_timed_out).

   The eh patch avoided entering the eh thread upon i/o timeouts if the rport 
was blocked.


- As mentioned in my prior email - there's a window where things can be 
entered before the target blocked state protects you. What if you are in the 
eh_handler when it occurs ?  Unfortunately, the eh thread is very black and 
white on abort/reset/io status - its either success or not. It doesn't 
validate the "not" cases, never looks at retry conditions, and just assumes 
hard failure - which was taking everyone down bad paths.  This is a rats nest 
to resolve right, and I think I mentioned it on the list a long time ago with 
Christoph. Thus the stall was added to plug the hole.

-- james s


Christof Schmitt wrote:
> On Tue, Oct 20, 2009 at 04:40:27PM +0200, Christof Schmitt wrote:
>> If the remote_port status is not BLOCKED, this will trigger the SCSI
>> midlayer error handling which cannot do much during the interruption
>> to the hardware and will mark the SCSI devices 'offline'. In order to
>> prevent this, the rule would be: First call fc_remote_port_delete to
>> set the remote port (or in the case of an HBA interruption all remote
>> ports) to BLOCKED, and only after this step call scsi_done to pass the
>> SCSI commands back to the upper layers.
> 
> I just stumbled across a loop that blocks the SCSI error handling
> thread:
> 
> 	spin_lock_irqsave(shost->host_lock, flags);
> 	while (rport->port_state == FC_PORTSTATE_BLOCKED) {
> 		spin_unlock_irqrestore(shost->host_lock, flags);
> 		msleep(1000);
> 		spin_lock_irqsave(shost->host_lock, flags);
> 	}
> 	spin_unlock_irqrestore(shost->host_lock, flags);
> 
> This seems to be popular among FC drivers. Is this the preferred way
> to synchronize the FC transport class state changes with the SCSI
> midlayer error recovery?
> 
> Christof
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: fc_remote_port_delete and returning SCSI commands from LLD

[Mike Christie]

Christof Schmitt wrote:
> I am looking again at how and when a FC LLD should call
> fc_remote_port_delete. Some help would be welcome to cover all
> requirements and to plug the holes...
> 
> One scenario i am looking at: The connection to the HBA has been
> temporarily lost and the LLD has to return all pending I/O requests to
> the upper layers, so they can be retried later. Now with the SCSI
> device being part of a multipath device, the first failed I/O request
> triggers path failover:
> 
> multipath_end_io
> do_end_io
> fail_path
> queue_work(kmultipathd, &pgpath->deactivate_path);
> 
> which then marks the following returned requests as timed out:
> 
> deactivate_path
> blk_abort_queue
> blk_abort_request
> blk_rq_timed_out
> scsi_times_out
> fc_timed_out
> 
> If the remote_port status is not BLOCKED, this will trigger the SCSI
> midlayer error handling which cannot do much during the interruption
> to the hardware and will mark the SCSI devices 'offline'. In order to
> prevent this, the rule would be: First call fc_remote_port_delete to
> set the remote port (or in the case of an HBA interruption all remote
> ports) to BLOCKED, and only after this step call scsi_done to pass the
> SCSI commands back to the upper layers.
> 

One other note when doing this.

For problems where you are deleting the rport, it is best to use 
something like DID_TRANSPORT_DISRUPTED to fail the cmd if you are 
failing it right away. If drivers block the rport, then fail commands 
immediately with DID_TRANSPORT_DISRUPTED, then they will not actually be 
failed to the block/mpath layer until the fast io fail timeout has 
fired. This will prevent very short problems from firing the mutlipath 
path offlining code.

If your driver deletes the rport and does not fail the cmd immediately 
so it can recover within the command or some other reason like the fw 
just works that way, then when the fast io fail timer fires and the 
terminate_rport_io callback is run you could actually use any error code 
since at this time when a IO is sent to the queuecommand the driver will 
call fc_remote_port_chkready and IO will be failed immediately with 
DID_TRANSPORT_FAILFAST).

Re: fc_remote_port_delete and returning SCSI commands from LLD

[Christof Schmitt]

On Wed, Oct 21, 2009 at 01:11:15PM -0500, Mike Christie wrote:
> Christof Schmitt wrote:
>> If the remote_port status is not BLOCKED, this will trigger the SCSI
>> midlayer error handling which cannot do much during the interruption
>> to the hardware and will mark the SCSI devices 'offline'. In order to
>> prevent this, the rule would be: First call fc_remote_port_delete to
>> set the remote port (or in the case of an HBA interruption all remote
>> ports) to BLOCKED, and only after this step call scsi_done to pass the
>> SCSI commands back to the upper layers.
>>
>
> One other note when doing this.
>
> For problems where you are deleting the rport, it is best to use  
> something like DID_TRANSPORT_DISRUPTED to fail the cmd if you are  
> failing it right away.

"something like DID_TRANSPORT_DISRUPTED" would be any error code that
goes through "maybe_retry" in scsi_decide_disposition? I guess moving
to DID_TRANSPORT_DISRUPTED is nice for consistency, but DID_ERROR
triggers the same code paths as far as i can see.

> If drivers block the rport, then fail commands  
> immediately with DID_TRANSPORT_DISRUPTED, then they will not actually be  
> failed to the block/mpath layer until the fast io fail timeout has  
> fired. This will prevent very short problems from firing the mutlipath  
> path offlining code.

Just to get the complete picture: Blocking the rport and then
returning DID_TRANSPORT_DISRUPTED will retry the command to the LLD
which then first calls fc_remote_port_chkready.
fc_remote_port_chkready will then keep the command between LLD and
SCSI midlayer until the rport state changes or the fast_fail fires.
Is this the complete picture or did i miss something?

> If your driver deletes the rport and does not fail the cmd immediately  
> so it can recover within the command or some other reason like the fw  
> just works that way, then when the fast io fail timer fires and the  
> terminate_rport_io callback is run you could actually use any error code  
> since at this time when a IO is sent to the queuecommand the driver will  
> call fc_remote_port_chkready and IO will be failed immediately with  
> DID_TRANSPORT_FAILFAST).

And the rport state is still BLOCKED, so at this point commands failed
in the upper layers with blk_abort_request will not end up in the SCSI
error recovery which cannot do much...

Thanks for the help, i am starting to get the complete picture...

Christof

Re: fc_remote_port_delete and returning SCSI commands from LLD

[Christof Schmitt]

On Wed, Oct 21, 2009 at 12:24:31PM -0400, James Smart wrote:
> Christof Schmitt wrote:
>> I am looking again at how and when a FC LLD should call
>> fc_remote_port_delete. Some help would be welcome to cover all
>> requirements and to plug the holes...
>
> It's pretty simple, as far as the FC transport is concerned.
> Call fc_remote_port_add() once connectivity is established.
> Call fc_remote_port_delete() once connectivity is lost.
> It is expected that there is a clear add->delete->add->delete->... sequence.
>
> Timing is considered "immediate", but there's always a window of delay.
>
> In general, the Transport ignores what happens to outstanding i/o, 
> letting the LLDD do something based on its policy, or let natural i/o 
> timers or the fast fail timer to fire.  The transport, at the delete 
> call, will start the fast fail timer if it is set.

Understood.

>> One scenario i am looking at: The connection to the HBA has been
>> temporarily lost and the LLD has to return all pending I/O requests to
>> the upper layers, so they can be retried later. Now with the SCSI
>> device being part of a multipath device, the first failed I/O request
>> triggers path failover:
>
> You are now asking a different question - how to make the upper layers play
> nice with the different responses from queuecommand, the LLDD's 
> interaction with the transport and midlayer, etc.
>
>>
>> multipath_end_io
>> do_end_io
>> fail_path
>> queue_work(kmultipathd, &pgpath->deactivate_path);
>>
>> which then marks the following returned requests as timed out:
>>
>> deactivate_path
>> blk_abort_queue
>> blk_abort_request
>> blk_rq_timed_out
>> scsi_times_out
>> fc_timed_out
>>
>> If the remote_port status is not BLOCKED, this will trigger the SCSI
>> midlayer error handling which cannot do much during the interruption
>> to the hardware and will mark the SCSI devices 'offline'.
>
> Well - this isn't absolute, but is pretty much true. We expect, when  
> connectivity is lost, for the block state to be temporarily entered. The  
> blocked state holds off further i/o and the eh handler as well, to 
> postpone the normal i/o failure cases which do lead to offline conditions 
> in most scenarios.
>
> But - this process is a coordinated effort between the driver and the 
> upper layers, and where the driver doesn't get helped by the transport 
> (the blocked state) it had better mimic the return codes at the different 
> points, and perhaps more, so that bad things don't happen.

"mimic the return codes" refers to fc_remote_port_chkready? Like
returning DID_IMM_RETRY when the rport is going to be BLOCKED, but
fc_remote_port_delete did not run yet?

>> In order to
>> prevent this, the rule would be: First call fc_remote_port_delete to
>> set the remote port (or in the case of an HBA interruption all remote
>> ports) to BLOCKED, and only after this step call scsi_done to pass the
>> SCSI commands back to the upper layers.
>
> True, although as mentioned, i/o termination is considered independent 
> from the rport/transport. But, you're best off if the target is blocked 
> due to the rport delete as we've prepped the upper layers to behave best 
> with this behavior.
>
> There will always be a few i/o's that sneak in or complete (timeout ?) in 
> between when the LLDD detects connectivity loss and when the  
> fc_remote_port_delete has been called. It's up to the LLDD to handle this 
> window.
>
> Completions, including i/o timeouts, are typically not a big deal and 
> should just return via scsi_done as they normally would. The caveat is 
> when those i/o's are from the eh thread.  Granted - if you are actively 
> aborting/failing i/o at the connectivity loss, and doing so before the 
> block is in place, you're causing more headaches for yourself in getting 
> the upper layers to play right with the LLDD - with the recommendation 
> being "don't do that".
>
> New i/o needs to be caught in queuecommand with the LLDD emulating the  
> transport status that would normally get returned.  E.g. the call to  
> fc_remote_port_chkready() won't catch it as the fc_remote_port_delete() 
> call hasn't completed yet - so the LLDD needs a 2nd check against it's 
> own structures, and if it detects the state, it should fail the i/o with 
> the same codes that chkready would. In reality, if you wanted to accept 
> the command, but never issue it and just leave it outstanding - waiting 
> for i/o timeout, or fast fail i/o timout, or devloss_tmo, I guess you 
> could.
>
>
>> This means, if the HBA problem is detected in interrupt context,
>> fc_remote_port_delete has to be called before calling scsi_done.
>
> Well - execution context is somewhat unrelated, as it depends on how the 
> LLDD is implemented, and what else its doing when connectivity is lost.
>
>>
>> But the description for fc_remote_port_delete states:
>>
>>  * Called from normal process context only - cannot be called from
>>  * interrupt.
>>  *
>>  * Notes:
>>  *	This routine assumes no locks are held on entry.
>>  */
>>
>> Looking at the functions called from fc_remote_port_delete, i don't
>> see a problem in calling fc_remote_port_delete from interrupt context
>> or with locks held. Does this mean the description should be fixed or
>> am i missing something?
>
> That's probably true. underlying routines have changed a bit over time 
> and it may be better now. I'd still hesitate with 
> fc_tgt_it_nexus_destroy() (although it shouldn't be applicable to you), 
> and scsi_target_block().  Creating additional lock hierachies between 
> LLDD locks and the locks in these paths (which the LLDD rarely sees/knows 
> about) isn't good.   Thus, we've mostly pushed LLDDs to use a pristine 
> context when calling the transport (such as a workq context) so that we 
> can disassociate low-level LLDD design from midlayer design.
>
>
>> fc_remote_port_add on the other hand can wait during flushes and has
>> to be called from process context. To summarize:
>> - A LLD has to call fc_remote_port_delete before returning SCSI
>>   commands from a failed port or failed HBA.
>
> not true, but best behavior.
>
>> - fc_remote_port_delete can be called from interrupt context before
>>   calling scsi_done if necessary
>
> part a (called from interrupt context) - do so at your own risk.  These 
> other paths can change at any time and its not fair for those developers 
> to know your driver dependencies.
>
> part b (before calling scsi_done) - recommended approach.
>
>> - fc_remote_port_add has to be called from process context
>
> True.
>
>> - The LLD has to serialize the fc_remote_port_add and
>>   fc_remote_port_delete calls to guarantee the add->delete->...
>>   sequence.
>
> True.

And, at least in zfcp, the notification from the hardware about I/O
completion to the call to scsi_done runs in softirq context. Calling
fc_remote_port_delete from this context is no good thing as you
mentioned and i don't see a good way to synchronize the
fc_remote_port_add/delete calls when going this way.

So far i see two possible solutions:

1) When the error is detected in softirq context, do not call
   scsi_done. Defer this call to the error handling thread/workqueue
   that will first call fc_remote_port_delete and then return all
   affected SCSI commands.

2) Have an LLD internal flag indicating "transitioning to rport
   blocked state", check for this in queuecommand and return
   DID_IMM_RETRY as fc_remote_port_chkready does. As soon as
   fc_remote_port_delete has been called, fc_remote_port_chkready will
   do the right thing.

It looks to me that 2) might be a short-term solution while 1) looks
like a proper way of handling interruptions on the host level in the
long term.

Anyway, thanks for the input.

I am tempted to summarize this for scsi_fc_transport.txt to have the
important requirements in one place. But this depends on the available
time, so no promises.

Christof

Re: fc_remote_port_delete and returning SCSI commands from LLD

[Christof Schmitt]

On Wed, Oct 21, 2009 at 12:33:25PM -0400, James Smart wrote:
> Here's what I remember about this from the past:
>
> - This was originally added when dealing with older kernels that didn't 
> have the eh patch that bounced the timeout handler when the rport was 
> blocked (see fc_timed_out).
>
>   The eh patch avoided entering the eh thread upon i/o timeouts if the 
> rport was blocked.
>
>
> - As mentioned in my prior email - there's a window where things can be  
> entered before the target blocked state protects you. What if you are in 
> the eh_handler when it occurs ?  Unfortunately, the eh thread is very 
> black and white on abort/reset/io status - its either success or not. It 
> doesn't validate the "not" cases, never looks at retry conditions, and 
> just assumes hard failure - which was taking everyone down bad paths.  
> This is a rats nest to resolve right, and I think I mentioned it on the 
> list a long time ago with Christoph. Thus the stall was added to plug the 
> hole.

I could imagine the case where something triggers the SCSI eh thread
(e.g.  a command timeout), but as soon as the eh thread starts the
recovery, something else prevents access to the remote port. Returning
DID_IMM_RETRY from fc_remote_port_chkready or the LLD does not help,
scsi_eh_completed_normally simply maps this error code to FAILED.
Holding off the SCSI eh thread until the rport leaves the BLOCKED
state will guarantee that we can either reach the rport or the SCSI
devices are being removed anyway.

It looks to me like this is required to prevent offline SCSI devices
in this case. Should this code be in a FC transport helper function
rather than being duplicated in each FC LLD?

Christof

> Christof Schmitt wrote:
>> On Tue, Oct 20, 2009 at 04:40:27PM +0200, Christof Schmitt wrote:
>>> If the remote_port status is not BLOCKED, this will trigger the SCSI
>>> midlayer error handling which cannot do much during the interruption
>>> to the hardware and will mark the SCSI devices 'offline'. In order to
>>> prevent this, the rule would be: First call fc_remote_port_delete to
>>> set the remote port (or in the case of an HBA interruption all remote
>>> ports) to BLOCKED, and only after this step call scsi_done to pass the
>>> SCSI commands back to the upper layers.
>>
>> I just stumbled across a loop that blocks the SCSI error handling
>> thread:
>>
>> 	spin_lock_irqsave(shost->host_lock, flags);
>> 	while (rport->port_state == FC_PORTSTATE_BLOCKED) {
>> 		spin_unlock_irqrestore(shost->host_lock, flags);
>> 		msleep(1000);
>> 		spin_lock_irqsave(shost->host_lock, flags);
>> 	}
>> 	spin_unlock_irqrestore(shost->host_lock, flags);
>>
>> This seems to be popular among FC drivers. Is this the preferred way
>> to synchronize the FC transport class state changes with the SCSI
>> midlayer error recovery?

Re: fc_remote_port_delete and returning SCSI commands from LLD

[James Smart]

Christof Schmitt wrote:
> On Wed, Oct 21, 2009 at 12:24:31PM -0400, James Smart wrote:
>> But - this process is a coordinated effort between the driver and the 
>> upper layers, and where the driver doesn't get helped by the transport 
>> (the blocked state) it had better mimic the return codes at the different 
>> points, and perhaps more, so that bad things don't happen.
> 
> "mimic the return codes" refers to fc_remote_port_chkready? Like
> returning DID_IMM_RETRY when the rport is going to be BLOCKED, but
> fc_remote_port_delete did not run yet?

Yes

Although, now that I look at chkready again, I'm surprised it didn't have one 
of Mike's TRANSPORT_DISRUPTED status's being returned.

> And, at least in zfcp, the notification from the hardware about I/O
> completion to the call to scsi_done runs in softirq context. Calling
> fc_remote_port_delete from this context is no good thing as you
> mentioned and i don't see a good way to synchronize the
> fc_remote_port_add/delete calls when going this way.

Should be ways to do it, as it's "just code in the LLDD". But I'm sure, the 
implementation is never that simple.

> 
> So far i see two possible solutions:
> 
> 1) When the error is detected in softirq context, do not call
>    scsi_done. Defer this call to the error handling thread/workqueue
>    that will first call fc_remote_port_delete and then return all
>    affected SCSI commands.
> 
> 2) Have an LLD internal flag indicating "transitioning to rport
>    blocked state", check for this in queuecommand and return
>    DID_IMM_RETRY as fc_remote_port_chkready does. As soon as
>    fc_remote_port_delete has been called, fc_remote_port_chkready will
>    do the right thing.
> 
> It looks to me that 2) might be a short-term solution while 1) looks
> like a proper way of handling interruptions on the host level in the
> long term.

True - depends on how successful (2) alone is. Both should be done, but if (2) 
at least exists, it significantly helps.

> 
> Anyway, thanks for the input.
> 
> I am tempted to summarize this for scsi_fc_transport.txt to have the
> important requirements in one place. But this depends on the available
> time, so no promises.

If you do - great!  It's been needed for a long time.

-- james s

Re: fc_remote_port_delete and returning SCSI commands from LLD

[James Smart]

Christof Schmitt wrote:
> It looks to me like this is required to prevent offline SCSI devices
> in this case. Should this code be in a FC transport helper function
> rather than being duplicated in each FC LLD?

That would make sense....

-- james s

Re: fc_remote_port_delete and returning SCSI commands from LLD

[Christof Schmitt]

On Fri, Oct 23, 2009 at 10:50:15AM -0400, James Smart wrote:
> Christof Schmitt wrote:
>> It looks to me like this is required to prevent offline SCSI devices
>> in this case. Should this code be in a FC transport helper function
>> rather than being duplicated in each FC LLD?
>
> That would make sense....
>
> -- james s

Here is a patch for this. It is only compile tested on x86, but the
code move is fairly trivial.

Christof
---
scsi_transport_fc: Introduce helper function for blocking scsi_eh

From: Christof Schmitt <christof.schmitt@de.ibm.com>

Move the duplicated code from FC LLDs to SCSI FC transport class.

Signed-off-by: Christof Schmitt <christof.schmitt@de.ibm.com>
---
 drivers/scsi/fnic/fnic_scsi.c    |   20 ++------------------
 drivers/scsi/lpfc/lpfc_scsi.c    |   30 ++++--------------------------
 drivers/scsi/qla2xxx/qla_os.c    |   25 ++++---------------------
 drivers/scsi/scsi_transport_fc.c |   26 ++++++++++++++++++++++++++
 include/scsi/scsi_transport_fc.h |    1 +
 5 files changed, 37 insertions(+), 65 deletions(-)

--- a/drivers/scsi/scsi_transport_fc.c	2009-10-27 17:50:56.000000000 +0100
+++ b/drivers/scsi/scsi_transport_fc.c	2009-10-27 17:51:18.000000000 +0100
@@ -27,6 +27,7 @@
  */
 #include <linux/module.h>
 #include <linux/init.h>
+#include <linux/delay.h>
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_transport.h>
@@ -3128,6 +3129,31 @@ fc_scsi_scan_rport(struct work_struct *w
 	spin_unlock_irqrestore(shost->host_lock, flags);
 }
 
+/**
+ * fc_block_scsi_eh - Block SCSI eh thread for blocked fc_rport
+ * @cmnd: SCSI command that scsi_eh is trying to recover
+ *
+ * This routine can be called from a FC LLD scsi_eh callback. It
+ * blocks the scsi_eh thread until the fc_rport leaves the
+ * FC_PORTSTATE_BLOCKED. This is necessary to avoid the scsi_eh
+ * failing recovery actions for blocked rports which would lead to
+ * offlined SCSI devices.
+ */
+void fc_block_scsi_eh(struct scsi_cmnd *cmnd)
+{
+	struct Scsi_Host *shost = cmnd->device->host;
+	struct fc_rport *rport = starget_to_rport(scsi_target(cmnd->device));
+	unsigned long flags;
+
+	spin_lock_irqsave(shost->host_lock, flags);
+	while (rport->port_state == FC_PORTSTATE_BLOCKED) {
+		spin_unlock_irqrestore(shost->host_lock, flags);
+		msleep(1000);
+		spin_lock_irqsave(shost->host_lock, flags);
+	}
+	spin_unlock_irqrestore(shost->host_lock, flags);
+}
+EXPORT_SYMBOL(fc_block_scsi_eh);
 
 /**
  * fc_vport_setup - allocates and creates a FC virtual port.
--- a/include/scsi/scsi_transport_fc.h	2009-10-27 17:50:57.000000000 +0100
+++ b/include/scsi/scsi_transport_fc.h	2009-10-27 17:51:18.000000000 +0100
@@ -807,5 +807,6 @@ void fc_host_post_vendor_event(struct Sc
 struct fc_vport *fc_vport_create(struct Scsi_Host *shost, int channel,
 		struct fc_vport_identifiers *);
 int fc_vport_terminate(struct fc_vport *vport);
+void fc_block_scsi_eh(struct scsi_cmnd *cmnd);
 
 #endif /* SCSI_TRANSPORT_FC_H */
--- a/drivers/scsi/fnic/fnic_scsi.c	2009-10-27 17:50:56.000000000 +0100
+++ b/drivers/scsi/fnic/fnic_scsi.c	2009-10-27 17:51:18.000000000 +0100
@@ -1224,22 +1224,6 @@ void fnic_terminate_rport_io(struct fc_r
 
 }
 
-static void fnic_block_error_handler(struct scsi_cmnd *sc)
-{
-	struct Scsi_Host *shost = sc->device->host;
-	struct fc_rport *rport = starget_to_rport(scsi_target(sc->device));
-	unsigned long flags;
-
-	spin_lock_irqsave(shost->host_lock, flags);
-	while (rport->port_state == FC_PORTSTATE_BLOCKED) {
-		spin_unlock_irqrestore(shost->host_lock, flags);
-		msleep(1000);
-		spin_lock_irqsave(shost->host_lock, flags);
-	}
-	spin_unlock_irqrestore(shost->host_lock, flags);
-
-}
-
 /*
  * This function is exported to SCSI for sending abort cmnds.
  * A SCSI IO is represented by a io_req in the driver.
@@ -1259,7 +1243,7 @@ int fnic_abort_cmd(struct scsi_cmnd *sc)
 	DECLARE_COMPLETION_ONSTACK(tm_done);
 
 	/* Wait for rport to unblock */
-	fnic_block_error_handler(sc);
+	fc_block_scsi_eh(sc);
 
 	/* Get local-port, check ready and link up */
 	lp = shost_priv(sc->device->host);
@@ -1541,7 +1525,7 @@ int fnic_device_reset(struct scsi_cmnd *
 	DECLARE_COMPLETION_ONSTACK(tm_done);
 
 	/* Wait for rport to unblock */
-	fnic_block_error_handler(sc);
+	fc_block_scsi_eh(sc);
 
 	/* Get local-port, check ready and link up */
 	lp = shost_priv(sc->device->host);
--- a/drivers/scsi/lpfc/lpfc_scsi.c	2009-10-27 17:50:56.000000000 +0100
+++ b/drivers/scsi/lpfc/lpfc_scsi.c	2009-10-27 17:51:18.000000000 +0100
@@ -2917,28 +2917,6 @@ lpfc_queuecommand(struct scsi_cmnd *cmnd
 }
 
 /**
- * lpfc_block_error_handler - Routine to block error  handler
- * @cmnd: Pointer to scsi_cmnd data structure.
- *
- *  This routine blocks execution till fc_rport state is not FC_PORSTAT_BLCOEKD.
- **/
-static void
-lpfc_block_error_handler(struct scsi_cmnd *cmnd)
-{
-	struct Scsi_Host *shost = cmnd->device->host;
-	struct fc_rport *rport = starget_to_rport(scsi_target(cmnd->device));
-
-	spin_lock_irq(shost->host_lock);
-	while (rport->port_state == FC_PORTSTATE_BLOCKED) {
-		spin_unlock_irq(shost->host_lock);
-		msleep(1000);
-		spin_lock_irq(shost->host_lock);
-	}
-	spin_unlock_irq(shost->host_lock);
-	return;
-}
-
-/**
  * lpfc_abort_handler - scsi_host_template eh_abort_handler entry point
  * @cmnd: Pointer to scsi_cmnd data structure.
  *
@@ -2961,7 +2939,7 @@ lpfc_abort_handler(struct scsi_cmnd *cmn
 	int ret = SUCCESS;
 	DECLARE_WAIT_QUEUE_HEAD_ONSTACK(waitq);
 
-	lpfc_block_error_handler(cmnd);
+	fc_block_scsi_eh(cmnd);
 	lpfc_cmd = (struct lpfc_scsi_buf *)cmnd->host_scribble;
 	BUG_ON(!lpfc_cmd);
 
@@ -3259,7 +3237,7 @@ lpfc_device_reset_handler(struct scsi_cm
 	struct lpfc_scsi_event_header scsi_event;
 	int status;
 
-	lpfc_block_error_handler(cmnd);
+	fc_block_scsi_eh(cmnd);
 
 	status = lpfc_chk_tgt_mapped(vport, cmnd);
 	if (status == FAILED) {
@@ -3318,7 +3296,7 @@ lpfc_target_reset_handler(struct scsi_cm
 	struct lpfc_scsi_event_header scsi_event;
 	int status;
 
-	lpfc_block_error_handler(cmnd);
+	fc_block_scsi_eh(cmnd);
 
 	status = lpfc_chk_tgt_mapped(vport, cmnd);
 	if (status == FAILED) {
@@ -3384,7 +3362,7 @@ lpfc_bus_reset_handler(struct scsi_cmnd 
 	fc_host_post_vendor_event(shost, fc_get_event_number(),
 		sizeof(scsi_event), (char *)&scsi_event, LPFC_NL_VENDOR_ID);
 
-	lpfc_block_error_handler(cmnd);
+	fc_block_scsi_eh(cmnd);
 
 	/*
 	 * Since the driver manages a single bus device, reset all
--- a/drivers/scsi/qla2xxx/qla_os.c	2009-10-27 17:50:56.000000000 +0100
+++ b/drivers/scsi/qla2xxx/qla_os.c	2009-10-27 17:51:18.000000000 +0100
@@ -727,23 +727,6 @@ qla2x00_abort_fcport_cmds(fc_port_t *fcp
 	spin_unlock_irqrestore(&ha->hardware_lock, flags);
 }
 
-static void
-qla2x00_block_error_handler(struct scsi_cmnd *cmnd)
-{
-	struct Scsi_Host *shost = cmnd->device->host;
-	struct fc_rport *rport = starget_to_rport(scsi_target(cmnd->device));
-	unsigned long flags;
-
-	spin_lock_irqsave(shost->host_lock, flags);
-	while (rport->port_state == FC_PORTSTATE_BLOCKED) {
-		spin_unlock_irqrestore(shost->host_lock, flags);
-		msleep(1000);
-		spin_lock_irqsave(shost->host_lock, flags);
-	}
-	spin_unlock_irqrestore(shost->host_lock, flags);
-	return;
-}
-
 /**************************************************************************
 * qla2xxx_eh_abort
 *
@@ -773,7 +756,7 @@ qla2xxx_eh_abort(struct scsi_cmnd *cmd)
 	struct req_que *req = vha->req;
 	srb_t *spt;
 
-	qla2x00_block_error_handler(cmd);
+	fc_block_scsi_eh(cmd);
 
 	if (!CMD_SP(cmd))
 		return SUCCESS;
@@ -904,7 +887,7 @@ __qla2xxx_eh_generic_reset(char *name, e
 	fc_port_t *fcport = (struct fc_port *) cmd->device->hostdata;
 	int err;
 
-	qla2x00_block_error_handler(cmd);
+	fc_block_scsi_eh(cmd);
 
 	if (!fcport)
 		return FAILED;
@@ -984,7 +967,7 @@ qla2xxx_eh_bus_reset(struct scsi_cmnd *c
 	unsigned long serial;
 	srb_t *sp = (srb_t *) CMD_SP(cmd);
 
-	qla2x00_block_error_handler(cmd);
+	fc_block_scsi_eh(cmd);
 
 	id = cmd->device->id;
 	lun = cmd->device->lun;
@@ -1047,7 +1030,7 @@ qla2xxx_eh_host_reset(struct scsi_cmnd *
 	srb_t *sp = (srb_t *) CMD_SP(cmd);
 	scsi_qla_host_t *base_vha = pci_get_drvdata(ha->pdev);
 
-	qla2x00_block_error_handler(cmd);
+	fc_block_scsi_eh(cmd);
 
 	id = cmd->device->id;
 	lun = cmd->device->lun;

Re: fc_remote_port_delete and returning SCSI commands from LLD

[James Smart]

Thanks Christof.

Acked-by: James Smart <james.smart@emulex.com>

-- james s


Christof Schmitt wrote:
> On Fri, Oct 23, 2009 at 10:50:15AM -0400, James Smart wrote:
>> Christof Schmitt wrote:
>>> It looks to me like this is required to prevent offline SCSI devices
>>> in this case. Should this code be in a FC transport helper function
>>> rather than being duplicated in each FC LLD?
>> That would make sense....
>>
>> -- james s
> 
> Here is a patch for this. It is only compile tested on x86, but the
> code move is fairly trivial.
> 
> Christof
> ---
> scsi_transport_fc: Introduce helper function for blocking scsi_eh
> 
> From: Christof Schmitt <christof.schmitt@de.ibm.com>
> 
> Move the duplicated code from FC LLDs to SCSI FC transport class.
> 
> Signed-off-by: Christof Schmitt <christof.schmitt@de.ibm.com>
> ---
>  drivers/scsi/fnic/fnic_scsi.c    |   20 ++------------------
>  drivers/scsi/lpfc/lpfc_scsi.c    |   30 ++++--------------------------
>  drivers/scsi/qla2xxx/qla_os.c    |   25 ++++---------------------
>  drivers/scsi/scsi_transport_fc.c |   26 ++++++++++++++++++++++++++
>  include/scsi/scsi_transport_fc.h |    1 +
>  5 files changed, 37 insertions(+), 65 deletions(-)
> 

Re: fc_remote_port_delete and returning SCSI commands from LLD

[Mike Christie]

Christof Schmitt wrote:
> On Wed, Oct 21, 2009 at 01:11:15PM -0500, Mike Christie wrote:
>> Christof Schmitt wrote:
>>> If the remote_port status is not BLOCKED, this will trigger the SCSI
>>> midlayer error handling which cannot do much during the interruption
>>> to the hardware and will mark the SCSI devices 'offline'. In order to
>>> prevent this, the rule would be: First call fc_remote_port_delete to
>>> set the remote port (or in the case of an HBA interruption all remote
>>> ports) to BLOCKED, and only after this step call scsi_done to pass the
>>> SCSI commands back to the upper layers.
>>>
>> One other note when doing this.
>>
>> For problems where you are deleting the rport, it is best to use  
>> something like DID_TRANSPORT_DISRUPTED to fail the cmd if you are  
>> failing it right away.
> 
> "something like DID_TRANSPORT_DISRUPTED" would be any error code that
> goes through "maybe_retry" in scsi_decide_disposition? I guess moving
> to DID_TRANSPORT_DISRUPTED is nice for consistency, but DID_ERROR
> triggers the same code paths as far as i can see.

It could be a little different. See scsi_noretry_cmd. If you used 
DID_ERROR and something set the driver failfast bit then it would be 
fast failed.


> 
>> If drivers block the rport, then fail commands  
>> immediately with DID_TRANSPORT_DISRUPTED, then they will not actually be  
>> failed to the block/mpath layer until the fast io fail timeout has  
>> fired. This will prevent very short problems from firing the mutlipath  
>> path offlining code.
> 
> Just to get the complete picture: Blocking the rport and then
> returning DID_TRANSPORT_DISRUPTED will retry the command to the LLD
> which then first calls fc_remote_port_chkready.
> fc_remote_port_chkready will then keep the command between LLD and
> SCSI midlayer until the rport state changes or the fast_fail fires.
> Is this the complete picture or did i miss something?

I think that is it.

> 
>> If your driver deletes the rport and does not fail the cmd immediately  
>> so it can recover within the command or some other reason like the fw  
>> just works that way, then when the fast io fail timer fires and the  
>> terminate_rport_io callback is run you could actually use any error code  
>> since at this time when a IO is sent to the queuecommand the driver will  
>> call fc_remote_port_chkready and IO will be failed immediately with  
>> DID_TRANSPORT_FAILFAST).
> 
> And the rport state is still BLOCKED, so at this point commands failed
> in the upper layers with blk_abort_request will not end up in the SCSI
> error recovery which cannot do much...
> 
> Thanks for the help, i am starting to get the complete picture...
> 
> Christof

Re: fc_remote_port_delete and returning SCSI commands from LLD

[Mike Christie]

James Smart wrote:
> 
> 
> Christof Schmitt wrote:
>> On Wed, Oct 21, 2009 at 12:24:31PM -0400, James Smart wrote:
>>> But - this process is a coordinated effort between the driver and the 
>>> upper layers, and where the driver doesn't get helped by the 
>>> transport (the blocked state) it had better mimic the return codes at 
>>> the different points, and perhaps more, so that bad things don't happen.
>>
>> "mimic the return codes" refers to fc_remote_port_chkready? Like
>> returning DID_IMM_RETRY when the rport is going to be BLOCKED, but
>> fc_remote_port_delete did not run yet?
> 
> Yes
> 
> Although, now that I look at chkready again, I'm surprised it didn't 
> have one of Mike's TRANSPORT_DISRUPTED status's being returned.
> 

We use DID_IMM_RETRY and do not use TRANSPORT_DISRUPTED in 
fc_remote_port_chkready because it would count against the retries which 
we do not want when we hit those state change races. I think 
Andrew/qlogic hit something where during those race windows we could 
exhaust all the retries.

Re: fc_remote_port_delete and returning SCSI commands from LLD

[Christof Schmitt]

On Tue, Oct 27, 2009 at 04:53:50PM -0500, Mike Christie wrote:
> Christof Schmitt wrote:
>> On Wed, Oct 21, 2009 at 01:11:15PM -0500, Mike Christie wrote:
>>> Christof Schmitt wrote:
>>>> If the remote_port status is not BLOCKED, this will trigger the SCSI
>>>> midlayer error handling which cannot do much during the interruption
>>>> to the hardware and will mark the SCSI devices 'offline'. In order to
>>>> prevent this, the rule would be: First call fc_remote_port_delete to
>>>> set the remote port (or in the case of an HBA interruption all remote
>>>> ports) to BLOCKED, and only after this step call scsi_done to pass the
>>>> SCSI commands back to the upper layers.
>>>>
>>> One other note when doing this.
>>>
>>> For problems where you are deleting the rport, it is best to use   
>>> something like DID_TRANSPORT_DISRUPTED to fail the cmd if you are   
>>> failing it right away.
>>
>> "something like DID_TRANSPORT_DISRUPTED" would be any error code that
>> goes through "maybe_retry" in scsi_decide_disposition? I guess moving
>> to DID_TRANSPORT_DISRUPTED is nice for consistency, but DID_ERROR
>> triggers the same code paths as far as i can see.
>
> It could be a little different. See scsi_noretry_cmd. If you used  
> DID_ERROR and something set the driver failfast bit then it would be  
> fast failed.

Ok. Somehow i missed scsi_noretry_cmd while looking at the SCSI code.
Then it makes sense to report DID_TRANSPORT_DISRUPTED from the zfcp. I
will target the zfcp patch for the next merge window.

Christof