[GIT,PULL] NVMe changes for 4.5-rc1

Commit Message

Keith Busch Jan. 22, 2016, 10:08 p.m. UTC

On Fri, Jan 22, 2016 at 09:29:37AM -0700, Jens Axboe wrote:
> On 01/22/2016 09:26 AM, Linus Torvalds wrote:
> >
> >On Jan 22, 2016 7:59 AM, "Jens Axboe" <[e-mail]
> ><mailto:[e-mail]>> wrote:
> > >>
> > >>   - nvme_dev_remove() got renamed to nvme_remove_namespaces(), but also
> > >> lost the "dev" argument (it takes "struct nvme_ctrl *ctrl" now).
> > >
> > > &dev->ctrl
> >
> >That's not the problem - the code does that already.
> >
> >But we want the dev argument *back*, since the function now wants to do
> >that nvme_io_incapable() check and then cancel the IO on it.
> >
> >And I didn't know if you guys would want to use container_of() or just
> >change the calling convention back to using "dev", or perhaps just
> >decide to do the IO cleanup in the caller instead. That last option
> >seems the cleanest, since now nvme_dev_remove_namespaces() is just about
> >the namespace, and maybe it would be better to re-introduce a whole new
> >nvme_dev_remove() that does the old thing.
> 
> The latter is the cleanest approach, we'll get it fixed up.

Looks like IO ending is the only conflict. The cleanup below will
get the functionality back.

You might notice nvme_dev_disable is called twice for a surprise removal,
case, but that's okay.
---
--
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Keith Busch Jan. 22, 2016, 10:16 p.m. UTC | #1

On Fri, Jan 22, 2016 at 10:08:05PM +0000, Keith Busch wrote:
> Looks like IO ending is the only conflict. The cleanup below will
> get the functionality back.

Oh sorry, but something's actually off with this. I tested removal w/
IO using O_DIRECT by mistake, but the failure this was supposed to
fix requires using page cache. I disabled direct in the fio profile,
and now getting a hung surprise removal.

I'll have to circle back on this on Monday.

> ---
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 72ef832..5d31cfc 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2118,6 +2118,15 @@ static void nvme_remove(struct pci_dev *pdev)
>  	pci_set_drvdata(pdev, NULL);
>  	flush_work(&dev->reset_work);
>  	flush_work(&dev->scan_work);
> +
> +	/*
> +	 * If the controller can't do IO (surprise removal, for example), we
> +	 * need to disable prior to deleting namespaces. This ends outstanding
> +	 * requests and prevents attempts to sync dirty data.
> +	 */
> +	if (nvme_io_incapable(&dev->ctrl))
> +		nvme_dev_disable(dev, true);
> +
>  	nvme_remove_namespaces(&dev->ctrl);
>  	nvme_uninit_ctrl(&dev->ctrl);
>  	nvme_dev_disable(dev, true);
> --
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 72ef832..5d31cfc 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2118,6 +2118,15 @@  static void nvme_remove(struct pci_dev *pdev)
 	pci_set_drvdata(pdev, NULL);
 	flush_work(&dev->reset_work);
 	flush_work(&dev->scan_work);
+
+	/*
+	 * If the controller can't do IO (surprise removal, for example), we
+	 * need to disable prior to deleting namespaces. This ends outstanding
+	 * requests and prevents attempts to sync dirty data.
+	 */
+	if (nvme_io_incapable(&dev->ctrl))
+		nvme_dev_disable(dev, true);
+
 	nvme_remove_namespaces(&dev->ctrl);
 	nvme_uninit_ctrl(&dev->ctrl);
 	nvme_dev_disable(dev, true);