[PATCH] NVMe: Fix IO for extended metadata formats

[PATCH] NVMe: Fix IO for extended metadata formats

[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.

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(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index a230b90..f225da7 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -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) {
-- 
1.7.10.4

[PATCH] NVMe: Fix IO for extended metadata formats

[Jens Axboe]

On 06/19/2015 11:07 AM, Keith Busch wrote:
> 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.

-- 
Jens Axboe

[PATCH] NVMe: Fix IO for extended metadata formats

[Christoph Hellwig]

On Fri, Jun 19, 2015@11:07:30AM -0600, Keith Busch wrote:
> 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.

[PATCH] NVMe: Fix IO for extended metadata formats

[Busch, Keith]

> On Fri, Jun 19, 2015@11:07:30AM -0600, Keith Busch wrote:
> > 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.

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
  #

[PATCH] NVMe: Fix IO for extended metadata formats

[Christoph Hellwig]

On Mon, Jun 22, 2015@03:17:07PM +0000, Busch, Keith wrote:
> 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.