[bug report] nvme-tcp: add NVMe over TCP host driver

[bug report] nvme-tcp: add NVMe over TCP host driver

[Dan Carpenter]

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

Re: [bug report] nvme-tcp: add NVMe over TCP host driver

[Sagi Grimberg]

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

Re: [bug report] nvme-tcp: add NVMe over TCP host driver

[Dan Carpenter]

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