nvme: utilize two queue maps, one for reads and one for writes

Quoted 33 lines · Guenter Roeck

Quoted 32 lines · Jens Axboe

Quoted 22 lines · Guenter Roeck

Hi,

Quoted 15 lines · Jens Axboe

NVMe does round-robin between queues by default, which means that sharing a queue map for both reads and writes can be problematic in terms of read servicing. It's much easier to flood the queue with writes and reduce the read servicing.

Implement two queue maps, one for reads and one for writes. The write queue count is configurable through the 'write_queues' parameter.

By default, we retain the previous behavior of having a single queue set, shared between reads and writes. Setting 'write_queues' to a non-zero value will create two queue sets, one for reads and one for writes, the latter using the configurable number of queues (hardware queue counts permitting).

Quoted 3 lines

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Keith Busch <[email protected]>

Signed-off-by Jens Axboe <[email protected]>

Quoted 5 lines

This patch causes hangs when running recent versions of -next with several architectures; see the -next column at kerneltests.org/builders for details. Bisect log below; this was run with qemu on alpha. Reverting this patch as well as "nvme: add separate poll queue map" fixes the problem.

I don't see anything related to what hung, the trace, and so on. Can you clue me in? Where are the test results with dmesg?

Quoted 5 lines

alpha just stalls during boot. parisc reports a hung task in nvme_reset_work. sparc64 reports EIO when instantiating the nvme driver, called from nvme_reset_work, and then stalls. In all three cases, reverting the two mentioned patches fixes the problem.

I think the below patch should fix it.

Quoted 34 lines

https://kerneltests.org/builders/qemu-parisc-next/builds/173/steps/qemubuildcommand_1/logs/stdio

is an example log for parisc.

I didn't check if the other boot failures (ppc looks bad) have the same root cause.

Quoted 2 lines

How to reproduce?

parisc:

qemu-system-hppa -kernel vmlinux -no-reboot \
	-snapshot -device nvme,serial=foo,drive=d0 \
	-drive file=rootfs.ext2,if=none,format=raw,id=d0 \
	-append 'root=/dev/nvme0n1 rw rootwait panic=-1 console=ttyS0,115200 ' \
	-nographic -monitor null

alpha:

qemu-system-alpha -M clipper -kernel arch/alpha/boot/vmlinux -no-reboot \
	-snapshot -device nvme,serial=foo,drive=d0 \
	-drive file=rootfs.ext2,if=none,format=raw,id=d0 \
	-append 'root=/dev/nvme0n1 rw rootwait panic=-1 console=ttyS0' \
	-m 128M -nographic -monitor null -serial stdio

sparc64:

qemu-system-sparc64 -M sun4u -cpu 'TI UltraSparc IIi' -m 512 \
	-snapshot -device nvme,serial=foo,drive=d0,bus=pciB \
	-drive file=rootfs.ext2,if=none,format=raw,id=d0 \
	-kernel arch/sparc/boot/image -no-reboot \
	-append 'root=/dev/nvme0n1 rw rootwait panic=-1 console=ttyS0' \
	-nographic -monitor none

Quoted 4 lines

The root file systems are available from the respective subdirectories of:

https://github.com/groeck/linux-build-test/tree/master/rootfs

This is useful, thanks! I haven't tried it yet, but I was able to reproduce on x86 with MSI turned off.

drivers/nvme/host/pci.c +1 -1

index 8df868afa363..6c03461ad988 100644
@@ -2098,7 +2098,7 @@ static int nvme_setup_irqs(struct nvme_dev *dev, int nr_io_queues)
 		.nr_sets = ARRAY_SIZE(irq_sets),
 		.sets = irq_sets,
 	};
-	int result;
+	int result = 0;
 
 	/*
 	 * For irq sets, we have to ask for minvec == maxvec. This passes

@@ -2113,9 +2113,16 @@ static int nvme_setup_irqs(struct nvme_dev *dev, int nr_io_queues)
 			affd.nr_sets = 1;
 
 		/*

drivers/nvme/host/pci.c +2 -0

+		else
+			nr_io_queues = irq_sets[0] + irq_sets[1] + 1;
 
 		result = pci_alloc_irq_vectors_affinity(pdev, nr_io_queues,
 				nr_io_queues,

Signature · 2 lines

-- 
Jens Axboe

Hi Jens,

Quoted 2 lines

I think the below patch should fix it.

Sorry I wasn't able to test this earlier. Looks like it does fix the problem; the problem is no longer seen in next-20181115. Minor comment below.

Quoted 6 lines

(quoted diff) +4 -1

 		/*
-		 * Need IRQs for read+write queues, and one for the admin queue
+		 * Need IRQs for read+write queues, and one for the admin queue.
+		 * If we can't get more than one vector, we have to share the
+		 * admin queue and IO queue vector. For that case, don't add
+		 * an extra vector for the admin queue, or we'll continue

Review

Quoted 5 lines

(quoted diff) +3 -1

+		 * asking for 2 and get -ENOSPC in return.
 		 */
-		nr_io_queues = irq_sets[0] + irq_sets[1] + 1;
+		if (result == -ENOSPC && nr_io_queues == 1)
+			nr_io_queues = 1;

Setting nr_io_queues to 1 when it already is set to 1 doesn't really do anything. Is this for clarification ?