[Bug Report] PCIe errinject and hot-unplug causes nvme driver hang

[Nilay Shroff]

On 4/24/24 23:06, Keith Busch wrote:
> On Tue, Apr 23, 2024 at 03:22:46PM +0530, Nilay Shroff wrote:

>> I understand that we don't want to cancel pending IO from the nvme_remove()
>> unconditionally as if the disk is not physically hot-unplug then we still 
>> want to  wait for the in-flight IO to be finished. Also looking through 
>> the above cases, I think that the nvme_timeout() might be the code path 
>> from where we want to cancel in-flight/pending IO if controller is 
>> in the terminal state (i.e. DELETING or DELETING_NOIO). Keeping this idea in
>> mind, I have worked out the below patch:
>>
>> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
>> index 8e0bb9692685..e45a54d84649 100644
>> --- a/drivers/nvme/host/pci.c
>> +++ b/drivers/nvme/host/pci.c
>> @@ -1286,6 +1286,9 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req)
>>         u32 csts = readl(dev->bar + NVME_REG_CSTS);
>>         u8 opcode;
>>  
>> +       if (nvme_state_terminal(&dev->ctrl))
>> +               goto disable;
>> +
>>         /* If PCI error recovery process is happening, we cannot reset or
>>          * the recovery mechanism will surely fail.
>>          */
>> @@ -1390,8 +1393,13 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req)
>>         return BLK_EH_RESET_TIMER;
>>  
>>  disable:
>> -       if (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_RESETTING))
>> +       if (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_RESETTING)) {
>> +               if (nvme_state_terminal(&dev->ctrl)) {
>> +                       nvme_dev_disable(dev, false);
>> +                       nvme_sync_queues(&dev->ctrl);
>> +               }
>>                 return BLK_EH_DONE;
>> +       }
>>  
>>         nvme_dev_disable(dev, false);
>>         if (nvme_try_sched_reset(&dev->ctrl))
>>
>> I have tested the above patch against all possible cases. Please let me know
>> if this looks good or if there are any further comments.
> 
> This looks okay to me. Just a couple things:
> 
> Set nvme_dev_disable's "shutdown" parameter to "true" since we're
> restarting the queues again from this state.
> 
> Remove "nvme_sync_queues()". I think that would deadlock: sync_queues
> waits for the timeout work to complete, but your calling it within the
> timeout work, so this would have it wait for itself.
> 
Thank you for reviewing the patch! And yes I agree with your suggestions.
I will make those changes and send a formal patch in another email.

Thanks,
--Nilay