On 9/30/20 2:31 PM, Victor Gladkov wrote:
> On 9/30/20 8:47 PM, Hannes Reinecke wrote:
>   
>>>    		opts->max_reconnects = DIV_ROUND_UP(ctrl_loss_tmo,
>>>    						opts->reconnect_delay);
>>> +		if (ctrl_loss_tmo < opts->fast_io_fail_tmo)
>>> +			pr_warn("failfast tmo (%d) larger than controller "
>>> +				"loss tmo (%d)\n",
>>> +				opts->fast_io_fail_tmo, ctrl_loss_tmo);
>>> +	}
>>
>> If we already check for that condition, shouldn't we disable fast_io_fail_tmo in
>> that situation to clarify things?
>>
> 
> OK for me. I don't mind
> 
>>>
>>>    	if (!opts->host) {
>>>    		kref_get(&nvmf_default_host->ref);
>>> @@ -985,7 +1004,7 @@ void nvmf_free_options(struct nvmf_ctrl_options
>>> *opts)
>>>    #define NVMF_ALLOWED_OPTS	(NVMF_OPT_QUEUE_SIZE |
>>> NVMF_OPT_NR_IO_QUEUES | \
>>>    				 NVMF_OPT_KATO | NVMF_OPT_HOSTNQN | \
>>>    				 NVMF_OPT_HOST_ID | NVMF_OPT_DUP_CONNECT
>>> |\
>>> -				 NVMF_OPT_DISABLE_SQFLOW)
>>> +				 NVMF_OPT_DISABLE_SQFLOW |
>>> NVMF_OPT_FAIL_FAST_TMO)
>>>
>>>    static struct nvme_ctrl *
>>>    nvmf_create_ctrl(struct device *dev, const char *buf) diff --git
>>> a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h index
> 
>>> diff --git a/drivers/nvme/host/multipath.c
>>> b/drivers/nvme/host/multipath.c index 54603bd..d8b7f45 100644
>>> --- a/drivers/nvme/host/multipath.c
>>> +++ b/drivers/nvme/host/multipath.c
>>> @@ -278,9 +278,12 @@ static bool nvme_available_path(struct
>>> nvme_ns_head *head)
>>>
>>>    	list_for_each_entry_rcu(ns, &head->list, siblings) {
>>>    		switch (ns->ctrl->state) {
>>> +		case NVME_CTRL_CONNECTING:
>>> +			if (test_bit(NVME_CTRL_FAILFAST_EXPIRED,
>>> +				     &ns->ctrl->flags))
>>> +				break;
>>
>> No. We shouldn't select this path, but that doesn't mean that all other paths in
>> this list can't be selected, either; they might be coming from different
>> controllers.
>> Please use 'continue' here.
>>
> 
>   The 'break' doesn't interrupt the 'for_each' loop, but only go out from 'switch'
> 
Ah. But still; that is not quite correct as we'll need to intercept 
things at nvme_ns_head_submit_bio() to make the correct decision there.

I've attached the modified version I'm working with; please check if 
you're okay with it.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer