Re: [SPDK] Report a potential race condition in nvmf target

Re: [SPDK] Report a potential race condition in nvmf target

[Yang, Ziye]

[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]

Hi Lorne,

I think that your concern is right. When a subsystem is adding/deleting a namespace. The event should be propagated to any active ctrlr built on this subsystem, and the operation on the ctrlr's operation should be executed on the thread who creates the ctrlr. 

Can you submit a patch to address this if you have time?

Thanks.




Best Regards
Ziye Yang 


-----Original Message-----
From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Lorne Li
Sent: Monday, June 24, 2019 3:15 PM
To: spdk-dev <spdk(a)lists.01.org>
Subject: [SPDK] Report a potential race condition in nvmf target



Hi all,


I think there may exist a race condition in nvmf target.


When adding or removing a namespace,  nvmf target using `spdk_nvmf_ctrlr_ns_changed` to notify its ctrlr that ns has been changed from subsystem's thread('app thread'). But I think func  `spdk_nvmf_ctrlr_ns_changed` should be called in ctrlr's thread, since it touches ctrlr's variables and even transfers out ctrlr's aer_req.


Last week, I launched a issue https://github.com/spdk/spdk/issues/837 in github to describe this race condition in detail and a panic likely caused by this.


It would be great if someone could take a look at it. Thanks in advance.


|
Lorne Li
lorneli(a)163.com
|
签名由网易邮箱大师定制
_______________________________________________
SPDK mailing list
SPDK(a)lists.01.org
https://lists.01.org/mailman/listinfo/spdk

Re: [SPDK] Report a potential race condition in nvmf target

[Sasha Kotchubievsky]

