Commit 1d77eec8 authored by Konrad Sztyber's avatar Konrad Sztyber Committed by Tomasz Zawadzki
Browse files

nvme: don't pack spdk_nvme_ns_cmd_ext_io_opts



We're often using structure packing throughout the code for the *_opts
structs to make sure their size will always change whenever a new field
is added.  However, packing a structure has a couple of drawbacks:
__attribute__((packed)) is a gcc extension and isn't supported on some
compilers (e.g. msvc), and taking the address of a member field might
lead to Waddress-of-packed-member warnings.

Instead, pad the structure manually and use a new macro, SPDK_SIZEOF(),
to calculate the size of the structure.  The macro takes two parameters:
pointer to a structure and the name of the last member in that
structure.  It returns the size of the structure up to and including
that member.  That way, even though a structure isn't packed, the size
calculated using SPDK_SIZEOF() won't include the padding at the end.

This method has one additional benefit.  When a structure in version X
looks like this:

struct spdk_foo {
	size_t size;
	char bar;
};

and is used as:

struct spdk_foo foo = {};

foo.size = /* sizeof(foo) or SPDK_SIZEOF(&foo, bar) */;
foo.bar = 'b';

changes in version X+1 to:

struct spdk_foo {
	size_t size;
	char bar;
	char baz;
};

and the code using it is recompiled without any changes.  If it uses
sizeof() foo.baz will be implicitly zero-initialized, while if it uses
SPDK_SIZEOF(), the library that foo is passed to will see that foo.baz
is unassigned and will be able to pick any value as the default.

Signed-off-by: default avatarKonrad Sztyber <konrad.sztyber@intel.com>
Change-Id: I049baf4db0cd08d7cc728e31eda2ee6bad47b264
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/18530


Community-CI: Mellanox Build Bot
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarTomasz Zawadzki <tomasz.zawadzki@intel.com>
parent 075d422f
Loading
Loading
Loading
Loading
+1 −0
Original line number Diff line number Diff line
@@ -1108,6 +1108,7 @@ spdk_fio_queue(struct thread_data *td, struct io_u *io_u)
			if (!fio_qpair->zone_append_enabled) {
#if FIO_HAS_FDP
				if (spdk_unlikely(io_u->dtype)) {
					ext_opts.size = SPDK_SIZEOF(&ext_opts, cdw13);
					ext_opts.io_flags = fio_qpair->io_flags | (io_u->dtype << 20);
					ext_opts.metadata = md_buf;
					ext_opts.cdw13 = (io_u->dspec << 16);
+2 −2
Original line number Diff line number Diff line
@@ -577,7 +577,7 @@ enum spdk_nvme_ctrlr_flags {
 * Structure with optional IO request parameters
 */
struct spdk_nvme_ns_cmd_ext_io_opts {
	/** size of this structure in bytes */
	/** size of this structure in bytes, use SPDK_SIZEOF(opts, last_member) to obtain it */
	size_t size;
	/** Memory domain which describes data payload in IO request. The controller must support
	 * the corresponding memory domain type, refer to \ref spdk_nvme_ctrlr_get_memory_domains */
@@ -596,7 +596,7 @@ struct spdk_nvme_ns_cmd_ext_io_opts {
	uint16_t apptag;
	/** Command dword 13 specific field. */
	uint32_t cdw13;
} __attribute__((packed));
};
SPDK_STATIC_ASSERT(sizeof(struct spdk_nvme_ns_cmd_ext_io_opts) == 48, "Incorrect size");

/**
+3 −0
Original line number Diff line number Diff line
@@ -28,6 +28,9 @@ extern "C" {

#define SPDK_CONTAINEROF(ptr, type, member) ((type *)((uintptr_t)ptr - offsetof(type, member)))

/** Returns size of an object pointer by ptr up to and including member */
#define SPDK_SIZEOF(ptr, member) (offsetof(__typeof__(*(ptr)), member) + sizeof((ptr)->member))

/**
 * Get the size of a member of a struct.
 */
+2 −2
Original line number Diff line number Diff line
@@ -7264,7 +7264,7 @@ bdev_nvme_readv(struct nvme_bdev_io *bio, struct iovec *iov, int iovcnt,
	bio->iov_offset = 0;

	if (domain != NULL) {
		bio->ext_opts.size = sizeof(struct spdk_nvme_ns_cmd_ext_io_opts);
		bio->ext_opts.size = SPDK_SIZEOF(&bio->ext_opts, cdw13);
		bio->ext_opts.memory_domain = domain;
		bio->ext_opts.memory_domain_ctx = domain_ctx;
		bio->ext_opts.io_flags = flags;
@@ -7310,7 +7310,7 @@ bdev_nvme_writev(struct nvme_bdev_io *bio, struct iovec *iov, int iovcnt,
	bio->iov_offset = 0;

	if (domain != NULL) {
		bio->ext_opts.size = sizeof(struct spdk_nvme_ns_cmd_ext_io_opts);
		bio->ext_opts.size = SPDK_SIZEOF(&bio->ext_opts, cdw13);
		bio->ext_opts.memory_domain = domain;
		bio->ext_opts.memory_domain_ctx = domain_ctx;
		bio->ext_opts.io_flags = flags;
+2 −1
Original line number Diff line number Diff line
@@ -6,6 +6,7 @@
#include "spdk/stdinc.h"
#include "spdk/nvme.h"
#include "spdk/env.h"
#include "spdk/util.h"

#define FDP_LOG_PAGE_SIZE		4096
#define FDP_NR_RUHS_DESC		256
@@ -285,7 +286,7 @@ check_fdp_write(struct spdk_nvme_ns *ns, struct spdk_nvme_qpair *qpair)
	g_outstanding_commands = 0;
	g_fdp_command_result = -1;

	ext_opts.size = sizeof(struct spdk_nvme_ns_cmd_ext_io_opts);
	ext_opts.size = SPDK_SIZEOF(&ext_opts, cdw13);
	ext_opts.io_flags = SPDK_NVME_IO_FLAGS_DATA_PLACEMENT_DIRECTIVE;
	ext_opts.metadata = NULL;
	ext_opts.cdw13 = (pid_for_ruhu << 16);
Loading