NVM Express driver

Hi everyone,

You may have seen the recent announcement of the NVM Express spec; here's the driver. I've put a git tree up on kernel.org with all the history that's fit to be released (earlier versions were full of Intel codenames and are not suited to public consumption). If you'd rather review the history, it's at
http://git.kernel.org/?p=linux/kernel/git/willy/nvme.git;a=summary

Jens has already offered to pull my nvme tree into his block tree, so that's the route I intend to take into Linus' tree. Comments appreciated, however in a masterpiece of bad timing I am going to be travelling for the next week, so please bear with me if I don't respond quickly.

Documentation/ioctl/ioctl-number.txt +0 -0

index 63ffd78..f8159ba 100644

Documentation/ioctl/ioctl-number.txt +20 -0

+static int nvme_download_firmware(struct nvme_ns *ns,
+						struct nvme_dlfw __user *udlfw)
+{
+	struct nvme_dev *dev = ns->dev;
+	struct nvme_dlfw dlfw;
+	struct nvme_command c;
+	int nents, status;
+	struct scatterlist *sg;
+	struct nvme_prps *prps;
+
+	if (copy_from_user(&dlfw, udlfw, sizeof(dlfw)))
+		return -EFAULT;
+	if (dlfw.length >= (1 << 30))
+		return -EINVAL;
+
+	nents = nvme_map_user_pages(dev, 1, dlfw.addr, dlfw.length * 4, &sg);
+	if (nents < 0)
+		return nents;
+
+	memset(&c, 0, sizeof(c));

Quoted 5 lines

Documentation/ioctl/ioctl-number.txt +0 -0

index 63ffd78..f8159ba 100644
@@ -148,6 +148,7 @@ Code  Seq#(hex)	Include File		Comments

Documentation/ioctl/ioctl-number.txt Code Seq#(hex) Include File Comments · line 151

Quoted 4 lines

Documentation/ioctl/ioctl-number.txt +1 -0

 'M'	01-03	drivers/scsi/megaraid/megaraid_sas.h
 'M'	00-0F	drivers/video/fsl-diu-fb.h	conflict!
 'N'	00-1F	drivers/usb/scanner.h
+'N'	40-7F	drivers/block/nvme.c

I hate to ask this, but why do you have ioctls for this? At first glance, a number of the ioctls you have should just be sysfs files to export the information. What am I misunderstanding here?

Quoted 25 lines

Documentation/ioctl/ioctl-number.txt +25 -0

+static int nvme_download_firmware(struct nvme_ns *ns,
+						struct nvme_dlfw __user *udlfw)
+{
+	struct nvme_dev *dev = ns->dev;
+	struct nvme_dlfw dlfw;
+	struct nvme_command c;
+	int nents, status;
+	struct scatterlist *sg;
+	struct nvme_prps *prps;
+
+	if (copy_from_user(&dlfw, udlfw, sizeof(dlfw)))
+		return -EFAULT;
+	if (dlfw.length >= (1 << 30))
+		return -EINVAL;
+
+	nents = nvme_map_user_pages(dev, 1, dlfw.addr, dlfw.length * 4, &sg);
+	if (nents < 0)
+		return nents;
+
+	memset(&c, 0, sizeof(c));
+	c.dlfw.opcode = nvme_admin_download_fw;
+	c.dlfw.numd = cpu_to_le32(dlfw.length);
+	c.dlfw.offset = cpu_to_le32(dlfw.offset);
+	prps = nvme_setup_prps(dev, &c.common, sg, dlfw.length * 4);
+

Documentation/ioctl/ioctl-number.txt

Quoted 5 lines

Documentation/ioctl/ioctl-number.txt +5 -0

+	status = nvme_submit_admin_cmd(dev, &c, NULL);
+	nvme_unmap_user_pages(dev, 0, dlfw.addr, dlfw.length * 4, sg, nents);
+	nvme_free_prps(dev, prps);
+	return status;
+}

Shouldn't you be using the build-in firmware kernel interface instead of rolling your own in an ioctl?

Quoted 5 lines · Greg KH

Quoted 5 lines

Documentation/ioctl/ioctl-number.txt +0 -0

index 63ffd78..f8159ba 100644
@@ -148,6 +148,7 @@ Code  Seq#(hex)	Include File		Comments

Quoted 8 lines

Documentation/ioctl/ioctl-number.txt Code Seq#(hex) Include File Comments · line 151

Quoted 4 lines

Documentation/ioctl/ioctl-number.txt +1 -0

 'M'	01-03	drivers/scsi/megaraid/megaraid_sas.h
 'M'	00-0F	drivers/video/fsl-diu-fb.h	conflict!
 'N'	00-1F	drivers/usb/scanner.h
+'N'	40-7F	drivers/block/nvme.c

I hate to ask this, but why do you have ioctls for this? At first glance, a number of the ioctls you have should just be sysfs files to export the information. What am I misunderstanding here?

I don't think you're arguing for SUBMIT_IO being done through sysfs, so some ioctls are clearly needed. I'll take a look at which ones can be moved to sysfs.

Quoted 25 lines

Quoted 25 lines

Documentation/ioctl/ioctl-number.txt +25 -0

+static int nvme_download_firmware(struct nvme_ns *ns,
+						struct nvme_dlfw __user *udlfw)
+{
+	struct nvme_dev *dev = ns->dev;
+	struct nvme_dlfw dlfw;
+	struct nvme_command c;
+	int nents, status;
+	struct scatterlist *sg;
+	struct nvme_prps *prps;
+
+	if (copy_from_user(&dlfw, udlfw, sizeof(dlfw)))
+		return -EFAULT;
+	if (dlfw.length >= (1 << 30))
+		return -EINVAL;
+
+	nents = nvme_map_user_pages(dev, 1, dlfw.addr, dlfw.length * 4, &sg);
+	if (nents < 0)
+		return nents;
+
+	memset(&c, 0, sizeof(c));
+	c.dlfw.opcode = nvme_admin_download_fw;
+	c.dlfw.numd = cpu_to_le32(dlfw.length);
+	c.dlfw.offset = cpu_to_le32(dlfw.offset);
+	prps = nvme_setup_prps(dev, &c.common, sg, dlfw.length * 4);
+

Quoted 8 lines

Documentation/ioctl/ioctl-number.txt

Quoted 5 lines

Documentation/ioctl/ioctl-number.txt +5 -0

+	status = nvme_submit_admin_cmd(dev, &c, NULL);
+	nvme_unmap_user_pages(dev, 0, dlfw.addr, dlfw.length * 4, sg, nents);
+	nvme_free_prps(dev, prps);
+	return status;
+}

Shouldn't you be using the build-in firmware kernel interface instead of rolling your own in an ioctl?

There's a bit of an impedence mismatch there. Think of this as being drive firmware instead of controller firmware. This isn't for request_firmware() kind of uses, it's for some admin tool to come along and tell the drive "Oh, here's some new firmware for you".

If you look at the spec [1], you'll see there are a number of firmware slots in the device, and it's up to the managability utility to decide which one to replace or activate. I dno't think you want to pull all that gnarly decision making code into the kernel, do you?

[1] http://download.intel.com/standards/nvmhci/NVM_Express_1_0_Gold.pdf

Heh, no, well, submit_io should just go through the block layer and not be a separate ioctl, right?

Quoted 4 lines

There's a bit of an impedence mismatch there. Think of this as being drive firmware instead of controller firmware. This isn't for request_firmware() kind of uses, it's for some admin tool to come along and tell the drive "Oh, here's some new firmware for you".

That's fine, request_firmware will work wonderfully for that.

Quoted 6 lines

If you look at the spec [1], you'll see there are a number of firmware slots in the device, and it's up to the managability utility to decide which one to replace or activate. I dno't think you want to pull all that gnarly decision making code into the kernel, do you?

[1] http://download.intel.com/standards/nvmhci/NVM_Express_1_0_Gold.pdf

No, just export multiple "slots" as firmware devices ready to be filled in by userspace whenever it wants/needs to. The management utility can just dump the firmware to those sysfs files when it determines it needs to update the firmware, no decision making in the kernel at all.

Quoted 2 lines · Greg KH

Heh, no, well, submit_io should just go through the block layer and not be a separate ioctl, right?

Just like with SG_IO, there are reasons to do I/Os without going through the block layer.

Quoted 6 lines

Quoted 4 lines

There's a bit of an impedence mismatch there. Think of this as being drive firmware instead of controller firmware. This isn't for request_firmware() kind of uses, it's for some admin tool to come along and tell the drive "Oh, here's some new firmware for you".

That's fine, request_firmware will work wonderfully for that.

How would the driver know that it should call request_firmware()? Do it every 60 seconds in case somebody's downloaded some new firmware?

Quoted 7 lines

Quoted 6 lines

If you look at the spec [1], you'll see there are a number of firmware slots in the device, and it's up to the managability utility to decide which one to replace or activate. I dno't think you want to pull all that gnarly decision making code into the kernel, do you?

[1] http://download.intel.com/standards/nvmhci/NVM_Express_1_0_Gold.pdf

Quoted 4 lines

No, just export multiple "slots" as firmware devices ready to be filled in by userspace whenever it wants/needs to. The management utility can just dump the firmware to those sysfs files when it determines it needs to update the firmware, no decision making in the kernel at all.

OK ... glad we decided to limit the number of slots. I still don't see (in Documentation/firmware_class/README) how this works for user-initiated firmware updates rather than kernel-initiated.

Quoted 6 lines · Matthew Wilcox

Quoted 2 lines · Greg KH

Heh, no, well, submit_io should just go through the block layer and not be a separate ioctl, right?

Just like with SG_IO, there are reasons to do I/Os without going through the block layer.

Ok, that makes sense.

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

There's a bit of an impedence mismatch there. Think of this as being drive firmware instead of controller firmware. This isn't for request_firmware() kind of uses, it's for some admin tool to come along and tell the drive "Oh, here's some new firmware for you".

That's fine, request_firmware will work wonderfully for that.

How would the driver know that it should call request_firmware()? Do it every 60 seconds in case somebody's downloaded some new firmware?

Ick, no, just use the function provided that lets you create a firmware request and be notified when it is written to, request_firmware_nowait(). That is what it is there for.

Quoted 12 lines

Quoted 11 lines

Quoted 6 lines

If you look at the spec [1], you'll see there are a number of firmware slots in the device, and it's up to the managability utility to decide which one to replace or activate. I dno't think you want to pull all that gnarly decision making code into the kernel, do you?

[1] http://download.intel.com/standards/nvmhci/NVM_Express_1_0_Gold.pdf

No, just export multiple "slots" as firmware devices ready to be filled in by userspace whenever it wants/needs to. The management utility can just dump the firmware to those sysfs files when it determines it needs to update the firmware, no decision making in the kernel at all.

Quoted 3 lines

OK ... glad we decided to limit the number of slots. I still don't see (in Documentation/firmware_class/README) how this works for user-initiated firmware updates rather than kernel-initiated.

I didn't even realize we had a firmware README file...

Anyway, just use request_firmware_nowait(), you will be fine.

Quoted 6 lines

I didn't even realize we had a firmware README file...

Anyway, just use request_firmware_nowait(), you will be fine.

Unless I'm misunderstanding the spec, this is for *nonvolatile* firmware updates. So if I have two of these devices and I stick a firmware file into /lib/firmware, should it update both devices?

Shouldn't reflashing the device firmware be something that the users asks for by something a little more specific than just creating a file?
(In any case, creating /lib/firmware/nvmhci-3 to flash slot 3 seems
rather silly.)

--Andy

Quoted 36 lines · Greg KH

Quoted 6 lines · Matthew Wilcox

Quoted 2 lines · Greg KH

Heh, no, well, submit_io should just go through the block layer and not be a separate ioctl, right?

Just like with SG_IO, there are reasons to do I/Os without going through the block layer.

Ok, that makes sense.

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

There's a bit of an impedence mismatch there. Think of this as being drive firmware instead of controller firmware. This isn't for request_firmware() kind of uses, it's for some admin tool to come along and tell the drive "Oh, here's some new firmware for you".

That's fine, request_firmware will work wonderfully for that.

How would the driver know that it should call request_firmware()? Do it every 60 seconds in case somebody's downloaded some new firmware?

Ick, no, just use the function provided that lets you create a firmware request and be notified when it is written to, request_firmware_nowait(). That is what it is there for.

Quoted 12 lines

Quoted 11 lines

Quoted 6 lines

If you look at the spec [1], you'll see there are a number of firmware slots in the device, and it's up to the managability utility to decide which one to replace or activate. I dno't think you want to pull all that gnarly decision making code into the kernel, do you?

[1] http://download.intel.com/standards/nvmhci/NVM_Express_1_0_Gold.pdf

No, just export multiple "slots" as firmware devices ready to be filled in by userspace whenever it wants/needs to. The management utility can just dump the firmware to those sysfs files when it determines it needs to update the firmware, no decision making in the kernel at all.

Quoted 8 lines

Quoted 3 lines

OK ... glad we decided to limit the number of slots. I still don't see (in Documentation/firmware_class/README) how this works for user-initiated firmware updates rather than kernel-initiated.

I didn't even realize we had a firmware README file...

Anyway, just use request_firmware_nowait(), you will be fine.

Unless I'm misunderstanding the spec, this is for *nonvolatile* firmware updates. So if I have two of these devices and I stick a firmware file into /lib/firmware, should it update both devices?

Shouldn't reflashing the device firmware be something that the users asks for by something a little more specific than just creating a file? (In any case, creating /lib/firmware/nvmhci-3 to flash slot 3 seems rather silly.)

Quoted 2 lines

How would the driver know that it should call request_firmware()? Do it every 60 seconds in case somebody's downloaded some new firmware?

Ick, no, just use the function provided that lets you create a firmware request and be notified when it is written to, request_firmware_nowait(). That is what it is there for.

Anyway, just use request_firmware_nowait(), you will be fine.

From a security perspective, a correctness perspective, for reliability

On Thu, 3 Mar 2011 14:22:26 -0800

Quoted 6 lines

Quoted 2 lines

How would the driver know that it should call request_firmware()? Do it every 60 seconds in case somebody's downloaded some new firmware?

Ick, no, just use the function provided that lets you create a firmware request and be notified when it is written to, request_firmware_nowait(). That is what it is there for.

Bunkum

It's there for automated loading of needed firmware

Quoted 1 line

Anyway, just use request_firmware_nowait(), you will be fine.

Not really. You've got no way with the request_firmware interface for the user space and kernel space to reliably time objects together eg to do user side work before a firmware load.

Almost all the other stuff in the kernel which is for firmware programming as opposed to automatic loading is not using request_firmware
- and for good reasons like needing to be able to specify the path to the object reliably - which cannot be done with namespaces.

From a security perspective, a correctness perspective, for reliability and managability request_firmware is the wrong interface for flashing new firmware.

And non-automated loading of firmware as well. Dell uses this for updating their BIOSes just fine, with a userspace tool that initiates the loading of the firmware.

How does Dell do it?

So, what could be changed in the current firmware interface to fix this problem in a manner which would solve these perceived issues?

Quoted 3 lines

And non-automated loading of firmware as well. Dell uses this for updating their BIOSes just fine, with a userspace tool that initiates the loading of the firmware.

Try using the Dell tool with namespaces.

Quoted 1 line

How does Dell do it?

How do most other apps do it.

Quoted 2 lines

So, what could be changed in the current firmware interface to fix this problem in a manner which would solve these perceived issues?

I'm not sure it can. The basis of the interface is driver requests firmware. That is done by using a pathname which in a namespaced environment isn't global and has races

(Several things break quite spectacularly if you request_firmware while updating a package, but of course nobody considered such details even for automatic stuff in many cases. Really there is some locking needed).

For manual updating of a block of firmware the interface most stuff wants is

copy_from_user()

or if you wanted to wrap it up nicely

	x = vmalloc_from_user(void __user *ptr, ssize_t len);

The app knows which firmware it is talking about and can inspect and compare it while holding an open file handle on the deivce. That protects against hotplug races and getting the wrong device second access, and ensures that the firmware, device and userspace are all talking about exactly the same thing.

It would nice to say "its an obscure corner case that will just error", but far too much hardware still gets semi permanently (or permanently) converted into junk if you goof a permanent flash firmware update.

Quoted 1 line

How do most other apps do it.

They write to the sysfs file with the firmware when they feel like it.

Quoted 6 lines

Quoted 2 lines

So, what could be changed in the current firmware interface to fix this problem in a manner which would solve these perceived issues?

I'm not sure it can. The basis of the interface is driver requests firmware. That is done by using a pathname which in a namespaced environment isn't global and has races

Of course, but those races don't show up in real systems, right?

Quoted 3 lines

(Several things break quite spectacularly if you request_firmware while updating a package, but of course nobody considered such details even for automatic stuff in many cases. Really there is some locking needed).

I've never heard of this race before as you usually do firmware upload either at boot time, or when a user specifically asks for it. Neither of those times is when packages are usually getting updated.

Quoted 10 lines

For manual updating of a block of firmware the interface most stuff wants is

copy_from_user()

or if you wanted to wrap it up nicely

	x = vmalloc_from_user(void __user *ptr, ssize_t len);

Quoted 5 lines

The app knows which firmware it is talking about and can inspect and compare it while holding an open file handle on the deivce. That protects against hotplug races and getting the wrong device second access, and ensures that the firmware, device and userspace are all talking about exactly the same thing.

But you do get this type of buffer from the firware interface to your driver today, right?

Still, I don't want this to all of a sudden be "open season" for every individual driver deciding to want to create an ioctl call for firmware updating just because the authors don't like the existing in-kernel interface. Please fix up the in-kernel one instead to meet your needs.

Quoted 3 lines

Quoted 1 line

How do most other apps do it.

They write to the sysfs file with the firmware when they feel like it.

Very few seem to do this. Quite a lot also have a much more complex interaction anyway than 'firmware please', 'zap' , 'ok' to be fair.

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

So, what could be changed in the current firmware interface to fix this problem in a manner which would solve these perceived issues?

Quoted 5 lines

Quoted 3 lines

I'm not sure it can. The basis of the interface is driver requests firmware. That is done by using a pathname which in a namespaced environment isn't global and has races

Of course, but those races don't show up in real systems, right?

They do. That's why I know they exist. If enough people use your distro then eventually someone catches it.

Quoted 7 lines

Quoted 3 lines

(Several things break quite spectacularly if you request_firmware while updating a package, but of course nobody considered such details even for automatic stuff in many cases. Really there is some locking needed).

I've never heard of this race before as you usually do firmware upload either at boot time, or when a user specifically asks for it. Neither of those times is when packages are usually getting updated.

Usually is not a good answer for correctness and security with firmware. It's really really not a good answer when flashing new firmware. "Usually your expensive hardware is not turned into a valueless brick" lacks a certain something.

For dynamically requested firmware the request_firmware interface is excellent stuff, and the right way to fix the races is to lock between the daemon and package manager. For flashing stuff it's not up to the job.

Quoted 9 lines

Quoted 9 lines

For manual updating of a block of firmware the interface most stuff wants is

copy_from_user()

or if you wanted to wrap it up nicely

	x = vmalloc_from_user(void __user *ptr, ssize_t len);

Quoted 8 lines

Quoted 5 lines

The app knows which firmware it is talking about and can inspect and compare it while holding an open file handle on the deivce. That protects against hotplug races and getting the wrong device second access, and ensures that the firmware, device and userspace are all talking about exactly the same thing.

But you do get this type of buffer from the firware interface to your driver today, right?

Providing you jump through a small army of hoops, get the right sysfs nodes, dodge any race conditions, hotplug and the like yes. But it should be robust and simple. Using request_firmware for this case is neither robust nor simple, nor is it easy to get the sysfs/driver open/hotplug race handling right so instead of the kernel code being very occasionally buggy (which we can spot) the user apps will be horribly buggy and we don't read those so your push for a complex mis-standard in the kernel actually makes the problem far worse than it would be without.

Quoted 4 lines

Still, I don't want this to all of a sudden be "open season" for every individual driver deciding to want to create an ioctl call for firmware updating just because the authors don't like the existing in-kernel interface. Please fix up the in-kernel one instead to meet your needs.

You are not creating an open season. The every driver having its own ioctl for firmware updating is the norm, and its in some ways a rather good norm because when it comes to flash updating there isn't much standardisation and you don't want standardisation as it just asks for the wrong tool to work in an unfortunate race or user mix-up. Flash updating is special - it's good that one app doesn't work on another device !

You could have a standardised helper easily enough I guess. Pick a standard struct and firmware descriptor and provide a

request_firmware_from_user(struct firmware_something __user *fw)

which hands back stuff in the form the rest of the firmware logic likes to play and has a standard user side struct format.

Ok, I guess I must be doing support for distros that never get used :)

