Re: [PATCH] multipath-tools: add ANA support for NVMe device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Lijie,

On Thu, 2018-11-08 at 14:09 +0800, lijie wrote:
> Add support for Asynchronous Namespace Access as specified in NVMe
> 1.3
> TP 4004. The states are updated through reading the ANA log page.
> 
> By default, the native nvme multipath takes over the nvme device.
> We can pass a false to the parameter 'multipath' of the nvme-core.ko
> module,when we want to use multipath-tools.

Thank you for the patch. It looks quite good to me. I've tested it with
a Linux target and found no problems so far.

I have a few questions and comments inline below.

I suggest you also have a look at detect_prio(); it seems to make sense
to use the ana prioritizer for NVMe paths automatically if ANA is
supported (with your patch, "detect_prio no" and "prio ana" have to be
configured explicitly). But that can be done in a later patch.

Martin

> ---
>  libmultipath/prio.h                |   1 +
>  libmultipath/prioritizers/Makefile |   1 +
>  libmultipath/prioritizers/ana.c    | 292
> +++++++++++++++++++++++++++++++++++++
>  libmultipath/prioritizers/ana.h    | 221
> ++++++++++++++++++++++++++++
>  multipath/multipath.conf.5         |   8 +
>  5 files changed, 523 insertions(+)
>  create mode 100644 libmultipath/prioritizers/ana.c
>  create mode 100644 libmultipath/prioritizers/ana.h
> 
> diff --git a/libmultipath/prio.h b/libmultipath/prio.h
> index aa587cc..599d1d8 100644
> --- a/libmultipath/prio.h
> +++ b/libmultipath/prio.h
> @@ -30,6 +30,7 @@ struct path;
>  #define PRIO_WEIGHTED_PATH	"weightedpath"
>  #define PRIO_SYSFS		"sysfs"
>  #define PRIO_PATH_LATENCY	"path_latency"
> +#define PRIO_ANA		"ana"
>  
>  /*
>   * Value used to mark the fact prio was not defined
> diff --git a/libmultipath/prioritizers/Makefile
> b/libmultipath/prioritizers/Makefile
> index ab7bc07..15afaba 100644
> --- a/libmultipath/prioritizers/Makefile
> +++ b/libmultipath/prioritizers/Makefile
> @@ -19,6 +19,7 @@ LIBS = \
>  	libpriordac.so \
>  	libprioweightedpath.so \
>  	libpriopath_latency.so \
> +	libprioana.so \
>  	libpriosysfs.so
>  
>  all: $(LIBS)
> diff --git a/libmultipath/prioritizers/ana.c
> b/libmultipath/prioritizers/ana.c
> new file mode 100644
> index 0000000..c5aaa5f
> --- /dev/null
> +++ b/libmultipath/prioritizers/ana.c
> @@ -0,0 +1,292 @@
> +/*
> + * (C) Copyright HUAWEI Technology Corp. 2017   All Rights Reserved.
> + *
> + * ana.c
> + * Version 1.00
> + *
> + * Tool to make use of a NVMe-feature called  Asymmetric Namespace
> Access.
> + * It determines the ANA state of a device and prints a priority
> value to stdout.
> + *
> + * Author(s): Cheng Jike <chengjike.cheng@xxxxxxxxxx>
> + *            Li Jie <lijie34@xxxxxxxxxx>
> + *
> + * This file is released under the GPL version 2, or any later
> version.
> + */
> +#include <stdio.h>
> +#include <sys/ioctl.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <stdbool.h>
> +
> +#include "debug.h"
> +#include "prio.h"
> +#include "structs.h"
> +#include "ana.h"
> +
> +enum {
> +	ANA_PRIO_OPTIMIZED		= 50,
> +	ANA_PRIO_NONOPTIMIZED		= 10,
> +	ANA_PRIO_INACCESSIBLE		= 5,
> +	ANA_PRIO_PERSISTENT_LOSS	= 1,
> +	ANA_PRIO_CHANGE			= 0,
> +	ANA_PRIO_RESERVED		= 0,
> +	ANA_PRIO_GETCTRL_FAILED		= -1,
> +	ANA_PRIO_NOT_SUPPORTED		= -2,
> +	ANA_PRIO_GETANAS_FAILED		= -3,
> +	ANA_PRIO_GETANALOG_FAILED	= -4,
> +	ANA_PRIO_GETNSID_FAILED		= -5,
> +	ANA_PRIO_GETNS_FAILED		= -6,
> +	ANA_PRIO_NO_MEMORY		= -7,
> +	ANA_PRIO_NO_INFORMATION		= -8,
> +};
> +
> +static const char * anas_string[] = {
> +	[NVME_ANA_OPTIMIZED]			= "ANA Optimized
> State",
> +	[NVME_ANA_NONOPTIMIZED]			= "ANA Non-Optimized
> State",
> +	[NVME_ANA_INACCESSIBLE]			= "ANA Inaccessible
> State",
> +	[NVME_ANA_PERSISTENT_LOSS]		= "ANA Persistent
> Loss State",
> +	[NVME_ANA_CHANGE]			= "ANA Change state",
> +	[NVME_ANA_RESERVED]			= "Invalid namespace
> group state!",
> +};
> +
> +static const char *aas_print_string(int rc)
> +{
> +	rc &= 0xff;
> +
> +	switch(rc) {
> +	case NVME_ANA_OPTIMIZED:
> +	case NVME_ANA_NONOPTIMIZED:
> +	case NVME_ANA_INACCESSIBLE:
> +	case NVME_ANA_PERSISTENT_LOSS:
> +	case NVME_ANA_CHANGE:
> +		return anas_string[rc];
> +	default:
> +		return anas_string[NVME_ANA_RESERVED];
> +	}
> +
> +	return anas_string[NVME_ANA_RESERVED];
> +}