[-- Attachment #1: Type: text/plain, Size: 1122 bytes --]

Hi,

I think, you're right. Completion for aer request should be done in the 
thread its QP belongs to, instead of thread handling RPC requests 
(app_thread).

Best regards

Sasha

On 24-Jun-19 10:15 AM, Lorne Li wrote:
>
> Hi all,
>
>
> I think there may exist a race condition in nvmf target.
>
>
> When adding or removing a namespace,  nvmf target using `spdk_nvmf_ctrlr_ns_changed` to notify its ctrlr that ns has been changed from subsystem's thread('app thread'). But I think func  `spdk_nvmf_ctrlr_ns_changed` should be called in ctrlr's thread, since it touches ctrlr's variables and even transfers out ctrlr's aer_req.
>
>
> Last week, I launched a issue https://github.com/spdk/spdk/issues/837 in github to describe this race condition in detail and a panic likely caused by this.
>
>
> It would be great if someone could take a look at it. Thanks in advance.
>
>
> |
> Lorne Li
> lorneli(a)163.com
> |
> 签名由网易邮箱大师定制
> _______________________________________________
> SPDK mailing list
> SPDK(a)lists.01.org
> https://lists.01.org/mailman/listinfo/spdk

Re: [SPDK] Report a potential race condition in nvmf target

[Sasha Kotchubievsky]

[-- Attachment #1: Type: text/plain, Size: 2737 bytes --]

Hi,

The fix looks fine with me.

Best regards

Sasha

On 24-Jun-19 4:34 PM, Lorne Li wrote:
> Hi Ziye, Sasha,
>
> Thanks for your quick response.
>
> I have submitted a patch trying to fix this. 
> https://review.gerrithub.io/c/spdk/spdk/+/459144
>
> Please correct me if I didn't fix it in the right way.
>
> Lorne Li
> lorneli(a)163.com
>
> <https://maas.mail.163.com/dashi-web-extend/html/proSignature.html?ftlId=2&name=Lorne+Li&uid=lorneli%40163.com&iconUrl=http%3A%2F%2Fmail-online.nosdn.127.net%2Fsm613e07779056b29675090ae14d921321.jpg&items=%5B%22%22%2C%22lorneli%40163.com%22%2C%22%22%2C%22%22%2C%22%22%5D> 
>
> 签名由 网易邮箱大师 <https://mail.163.com/dashi/dlpro.html?from=mail81> 
> 定制
> On 6/24/2019 16:22，Yang, Ziye<ziye.yang(a)intel.com> 
> <mailto:ziye.yang(a)intel.com> wrote：
>
>     Hi Lorne,
>
>     I think that your concern is right. When a subsystem is
>     adding/deleting a namespace. The event should be propagated to any
>     active ctrlr built on this subsystem, and the operation on the
>     ctrlr's operation should be executed on the thread who creates the
>     ctrlr.
>
>     Can you submit a patch to address this if you have time?
>
>     Thanks.
>
>
>
>
>     Best Regards
>     Ziye Yang
>
>
>     -----Original Message-----
>     From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Lorne Li
>     Sent: Monday, June 24, 2019 3:15 PM
>     To: spdk-dev <spdk(a)lists.01.org>
>     Subject: [SPDK] Report a potential race condition in nvmf target
>
>
>
>     Hi all,
>
>
>     I think there may exist a race condition in nvmf target.
>
>
>     When adding or removing a namespace,  nvmf target using
>     `spdk_nvmf_ctrlr_ns_changed` to notify its ctrlr that ns has been
>     changed from subsystem's thread('app thread'). But I think func
>      `spdk_nvmf_ctrlr_ns_changed` should be called in ctrlr's thread,
>     since it touches ctrlr's variables and even transfers out ctrlr's
>     aer_req.
>
>
>     Last week, I launched a issue
>     https://github.com/spdk/spdk/issues/837 in github to describe this
>     race condition in detail and a panic likely caused by this.
>
>
>     It would be great if someone could take a look at it. Thanks in
>     advance.
>
>
>     |
>     Lorne Li
>     lorneli(a)163.com
>     |
>     签名由网易邮箱大师定制
>     _______________________________________________
>     SPDK mailing list
>     SPDK(a)lists.01.org
>     https://lists.01.org/mailman/listinfo/spdk
>     _______________________________________________
>     SPDK mailing list
>     SPDK(a)lists.01.org
>     https://lists.01.org/mailman/listinfo/spdk
>

Re: [SPDK] Report a potential race condition in nvmf target

[Lorne Li]

[-- Attachment #1: Type: text/plain, Size: 1963 bytes --]

Hi Ziye, Sasha,


Thanks for your quick response.


I have submitted a patch trying to fix this.   https://review.gerrithub.io/c/spdk/spdk/+/459144


Please correct me if I didn't fix it in the right way.


|
Lorne Li
lorneli(a)163.com
|
签名由网易邮箱大师定制
On 6/24/2019 16:22，Yang, Ziye<ziye.yang(a)intel.com> wrote：
Hi Lorne,

I think that your concern is right. When a subsystem is adding/deleting a namespace. The event should be propagated to any active ctrlr built on this subsystem, and the operation on the ctrlr's operation should be executed on the thread who creates the ctrlr.

Can you submit a patch to address this if you have time?

Thanks.




Best Regards
Ziye Yang


-----Original Message-----
From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Lorne Li
Sent: Monday, June 24, 2019 3:15 PM
To: spdk-dev <spdk(a)lists.01.org>
Subject: [SPDK] Report a potential race condition in nvmf target



Hi all,


I think there may exist a race condition in nvmf target.


When adding or removing a namespace,  nvmf target using `spdk_nvmf_ctrlr_ns_changed` to notify its ctrlr that ns has been changed from subsystem's thread('app thread'). But I think func  `spdk_nvmf_ctrlr_ns_changed` should be called in ctrlr's thread, since it touches ctrlr's variables and even transfers out ctrlr's aer_req.


Last week, I launched a issue https://github.com/spdk/spdk/issues/837 in github to describe this race condition in detail and a panic likely caused by this.


It would be great if someone could take a look at it. Thanks in advance.


|
Lorne Li
lorneli(a)163.com
|
签名由网易邮箱大师定制
_______________________________________________
SPDK mailing list
SPDK(a)lists.01.org
https://lists.01.org/mailman/listinfo/spdk
_______________________________________________
SPDK mailing list
SPDK(a)lists.01.org
https://lists.01.org/mailman/listinfo/spdk

[SPDK] Report a potential race condition in nvmf target

[Lorne Li]

[-- Attachment #1: Type: text/plain, Size: 719 bytes --]



Hi all,


I think there may exist a race condition in nvmf target.


When adding or removing a namespace,  nvmf target using `spdk_nvmf_ctrlr_ns_changed` to notify its ctrlr that ns has been changed from subsystem's thread('app thread'). But I think func  `spdk_nvmf_ctrlr_ns_changed` should be called in ctrlr's thread, since it touches ctrlr's variables and even transfers out ctrlr's aer_req.


Last week, I launched a issue https://github.com/spdk/spdk/issues/837 in github to describe this race condition in detail and a panic likely caused by this.


It would be great if someone could take a look at it. Thanks in advance.


|
Lorne Li
lorneli(a)163.com
|
签名由网易邮箱大师定制