Have pointers to bug reports?

Quoted 7 lines

You could have a standardised helper easily enough I guess. Pick a standard struct and firmware descriptor and provide a

request_firmware_from_user(struct firmware_something __user *fw)

which hands back stuff in the form the rest of the firmware logic likes to play and has a standard user side struct format.

Ok, patches gladly accepted :)

There's a bit of an impedence mismatch there. Think of this as being drive firmware instead of controller firmware. This isn't for request_firmware() kind of uses, it's for some admin tool to come along and tell the drive "Oh, here's some new firmware for you".

drivers/block/nvme.c new file +14 -0

new file mode 100644
index 0000000..a8549df
@@ -0,0 +1,1622 @@
+/*
+ * NVM Express device driver
+ * Copyright (c) 2011, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with

+/**
+ * alloc_cmdid - Allocate a Command ID
+ * @param nvmeq The queue that will be used for this command
+ * @param ctx A pointer that will be passed to the handler
+ * @param handler The ID of the handler to call
+ *

+ * Allocate a Command ID for a queue. The data passed in will
+ * be passed to the completion handler. This is implemented by using
+ * the bottom two bits of the ctx pointer to store the handler ID.
+ * Passing in a pointer that's not 4-byte aligned will cause a BUG.
+ * We can change this if it becomes a problem.
+ */