nit: the last statement is superfluous.

> +
> +static int nvme_get_nsid(int fd, unsigned *nsid)
> +{
> +	static struct stat nvme_stat;
> +	int err = fstat(fd, &nvme_stat);
> +	if (err < 0)
> +		return 1;
> +
> +	if (!S_ISBLK(nvme_stat.st_mode)) {
> +		condlog(0, "Error: requesting namespace-id from non-
> block device\n");
> +		return 1;
> +	}
> +
> +	*nsid = ioctl(fd, NVME_IOCTL_ID);
> +	return 0;
> +}
> +
> +static int nvme_submit_admin_passthru(int fd, struct
> nvme_passthru_cmd *cmd)
> +{
> +	return ioctl(fd, NVME_IOCTL_ADMIN_CMD, cmd);
> +}
> +
> +int nvme_get_log13(int fd, __u32 nsid, __u8 log_id, __u8 lsp, __u64
> lpo,
> +                 __u16 lsi, bool rae, __u32 data_len, void *data)

lpo and lsi are 0 in all callers.  Likewise, rae is always true. You
might consider simplifying your code by omitting these paramters.

I can see that you did it this way because you have copied the code
from nvme-cli/nvme-ioctl.c, which is fine. In this case, however, we
may want to copy the entire file, in order to be able to track more
easily if our code is up-to-date. I wish something like libnvmecli
existed.

> +{
> +	struct nvme_admin_cmd cmd = {
> +		.opcode		= nvme_admin_get_log_page,
> +		.nsid		= nsid,
> +		.addr		= (__u64)(uintptr_t) data,
> +		.data_len	= data_len,
> +	};
> +	__u32 numd = (data_len >> 2) - 1;
> +	__u16 numdu = numd >> 16, numdl = numd & 0xffff;
> +
> +	cmd.cdw10 = log_id | (numdl << 16) | (rae ? 1 << 15 : 0);
> +	if (lsp)log143
> +		cmd.cdw10 |= lsp << 8;
> +
> +	cmd.cdw11 = numdu | (lsi << 16);
> +	cmd.cdw12 = lpo;
> +	cmd.cdw13 = (lpo >> 32);
> +
> +	return nvme_submit_admin_passthru(fd, &cmd);
> +
> +}
> +
> +int nvme_identify13(int fd, __u32 nsid, __u32 cdw10, __u32 cdw11,
> void *data)
> +{
> +	struct nvme_admin_cmd cmd = {
> +		.opcode		= nvme_admin_identify,
> +		.nsid		= nsid,
> +		.addr		= (__u64)(uintptr_t) data,
> +		.data_len	= NVME_IDENTIFY_DATA_SIZE,
> +		.cdw10		= cdw10,
> +		.cdw11		= cdw11,

Along similar lines as above, this could be simplified, as cdw11 is
never used.

> +	};
> +
> +	return nvme_submit_admin_passthru(fd, &cmd);
> +}
> +
> +int nvme_identify(int fd, __u32 nsid, __u32 cdw10, void *data)
> +{
> +	return nvme_identify13(fd, nsid, cdw10, 0, data);
> +}
> +
> +int nvme_identify_ctrl(int fd, void *data)
> +{
> +	return nvme_identify(fd, 0, NVME_ID_CNS_CTRL, data);
> +}
> +
> +int nvme_identify_ns(int fd, __u32 nsid, void *data)
> +{
> +	return nvme_identify(fd, nsid, NVME_ID_CNS_NS, data);
> +}
> +
> +int nvme_ana_log(int fd, void *ana_log, size_t ana_log_len, int rgo)
> +{
> +	__u64 lpo = 0;
> +
> +	return nvme_get_log13(fd, NVME_NSID_ALL, NVME_LOG_ANA, rgo,
> lpo, 0,
> +			true, ana_log_len, ana_log);
> +}
> +
> +static int get_ana_state(__u32 nsid, __u32 anagrpid, void *ana_log)
> +{
> +	int	rc = ANA_PRIO_GETANAS_FAILED;
> +	void *base = ana_log;
> +	struct nvme_ana_rsp_hdr *hdr = base;
> +	struct nvme_ana_group_desc *ana_desc;
> +	int offset = sizeof(struct nvme_ana_rsp_hdr);
> +	__u32 nr_nsids;
> +	size_t nsid_buf_size;
> +	int i, j;
> +
> +	for (i = 0; i < le16_to_cpu(hdr->ngrps); i++) {
> +		ana_desc = base + offset;
> +		nr_nsids = le32_to_cpu(ana_desc->nnsids);
> +		nsid_buf_size = nr_nsids * sizeof(__le32);
> +
> +		offset += sizeof(*ana_desc);
> +
> +		for (j = 0; j < nr_nsids; j++) {
> +			if (nsid == le32_to_cpu(ana_desc->nsids[j]))
> +				return ana_desc->state;
> +		}
> +
> +		if (anagrpid != 0 && anagrpid == le32_to_cpu(ana_desc-
> >grpid))
> +			rc = ana_desc->state;


Shouldn't you check this condition before iterating over the nsids?
And if the anagrpid matches, shouldn't you return the state
immediately?

Some comments would be helpful in order to understand this logic.

> +
> +		offset += nsid_buf_size;
> +	}
> +
> +	return rc;
> +}
> +
> +int get_ana_info(struct path * pp, unsigned int timeout)

This should be a static function.

> +{
> +	int	rc;
> +	__u32 nsid;
> +	struct nvme_id_ctrl ctrl;
> +	struct nvme_id_ns ns;
> +	void *ana_log;
> +	size_t ana_log_len;
> +
> +	rc = nvme_identify_ctrl(pp->fd, &ctrl);
> +	if (rc)
> +		return ANA_PRIO_GETCTRL_FAILED;
> +
> +	if(!(ctrl.cmic & (1 << 3)))
> +		return ANA_PRIO_NOT_SUPPORTED;
> +
> +	rc = nvme_get_nsid(pp->fd, &nsid);
> +	if (rc)
> +		return ANA_PRIO_GETNSID_FAILED;
> +
> +	rc = nvme_identify_ns(pp->fd, nsid, &ns);
> +	if (rc)
> +		return ANA_PRIO_GETNS_FAILED;
> +
> +	ana_log_len = sizeof(struct nvme_ana_rsp_hdr) +
> +		le32_to_cpu(ctrl.nanagrpid) * sizeof(struct
> nvme_ana_group_desc);
> +	if (!(ctrl.anacap & (1 << 6)))
> +		ana_log_len += le32_to_cpu(ctrl.mnan) * sizeof(__le32);

I'd like to see a sanity check for ana_log_len here.

> +
> +	ana_log = malloc(ana_log_len);
> +	if (!ana_log)
> +		return ANA_PRIO_NO_MEMORY;
> +
> +	rc = nvme_ana_log(pp->fd, ana_log, ana_log_len,
> +		(ctrl.anacap & (1 << 6)) ? NVME_ANA_LOG_RGO : 0);
> +	if (rc) {
> +		free(ana_log);
> +		return ANA_PRIO_GETANALOG_FAILED;
> +	}

Please use a "goto error;" like construct here.

> +
> +	rc = get_ana_state(nsid, le32_to_cpu(ns.anagrpid), ana_log);
> +	if (rc < 0){
> +		free(ana_log);
> +		return ANA_PRIO_GETANAS_FAILED;
> +	}
> +
> +	free(ana_log);
> +	condlog(3, "%s: ana state = %02x [%s]", pp->dev, rc,
> aas_print_string(rc));

Please put an "error:" label here and free ana_log there.

> +
> +	return rc;
> +}
> +
> +int getprio(struct path * pp, char * args, unsigned int timeout)
> +{
> +	int rc;
> +
> +	if (pp->fd < 0)
> +		return ANA_PRIO_NO_INFORMATION;
> +
> +	rc = get_ana_info(pp, timeout);
> +	if (rc >= 0) {
> +		rc &= 0x0f;
> +		switch(rc) {
> +		case NVME_ANA_OPTIMIZED:
> +			rc = ANA_PRIO_OPTIMIZED;
> +			break;
> +		case NVME_ANA_NONOPTIMIZED:
> +			rc = ANA_PRIO_NONOPTIMIZED;
> +			break;
> +		case NVME_ANA_INACCESSIBLE:
> +			rc = ANA_PRIO_INACCESSIBLE;
> +			break;
> +		case NVME_ANA_PERSISTENT_LOSS:
> +			rc = ANA_PRIO_PERSISTENT_LOSS;
> +			break;
> +		case NVME_ANA_CHANGE:
> +			rc = ANA_PRIO_CHANGE;
> +			break;
> +		default:
> +			rc = ANA_PRIO_RESERVED;
> +		}
> +	} else {
> +		switch(rc) {
> +		case ANA_PRIO_GETCTRL_FAILED:
> +			condlog(0, "%s: couldn't get ctrl info", pp-
> >dev);
> +			break;
> +		case ANA_PRIO_NOT_SUPPORTED:
> +			condlog(0, "%s: ana not supported", pp->dev);
> +			break;
> +		case ANA_PRIO_GETANAS_FAILED:
> +			condlog(0, "%s: couldn't get ana state", pp-
> >dev);
> +			break;
> +		case ANA_PRIO_GETANALOG_FAILED:
> +			condlog(0, "%s: couldn't get ana log", pp-
> >dev);
> +			break;
> +		case ANA_PRIO_GETNS_FAILED:
> +			condlog(0, "%s: couldn't get namespace", pp-
> >dev);
> +			break;
> +		case ANA_PRIO_GETNSID_FAILED:
> +			condlog(0, "%s: couldn't get namespace id", pp-
> >dev);
> +			break;
> +		case ANA_PRIO_NO_MEMORY:
> +			condlog(0, "%s: couldn't alloc memory", pp-
> >dev);
> +			break;
> +		}
> +	}
> +	return rc;
> +}
> +
> diff --git a/libmultipath/prioritizers/ana.h
> b/libmultipath/prioritizers/ana.h
> new file mode 100644
> index 0000000..92cfa9e
> --- /dev/null
> +++ b/libmultipath/prioritizers/ana.h
> @@ -0,0 +1,221 @@
> +#ifndef _ANA_H
> +#define _ANA_H
> +
> +#include <linux/types.h>
> +
> +#define NVME_NSID_ALL			0xffffffff
> +#define NVME_IDENTIFY_DATA_SIZE 	4096
> +
> +#define NVME_LOG_ANA			0x0c
> +
> +/* Admin commands */
> +enum nvme_admin_opcode {
> +	nvme_admin_get_log_page		= 0x02,
> +	nvme_admin_identify		= 0x06,
> +};
> +
> +enum {
> +	NVME_ID_CNS_NS			= 0x00,
> +	NVME_ID_CNS_CTRL		= 0x01,
> +};
> +
> +/* nvme ioctl start */
> +struct nvme_passthru_cmd {
> +	__u8	opcode;
> +	__u8	flags;
> +	__u16	rsvd1;
> +	__u32	nsid;
> +	__u32	cdw2;
> +	__u32	cdw3;
> +	__u64	metadata;
> +	__u64	addr;
> +	__u32	metadata_len;
> +	__u32	data_len;
> +	__u32	cdw10;
> +	__u32	cdw11;
> +	__u32	cdw12;
> +	__u32	cdw13;
> +	__u32	cdw14;
> +	__u32	cdw15;
> +	__u32	timeout_ms;
> +	__u32	result;
> +};
> +
> +#define nvme_admin_cmd nvme_passthru_cmd
> +
> +#define NVME_IOCTL_ID		_IO('N', 0x40)
> +#define NVME_IOCTL_ADMIN_CMD	_IOWR('N', 0x41, struct nvme_admin_cmd)
> +/* nvme ioctl end */

Please #include <linux/nvme_ioctl.h> rather than copying its contents
here.

> +
> +/* nvme id ctrl start */
> +struct nvme_id_power_state {
> +	__le16			max_power;	/* centiwatts */
> +	__u8			rsvd2;
> +	__u8			flags;
> +	__le32			entry_lat;	/* microseconds */
> +	__le32			exit_lat;	/* microseconds */
> +	__u8			read_tput;
> +	__u8			read_lat;
> +	__u8			write_tput;
> +	__u8			write_lat;
> +	__le16			idle_power;
> +	__u8			idle_scale;
> +	__u8			rsvd19;
> +	__le16			active_power;
> +	__u8			active_work_scale;
> +	__u8			rsvd23[9];
> +};
> +
> +struct nvme_id_ctrl {
> +	__le16			vid;
> +	__le16			ssvid;
> +	char			sn[20];
> +	char			mn[40];
> +	char			fr[8];
> +	__u8			rab;
> +	__u8			ieee[3];
> +	__u8			cmic;
> +	__u8			mdts;
> +	__le16			cntlid;
> +	__le32			ver;
> +	__le32			rtd3r;
> +	__le32			rtd3e;
> +	__le32			oaes;
> +	__le32			ctratt;
> +	__u8			rsvd100[156];
> +	__le16			oacs;
> +	__u8			acl;
> +	__u8			aerl;
> +	__u8			frmw;
> +	__u8			lpa;
> +	__u8			elpe;
> +	__u8			npss;
> +	__u8			avscc;
> +	__u8			apsta;
> +	__le16			wctemp;
> +	__le16			cctemp;
> +	__le16			mtfa;
> +	__le32			hmpre;
> +	__le32			hmmin;
> +	__u8			tnvmcap[16];
> +	__u8			unvmcap[16];
> +	__le32			rpmbs;
> +	__le16			edstt;
> +	__u8			dsto;
> +	__u8			fwug;
> +	__le16			kas;
> +	__le16			hctma;
> +	__le16			mntmt;
> +	__le16			mxtmt;
> +	__le32			sanicap;
> +	__le32			hmminds;
> +	__le16			hmmaxd;
> +	__u8			rsvd338[4];
> +	__u8			anatt;
> +	__u8			anacap;
> +	__le32			anagrpmax;
> +	__le32			nanagrpid;
> +	__u8			rsvd352[160];
> +	__u8			sqes;
> +	__u8			cqes;
> +	__le16			maxcmd;
> +	__le32			nn;
> +	__le16			oncs;
> +	__le16			fuses;
> +	__u8			fna;
> +	__u8			vwc;
> +	__le16			awun;
> +	__le16			awupf;
> +	__u8			nvscc;
> +	__u8			nwpc;
> +	__le16			acwu;
> +	__u8			rsvd534[2];
> +	__le32			sgls;
> +	__le32			mnan;
> +	__u8			rsvd544[224];
> +	char			subnqn[256];
> +	__u8			rsvd1024[768];
> +	__le32			ioccsz;
> +	__le32			iorcsz;
> +	__le16			icdoff;
> +	__u8			ctrattr;
> +	__u8			msdbd;
> +	__u8			rsvd1804[244];
> +	struct nvme_id_power_state	psd[32];
> +	__u8			vs[1024];
> +};
> +/* nvme id ctrl end */
> +
> +/* nvme id ns start */
> +struct nvme_lbaf {
> +	__le16			ms;
> +	__u8			ds;
> +	__u8			rp;
> +};
> +
> +struct nvme_id_ns {
> +	__le64			nsze;
> +	__le64			ncap;
> +	__le64			nuse;
> +	__u8			nsfeat;
> +	__u8			nlbaf;
> +	__u8			flbas;
> +	__u8			mc;
> +	__u8			dpc;
> +	__u8			dps;
> +	__u8			nmic;
> +	__u8			rescap;
> +	__u8			fpi;
> +	__u8			rsvd33;
> +	__le16			nawun;
> +	__le16			nawupf;
> +	__le16			nacwu;
> +	__le16			nabsn;
> +	__le16			nabo;
> +	__le16			nabspf;
> +	__le16			noiob;
> +	__u8			nvmcap[16];
> +	__u8			rsvd64[28];
> +	__le32			anagrpid;
> +	__u8			rsvd96[3];
> +	__u8			nsattr;
> +	__u8			rsvd100[4];
> +	__u8			nguid[16];
> +	__u8			eui64[8];
> +	struct nvme_lbaf	lbaf[16];
> +	__u8			rsvd192[192];
> +	__u8			vs[3712];
> +};
> +/* nvme id ns end */
> +
> +/* nvme ana start */
> +enum nvme_ana_state {
> +	NVME_ANA_OPTIMIZED		= 0x01,
> +	NVME_ANA_NONOPTIMIZED		= 0x02,
> +	NVME_ANA_INACCESSIBLE		= 0x03,
> +	NVME_ANA_PERSISTENT_LOSS	= 0x04,
> +	NVME_ANA_CHANGE			= 0x0f,
> +	NVME_ANA_RESERVED		= 0x05,
> +};
> +
> +struct nvme_ana_rsp_hdr {
> +	__le64	chgcnt;
> +	__le16	ngrps;
> +	__le16	rsvd10[3];
> +};
> +
> +struct nvme_ana_group_desc {
> +	__le32	grpid;
> +	__le32	nnsids;
> +	__le64	chgcnt;
> +	__u8	state;
> +	__u8	rsvd17[15];
> +	__le32	nsids[];
> +};
> +
> +/* flag for the log specific field of the ANA log */
> +#define NVME_ANA_LOG_RGO	(1 << 0)
> +
> +/* nvme ana end */
> +
> +#endif
> diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> index 6333366..c5b2fb6 100644
> --- a/multipath/multipath.conf.5
> +++ b/multipath/multipath.conf.5
> @@ -334,6 +334,10 @@ priority provided as argument. Requires
> prio_args keyword.
>  Generate the path priority based on a latency algorithm.
>  Requires prio_args keyword.
>  .TP
> +.I ana
> +(Hardware-dependent)
> +Generate the path priority based on the NVMe ANA settings.
> +.TP
>  .I datacore
>  (Hardware-dependent)
>  Generate the path priority for some DataCore storage arrays.
> Requires prio_args
> @@ -1391,6 +1395,10 @@ Active/Standby mode exclusively.
>  .I 1 alua
>  (Hardware-dependent)
>  Hardware handler for SCSI-3 ALUA compatible arrays.
> +.TP
> +.I 1 ana
> +(Hardware-dependent)
> +Hardware handler for NVMe ANA compatible arrays.

No. hardware handlers are for SCSI only.

Regards,
Martin

-- 
Dr. Martin Wilck <mwilck@xxxxxxxx>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)


--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel




[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux