[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