[PATCH v9] nvme-fabrics: reject I/O to offline device

[Hannes Reinecke]

On 11/17/20 9:39 AM, Sagi Grimberg wrote:
> 
>>>> @@ -151,12 +151,16 @@ EXPORT_SYMBOL_GPL(nvme_try_sched_reset);
>>>>    static void nvme_failfast_work(struct work_struct *work) {
>>>>           struct nvme_ctrl *ctrl = container_of(to_delayed_work(work),
>>>>                           struct nvme_ctrl, failfast_work);
>>>> +       struct nvme_ns *ns;
>>>>
>>>> -       if (ctrl->state != NVME_CTRL_CONNECTING)
>>>> -               return;
>>>> -
>>>> -
>>>> -       set_bit(NVME_CTRL_FAILFAST_EXPIRED, &ctrl->flags);
>>>> +       down_read(&ctrl->namespaces_rwsem);
>>>> +       list_for_each_entry(ns, &ctrl->namespaces, list) {
>>>> +               if (ctrl->state != NVME_CTRL_LIVE ||
>>>> +                   (ns->ana_state != NVME_ANA_OPTIMIZED &&
>>>> +                    ns->ana_state != NVME_ANA_NONOPTIMIZED))
>>>> +                       set_bit(NVME_NS_FAILFAST_EXPIRED, &ns->flags);
>>>> +       }
>>>> +       up_read(&ctrl->namespaces_rwsem);
>>>>           dev_info(ctrl->device, "failfast expired\n");
>>>>
>>>> ...and we could leave the failfast worker running even after the 
>>>> controller
>>>> transitioned to LIVE.
>>>> Cf the attached patch for details.
>>>>
>>>> Cheers,
>>>>
>>>> Hannes
>>>> -- 
>>>
>>> I'm not sure what makes sense to move the FAILFAST_EXPIRED bit into 
>>> the namespace,
>>> Because the failfast mechanism characterizes the controller as a whole.
>>>
>> Oh, yes, I'm aware of that. But the problem here is with multipath; 
>> how do we handle the situation where all controllers have the 
>> 'failfast_expired' bit set?
>> Should I/O be terminated (which I think it should, given that failfast 
>> is supposed to terminate the I/O)?
>> Or should I/O continue to run (as it does with your original patch)?
> 
> I do agree that fast_io_fail_tmo _is_ a controller attribute and should
> remain as such.
> 
> I do see what is your point Hannes, however I also think it's
> problematic that the host may fail arbitrary I/O if the controller
> happens to enter ANA inaccessible state (or have state transition
> timeout) for a period that happens to be longer than what the user
> happen to set (without communicating any of this to the controller).
> 
> IFF we want to address this (I'm still not sure), we probably want
> to activate failfast timeout in ANA state transition (and clear it
> when we exit it). Then we can modify nvme_available_path() to take
> NVME_CTRL_FAILFAST_EXPIRED into account.
> 
> Anyways, I think that this can be an incremental patch because it
> doesn't change the behavior today with respect to ANA states (or
> transition between them) e.g. queue the outstanding I/O.
> 
Okay then.
You can add:

Reviewed-by: Hannes Reinecke <hare at suse.de>

Cheers,

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