Commit 4249dc10 authored by Niklas Cassel's avatar Niklas Cassel Committed by Tomasz Zawadzki
Browse files

nvme: account for PRACT when calculating max sectors per transfer 



There is a special case when using 8-byte metadata + PI + PRACT
where no metadata is transferred to/from controller.

Since _nvme_ns_cmd_rw() already calculates the proper sector size
using _nvme_get_host_buffer_sector_size(), which takes PRACT into
account, change the sectors_per_max_io calculation to also take
PRACT into account.

This will avoid certain requests that don't need splitting getting
split.

Signed-off-by: default avatarNiklas Cassel <niklas.cassel@wdc.com>
Change-Id: I8d450d37c2458453701189f0e0eca4b8fe71173b
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/6247


Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarAleksey Marchuk <alexeymar@mellanox.com>
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
parent b7c33b5e

lib/nvme/nvme_internal.h

+1 −0
Original line number Diff line number Diff line
@@ -481,6 +481,7 @@ struct spdk_nvme_ns { 
	uint32_t			md_size;

	uint32_t			pi_type;

	uint32_t			sectors_per_max_io;

	uint32_t			sectors_per_max_io_no_md;

	uint32_t			sectors_per_stripe;

	uint32_t			id;

	uint16_t			flags;

lib/nvme/nvme_ns.c

+2 −0
Original line number Diff line number Diff line
@@ -64,6 +64,7 @@ nvme_ns_set_identify_data(struct spdk_nvme_ns *ns) 
	}



	ns->sectors_per_max_io = spdk_nvme_ns_get_max_io_xfer_size(ns) / ns->extended_lba_size;

	ns->sectors_per_max_io_no_md = spdk_nvme_ns_get_max_io_xfer_size(ns) / ns->sector_size;



	if (nsdata->noiob) {

		ns->sectors_per_stripe = nsdata->noiob;
@@ -556,6 +557,7 @@ void nvme_ns_destruct(struct spdk_nvme_ns *ns) 
	ns->md_size = 0;

	ns->pi_type = 0;

	ns->sectors_per_max_io = 0;

	ns->sectors_per_max_io_no_md = 0;

	ns->sectors_per_stripe = 0;

	ns->flags = 0;

	ns->csi = SPDK_NVME_CSI_NVM;

lib/nvme/nvme_ns_cmd.c

+18 −10
Original line number Diff line number Diff line
@@ -62,19 +62,27 @@ nvme_ns_check_request_length(uint32_t lba_count, uint32_t sectors_per_max_io, 
	return child_per_io >= qdepth;

}



static inline uint32_t

_nvme_get_host_buffer_sector_size(struct spdk_nvme_ns *ns, uint32_t io_flags)

static inline bool

_nvme_md_excluded_from_xfer(struct spdk_nvme_ns *ns, uint32_t io_flags)

{

	uint32_t sector_size = ns->extended_lba_size;



	if ((io_flags & SPDK_NVME_IO_FLAGS_PRACT) &&

	return (io_flags & SPDK_NVME_IO_FLAGS_PRACT) &&

	       (ns->flags & SPDK_NVME_NS_EXTENDED_LBA_SUPPORTED) &&

	       (ns->flags & SPDK_NVME_NS_DPS_PI_SUPPORTED) &&

	    (ns->md_size == 8)) {

		sector_size -= 8;

	       (ns->md_size == 8);

}



	return sector_size;

static inline uint32_t

_nvme_get_host_buffer_sector_size(struct spdk_nvme_ns *ns, uint32_t io_flags)

{

	return _nvme_md_excluded_from_xfer(ns, io_flags) ?

	       ns->sector_size : ns->extended_lba_size;

}



static inline uint32_t

_nvme_get_sectors_per_max_io(struct spdk_nvme_ns *ns, uint32_t io_flags)

{

	return _nvme_md_excluded_from_xfer(ns, io_flags) ?

	       ns->sectors_per_max_io_no_md : ns->sectors_per_max_io;

}



static struct nvme_request *
@@ -393,7 +401,7 @@ _nvme_ns_cmd_rw(struct spdk_nvme_ns *ns, struct spdk_nvme_qpair *qpair, 
{

	struct nvme_request	*req;

	uint32_t		sector_size = _nvme_get_host_buffer_sector_size(ns, io_flags);

	uint32_t		sectors_per_max_io = ns->sectors_per_max_io;

	uint32_t		sectors_per_max_io = _nvme_get_sectors_per_max_io(ns, io_flags);

	uint32_t		sectors_per_stripe = ns->sectors_per_stripe;



	req = nvme_allocate_request(qpair, payload, lba_count * sector_size, lba_count * ns->md_size,

test/unit/lib/nvme/nvme_ns_cmd.c/nvme_ns_cmd_ut.c

+16 −48
Original line number Diff line number Diff line
@@ -240,6 +240,7 @@ prepare_for_test(struct spdk_nvme_ns *ns, struct spdk_nvme_ctrlr *ctrlr, 
	}

	ns->md_size = md_size;

	ns->sectors_per_max_io = spdk_nvme_ns_get_max_io_xfer_size(ns) / ns->extended_lba_size;

	ns->sectors_per_max_io_no_md = spdk_nvme_ns_get_max_io_xfer_size(ns) / ns->sector_size;

	ns->sectors_per_stripe = stripe_size / ns->extended_lba_size;



	memset(qpair, 0, sizeof(*qpair));
@@ -909,11 +910,7 @@ test_nvme_ns_cmd_comparev_with_md(void) 
	 * Protection information enabled + PRACT

	 *

	 * Special case for 8-byte metadata + PI + PRACT: no metadata transferred

	 * In theory, 256 blocks * 512 bytes per block = one I/O (128 KB)

	 * However, the splitting code does not account for PRACT when calculating

	 * max sectors per transfer, so we actually get two I/Os:

	 *   child 0: 252 blocks

	 *   child 1: 4 blocks

	 * 256 blocks * 512 bytes per block = single 128 KB I/O (no splitting required)

	 */

	prepare_for_test(&ns, &ctrlr, &qpair, 512, 8, 128 * 1024, 0, true);

	ns.flags |= SPDK_NVME_NS_DPS_PI_SUPPORTED;
@@ -923,18 +920,11 @@ test_nvme_ns_cmd_comparev_with_md(void) 


	SPDK_CU_ASSERT_FATAL(rc == 0);

	SPDK_CU_ASSERT_FATAL(g_request != NULL);

	SPDK_CU_ASSERT_FATAL(g_request->num_children == 2);

	child0 = TAILQ_FIRST(&g_request->children);



	SPDK_CU_ASSERT_FATAL(child0 != NULL);

	CU_ASSERT(child0->payload_offset == 0);

	CU_ASSERT(child0->payload_size == 252 * 512); /* NOTE: does not include metadata! */

	child1 = TAILQ_NEXT(child0, child_tailq);

	SPDK_CU_ASSERT_FATAL(g_request->num_children == 0);



	SPDK_CU_ASSERT_FATAL(child1 != NULL);

	CU_ASSERT(child1->payload.md == NULL);

	CU_ASSERT(child1->payload_offset == 252 * 512);

	CU_ASSERT(child1->payload_size == 4 * 512);

	CU_ASSERT(g_request->payload.md == NULL);

	CU_ASSERT(g_request->payload_offset == 0);

	CU_ASSERT(g_request->payload_size == 256 * 512); /* NOTE: does not include metadata! */



	nvme_request_free_children(g_request);

	nvme_free_request(g_request);
@@ -1373,11 +1363,7 @@ test_nvme_ns_cmd_write_with_md(void) 
	 * Protection information enabled + PRACT

	 *

	 * Special case for 8-byte metadata + PI + PRACT: no metadata transferred

	 * In theory, 256 blocks * 512 bytes per block = one I/O (128 KB)

	 * However, the splitting code does not account for PRACT when calculating

	 * max sectors per transfer, so we actually get two I/Os:

	 *   child 0: 252 blocks

	 *   child 1: 4 blocks

	 * 256 blocks * 512 bytes per block = single 128 KB I/O (no splitting required)

	 */

	prepare_for_test(&ns, &ctrlr, &qpair, 512, 8, 128 * 1024, 0, true);

	ns.flags |= SPDK_NVME_NS_DPS_PI_SUPPORTED;
@@ -1387,18 +1373,11 @@ test_nvme_ns_cmd_write_with_md(void) 


	SPDK_CU_ASSERT_FATAL(rc == 0);

	SPDK_CU_ASSERT_FATAL(g_request != NULL);

	SPDK_CU_ASSERT_FATAL(g_request->num_children == 2);

	child0 = TAILQ_FIRST(&g_request->children);



	SPDK_CU_ASSERT_FATAL(child0 != NULL);

	CU_ASSERT(child0->payload_offset == 0);

	CU_ASSERT(child0->payload_size == 252 * 512); /* NOTE: does not include metadata! */

	child1 = TAILQ_NEXT(child0, child_tailq);

	SPDK_CU_ASSERT_FATAL(g_request->num_children == 0);



	SPDK_CU_ASSERT_FATAL(child1 != NULL);

	CU_ASSERT(child1->payload.md == NULL);

	CU_ASSERT(child1->payload_offset == 252 * 512);

	CU_ASSERT(child1->payload_size == 4 * 512);

	CU_ASSERT(g_request->payload.md == NULL);

	CU_ASSERT(g_request->payload_offset == 0);

	CU_ASSERT(g_request->payload_size == 256 * 512); /* NOTE: does not include metadata! */



	nvme_request_free_children(g_request);

	nvme_free_request(g_request);
@@ -1758,11 +1737,7 @@ test_nvme_ns_cmd_compare_with_md(void) 
	 * Protection information enabled + PRACT

	 *

	 * Special case for 8-byte metadata + PI + PRACT: no metadata transferred

	 * In theory, 256 blocks * 512 bytes per block = one I/O (128 KB)

	 * However, the splitting code does not account for PRACT when calculating

	 * max sectors per transfer, so we actually get two I/Os:

	 *   child 0: 252 blocks

	 *   child 1: 4 blocks

	 * 256 blocks * 512 bytes per block = single 128 KB I/O (no splitting required)

	 */

	prepare_for_test(&ns, &ctrlr, &qpair, 512, 8, 128 * 1024, 0, true);

	ns.flags |= SPDK_NVME_NS_DPS_PI_SUPPORTED;
@@ -1772,18 +1747,11 @@ test_nvme_ns_cmd_compare_with_md(void) 


	SPDK_CU_ASSERT_FATAL(rc == 0);

	SPDK_CU_ASSERT_FATAL(g_request != NULL);

	SPDK_CU_ASSERT_FATAL(g_request->num_children == 2);

	child0 = TAILQ_FIRST(&g_request->children);



	SPDK_CU_ASSERT_FATAL(child0 != NULL);

	CU_ASSERT(child0->payload_offset == 0);

	CU_ASSERT(child0->payload_size == 252 * 512); /* NOTE: does not include metadata! */

	child1 = TAILQ_NEXT(child0, child_tailq);

	SPDK_CU_ASSERT_FATAL(g_request->num_children == 0);



	SPDK_CU_ASSERT_FATAL(child1 != NULL);

	CU_ASSERT(child1->payload.md == NULL);

	CU_ASSERT(child1->payload_offset == 252 * 512);

	CU_ASSERT(child1->payload_size == 4 * 512);

	CU_ASSERT(g_request->payload.md == NULL);

	CU_ASSERT(g_request->payload_offset == 0);

	CU_ASSERT(g_request->payload_size == 256 * 512); /* NOTE: does not include metadata! */



	nvme_request_free_children(g_request);

	nvme_free_request(g_request);