[PATCH rfc 1/2] nvme: don't remove namespace if revalidate failed because of controller reset

[kbusch@kernel.org (Keith Busch)]

On Tue, Jul 30, 2019@10:12:42AM -0700, Sagi Grimberg wrote:
> 
> > > Yes, and again, addresses the case that the namespace is going away.
> > > 
> > > So I think we are in agreement? I only need to change the commit
> > > message from: "the revalidation I/O" to "the admin I/O" ?
> > 
> > That words of 'admin I/O' isn't related with the patch or issue.
> 
> But it is, the original issue was due to the fact that
> nvme_revalidate_disk() I/Os such as nvme_identify_ns() or
> nvme_identify_ns_descs(). This was the original issue.
> 
> > > Yea, this should do the trick I guess:
> > > --
> > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > > index fa31da0762b9..d01976c93160 100644
> > > --- a/drivers/nvme/host/core.c
> > > +++ b/drivers/nvme/host/core.c
> > > @@ -3428,7 +3428,8 @@ static void nvme_validate_ns(struct nvme_ctrl
> > > *ctrl, unsigned nsid)
> > > 
> > >           ns = nvme_find_get_ns(ctrl, nsid);
> > >           if (ns) {
> > > -               if (ns->disk && revalidate_disk(ns->disk))
> > > +               if (ns->disk && ctrl->state == NVME_CTRL_LIVE &&
> > > +                   revalidate_disk(ns->disk)
> > >                           nvme_ns_remove(ns);
> > >                   nvme_put_ns(ns);
> > >           } else
> > 
> > If RESET is triggered just inside revalidate_disk(), and not done after
> > revalidate_disk() returns,  there is still race between reset and scan work.
> > 
> 
> You are correct, this was why I had the ctrl->state check after
> revalidate_disk so if it failed because we are in a reset we should
> not remove the namespace.
> 
> We need a reliable way to NOT remove the namespace if revalidate_disk
> failed because the controller is resetting and we don't have a channel
> to the controller at this very moment...
> 
> Keith,
> 
> As for the failure during reset scenario, this is happening only when
> the namespace is about to go away or something is seriously wrong right
> (looking from where nvme_kill_queues is called).
> 
> Do you still think we should avoid calling the revalidate_disk if the
> controller is resetting?

I was considering if a reset happens to trigger when nvme's
revalidate_disk tries to read identify namespace. It's possible that
command gets aborted, and we don't retry admin commands, so we'd return
-ENODEV and nvme_validate_ns() removes an otherwise healthy namespace.

I'm not too concerned about this corner case actually occuring in
practice, though.