* [bug report] nvme-tcp: add NVMe over TCP host driver
@ 2021-08-24 13:16 Dan Carpenter
2021-09-01 14:56 ` Sagi Grimberg
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2021-08-24 13:16 UTC (permalink / raw)
To: sagi; +Cc: linux-nvme
Hello Sagi Grimberg,
The patch 3f2304f8c6d6: "nvme-tcp: add NVMe over TCP host driver"
from Dec 3, 2018, leads to the following
Smatch static checker warning:
drivers/nvme/host/multipath.c:101 nvme_kick_requeue_lists()
warn: sleeping in atomic context
drivers/nvme/host/multipath.c
97 void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl)
98 {
99 struct nvme_ns *ns;
100
--> 101 down_read(&ctrl->namespaces_rwsem);
102 list_for_each_entry(ns, &ctrl->namespaces, list) {
103 if (ns->head->disk)
104 kblockd_schedule_work(&ns->head->requeue_work);
105 }
106 up_read(&ctrl->namespaces_rwsem);
107 }
This is a new Smatch warning I'm working on and it's sort of
overwhelming because it has generates too many warnings and they're
complicated to analyze and report. I'm trying to send these
automatically generated call trees to see if it will help.
nvme_fc_unregister_remoteport() <- disables preempt
-> nvme_fc_ctrl_connectivity_loss()
-> nvme_reset_ctrl()
nvme_fc_ctrl_connectivity_loss() <duplicate>
nvme_fc_unregister_remoteport() <- disables preempt <duplicate>
nvme_fc_exit_module() <- disables preempt
-> nvme_fc_cleanup_for_unload()
-> nvme_fc_delete_controllers() <- disables preempt
-> nvme_delete_ctrl()
nvme_tcp_state_change() <- disables preempt
-> nvme_tcp_error_recovery()
-> nvme_change_ctrl_state()
-> nvme_kick_requeue_lists()
I looked at the last call tree and it seems like potentially a real bug.
nvme_tcp_state_change() <- disables preempt
-> nvme_tcp_error_recovery()
-> nvme_change_ctrl_state()
-> nvme_kick_requeue_lists()
regards,
dan carpenter
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] nvme-tcp: add NVMe over TCP host driver
2021-08-24 13:16 [bug report] nvme-tcp: add NVMe over TCP host driver Dan Carpenter
@ 2021-09-01 14:56 ` Sagi Grimberg
2021-09-02 9:37 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Sagi Grimberg @ 2021-09-01 14:56 UTC (permalink / raw)
To: Dan Carpenter, sagi; +Cc: linux-nvme
On 8/24/21 4:16 PM, Dan Carpenter wrote:
> Hello Sagi Grimberg,
>
> The patch 3f2304f8c6d6: "nvme-tcp: add NVMe over TCP host driver"
> from Dec 3, 2018, leads to the following
> Smatch static checker warning:
>
> drivers/nvme/host/multipath.c:101 nvme_kick_requeue_lists()
> warn: sleeping in atomic context
>
> drivers/nvme/host/multipath.c
> 97 void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl)
> 98 {
> 99 struct nvme_ns *ns;
> 100
> --> 101 down_read(&ctrl->namespaces_rwsem);
> 102 list_for_each_entry(ns, &ctrl->namespaces, list) {
> 103 if (ns->head->disk)
> 104 kblockd_schedule_work(&ns->head->requeue_work);
> 105 }
> 106 up_read(&ctrl->namespaces_rwsem);
> 107 }
>
> This is a new Smatch warning I'm working on and it's sort of
> overwhelming because it has generates too many warnings and they're
> complicated to analyze and report. I'm trying to send these
> automatically generated call trees to see if it will help.
>
> nvme_fc_unregister_remoteport() <- disables preempt
> -> nvme_fc_ctrl_connectivity_loss()
> -> nvme_reset_ctrl()
> nvme_fc_ctrl_connectivity_loss() <duplicate>
> nvme_fc_unregister_remoteport() <- disables preempt <duplicate>
> nvme_fc_exit_module() <- disables preempt
> -> nvme_fc_cleanup_for_unload()
> -> nvme_fc_delete_controllers() <- disables preempt
> -> nvme_delete_ctrl()
> nvme_tcp_state_change() <- disables preempt
> -> nvme_tcp_error_recovery()
> -> nvme_change_ctrl_state()
> -> nvme_kick_requeue_lists()
>
>
> I looked at the last call tree and it seems like potentially a real bug.
>
> nvme_tcp_state_change() <- disables preempt
> -> nvme_tcp_error_recovery()
> -> nvme_change_ctrl_state()
> -> nvme_kick_requeue_lists()
That is a correct analysis. However from this flow, it is impossible to
step into the condition that triggers nvme_kick_requeue_lists because we
call nvme_change_ctrl_state with state NVME_CTRL_RESETTING from
nvme_tcp_error_recovery which means that the new state cannot be
NVME_CTRL_LIVE as the state will either transition to the desired state
or fail and be unchanged.
I wander what action should we take here? make the effort to defer the
call to a workqueue context? or we can safely ignore it?
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] nvme-tcp: add NVMe over TCP host driver
2021-09-01 14:56 ` Sagi Grimberg
@ 2021-09-02 9:37 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2021-09-02 9:37 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: sagi, linux-nvme
On Wed, Sep 01, 2021 at 05:56:55PM +0300, Sagi Grimberg wrote:
>
>
> On 8/24/21 4:16 PM, Dan Carpenter wrote:
> > Hello Sagi Grimberg,
> >
> > The patch 3f2304f8c6d6: "nvme-tcp: add NVMe over TCP host driver"
> > from Dec 3, 2018, leads to the following
> > Smatch static checker warning:
> >
> > drivers/nvme/host/multipath.c:101 nvme_kick_requeue_lists()
> > warn: sleeping in atomic context
> >
> > drivers/nvme/host/multipath.c
> > 97 void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl)
> > 98 {
> > 99 struct nvme_ns *ns;
> > 100
> > --> 101 down_read(&ctrl->namespaces_rwsem);
> > 102 list_for_each_entry(ns, &ctrl->namespaces, list) {
> > 103 if (ns->head->disk)
> > 104 kblockd_schedule_work(&ns->head->requeue_work);
> > 105 }
> > 106 up_read(&ctrl->namespaces_rwsem);
> > 107 }
> >
> > This is a new Smatch warning I'm working on and it's sort of
> > overwhelming because it has generates too many warnings and they're
> > complicated to analyze and report. I'm trying to send these
> > automatically generated call trees to see if it will help.
> >
> > nvme_fc_unregister_remoteport() <- disables preempt
> > -> nvme_fc_ctrl_connectivity_loss()
> > -> nvme_reset_ctrl()
> > nvme_fc_ctrl_connectivity_loss() <duplicate>
> > nvme_fc_unregister_remoteport() <- disables preempt <duplicate>
> > nvme_fc_exit_module() <- disables preempt
> > -> nvme_fc_cleanup_for_unload()
> > -> nvme_fc_delete_controllers() <- disables preempt
> > -> nvme_delete_ctrl()
> > nvme_tcp_state_change() <- disables preempt
> > -> nvme_tcp_error_recovery()
> > -> nvme_change_ctrl_state()
> > -> nvme_kick_requeue_lists()
> >
> >
> > I looked at the last call tree and it seems like potentially a real bug.
> >
> > nvme_tcp_state_change() <- disables preempt
> > -> nvme_tcp_error_recovery()
> > -> nvme_change_ctrl_state()
> > -> nvme_kick_requeue_lists()
>
> That is a correct analysis. However from this flow, it is impossible to
> step into the condition that triggers nvme_kick_requeue_lists because we
> call nvme_change_ctrl_state with state NVME_CTRL_RESETTING from
> nvme_tcp_error_recovery which means that the new state cannot be
> NVME_CTRL_LIVE as the state will either transition to the desired state or
> fail and be unchanged.
>
> I wander what action should we take here? make the effort to defer the
> call to a workqueue context? or we can safely ignore it?
Yeah. Just ignore it. Later versions of Smatch might be clever enough
to figure this out.
regards,
dan carpenter
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-09-02 9:38 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-24 13:16 [bug report] nvme-tcp: add NVMe over TCP host driver Dan Carpenter
2021-09-01 14:56 ` Sagi Grimberg
2021-09-02 9:37 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox