[PATCH] nvme-fabrics: fix crash for no IO queues

[PATCH] nvme-fabrics: fix crash for no IO queues

[Chao Leng]

A crash happens when set feature(NVME_FEAT_NUM_QUEUES) timeout in nvme
over rdma(roce) reconnection, the reason is use the queue which is not
alloced.

If queue is not live, should not allow queue request.

Signed-off-by: Chao Leng <lengchao@huawei.com>
---
 drivers/nvme/host/fabrics.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index 733010d2eafd..2479744fc349 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -189,7 +189,7 @@ static inline bool nvmf_check_ready(struct nvme_ctrl *ctrl, struct request *rq,
 {
 	if (likely(ctrl->state == NVME_CTRL_LIVE ||
 		   ctrl->state == NVME_CTRL_DELETING))
-		return true;
+		return queue_live;
 	return __nvmf_check_ready(ctrl, rq, queue_live);
 }
 
-- 
2.16.4


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-fabrics: fix crash for no IO queues

[Sagi Grimberg]

> A crash happens when set feature(NVME_FEAT_NUM_QUEUES) timeout in nvme
> over rdma(roce) reconnection, the reason is use the queue which is not
> alloced.
> 
> If queue is not live, should not allow queue request.

Can you describe exactly the scenario here? What is the state
here? LIVE? or DELETING?

> 
> Signed-off-by: Chao Leng <lengchao@huawei.com>
> ---
>   drivers/nvme/host/fabrics.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
> index 733010d2eafd..2479744fc349 100644
> --- a/drivers/nvme/host/fabrics.h
> +++ b/drivers/nvme/host/fabrics.h
> @@ -189,7 +189,7 @@ static inline bool nvmf_check_ready(struct nvme_ctrl *ctrl, struct request *rq,
>   {
>   	if (likely(ctrl->state == NVME_CTRL_LIVE ||
>   		   ctrl->state == NVME_CTRL_DELETING))
> -		return true;
> +		return queue_live;
>   	return __nvmf_check_ready(ctrl, rq, queue_live);
>   }

There were some issues in the past that made us allow submitting
requests in DELETING state and introducing DELETING_NOIO. See
patch ecca390e8056 ("nvme: fix deadlock in disconnect during scan_work 
and/or ana_work")

The driver should be able to accept I/O in DELETING because the core
changes the state to DELETING_NOIO _before_ it calls ->delete_ctrl so I
don't understand how you get to this if the queue is not allocated...

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-fabrics: fix crash for no IO queues

[Chao Leng]

On 2021/3/6 4:58, Sagi Grimberg wrote:
> 
>> A crash happens when set feature(NVME_FEAT_NUM_QUEUES) timeout in nvme
>> over rdma(roce) reconnection, the reason is use the queue which is not
>> alloced.
>>
>> If queue is not live, should not allow queue request.
> 
> Can you describe exactly the scenario here? What is the state
> here? LIVE? or DELETING?
If seting feature(NVME_FEAT_NUM_QUEUES) failed due to time out or
the target return 0 io queues, nvme_set_queue_count will return 0,
and then reconnection will continue and success. The state of controller
is LIVE. The request will continue to deliver by call ->queue_rq(),
and then crash happens.
> 
>>
>> Signed-off-by: Chao Leng <lengchao@huawei.com>
>> ---
>>   drivers/nvme/host/fabrics.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
>> index 733010d2eafd..2479744fc349 100644
>> --- a/drivers/nvme/host/fabrics.h
>> +++ b/drivers/nvme/host/fabrics.h
>> @@ -189,7 +189,7 @@ static inline bool nvmf_check_ready(struct nvme_ctrl *ctrl, struct request *rq,
>>   {
>>       if (likely(ctrl->state == NVME_CTRL_LIVE ||
>>              ctrl->state == NVME_CTRL_DELETING))
>> -        return true;
>> +        return queue_live;
>>       return __nvmf_check_ready(ctrl, rq, queue_live);
>>   }
> 
> There were some issues in the past that made us allow submitting
> requests in DELETING state and introducing DELETING_NOIO. See
> patch ecca390e8056 ("nvme: fix deadlock in disconnect during scan_work and/or ana_work")
This doesn't make any difference. When in deleting state the queue is
still live.
> 
> The driver should be able to accept I/O in DELETING because the core
> changes the state to DELETING_NOIO _before_ it calls ->delete_ctrl so I
> don't understand how you get to this if the queue is not allocated...
The state of controller is live. The deletion process looks good.
> .

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-fabrics: fix crash for no IO queues

[Sagi Grimberg]

>>> A crash happens when set feature(NVME_FEAT_NUM_QUEUES) timeout in nvme
>>> over rdma(roce) reconnection, the reason is use the queue which is not
>>> alloced.
>>>
>>> If queue is not live, should not allow queue request.
>>
>> Can you describe exactly the scenario here? What is the state
>> here? LIVE? or DELETING?
> If seting feature(NVME_FEAT_NUM_QUEUES) failed due to time out or
> the target return 0 io queues, nvme_set_queue_count will return 0,
> and then reconnection will continue and success. The state of controller
> is LIVE. The request will continue to deliver by call ->queue_rq(),
> and then crash happens.

Thinking about this again, we should absolutely fail the reconnection
when we are unable to set any I/O queues, it is just wrong to
keep this controller alive...

This should be fixed for both rdma and tcp.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-fabrics: fix crash for no IO queues

[Chao Leng]

On 2021/3/16 1:08, Sagi Grimberg wrote:
> 
>>>> A crash happens when set feature(NVME_FEAT_NUM_QUEUES) timeout in nvme
>>>> over rdma(roce) reconnection, the reason is use the queue which is not
>>>> alloced.
>>>>
>>>> If queue is not live, should not allow queue request.
>>>
>>> Can you describe exactly the scenario here? What is the state
>>> here? LIVE? or DELETING?
>> If seting feature(NVME_FEAT_NUM_QUEUES) failed due to time out or
>> the target return 0 io queues, nvme_set_queue_count will return 0,
>> and then reconnection will continue and success. The state of controller
>> is LIVE. The request will continue to deliver by call ->queue_rq(),
>> and then crash happens.
> 
> Thinking about this again, we should absolutely fail the reconnection
> when we are unable to set any I/O queues, it is just wrong to
> keep this controller alive...
Keith think keeping the controller alive for diagnose is better.
This is the patch which failed the connection.
https://lore.kernel.org/linux-nvme/20210223072602.3196-1-lengchao@huawei.com/

Now we have 2 choice:
1.failed the connection when unable to set any I/O queues.
2.do not allow queue request when queue is not live.

 From a service continuity perspective, I think it is better that failed
the connection when unable to set any I/O queues.
Diagnose is less important, I prefer service continuity, because if failed
the reconnection and then try new reconnection, it is possible to recover.

> 
> This should be fixed for both rdma and tcp.
> .

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-fabrics: fix crash for no IO queues

[Keith Busch]

On Tue, Mar 16, 2021 at 09:23:21AM +0800, Chao Leng wrote:
> 
> 
> On 2021/3/16 1:08, Sagi Grimberg wrote:
> > 
> > > > > A crash happens when set feature(NVME_FEAT_NUM_QUEUES) timeout in nvme
> > > > > over rdma(roce) reconnection, the reason is use the queue which is not
> > > > > alloced.
> > > > > 
> > > > > If queue is not live, should not allow queue request.
> > > > 
> > > > Can you describe exactly the scenario here? What is the state
> > > > here? LIVE? or DELETING?
> > > If seting feature(NVME_FEAT_NUM_QUEUES) failed due to time out or
> > > the target return 0 io queues, nvme_set_queue_count will return 0,
> > > and then reconnection will continue and success. The state of controller
> > > is LIVE. The request will continue to deliver by call ->queue_rq(),
> > > and then crash happens.
> > 
> > Thinking about this again, we should absolutely fail the reconnection
> > when we are unable to set any I/O queues, it is just wrong to
> > keep this controller alive...
> Keith think keeping the controller alive for diagnose is better.
> This is the patch which failed the connection.
> https://lore.kernel.org/linux-nvme/20210223072602.3196-1-lengchao@huawei.com/
> 
> Now we have 2 choice:
> 1.failed the connection when unable to set any I/O queues.
> 2.do not allow queue request when queue is not live.

Okay, so there are different views on how to handles this. I personally find
in-band administration for a misbehaving device is a good thing to have, but I
won't 'nak' if the consensus from the people using this is for the other way.
 
> From a service continuity perspective, I think it is better that failed
> the connection when unable to set any I/O queues.
> Diagnose is less important, I prefer service continuity, because if failed
> the reconnection and then try new reconnection, it is possible to recover.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-fabrics: fix crash for no IO queues

[Sagi Grimberg]

>>>>>> A crash happens when set feature(NVME_FEAT_NUM_QUEUES) timeout in nvme
>>>>>> over rdma(roce) reconnection, the reason is use the queue which is not
>>>>>> alloced.
>>>>>>
>>>>>> If queue is not live, should not allow queue request.
>>>>>
>>>>> Can you describe exactly the scenario here? What is the state
>>>>> here? LIVE? or DELETING?
>>>> If seting feature(NVME_FEAT_NUM_QUEUES) failed due to time out or
>>>> the target return 0 io queues, nvme_set_queue_count will return 0,
>>>> and then reconnection will continue and success. The state of controller
>>>> is LIVE. The request will continue to deliver by call ->queue_rq(),
>>>> and then crash happens.
>>>
>>> Thinking about this again, we should absolutely fail the reconnection
>>> when we are unable to set any I/O queues, it is just wrong to
>>> keep this controller alive...
>> Keith think keeping the controller alive for diagnose is better.
>> This is the patch which failed the connection.
>> https://lore.kernel.org/linux-nvme/20210223072602.3196-1-lengchao@huawei.com/
>>
>> Now we have 2 choice:
>> 1.failed the connection when unable to set any I/O queues.
>> 2.do not allow queue request when queue is not live.
> 
> Okay, so there are different views on how to handles this. I personally find
> in-band administration for a misbehaving device is a good thing to have, but I
> won't 'nak' if the consensus from the people using this is for the other way.

While I understand that this can be useful, I've seen it do more harm
than good. It is really puzzling to people when the controller state
reflected is live (and even optimized) and no I/O is making progress for
unknown reason. And logs are rarely accessed in these cases.

I am also opting for failing it and rescheduling a reconnect.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-fabrics: fix crash for no IO queues

[James Smart]

On 3/15/2021 10:08 PM, Sagi Grimberg wrote:
> 
>>>>>>> A crash happens when set feature(NVME_FEAT_NUM_QUEUES) timeout in 
>>>>>>> nvme
>>>>>>> over rdma(roce) reconnection, the reason is use the queue which 
>>>>>>> is not
>>>>>>> alloced.
>>>>>>>
>>>>>>> If queue is not live, should not allow queue request.
>>>>>>
>>>>>> Can you describe exactly the scenario here? What is the state
>>>>>> here? LIVE? or DELETING?
>>>>> If seting feature(NVME_FEAT_NUM_QUEUES) failed due to time out or
>>>>> the target return 0 io queues, nvme_set_queue_count will return 0,
>>>>> and then reconnection will continue and success. The state of 
>>>>> controller
>>>>> is LIVE. The request will continue to deliver by call ->queue_rq(),
>>>>> and then crash happens.
>>>>
>>>> Thinking about this again, we should absolutely fail the reconnection
>>>> when we are unable to set any I/O queues, it is just wrong to
>>>> keep this controller alive...
>>> Keith think keeping the controller alive for diagnose is better.
>>> This is the patch which failed the connection.
>>> https://lore.kernel.org/linux-nvme/20210223072602.3196-1-lengchao@huawei.com/ 
>>>
>>>
>>> Now we have 2 choice:
>>> 1.failed the connection when unable to set any I/O queues.
>>> 2.do not allow queue request when queue is not live.
>>
>> Okay, so there are different views on how to handles this. I 
>> personally find
>> in-band administration for a misbehaving device is a good thing to 
>> have, but I
>> won't 'nak' if the consensus from the people using this is for the 
>> other way.
> 
> While I understand that this can be useful, I've seen it do more harm
> than good. It is really puzzling to people when the controller state
> reflected is live (and even optimized) and no I/O is making progress for
> unknown reason. And logs are rarely accessed in these cases.
> 
> I am also opting for failing it and rescheduling a reconnect.

Agree with Sagi. We also hit this issue a long time ago and I made the 
same change (commit 834d3710a093a) that Sagi is suggesting:  if the 
prior controller instance had io queues, but the new/reconnected 
controller fails to create io queues, then the controller create is 
failed and a reconnect is scheduled.

-- james



_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-fabrics: fix crash for no IO queues

[Keith Busch]

On Tue, Mar 16, 2021 at 01:57:16PM -0700, James Smart wrote:
> On 3/15/2021 10:08 PM, Sagi Grimberg wrote:
> > 
> > > > > > > > A crash happens when set
> > > > > > > > feature(NVME_FEAT_NUM_QUEUES) timeout in nvme
> > > > > > > > over rdma(roce) reconnection, the reason is use
> > > > > > > > the queue which is not
> > > > > > > > alloced.
> > > > > > > > 
> > > > > > > > If queue is not live, should not allow queue request.
> > > > > > > 
> > > > > > > Can you describe exactly the scenario here? What is the state
> > > > > > > here? LIVE? or DELETING?
> > > > > > If seting feature(NVME_FEAT_NUM_QUEUES) failed due to time out or
> > > > > > the target return 0 io queues, nvme_set_queue_count will return 0,
> > > > > > and then reconnection will continue and success. The
> > > > > > state of controller
> > > > > > is LIVE. The request will continue to deliver by call ->queue_rq(),
> > > > > > and then crash happens.
> > > > > 
> > > > > Thinking about this again, we should absolutely fail the reconnection
> > > > > when we are unable to set any I/O queues, it is just wrong to
> > > > > keep this controller alive...
> > > > Keith think keeping the controller alive for diagnose is better.
> > > > This is the patch which failed the connection.
> > > > https://lore.kernel.org/linux-nvme/20210223072602.3196-1-lengchao@huawei.com/
> > > > 
> > > > 
> > > > Now we have 2 choice:
> > > > 1.failed the connection when unable to set any I/O queues.
> > > > 2.do not allow queue request when queue is not live.
> > > 
> > > Okay, so there are different views on how to handles this. I
> > > personally find
> > > in-band administration for a misbehaving device is a good thing to
> > > have, but I
> > > won't 'nak' if the consensus from the people using this is for the
> > > other way.
> > 
> > While I understand that this can be useful, I've seen it do more harm
> > than good. It is really puzzling to people when the controller state
> > reflected is live (and even optimized) and no I/O is making progress for
> > unknown reason. And logs are rarely accessed in these cases.
> > 
> > I am also opting for failing it and rescheduling a reconnect.
> 
> Agree with Sagi. We also hit this issue a long time ago and I made the same
> change (commit 834d3710a093a) that Sagi is suggesting:  if the prior
> controller instance had io queues, but the new/reconnected controller fails
> to create io queues, then the controller create is failed and a reconnect is
> scheduled.

Okay, fair enough.

One more question: if the controller is in such a bad way that it will
never create IO queues without additional intervention, will this
behavior have the driver schedule reconnect indefinitely?

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-fabrics: fix crash for no IO queues

[Sagi Grimberg]

>>>>> Now we have 2 choice:
>>>>> 1.failed the connection when unable to set any I/O queues.
>>>>> 2.do not allow queue request when queue is not live.
>>>>
>>>> Okay, so there are different views on how to handles this. I
>>>> personally find
>>>> in-band administration for a misbehaving device is a good thing to
>>>> have, but I
>>>> won't 'nak' if the consensus from the people using this is for the
>>>> other way.
>>>
>>> While I understand that this can be useful, I've seen it do more harm
>>> than good. It is really puzzling to people when the controller state
>>> reflected is live (and even optimized) and no I/O is making progress for
>>> unknown reason. And logs are rarely accessed in these cases.
>>>
>>> I am also opting for failing it and rescheduling a reconnect.
>>
>> Agree with Sagi. We also hit this issue a long time ago and I made the same
>> change (commit 834d3710a093a) that Sagi is suggesting:  if the prior
>> controller instance had io queues, but the new/reconnected controller fails
>> to create io queues, then the controller create is failed and a reconnect is
>> scheduled.
> 
> Okay, fair enough.
> 
> One more question: if the controller is in such a bad way that it will
> never create IO queues without additional intervention, will this
> behavior have the driver schedule reconnect indefinitely?

Until either ctrl_loss_tmo expires or the user is tired of this
controller and manually disconnects.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-fabrics: fix crash for no IO queues

[James Smart]

> 
> One more question: if the controller is in such a bad way that it will
> never create IO queues without additional intervention, will this
> behavior have the driver schedule reconnect indefinitely?
> 

No.. in fabrics, there is a max controller reconnect timeout that will 
expire (dflt 600s), stop the reconnect attempts and delete the controller.

-- james

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH] nvme-fabrics: fix crash for no IO queues

[Chao Leng]

A crash happens when set feature(NVME_FEAT_NUM_QUEUES) timeout in nvme
over rdma(roce) reconnection, the reason is use the queue which is not
alloced.

If queue is not live, should not allow queue request.

Signed-off-by: Chao Leng <lengchao@huawei.com>
---
 drivers/nvme/host/fabrics.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index 733010d2eafd..2479744fc349 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -189,7 +189,7 @@ static inline bool nvmf_check_ready(struct nvme_ctrl *ctrl, struct request *rq,
 {
 	if (likely(ctrl->state == NVME_CTRL_LIVE ||
 		   ctrl->state == NVME_CTRL_DELETING))
-		return true;
+		return queue_live;
 	return __nvmf_check_ready(ctrl, rq, queue_live);
 }
 
-- 
2.16.4


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme