Skip to contentSkip to conversation

NVMe: Fix IO for extended metadata formats

PATCH New Keith Busch 3 participants 5 messages Jun 19, 2015 – Jun 22, 2015

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 NVMe: Fix IO for extended metadata formats

This fixes io submit ioctl handling when using extended metadata formats. When these formats are used, the user provides a single virtually contiguous buffer containing both the block and metadata interleaved, so the metadata size needs to be added to the total length and not mapped as a separate transfer.

The command is also driver generated, so this patch does not enforce blk-integrity extensions provide the metadata buffer.

Reported-by Marcin Dziegielewski <marcin.dziegielewski at intel.com>
Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/block/nvme-core.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/block/nvme-core.c +6 -6
index a230b90..f225da7 100644
@@ -862,7 +862,8 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 	 * stripped/generated by the controller with PRACT=1.
 	 */
 	if (ns && ns->ms && !blk_integrity_rq(req)) {
-		if (!(ns->pi_type && ns->ms == 8)) {
+		if (!(ns->pi_type && ns->ms == 8) &&
+					req->cmd_type != REQ_TYPE_DRV_PRIV) {
 			req->errors = -EFAULT;
 			blk_mq_complete_request(req);
 			return BLK_MQ_RQ_QUEUE_OK;
@@ -1759,15 +1760,14 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 	meta_len = (io.nblocks + 1) * ns->ms;
 	write = io.opcode & 1;
 
+	if (ns->ext) {
+		length += meta_len;
+		meta_len = 0;
+	}
 	if (meta_len) {
 		if (((io.metadata & 3) || !io.metadata) && !ns->ext)
 			return -EINVAL;
 
-		if (ns->ext) {
-			length += meta_len;
-			meta_len = 0;
-		}
-
 		meta = dma_alloc_coherent(dev->dev, meta_len,
 						&meta_dma, GFP_KERNEL);
 		if (!meta) {
Signature · 1 line
1.7.10.4
#1 lore raw

PATCH 1/1 NVMe: Fix IO for extended metadata formats

Quoted 8 lines · Keith Busch

This fixes io submit ioctl handling when using extended metadata formats. When these formats are used, the user provides a single virtually contiguous buffer containing both the block and metadata interleaved, so the metadata size needs to be added to the total length and not mapped as a separate transfer.

The command is also driver generated, so this patch does not enforce blk-integrity extensions provide the metadata buffer.

Applied for 4.2.

Signature · 1 line
Jens Axboe
#2 lore raw

PATCH 1/1 NVMe: Fix IO for extended metadata formats

In reply to Keith Busch
Quoted 8 lines · Keith Busch

This fixes io submit ioctl handling when using extended metadata formats. When these formats are used, the user provides a single virtually contiguous buffer containing both the block and metadata interleaved, so the metadata size needs to be added to the total length and not mapped as a separate transfer.

The command is also driver generated, so this patch does not enforce blk-integrity extensions provide the metadata buffer.

Does this actually work properly? I'd assume the block layer might not be too happy if we have weird arbitrary sizes for this additional metadata.

#3 lore raw

PATCH 1/1 NVMe: Fix IO for extended metadata formats

Quoted 7 lines · Keith Busch
Quoted 6 lines

This fixes io submit ioctl handling when using extended metadata formats. When these formats are used, the user provides a single virtually contiguous buffer containing both the block and metadata interleaved, so the metadata size needs to be added to the total length and not mapped as a separate transfer.

Quoted 6 lines
Quoted 2 lines

The command is also driver generated, so this patch does not enforce blk-integrity extensions provide the metadata buffer.

Does this actually work properly? I'd assume the block layer might not be too happy if we have weird arbitrary sizes for this additional metadata.

Similar to SG_IO, we can submit arbitrary length commands using the nvme passthrough. It seems to work. Here's a test I did using the "nvme-cli" program.

First showing the namespace is formatted using a metadata size 8, capable of only extended settings.

  # nvme id-ns /dev/nvme0n1
  ...
  mc      : 0x1
  ...
  lbaf  1 : ms:8   ds:9  rp:0x2 (in use)
  ...

Create a random 520 byte file (512 bytes for data + 8 for metadata):

  # dd if=/dev/urandom of=rand.file.out bs=520 count=1

Write that 520 bytes using the write passthru:

  # nvme write /dev/nvme0n1 --data-size=520 --data=rand.file.out
  write success

Read it back:

  # nvme read /dev/nvme0n1 --data-size=520 --data=rand.file.in
  read success

Verify the two files are the same:

  # diff rand.file.in rand.file.out
  # echo $?
  0

Similar test using 8 blocks instead of 1:

  # dd if=/dev/urandom of=rand.file.out bs=520 count=8
  # nvme write /dev/nvme0n1 --block-count=7 --data-size=4160 --data=rand.file.out
  # nvme read /dev/nvme0n1 --block-count=7 --data-size=4160 --data=rand.file.in
  # diff rand.file.out rand.file.in
  #
#4 lore raw

PATCH 1/1 NVMe: Fix IO for extended metadata formats

Quoted 3 lines · Keith

Similar to SG_IO, we can submit arbitrary length commands using the nvme passthrough. It seems to work. Here's a test I did using the "nvme-cli" program.

Yeah, I was mostly worried about the request mapping code tripping up on these unaligned sized. I don't have a device that supports it so I couldn't test it myself.

Btw, given your test below: I had thought for a while to add unit tests to the nvme-cli package for cases like the one you had below.

The use case would be mostly to verify the driver, less the hardware unless the two are tangled up or the hardware specific cases are simple enough. I've had good succeed with the cunit based SCSI testsuite in libiscsi, so I could come up with a skeleton for it.