Re: scsi_dh_alua: add missing transitioning state support

[Mike Christie <michaelc@cs.wisc.edu>]

On 09/21/2010 02:33 PM, Mike Snitzer wrote:
> On Mon, Sep 20 2010 at 10:27pm -0400,
> Mike Christie<michaelc@cs.wisc.edu>  wrote:
>
>> On 09/20/2010 10:35 AM, Mike Snitzer wrote:
>>> Hi Hannes,
>>>
>>> On Tue, Aug 31 2010 at 11:11am -0400,
>>> Mike Snitzer<snitzer@redhat.com>   wrote:
>>>
>>>> On Mon, Aug 30 2010 at  5:36am -0400,
>>>> Hannes Reinecke<hare@suse.de>   wrote:
>>>>
>>>>> Nicholas A. Bellinger wrote:
>>>>>> On Tue, 2010-08-17 at 15:05 -0400, Mike Snitzer wrote:
>>>>>>> Handle transitioning in the prep_fn.
>>>>>>> Handle transitioning in alua_rtpg's implicit alua code too.
>>>>>>>
>>>>>>> These gaps were identified during controller failover testing of an
>>>>>>> ALUA array.
>>>>>>>
>>>>>>> Signed-off-by: Mike Snitzer<snitzer@redhat.com>
>>>>>>> ---
>>>>>>>   drivers/scsi/device_handler/scsi_dh_alua.c |   10 +++++++---
>>>>>>>   1 files changed, 7 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
>>>>>>> index 1a970a7..c1eedc5 100644
>>>>>>> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
>>>>>>> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
>>>>>>> @@ -616,6 +616,9 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
>>>>>>>   		    h->state == TPGS_STATE_STANDBY)
>>>>>>>   			/* Useable path if active */
>>>>>>>   			err = SCSI_DH_OK;
>>>>>>> +		else if (h->state == TPGS_STATE_TRANSITIONING)
>>>>>>> +			/* State transition, retry */
>>>>>>> +			goto retry;
>>>>>>>   		else
>>>>>>>   			/* Path unuseable for unavailable/offline */
>>>>>>>   			err = SCSI_DH_DEV_OFFLINED;
>>>>>>> @@ -698,13 +701,14 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
>>>>>>>   	struct alua_dh_data *h = get_alua_data(sdev);
>>>>>>>   	int ret = BLKPREP_OK;
>>>>>>>
>>>>>>> -	if (h->state != TPGS_STATE_OPTIMIZED&&
>>>>>>> -	    h->state != TPGS_STATE_NONOPTIMIZED) {
>>>>>>> +	if (h->state == TPGS_STATE_TRANSITIONING)
>>>>>>> +		ret = BLKPREP_DEFER;
>>>>>>> +	else if (h->state != TPGS_STATE_OPTIMIZED&&
>>>>>>> +		 h->state != TPGS_STATE_NONOPTIMIZED) {
>>>>>>>   		ret = BLKPREP_KILL;
>>>>>>>   		req->cmd_flags |= REQ_QUIET;
>>>>>>>   	}
>>>>>>>   	return ret;
>>>>>>> -
>>>>>>>   }
>>>>>>>
>>>>>>
>>>>>> Makes sense to me..
>>>>>>
>>>>>> Acked-by: Nicholas A. Bellinger<nab@linux-iscsi.org>
>>>>>>
>>>>> Not so fast. There are two problems with this approach:
>>>>>
>>>>> The path is retried indefinitely. Arrays are _supposed_ to be in 'transitioning'
>>>>> only temporary; however, if the array is stuck due to a fw error we're stuck in 'defer',
>>>>> too.
>>>>
>>>> And what is the problem with that?  The IO will eventually time out.
>>>
>>> To restate as a question: even though we'll retry in alua_rtpg();
>>> shouldn't the SCSI command eventually time out (via
>>> scsi_attempt_requeue_command)?
>>
>> That function is only in RHEL. Requests that are prepd and sent to
>> the scsi layer and driver would eventually timeout in
>> scsi_softirq_done in upstream.
>>
>> alua_prep_fn prevents the IO from getting sent to the scsi layer so
>> we do not hit the check in scsi_softirq_done though.
>
> That is only the case if alua_prep_fn were to return BLKPREP_DEFER
> right?

Yeah. I misread the email above. Just to clarify..

For the alua case, we will always retry due the prep_fn issue I mentioned.

For the alua_rtpg() retry case you were discussing we will also always 
retry, because the timer check in 
scsi_attempt_requeue_command/scsi_softirq_done will break us from 
retrying forever in that execution of the request started by the 
submit_rtpg call. However, the added goto retry will would just end up 
starting a another execution. To handle Hannes comment I think you would 
want to add some retry/timer checks in alua_rtpg to prevent that from 
retrying forever.

>
> 2) the patch also modified alua_rtpg() so implicit ALUA would retry
>     (just like explicit ALUA currently does) if TPGS_STATE_TRANSITIONING
>     - so why should we avoid retry for implicit but do it for explicit?

Leaving that for Hannes. I cannot think of a reason. Probably just did 
not do it.