drivers/block/nvme.c new file +23 -0

+static int alloc_cmdid(struct nvme_queue *nvmeq, void *ctx, int handler,
+							unsigned timeout)
+{
+	int depth = nvmeq->q_depth - 1;
+	struct nvme_cmd_info *info = nvme_cmd_info(nvmeq);
+	int cmdid;
+
+	BUG_ON((unsigned long)ctx & 3);
+
+	do {
+		cmdid = find_first_zero_bit(nvmeq->cmdid_data, depth);
+		if (cmdid >= depth)
+			return -EBUSY;
+	} while (test_and_set_bit(cmdid, nvmeq->cmdid_data));
+
+	info[cmdid].ctx = (unsigned long)ctx | handler;
+	info[cmdid].timeout = jiffies + timeout;
+	return cmdid;
+}
+
+static int alloc_cmdid_killable(struct nvme_queue *nvmeq, void *ctx,
+						int handler, unsigned timeout)
+{

What's the reason to make this a make_request based driver? That loses all the intelligence that has been put into the queueing layer, like I/O bandwith controlling and fair scheduling. Is it just queue_lock contention? If so it would be great if you could help with testing Jens' stack-plug and request allocation scalability patches.

drivers/block/nvme.c

Quoted 9 lines · Matthew Wilcox

drivers/block/nvme.c new file +9 -0

+
+static struct nvme_queue *get_nvmeq(struct nvme_ns *ns)
+{
+	int qid, cpu = get_cpu();
+	if (cpu < ns->dev->queue_count)
+		qid = cpu + 1;
+	else
+		qid = (cpu % rounddown_pow_of_two(ns->dev->queue_count))
+ 1;

This will be likely a full divide, better use a mask.

Quoted 4 lines

drivers/block/nvme.c new file +4 -0

+	nprps = DIV_ROUND_UP(length, PAGE_SIZE);
+	npages = DIV_ROUND_UP(8 * nprps, PAGE_SIZE);
+	prps = kmalloc(sizeof(*prps) + sizeof(__le64 *) * npages, GFP_ATOMIC);
+	prp_page = 0;

drivers/block/nvme.c line 367

Quoted 3 lines

drivers/block/nvme.c new file +3 -0

+	if (nprps <= (256 / 8)) {
+		pool = dev->prp_small_pool;
+		prps->npages = 0;

Unchecked GFP_ATOMIC allocation? That will oops soon. Besides GFP_ATOMIC a very risky thing to do on a low memory situation, which can trigger writeouts.

Is there a good API to iterate through each socket, then each core in a socket, then each HT sibling? eg, if I have 20 queues and 2x6x2 CPUs,

I want to assign at least one queue to each core; some threads will get their own queues and others will have to share with their HT sibling.

Quoted 13 lines

drivers/block/nvme.c line 367

Quoted 8 lines

drivers/block/nvme.c new file +7 -0

+	nprps = DIV_ROUND_UP(length, PAGE_SIZE);
+	npages = DIV_ROUND_UP(8 * nprps, PAGE_SIZE);
+	prps = kmalloc(sizeof(*prps) + sizeof(__le64 *) * npages, GFP_ATOMIC);
+	prp_page = 0;
+	if (nprps <= (256 / 8)) {
+		pool = dev->prp_small_pool;
+		prps->npages = 0;

Unchecked GFP_ATOMIC allocation? That will oops soon. Besides GFP_ATOMIC a very risky thing to do on a low memory situation, which can trigger writeouts.

Ah yes, thank you. There are a few other places like this. Bizarrely, they've not oopsed during the xfstests runs.

My plan for this is, instead of using a mempool, to submit partial I/Os in the rare cases where a write cannot allocate memory. I have the design in my head, just not committed to code yet. The design also avoids allocating any memory in the driver for I/Os that do not cross a page boundary.

You may have seen the recent announcement of the NVM Express spec; here's the driver. I've put a git tree up on kernel.org with all the history that's fit to be released (earlier versions were full of Intel codenames and are not suited to public consumption). If you'd rather review the history, it's at
http://git.kernel.org/?p=linux/kernel/git/willy/nvme.git;a=summary

drivers/block/nvme.c new file +12 -0

+static int nvme_ioctl(struct block_device *bdev, fmode_t mode, unsigned int cmd,
+                                                       unsigned long arg)
+{
+       struct nvme_ns *ns = bdev->bd_disk->private_data;
+
+       switch (cmd) {
+       case NVME_IOCTL_IDENTIFY_NS:
+               return nvme_identify(ns, arg, 0);
+       case NVME_IOCTL_IDENTIFY_CTRL:
+               return nvme_identify(ns, arg, 1);
+       case NVME_IOCTL_GET_RANGE_TYPE:
+               return nvme_get_range_type(ns, arg);