[PATCH 0/5] nvme-cli: lnvm: introduce chunk-log command

[PATCH 0/5] nvme-cli: lnvm: introduce chunk-log command

[Minwoo Im]

Hi all,

This series introduces a "chunk-log" subcommand for lnvm extension
plugin.  This command will send a get-log page NVMe admin command to the
given namespace which is for OCSSD.  We just can use nvme get-log
command with log id, but this subcommand is much more easier to use
because it gives parameters.

liblightnvm also provides nvm_cmd rprt_all command to retrieve this
information, but nvme-cli can do it also.  If you have something else
for this, please let me know.

This series also holds few clean-ups for the existing codes.

The last patch is introducing alias command name "geometry" for the
id-ns subcommand.

Please reivew.

Thanks!

Minwoo Im (5):
  lnvm: make data_len to sizeof() instead of magic number
  lnvm: export lnvm_get_identity
  lnvm: add chunk_info log page structure
  lnvm: introduce chunk-log command for chunk info
  lnvm: introduce alias geometry for id-ns for lnvm

 nvme-lightnvm.c          | 41 +++++++++++++++++++-
 nvme-lightnvm.h          | 17 ++++++++
 plugins/lnvm/lnvm-nvme.c | 84 ++++++++++++++++++++++++++++++++++++++++
 plugins/lnvm/lnvm-nvme.h |  3 +-
 4 files changed, 142 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH 1/5] lnvm: make data_len to sizeof() instead of magic number

[Minwoo Im]

We can have it as a sizeof() instead of the hard-coded value for the
data structure.

Cc: Keith Busch <kbusch at kernel.org>
Cc: Matias Bjorling <mb at lightnvm.io>
Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
---
 nvme-lightnvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nvme-lightnvm.c b/nvme-lightnvm.c
index 0b99786..e8cdccd 100644
--- a/nvme-lightnvm.c
+++ b/nvme-lightnvm.c
@@ -442,7 +442,7 @@ static int lnvm_get_identity(int fd, int nsid, struct nvme_nvm_id *nvm_id)
 		.opcode		= nvme_nvm_admin_identity,
 		.nsid		= nsid,
 		.addr		= (__u64)(uintptr_t)nvm_id,
-		.data_len	= 0x1000,
+		.data_len	= sizeof(struct nvme_nvm_id),
 	};
 
 	return nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &cmd);
-- 
2.17.1

[PATCH 2/5] lnvm: export lnvm_get_identity

[Minwoo Im]

When a subcommand wants to fetch Geometry, this function would be the
one to be taken.

Cc: Keith Busch <kbusch at kernel.org>
Cc: Matias Bjorling <mb at lightnvm.io>
Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
---
 nvme-lightnvm.c | 2 +-
 nvme-lightnvm.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/nvme-lightnvm.c b/nvme-lightnvm.c
index e8cdccd..2e665bf 100644
--- a/nvme-lightnvm.c
+++ b/nvme-lightnvm.c
@@ -436,7 +436,7 @@ static void show_lnvm_id_ns(struct nvme_nvm_id *id, unsigned int flags)
 	}
 }
 
-static int lnvm_get_identity(int fd, int nsid, struct nvme_nvm_id *nvm_id)
+int lnvm_get_identity(int fd, int nsid, struct nvme_nvm_id *nvm_id)
 {
 	struct nvme_admin_cmd cmd = {
 		.opcode		= nvme_nvm_admin_identity,
diff --git a/nvme-lightnvm.h b/nvme-lightnvm.h
index 9dea912..9dc1868 100644
--- a/nvme-lightnvm.h
+++ b/nvme-lightnvm.h
@@ -299,6 +299,8 @@ static inline struct ppa_addr generic_to_dev_addr(
 	return l;
 }
 
+int lnvm_get_identity(int fd, int nsid, struct nvme_nvm_id *nvm_id);
+
 int lnvm_do_init(char *, char *);
 int lnvm_do_list_devices(void);
 int lnvm_do_info(void);
-- 
2.17.1

[PATCH 3/5] lnvm: add chunk_info log page structure

[Minwoo Im]

The log ID is 0xCA which is defined in OCSSD 2.0 spec.  Also it has
32bytes sized structure for each entries.

Cc: Keith Busch <kbusch at kernel.org>
Cc: Matias Bjorling <mb at lightnvm.io>
Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
---
 nvme-lightnvm.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/nvme-lightnvm.h b/nvme-lightnvm.h
index 9dc1868..7a8ef7d 100644
--- a/nvme-lightnvm.h
+++ b/nvme-lightnvm.h
@@ -246,6 +246,20 @@ struct nvme_nvm_id {
 	__u8			resv[4095];
 } __attribute__((packed));
 
+enum {
+	NVM_LID_CHUNK_INFO = 0xCA,
+};
+
+struct nvme_nvm_chunk_desc {
+	__u8	cs;
+	__u8	ct;
+	__u8	wli;
+	__u8	rsvd_7_3[5];
+	__u64	slba;
+	__u64	cnlb;
+	__u64	wp;
+};
+
 struct nvme_nvm_bb_tbl {
 	__u8	tblid[4];
 	__le16	verid;
-- 
2.17.1

[PATCH 4/5] lnvm: introduce chunk-log command for chunk info

[Minwoo Im]

To retrieve the chunk information from the nvme namespae for the given
OCSSD, we can just do like:
	nvme lnvm chunk-log /dev/nvme0n1 --output-format=normal

This will calculate the data length from the geometry data structure
which might be retrieved by a Geometry command(Identity for 1.2 spec.).
Then it will request get log page API for 1.3 NVMe spec to get the
entries which indicate chunk information.

Cc: Keith Busch <kbusch at kernel.org>
Cc: Matias Bjorling <mb at lightnvm.io>
Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
---
 nvme-lightnvm.c          | 37 ++++++++++++++++++
 nvme-lightnvm.h          |  1 +
 plugins/lnvm/lnvm-nvme.c | 84 ++++++++++++++++++++++++++++++++++++++++
 plugins/lnvm/lnvm-nvme.h |  1 +
 4 files changed, 123 insertions(+)

diff --git a/nvme-lightnvm.c b/nvme-lightnvm.c
index 2e665bf..8fa8f3f 100644
--- a/nvme-lightnvm.c
+++ b/nvme-lightnvm.c
@@ -465,6 +465,43 @@ int lnvm_do_id_ns(int fd, int nsid, unsigned int flags)
 	return err;
 }
 
+static void show_lnvm_chunk_log(struct nvme_nvm_chunk_desc *chunk_log,
+				__u32 data_len)
+{
+	int nr_entry = data_len / sizeof(struct nvme_nvm_chunk_desc);
+	int idx;
+
+	for (idx = 0; idx < nr_entry; idx++) {
+		struct nvme_nvm_chunk_desc *desc = &chunk_log[idx];
+
+		printf(" [%5d] { ", idx);
+		printf("cs: %#x", desc->cs);
+		printf(", ct: %#x", desc->ct);
+		printf(", wli: %d", desc->wli);
+		printf(", slba: 0x%016"PRIx64, le64_to_cpu(desc->slba));
+		printf(", cnlb: 0x%016"PRIx64, le64_to_cpu(desc->cnlb));
+		printf(", wp: 0x%016"PRIx64" }\n", le64_to_cpu(desc->wp));
+	}
+}
+
+int lnvm_do_chunk_log(int fd, __u32 nsid, __u32 data_len, void *data,
+			unsigned int flags)
+{
+	int err;
+
+	err = nvme_get_log13(fd, nsid, NVM_LID_CHUNK_INFO, 0, 0, 0,
+			false, data_len, data);
+	if (!err) {
+		if (flags & RAW)
+			d_raw(data, data_len);
+		else
+			show_lnvm_chunk_log(data, data_len);
+	} else if (err > 0)
+		fprintf(stderr, "NVMe Status:%s(%x) NSID:%d\n",
+			nvme_status_to_string(err), err, nsid);
+	return err;
+}
+
 static void show_lnvm_bbtbl(struct nvme_nvm_bb_tbl *tbl)
 {
 	printf("verid    : %#x\n", (uint16_t)le16_to_cpu(tbl->verid));
diff --git a/nvme-lightnvm.h b/nvme-lightnvm.h
index 7a8ef7d..19660b7 100644
--- a/nvme-lightnvm.h
+++ b/nvme-lightnvm.h
@@ -322,6 +322,7 @@ int lnvm_do_create_tgt(char *, char *, char *, int, int, int, int);
 int lnvm_do_remove_tgt(char *);
 int lnvm_do_factory_init(char *, int, int, int);
 int lnvm_do_id_ns(int, int, unsigned int);
+int lnvm_do_chunk_log(int, __u32, __u32, void *, unsigned int);
 int lnvm_do_get_bbtbl(int, int, int, int, unsigned int);
 int lnvm_do_set_bbtbl(int, int, int, int, int, int, __u8);
 
diff --git a/plugins/lnvm/lnvm-nvme.c b/plugins/lnvm/lnvm-nvme.c
index 754931a..6f2724a 100644
--- a/plugins/lnvm/lnvm-nvme.c
+++ b/plugins/lnvm/lnvm-nvme.c
@@ -1,5 +1,7 @@
 #include <stdio.h>
 #include <errno.h>
+#include <stdlib.h>
+#include <unistd.h>
 
 #include "nvme.h"
 #include "nvme-print.h"
@@ -127,6 +129,88 @@ static int lnvm_id_ns(int argc, char **argv, struct command *cmd, struct plugin
 	return lnvm_do_id_ns(fd, cfg.namespace_id, flags);
 }
 
+static int lnvm_chunk_log(int argc, char **argv, struct command *cmd, struct plugin *plugin)
+{
+	const char *desc = "Retrieve the chunk information log for the "\
+		"specified given LightNVM device, returns in either "\
+		"human-readable or binary format.\n"\
+		"This will request Geometry first to get the "\
+		"num_grp,num_pu,num_chk first to figure out the total size "\
+		"of the log pages."\
+		;
+	const char *output_format = "Output format: normal|binary";
+	const char *human_readable = "Print normal in readable format";
+	int err, fmt, fd;
+	struct nvme_nvm_id20 geo;
+	struct nvme_nvm_chunk_desc *chunk_log;
+	__u32 nsid;
+	__u32 data_len;
+	unsigned int flags = 0;
+
+	struct config {
+		char *output_format;
+		int human_readable;
+	};
+
+	struct config cfg = {
+		.output_format = "normal",
+	};
+
+	const struct argconfig_commandline_options command_line_options[] = {
+		{"output-format", 'o', "FMT", CFG_STRING, &cfg.output_format, required_argument, output_format},
+		{"human-readable",'H', "",    CFG_NONE,   &cfg.human_readable,no_argument,       human_readable},
+		{NULL}
+	};
+
+	fd = parse_and_open(argc, argv, desc, command_line_options, &cfg,
+				sizeof(cfg));
+	if (fd < 0) {
+		err = fd;
+		goto ret;
+	}
+
+	fmt = validate_output_format(cfg.output_format);
+	if (fmt < 0) {
+		err = fmt;
+		goto close;
+	}
+
+	if (fmt == BINARY)
+		flags |= RAW;
+	else if (cfg.human_readable)
+		flags |= HUMAN;
+
+	nsid = nvme_get_nsid(fd);
+
+	/*
+	 * It needs to figure out how many bytes will be requested by this
+	 * subcommand by the (num_grp * num_pu * num_chk) from the Geometry.
+	 */
+	err = lnvm_get_identity(fd, nsid, (struct nvme_nvm_id *) &geo);
+	if (err)
+		goto close;
+
+	data_len = (geo.num_grp * geo.num_pu * geo.num_chk) *
+			sizeof(struct nvme_nvm_chunk_desc);
+	chunk_log = malloc(data_len);
+	if (!chunk_log) {
+		fprintf(stderr, "cound not alloc for chunk log %dbytes\n",
+				data_len);
+		err = -ENOMEM;
+		goto close;
+	}
+
+	err = lnvm_do_chunk_log(fd, nsid, data_len, chunk_log, flags);
+	if (err)
+		fprintf(stderr, "get log page for chunk information failed\n");
+
+	free(chunk_log);
+close:
+	close(fd);
+ret:
+	return err;
+}
+
 static int lnvm_create_tgt(int argc, char **argv, struct command *cmd, struct plugin *plugin)
 {
 	const char *desc = "Instantiate a target on top of a LightNVM enabled device.";
diff --git a/plugins/lnvm/lnvm-nvme.h b/plugins/lnvm/lnvm-nvme.h
index 3d5cbc5..f091f7b 100644
--- a/plugins/lnvm/lnvm-nvme.h
+++ b/plugins/lnvm/lnvm-nvme.h
@@ -12,6 +12,7 @@ PLUGIN(NAME("lnvm", "LightNVM specific extensions"),
 		ENTRY("list", "List available LightNVM devices", lnvm_list)
 		ENTRY("info", "List general information and available target engines", lnvm_info)
 		ENTRY("id-ns", "List geometry for LightNVM device", lnvm_id_ns)
+		ENTRY("chunk-log", "Chunk information by Get Log Page", lnvm_chunk_log)
 		ENTRY("init", "Initialize media manager on LightNVM device", lnvm_init)
 		ENTRY("create", "Create target on top of a LightNVM device", lnvm_create_tgt)
 		ENTRY("remove", "Remove target from device", lnvm_remove_tgt)
-- 
2.17.1

[PATCH 5/5] lnvm: introduce alias geometry for id-ns for lnvm

[Minwoo Im]

Now we have 2.0 OCSSD spec which introudces Geometry command instead of
Identity or something else.  This patch just adds an alias for this
command for the given NVMe namespace with backward compatibility.

Cc: Keith Busch <kbusch at kernel.org>
Cc: Matias Bjorling <mb at lightnvm.io>
Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
---
 plugins/lnvm/lnvm-nvme.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/plugins/lnvm/lnvm-nvme.h b/plugins/lnvm/lnvm-nvme.h
index f091f7b..2879a0b 100644
--- a/plugins/lnvm/lnvm-nvme.h
+++ b/plugins/lnvm/lnvm-nvme.h
@@ -11,7 +11,7 @@ PLUGIN(NAME("lnvm", "LightNVM specific extensions"),
 	COMMAND_LIST(
 		ENTRY("list", "List available LightNVM devices", lnvm_list)
 		ENTRY("info", "List general information and available target engines", lnvm_info)
-		ENTRY("id-ns", "List geometry for LightNVM device", lnvm_id_ns)
+		ENTRY("id-ns", "List geometry for LightNVM device", lnvm_id_ns, "geometry")
 		ENTRY("chunk-log", "Chunk information by Get Log Page", lnvm_chunk_log)
 		ENTRY("init", "Initialize media manager on LightNVM device", lnvm_init)
 		ENTRY("create", "Create target on top of a LightNVM device", lnvm_create_tgt)
-- 
2.17.1

[PATCH 1/5] lnvm: make data_len to sizeof() instead of magic number

[Javier González]

> On 23 Jul 2019,@20.35, Minwoo Im <minwoo.im.dev@gmail.com> wrote:
> 
> We can have it as a sizeof() instead of the hard-coded value for the
> data structure.
> 
> Cc: Keith Busch <kbusch at kernel.org>
> Cc: Matias Bjorling <mb at lightnvm.io>
> Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
> ---
> nvme-lightnvm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/nvme-lightnvm.c b/nvme-lightnvm.c
> index 0b99786..e8cdccd 100644
> --- a/nvme-lightnvm.c
> +++ b/nvme-lightnvm.c
> @@ -442,7 +442,7 @@ static int lnvm_get_identity(int fd, int nsid, struct nvme_nvm_id *nvm_id)
> 		.opcode		= nvme_nvm_admin_identity,
> 		.nsid		= nsid,
> 		.addr		= (__u64)(uintptr_t)nvm_id,
> -		.data_len	= 0x1000,
> +		.data_len	= sizeof(struct nvme_nvm_id),
> 	};
> 
> 	return nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &cmd);
> --
> 2.17.1

Looks good.

Reviewed-by: Javier Gonz?lez <javier at javigon.com>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Message signed with OpenPGP
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20190724/1fd30f1f/attachment-0001.sig>

[PATCH 4/5] lnvm: introduce chunk-log command for chunk info

[Javier González]

> On 23 Jul 2019,@20.36, Minwoo Im <minwoo.im.dev@gmail.com> wrote:
> 
> To retrieve the chunk information from the nvme namespae for the given
> OCSSD, we can just do like:
> 	nvme lnvm chunk-log /dev/nvme0n1 --output-format=normal
> 
> This will calculate the data length from the geometry data structure
> which might be retrieved by a Geometry command(Identity for 1.2 spec.).
> Then it will request get log page API for 1.3 NVMe spec to get the
> entries which indicate chunk information.
> 
> Cc: Keith Busch <kbusch at kernel.org>
> Cc: Matias Bjorling <mb at lightnvm.io>
> Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
> ---
> nvme-lightnvm.c          | 37 ++++++++++++++++++
> nvme-lightnvm.h          |  1 +
> plugins/lnvm/lnvm-nvme.c | 84 ++++++++++++++++++++++++++++++++++++++++
> plugins/lnvm/lnvm-nvme.h |  1 +
> 4 files changed, 123 insertions(+)
> 
> diff --git a/nvme-lightnvm.c b/nvme-lightnvm.c
> index 2e665bf..8fa8f3f 100644
> --- a/nvme-lightnvm.c
> +++ b/nvme-lightnvm.c
> @@ -465,6 +465,43 @@ int lnvm_do_id_ns(int fd, int nsid, unsigned int flags)
> 	return err;
> }
> 
> +static void show_lnvm_chunk_log(struct nvme_nvm_chunk_desc *chunk_log,
> +				__u32 data_len)
> +{
> +	int nr_entry = data_len / sizeof(struct nvme_nvm_chunk_desc);
> +	int idx;
> +
> +	for (idx = 0; idx < nr_entry; idx++) {
> +		struct nvme_nvm_chunk_desc *desc = &chunk_log[idx];
> +
> +		printf(" [%5d] { ", idx);
> +		printf("cs: %#x", desc->cs);
> +		printf(", ct: %#x", desc->ct);
> +		printf(", wli: %d", desc->wli);
> +		printf(", slba: 0x%016"PRIx64, le64_to_cpu(desc->slba));
> +		printf(", cnlb: 0x%016"PRIx64, le64_to_cpu(desc->cnlb));
> +		printf(", wp: 0x%016"PRIx64" }\n", le64_to_cpu(desc->wp));
> +	}
> +}
> +
> +int lnvm_do_chunk_log(int fd, __u32 nsid, __u32 data_len, void *data,
> +			unsigned int flags)
> +{
> +	int err;
> +
> +	err = nvme_get_log13(fd, nsid, NVM_LID_CHUNK_INFO, 0, 0, 0,
> +			false, data_len, data);
> +	if (!err) {
> +		if (flags & RAW)
> +			d_raw(data, data_len);
> +		else
> +			show_lnvm_chunk_log(data, data_len);
> +	} else if (err > 0)
> +		fprintf(stderr, "NVMe Status:%s(%x) NSID:%d\n",
> +			nvme_status_to_string(err), err, nsid);
> +	return err;

What about avoiding the nested if?

If (err) {
	fprintf(stderr, "NVMe Status:%s(%x) NSID:%d\n",
			nvme_status_to_string(err), err, nsid);

	goto out;
}

If (flags & RAW)
	d_raw(data, data_len);
else
	show_lnvm_chunk_log(data, data_len);

out:
return err;

> +}
> +
> static void show_lnvm_bbtbl(struct nvme_nvm_bb_tbl *tbl)
> {
> 	printf("verid    : %#x\n", (uint16_t)le16_to_cpu(tbl->verid));
> diff --git a/nvme-lightnvm.h b/nvme-lightnvm.h
> index 7a8ef7d..19660b7 100644
> --- a/nvme-lightnvm.h
> +++ b/nvme-lightnvm.h
> @@ -322,6 +322,7 @@ int lnvm_do_create_tgt(char *, char *, char *, int, int, int, int);
> int lnvm_do_remove_tgt(char *);
> int lnvm_do_factory_init(char *, int, int, int);
> int lnvm_do_id_ns(int, int, unsigned int);
> +int lnvm_do_chunk_log(int, __u32, __u32, void *, unsigned int);
> int lnvm_do_get_bbtbl(int, int, int, int, unsigned int);
> int lnvm_do_set_bbtbl(int, int, int, int, int, int, __u8);
> 
> diff --git a/plugins/lnvm/lnvm-nvme.c b/plugins/lnvm/lnvm-nvme.c
> index 754931a..6f2724a 100644
> --- a/plugins/lnvm/lnvm-nvme.c
> +++ b/plugins/lnvm/lnvm-nvme.c
> @@ -1,5 +1,7 @@
> #include <stdio.h>
> #include <errno.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> 
> #include "nvme.h"
> #include "nvme-print.h"
> @@ -127,6 +129,88 @@ static int lnvm_id_ns(int argc, char **argv, struct command *cmd, struct plugin
> 	return lnvm_do_id_ns(fd, cfg.namespace_id, flags);
> }
> 
> +static int lnvm_chunk_log(int argc, char **argv, struct command *cmd, struct plugin *plugin)
> +{
> +	const char *desc = "Retrieve the chunk information log for the "\
> +		"specified given LightNVM device, returns in either "\
> +		"human-readable or binary format.\n"\
> +		"This will request Geometry first to get the "\
> +		"num_grp,num_pu,num_chk first to figure out the total size "\
> +		"of the log pages."\
> +		;
> +	const char *output_format = "Output format: normal|binary?;

normalbinary -> normal binary?

> +	const char *human_readable = "Print normal in readable format";
> +	int err, fmt, fd;
> +	struct nvme_nvm_id20 geo;
> +	struct nvme_nvm_chunk_desc *chunk_log;
> +	__u32 nsid;
> +	__u32 data_len;
> +	unsigned int flags = 0;
> +
> +	struct config {
> +		char *output_format;
> +		int human_readable;
> +	};
> +
> +	struct config cfg = {
> +		.output_format = "normal",
> +	};
> +
> +	const struct argconfig_commandline_options command_line_options[] = {
> +		{"output-format", 'o', "FMT", CFG_STRING, &cfg.output_format, required_argument, output_format},
> +		{"human-readable",'H', "",    CFG_NONE,   &cfg.human_readable,no_argument,       human_readable},
> +		{NULL}
> +	};
> +
> +	fd = parse_and_open(argc, argv, desc, command_line_options, &cfg,
> +				sizeof(cfg));
> +	if (fd < 0) {
> +		err = fd;
> +		goto ret;
> +	}

return fd;

> +
> +	fmt = validate_output_format(cfg.output_format);
> +	if (fmt < 0) {
> +		err = fmt;
> +		goto close;
> +	}
> +
> +	if (fmt == BINARY)
> +		flags |= RAW;
> +	else if (cfg.human_readable)
> +		flags |= HUMAN;
> +
> +	nsid = nvme_get_nsid(fd);
> +
> +	/*
> +	 * It needs to figure out how many bytes will be requested by this
> +	 * subcommand by the (num_grp * num_pu * num_chk) from the Geometry.
> +	 */
> +	err = lnvm_get_identity(fd, nsid, (struct nvme_nvm_id *) &geo);
> +	if (err)
> +		goto close;
> +
> +	data_len = (geo.num_grp * geo.num_pu * geo.num_chk) *
> +			sizeof(struct nvme_nvm_chunk_desc);
> +	chunk_log = malloc(data_len);
> +	if (!chunk_log) {
> +		fprintf(stderr, "cound not alloc for chunk log %dbytes\n",
> +				data_len);
> +		err = -ENOMEM;
> +		goto close;
> +	}
> +
> +	err = lnvm_do_chunk_log(fd, nsid, data_len, chunk_log, flags);
> +	if (err)
> +		fprintf(stderr, "get log page for chunk information failed\n");
> +
> +	free(chunk_log);
> +close:
> +	close(fd);
> +ret:
> +	return err;
> +}
> +
> static int lnvm_create_tgt(int argc, char **argv, struct command *cmd, struct plugin *plugin)
> {
> 	const char *desc = "Instantiate a target on top of a LightNVM enabled device.";
> diff --git a/plugins/lnvm/lnvm-nvme.h b/plugins/lnvm/lnvm-nvme.h
> index 3d5cbc5..f091f7b 100644
> --- a/plugins/lnvm/lnvm-nvme.h
> +++ b/plugins/lnvm/lnvm-nvme.h
> @@ -12,6 +12,7 @@ PLUGIN(NAME("lnvm", "LightNVM specific extensions"),
> 		ENTRY("list", "List available LightNVM devices", lnvm_list)
> 		ENTRY("info", "List general information and available target engines", lnvm_info)
> 		ENTRY("id-ns", "List geometry for LightNVM device", lnvm_id_ns)
> +		ENTRY("chunk-log", "Chunk information by Get Log Page", lnvm_chunk_log)
> 		ENTRY("init", "Initialize media manager on LightNVM device", lnvm_init)
> 		ENTRY("create", "Create target on top of a LightNVM device", lnvm_create_tgt)
> 		ENTRY("remove", "Remove target from device", lnvm_remove_tgt)
> --
> 2.17.1

We have typically retrieved this directly using passthru commands, but I
have nothing against making this a dedicated lnvm command.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Message signed with OpenPGP
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20190724/5bbb307b/attachment.sig>

[PATCH 2/5] lnvm: export lnvm_get_identity

[Javier González]

> On 23 Jul 2019,@20.35, Minwoo Im <minwoo.im.dev@gmail.com> wrote:
> 
> When a subcommand wants to fetch Geometry, this function would be the
> one to be taken.
> 
> Cc: Keith Busch <kbusch at kernel.org>
> Cc: Matias Bjorling <mb at lightnvm.io>
> Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
> ---
> nvme-lightnvm.c | 2 +-
> nvme-lightnvm.h | 2 ++
> 2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/nvme-lightnvm.c b/nvme-lightnvm.c
> index e8cdccd..2e665bf 100644
> --- a/nvme-lightnvm.c
> +++ b/nvme-lightnvm.c
> @@ -436,7 +436,7 @@ static void show_lnvm_id_ns(struct nvme_nvm_id *id, unsigned int flags)
> 	}
> }
> 
> -static int lnvm_get_identity(int fd, int nsid, struct nvme_nvm_id *nvm_id)
> +int lnvm_get_identity(int fd, int nsid, struct nvme_nvm_id *nvm_id)
> {
> 	struct nvme_admin_cmd cmd = {
> 		.opcode		= nvme_nvm_admin_identity,
> diff --git a/nvme-lightnvm.h b/nvme-lightnvm.h
> index 9dea912..9dc1868 100644
> --- a/nvme-lightnvm.h
> +++ b/nvme-lightnvm.h
> @@ -299,6 +299,8 @@ static inline struct ppa_addr generic_to_dev_addr(
> 	return l;
> }
> 
> +int lnvm_get_identity(int fd, int nsid, struct nvme_nvm_id *nvm_id);
> +
> int lnvm_do_init(char *, char *);
> int lnvm_do_list_devices(void);
> int lnvm_do_info(void);
> --
> 2.17.1

This and the other helpers for patch 4/5 looks good.


Reviewed-by: Javier Gonz?lez <javier at javigon.com>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Message signed with OpenPGP
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20190724/5bdddd09/attachment.sig>

[PATCH 4/5] lnvm: introduce chunk-log command for chunk info

[Matias Bjørling]

On 23/07/2019 20.36, Minwoo Im wrote:
> To retrieve the chunk information from the nvme namespae for the given
> OCSSD, we can just do like:
> 	nvme lnvm chunk-log /dev/nvme0n1 --output-format=normal
>
> This will calculate the data length from the geometry data structure
> which might be retrieved by a Geometry command(Identity for 1.2 spec.).
> Then it will request get log page API for 1.3 NVMe spec to get the
> entries which indicate chunk information.
>
> Cc: Keith Busch <kbusch at kernel.org>
> Cc: Matias Bjorling <mb at lightnvm.io>
> Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
> ---
>   nvme-lightnvm.c          | 37 ++++++++++++++++++
>   nvme-lightnvm.h          |  1 +
>   plugins/lnvm/lnvm-nvme.c | 84 ++++++++++++++++++++++++++++++++++++++++
>   plugins/lnvm/lnvm-nvme.h |  1 +
>   4 files changed, 123 insertions(+)
>
> diff --git a/nvme-lightnvm.c b/nvme-lightnvm.c
> index 2e665bf..8fa8f3f 100644
> --- a/nvme-lightnvm.c
> +++ b/nvme-lightnvm.c
> @@ -465,6 +465,43 @@ int lnvm_do_id_ns(int fd, int nsid, unsigned int flags)
>   	return err;
>   }
>   
> +static void show_lnvm_chunk_log(struct nvme_nvm_chunk_desc *chunk_log,
> +				__u32 data_len)
> +{
> +	int nr_entry = data_len / sizeof(struct nvme_nvm_chunk_desc);
> +	int idx;
> +
> +	for (idx = 0; idx < nr_entry; idx++) {
> +		struct nvme_nvm_chunk_desc *desc = &chunk_log[idx];
> +
> +		printf(" [%5d] { ", idx);
> +		printf("cs: %#x", desc->cs);
> +		printf(", ct: %#x", desc->ct);
> +		printf(", wli: %d", desc->wli);
> +		printf(", slba: 0x%016"PRIx64, le64_to_cpu(desc->slba));
> +		printf(", cnlb: 0x%016"PRIx64, le64_to_cpu(desc->cnlb));
> +		printf(", wp: 0x%016"PRIx64" }\n", le64_to_cpu(desc->wp));
> +	}
> +}
> +
> +int lnvm_do_chunk_log(int fd, __u32 nsid, __u32 data_len, void *data,
> +			unsigned int flags)
> +{
> +	int err;
> +
> +	err = nvme_get_log13(fd, nsid, NVM_LID_CHUNK_INFO, 0, 0, 0,
> +			false, data_len, data);
> +	if (!err) {
> +		if (flags & RAW)
> +			d_raw(data, data_len);
> +		else
> +			show_lnvm_chunk_log(data, data_len);
> +	} else if (err > 0)
> +		fprintf(stderr, "NVMe Status:%s(%x) NSID:%d\n",
> +			nvme_status_to_string(err), err, nsid);
> +	return err;
> +}
> +
>   static void show_lnvm_bbtbl(struct nvme_nvm_bb_tbl *tbl)
>   {
>   	printf("verid    : %#x\n", (uint16_t)le16_to_cpu(tbl->verid));
> diff --git a/nvme-lightnvm.h b/nvme-lightnvm.h
> index 7a8ef7d..19660b7 100644
> --- a/nvme-lightnvm.h
> +++ b/nvme-lightnvm.h
> @@ -322,6 +322,7 @@ int lnvm_do_create_tgt(char *, char *, char *, int, int, int, int);
>   int lnvm_do_remove_tgt(char *);
>   int lnvm_do_factory_init(char *, int, int, int);
>   int lnvm_do_id_ns(int, int, unsigned int);
> +int lnvm_do_chunk_log(int, __u32, __u32, void *, unsigned int);
>   int lnvm_do_get_bbtbl(int, int, int, int, unsigned int);
>   int lnvm_do_set_bbtbl(int, int, int, int, int, int, __u8);
>   
> diff --git a/plugins/lnvm/lnvm-nvme.c b/plugins/lnvm/lnvm-nvme.c
> index 754931a..6f2724a 100644
> --- a/plugins/lnvm/lnvm-nvme.c
> +++ b/plugins/lnvm/lnvm-nvme.c
> @@ -1,5 +1,7 @@
>   #include <stdio.h>
>   #include <errno.h>
> +#include <stdlib.h>
> +#include <unistd.h>
>   
>   #include "nvme.h"
>   #include "nvme-print.h"
> @@ -127,6 +129,88 @@ static int lnvm_id_ns(int argc, char **argv, struct command *cmd, struct plugin
>   	return lnvm_do_id_ns(fd, cfg.namespace_id, flags);
>   }
>   
> +static int lnvm_chunk_log(int argc, char **argv, struct command *cmd, struct plugin *plugin)
> +{
> +	const char *desc = "Retrieve the chunk information log for the "\
> +		"specified given LightNVM device, returns in either "\
> +		"human-readable or binary format.\n"\
> +		"This will request Geometry first to get the "\
> +		"num_grp,num_pu,num_chk first to figure out the total size "\
> +		"of the log pages."\
> +		;
> +	const char *output_format = "Output format: normal|binary";
> +	const char *human_readable = "Print normal in readable format";
> +	int err, fmt, fd;
> +	struct nvme_nvm_id20 geo;
> +	struct nvme_nvm_chunk_desc *chunk_log;
> +	__u32 nsid;
> +	__u32 data_len;
> +	unsigned int flags = 0;
> +
> +	struct config {
> +		char *output_format;
> +		int human_readable;
> +	};
> +
> +	struct config cfg = {
> +		.output_format = "normal",
> +	};
> +
> +	const struct argconfig_commandline_options command_line_options[] = {
> +		{"output-format", 'o', "FMT", CFG_STRING, &cfg.output_format, required_argument, output_format},
> +		{"human-readable",'H', "",    CFG_NONE,   &cfg.human_readable,no_argument,       human_readable},
> +		{NULL}
> +	};
> +
> +	fd = parse_and_open(argc, argv, desc, command_line_options, &cfg,
> +				sizeof(cfg));
> +	if (fd < 0) {
> +		err = fd;
> +		goto ret;
> +	}
> +
> +	fmt = validate_output_format(cfg.output_format);
> +	if (fmt < 0) {
> +		err = fmt;
> +		goto close;
> +	}
> +
> +	if (fmt == BINARY)
> +		flags |= RAW;
> +	else if (cfg.human_readable)
> +		flags |= HUMAN;
> +
> +	nsid = nvme_get_nsid(fd);
> +
> +	/*
> +	 * It needs to figure out how many bytes will be requested by this
> +	 * subcommand by the (num_grp * num_pu * num_chk) from the Geometry.
> +	 */
> +	err = lnvm_get_identity(fd, nsid, (struct nvme_nvm_id *) &geo);
> +	if (err)
> +		goto close;
> +
> +	data_len = (geo.num_grp * geo.num_pu * geo.num_chk) *
> +			sizeof(struct nvme_nvm_chunk_desc);
> +	chunk_log = malloc(data_len);
> +	if (!chunk_log) {
> +		fprintf(stderr, "cound not alloc for chunk log %dbytes\n",
> +				data_len);
> +		err = -ENOMEM;
> +		goto close;
> +	}
> +
> +	err = lnvm_do_chunk_log(fd, nsid, data_len, chunk_log, flags);
> +	if (err)
> +		fprintf(stderr, "get log page for chunk information failed\n");
> +
> +	free(chunk_log);
> +close:
> +	close(fd);
> +ret:
> +	return err;
> +}
> +
>   static int lnvm_create_tgt(int argc, char **argv, struct command *cmd, struct plugin *plugin)
>   {
>   	const char *desc = "Instantiate a target on top of a LightNVM enabled device.";
> diff --git a/plugins/lnvm/lnvm-nvme.h b/plugins/lnvm/lnvm-nvme.h
> index 3d5cbc5..f091f7b 100644
> --- a/plugins/lnvm/lnvm-nvme.h
> +++ b/plugins/lnvm/lnvm-nvme.h
> @@ -12,6 +12,7 @@ PLUGIN(NAME("lnvm", "LightNVM specific extensions"),
>   		ENTRY("list", "List available LightNVM devices", lnvm_list)
>   		ENTRY("info", "List general information and available target engines", lnvm_info)
>   		ENTRY("id-ns", "List geometry for LightNVM device", lnvm_id_ns)
> +		ENTRY("chunk-log", "Chunk information by Get Log Page", lnvm_chunk_log)

Chunk Information Log Page ?

>   		ENTRY("init", "Initialize media manager on LightNVM device", lnvm_init)
>   		ENTRY("create", "Create target on top of a LightNVM device", lnvm_create_tgt)
>   		ENTRY("remove", "Remove target from device", lnvm_remove_tgt)

Hi Minwoo,

Could you squash patch 2, 3 and 4 together (4 being the main patch) - 
They belong together to implement one feature.

As a side-note, we have the same command for ZNS (that will be pushed 
when the ZNS TP is ratified) - In that, we've also added support for 
supplying start lba and number of chunks (zones) to return. Could you 
add that as well to this? Then there is coherency between the two?

-Matias

[PATCH 5/5] lnvm: introduce alias geometry for id-ns for lnvm

[Matias Bjørling]

On 23/07/2019 20.36, Minwoo Im wrote:
> Now we have 2.0 OCSSD spec which introudces Geometry command instead of
> Identity or something else.  This patch just adds an alias for this
> command for the given NVMe namespace with backward compatibility.
>
> Cc: Keith Busch <kbusch at kernel.org>
> Cc: Matias Bjorling <mb at lightnvm.io>
> Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
> ---
>   plugins/lnvm/lnvm-nvme.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/plugins/lnvm/lnvm-nvme.h b/plugins/lnvm/lnvm-nvme.h
> index f091f7b..2879a0b 100644
> --- a/plugins/lnvm/lnvm-nvme.h
> +++ b/plugins/lnvm/lnvm-nvme.h
> @@ -11,7 +11,7 @@ PLUGIN(NAME("lnvm", "LightNVM specific extensions"),
>   	COMMAND_LIST(
>   		ENTRY("list", "List available LightNVM devices", lnvm_list)
>   		ENTRY("info", "List general information and available target engines", lnvm_info)
> -		ENTRY("id-ns", "List geometry for LightNVM device", lnvm_id_ns)
> +		ENTRY("id-ns", "List geometry for LightNVM device", lnvm_id_ns, "geometry")
>   		ENTRY("chunk-log", "Chunk information by Get Log Page", lnvm_chunk_log)
>   		ENTRY("init", "Initialize media manager on LightNVM device", lnvm_init)
>   		ENTRY("create", "Create target on top of a LightNVM device", lnvm_create_tgt)

Hi Minwoo,

How about also rewording the text to:

List geometry structure for LightNVM device?

Also, do we want to use a short hand (instead of writing geometry in 
full)? How is it done elsewhere in the nvme-cli code?

Regards,

Matias

[PATCH 1/5] lnvm: make data_len to sizeof() instead of magic number

[Matias Bjørling]

On 23/07/2019 20.35, Minwoo Im wrote:
> We can have it as a sizeof() instead of the hard-coded value for the
> data structure.
>
> Cc: Keith Busch <kbusch at kernel.org>
> Cc: Matias Bjorling <mb at lightnvm.io>
> Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
> ---
>   nvme-lightnvm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/nvme-lightnvm.c b/nvme-lightnvm.c
> index 0b99786..e8cdccd 100644
> --- a/nvme-lightnvm.c
> +++ b/nvme-lightnvm.c
> @@ -442,7 +442,7 @@ static int lnvm_get_identity(int fd, int nsid, struct nvme_nvm_id *nvm_id)
>   		.opcode		= nvme_nvm_admin_identity,
>   		.nsid		= nsid,
>   		.addr		= (__u64)(uintptr_t)nvm_id,
> -		.data_len	= 0x1000,
> +		.data_len	= sizeof(struct nvme_nvm_id),
>   	};
>   
>   	return nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &cmd);

Works for me.

Please use use my Signed-off-by instead of Reviewed-by - traditionally 
Keith has been kind to wait to pull in patches until I got a chance to 
look at them on Github. Having the review here on the mailing list and 
later as a pull request on github makes it explicit that I signed off on it.

Signed-off-by: Matias Bj?rling <matias.bjorling at wdc.com>

Thank you!

- Matias

[PATCH 4/5] lnvm: introduce chunk-log command for chunk info

[Minwoo Im]

Hi Javier,

Thanks for the review.

> > +int lnvm_do_chunk_log(int fd, __u32 nsid, __u32 data_len, void *data,
> > +			unsigned int flags)
> > +{
> > +	int err;
> > +
> > +	err = nvme_get_log13(fd, nsid, NVM_LID_CHUNK_INFO, 0, 0, 0,
> > +			false, data_len, data);
> > +	if (!err) {
> > +		if (flags & RAW)
> > +			d_raw(data, data_len);
> > +		else
> > +			show_lnvm_chunk_log(data, data_len);
> > +	} else if (err > 0)
> > +		fprintf(stderr, "NVMe Status:%s(%x) NSID:%d\n",
> > +			nvme_status_to_string(err), err, nsid);
> > +	return err;
> 
> What about avoiding the nested if?
> 
> If (err) {
> 	fprintf(stderr, "NVMe Status:%s(%x) NSID:%d\n",
> 			nvme_status_to_string(err), err, nsid);
> 
> 	goto out;
> }
> 
> If (flags & RAW)
> 	d_raw(data, data_len);
> else
> 	show_lnvm_chunk_log(data, data_len);
> 
> out:
> return err;

That makes sense, we should avoid the case when "err" is negative value
which means the internal error from the nvme_get_log13() in printing the
nvme status.  So how about this?

```c
	if (err > 0) {
		fprintf(stderr, "NVMe Status:%s(%x) NSID:%d\n",
			nvme_status_to_string(err), err, nsid);

		goto out;
	} else if (err < 0) {
		err = -errno;
		perror("nvme_get_log13");

		goto out;
	}

	if (flags & RAW)
		d_raw(data, data_len);
	else
		show_lnvm_chunk_log(data, data_len);

out:
	return err;
```

> > +}
> > +
> > static void show_lnvm_bbtbl(struct nvme_nvm_bb_tbl *tbl)
> > {
> > 	printf("verid    : %#x\n", (uint16_t)le16_to_cpu(tbl->verid));
> > diff --git a/nvme-lightnvm.h b/nvme-lightnvm.h
> > index 7a8ef7d..19660b7 100644
> > --- a/nvme-lightnvm.h
> > +++ b/nvme-lightnvm.h
> > @@ -322,6 +322,7 @@ int lnvm_do_create_tgt(char *, char *, char *, int, int, int, int);
> > int lnvm_do_remove_tgt(char *);
> > int lnvm_do_factory_init(char *, int, int, int);
> > int lnvm_do_id_ns(int, int, unsigned int);
> > +int lnvm_do_chunk_log(int, __u32, __u32, void *, unsigned int);
> > int lnvm_do_get_bbtbl(int, int, int, int, unsigned int);
> > int lnvm_do_set_bbtbl(int, int, int, int, int, int, __u8);
> > 
> > diff --git a/plugins/lnvm/lnvm-nvme.c b/plugins/lnvm/lnvm-nvme.c
> > index 754931a..6f2724a 100644
> > --- a/plugins/lnvm/lnvm-nvme.c
> > +++ b/plugins/lnvm/lnvm-nvme.c
> > @@ -1,5 +1,7 @@
> > #include <stdio.h>
> > #include <errno.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > 
> > #include "nvme.h"
> > #include "nvme-print.h"
> > @@ -127,6 +129,88 @@ static int lnvm_id_ns(int argc, char **argv, struct command *cmd, struct plugin
> > 	return lnvm_do_id_ns(fd, cfg.namespace_id, flags);
> > }
> > 
> > +static int lnvm_chunk_log(int argc, char **argv, struct command *cmd, struct plugin *plugin)
> > +{
> > +	const char *desc = "Retrieve the chunk information log for the "\
> > +		"specified given LightNVM device, returns in either "\
> > +		"human-readable or binary format.\n"\
> > +		"This will request Geometry first to get the "\
> > +		"num_grp,num_pu,num_chk first to figure out the total size "\
> > +		"of the log pages."\
> > +		;
> > +	const char *output_format = "Output format: normal|binary?;
> 
> normalbinary -> normal binary?

Oh It has "|" between them.  But I'm fine with that too.  It was just
derieved from the nvme.c core module which builtin commands are taking
now.

> > +	const char *human_readable = "Print normal in readable format";
> > +	int err, fmt, fd;
> > +	struct nvme_nvm_id20 geo;
> > +	struct nvme_nvm_chunk_desc *chunk_log;
> > +	__u32 nsid;
> > +	__u32 data_len;
> > +	unsigned int flags = 0;
> > +
> > +	struct config {
> > +		char *output_format;
> > +		int human_readable;
> > +	};
> > +
> > +	struct config cfg = {
> > +		.output_format = "normal",
> > +	};
> > +
> > +	const struct argconfig_commandline_options command_line_options[] = {
> > +		{"output-format", 'o', "FMT", CFG_STRING, &cfg.output_format, required_argument, output_format},
> > +		{"human-readable",'H', "",    CFG_NONE,   &cfg.human_readable,no_argument,       human_readable},
> > +		{NULL}
> > +	};
> > +
> > +	fd = parse_and_open(argc, argv, desc, command_line_options, &cfg,
> > +				sizeof(cfg));
> > +	if (fd < 0) {
> > +		err = fd;
> > +		goto ret;
> > +	}
> 
> return fd;

This is absolutely no need to set the "err" value in this step.  Will
update.

> 
> > +
> > +	fmt = validate_output_format(cfg.output_format);
> > +	if (fmt < 0) {
> > +		err = fmt;
> > +		goto close;
> > +	}
> > +
> > +	if (fmt == BINARY)
> > +		flags |= RAW;
> > +	else if (cfg.human_readable)
> > +		flags |= HUMAN;
> > +
> > +	nsid = nvme_get_nsid(fd);
> > +
> > +	/*
> > +	 * It needs to figure out how many bytes will be requested by this
> > +	 * subcommand by the (num_grp * num_pu * num_chk) from the Geometry.
> > +	 */
> > +	err = lnvm_get_identity(fd, nsid, (struct nvme_nvm_id *) &geo);
> > +	if (err)
> > +		goto close;
> > +
> > +	data_len = (geo.num_grp * geo.num_pu * geo.num_chk) *
> > +			sizeof(struct nvme_nvm_chunk_desc);
> > +	chunk_log = malloc(data_len);
> > +	if (!chunk_log) {
> > +		fprintf(stderr, "cound not alloc for chunk log %dbytes\n",
> > +				data_len);
> > +		err = -ENOMEM;
> > +		goto close;
> > +	}
> > +
> > +	err = lnvm_do_chunk_log(fd, nsid, data_len, chunk_log, flags);
> > +	if (err)
> > +		fprintf(stderr, "get log page for chunk information failed\n");
> > +
> > +	free(chunk_log);
> > +close:
> > +	close(fd);
> > +ret:
> > +	return err;
> > +}
> > +
> > static int lnvm_create_tgt(int argc, char **argv, struct command *cmd, struct plugin *plugin)
> > {
> > 	const char *desc = "Instantiate a target on top of a LightNVM enabled device.";
> > diff --git a/plugins/lnvm/lnvm-nvme.h b/plugins/lnvm/lnvm-nvme.h
> > index 3d5cbc5..f091f7b 100644
> > --- a/plugins/lnvm/lnvm-nvme.h
> > +++ b/plugins/lnvm/lnvm-nvme.h
> > @@ -12,6 +12,7 @@ PLUGIN(NAME("lnvm", "LightNVM specific extensions"),
> > 		ENTRY("list", "List available LightNVM devices", lnvm_list)
> > 		ENTRY("info", "List general information and available target engines", lnvm_info)
> > 		ENTRY("id-ns", "List geometry for LightNVM device", lnvm_id_ns)
> > +		ENTRY("chunk-log", "Chunk information by Get Log Page", lnvm_chunk_log)
> > 		ENTRY("init", "Initialize media manager on LightNVM device", lnvm_init)
> > 		ENTRY("create", "Create target on top of a LightNVM device", lnvm_create_tgt)
> > 		ENTRY("remove", "Remove target from device", lnvm_remove_tgt)
> > --
> > 2.17.1
> 
> We have typically retrieved this directly using passthru commands, but I
> have nothing against making this a dedicated lnvm command.

Thanks for letting me know.  I was just trying to provide a dedicated
command just like the built-in commands like fw-log and etc.

Thanks for the all these kindly review!  I will prepare the next version
with these points.

[PATCH 4/5] lnvm: introduce chunk-log command for chunk info

[Minwoo Im]

Hi Matias,

On 19-07-24 14:27:10, Matias Bj?rling wrote:
> On 23/07/2019 20.36, Minwoo Im wrote:
> > To retrieve the chunk information from the nvme namespae for the given
> > OCSSD, we can just do like:
> > 	nvme lnvm chunk-log /dev/nvme0n1 --output-format=normal
> > 
> > This will calculate the data length from the geometry data structure
> > which might be retrieved by a Geometry command(Identity for 1.2 spec.).
> > Then it will request get log page API for 1.3 NVMe spec to get the
> > entries which indicate chunk information.
> > 
> > Cc: Keith Busch <kbusch at kernel.org>
> > Cc: Matias Bjorling <mb at lightnvm.io>
> > Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
> > ---
> > diff --git a/plugins/lnvm/lnvm-nvme.h b/plugins/lnvm/lnvm-nvme.h
> > index 3d5cbc5..f091f7b 100644
> > --- a/plugins/lnvm/lnvm-nvme.h
> > +++ b/plugins/lnvm/lnvm-nvme.h
> > @@ -12,6 +12,7 @@ PLUGIN(NAME("lnvm", "LightNVM specific extensions"),
> >   		ENTRY("list", "List available LightNVM devices", lnvm_list)
> >   		ENTRY("info", "List general information and available target engines", lnvm_info)
> >   		ENTRY("id-ns", "List geometry for LightNVM device", lnvm_id_ns)
> > +		ENTRY("chunk-log", "Chunk information by Get Log Page", lnvm_chunk_log)
> 
> Chunk Information Log Page ?

Sure, I'm fine with that also.

> >   		ENTRY("init", "Initialize media manager on LightNVM device", lnvm_init)
> >   		ENTRY("create", "Create target on top of a LightNVM device", lnvm_create_tgt)
> >   		ENTRY("remove", "Remove target from device", lnvm_remove_tgt)
> 
> Hi Minwoo,
> 
> Could you squash patch 2, 3 and 4 together (4 being the main patch) - They
> belong together to implement one feature.
> 
> As a side-note, we have the same command for ZNS (that will be pushed when
> the ZNS TP is ratified) - In that, we've also added support for supplying
> start lba and number of chunks (zones) to return. Could you add that as well
> to this? Then there is coherency between the two?

Thanks for the review.  Yes I will if the TP has been ratified.  Also
I'll prepare a new series with squashed what you just mentioned here :)

Thanks!

[PATCH 1/5] lnvm: make data_len to sizeof() instead of magic number

[Minwoo Im]

Hi Matias,

On 19-07-24 14:33:39, Matias Bj?rling wrote:
> On 23/07/2019 20.35, Minwoo Im wrote:
> > We can have it as a sizeof() instead of the hard-coded value for the
> > data structure.
> > 
> > Cc: Keith Busch <kbusch at kernel.org>
> > Cc: Matias Bjorling <mb at lightnvm.io>
> > Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
> > ---
> >   nvme-lightnvm.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/nvme-lightnvm.c b/nvme-lightnvm.c
> > index 0b99786..e8cdccd 100644
> > --- a/nvme-lightnvm.c
> > +++ b/nvme-lightnvm.c
> > @@ -442,7 +442,7 @@ static int lnvm_get_identity(int fd, int nsid, struct nvme_nvm_id *nvm_id)
> >   		.opcode		= nvme_nvm_admin_identity,
> >   		.nsid		= nsid,
> >   		.addr		= (__u64)(uintptr_t)nvm_id,
> > -		.data_len	= 0x1000,
> > +		.data_len	= sizeof(struct nvme_nvm_id),
> >   	};
> >   	return nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &cmd);
> 
> Works for me.
> 
> Please use use my Signed-off-by instead of Reviewed-by - traditionally Keith
> has been kind to wait to pull in patches until I got a chance to look at
> them on Github. Having the review here on the mailing list and later as a
> pull request on github makes it explicit that I signed off on it.

Oh okay.  I'll keep that in mind.  But now I'll prepare a V2 series so
that if you review that one also, I'll put your tag below into the
commit and post as a PR on Github once Javier gives some comments on the
previous patch review.

Thanks!

> Signed-off-by: Matias Bj?rling <matias.bjorling at wdc.com>
> 
> Thank you!
> 
> - Matias

[PATCH 4/5] lnvm: introduce chunk-log command for chunk info

[Javier González]

> On 25 Jul 2019,@15.37, Minwoo Im <minwoo.im.dev@gmail.com> wrote:
> 
> Hi Javier,
> 
> Thanks for the review.
> 
>>> +int lnvm_do_chunk_log(int fd, __u32 nsid, __u32 data_len, void *data,
>>> +			unsigned int flags)
>>> +{
>>> +	int err;
>>> +
>>> +	err = nvme_get_log13(fd, nsid, NVM_LID_CHUNK_INFO, 0, 0, 0,
>>> +			false, data_len, data);
>>> +	if (!err) {
>>> +		if (flags & RAW)
>>> +			d_raw(data, data_len);
>>> +		else
>>> +			show_lnvm_chunk_log(data, data_len);
>>> +	} else if (err > 0)
>>> +		fprintf(stderr, "NVMe Status:%s(%x) NSID:%d\n",
>>> +			nvme_status_to_string(err), err, nsid);
>>> +	return err;
>> 
>> What about avoiding the nested if?
>> 
>> If (err) {
>> 	fprintf(stderr, "NVMe Status:%s(%x) NSID:%d\n",
>> 			nvme_status_to_string(err), err, nsid);
>> 
>> 	goto out;
>> }
>> 
>> If (flags & RAW)
>> 	d_raw(data, data_len);
>> else
>> 	show_lnvm_chunk_log(data, data_len);
>> 
>> out:
>> return err;
> 
> That makes sense, we should avoid the case when "err" is negative value
> which means the internal error from the nvme_get_log13() in printing the
> nvme status.  So how about this?
> 
> ```c
> 	if (err > 0) {
> 		fprintf(stderr, "NVMe Status:%s(%x) NSID:%d\n",
> 			nvme_status_to_string(err), err, nsid);
> 
> 		goto out;
> 	} else if (err < 0) {
> 		err = -errno;
> 		perror("nvme_get_log13");
> 
> 		goto out;
> 	}
> 
> 	if (flags & RAW)
> 		d_raw(data, data_len);
> 	else
> 		show_lnvm_chunk_log(data, data_len);
> 
> out:
> 	return err;
> ```

Looks good.

> 
>>> +}
>>> +
>>> static void show_lnvm_bbtbl(struct nvme_nvm_bb_tbl *tbl)
>>> {
>>> 	printf("verid    : %#x\n", (uint16_t)le16_to_cpu(tbl->verid));
>>> diff --git a/nvme-lightnvm.h b/nvme-lightnvm.h
>>> index 7a8ef7d..19660b7 100644
>>> --- a/nvme-lightnvm.h
>>> +++ b/nvme-lightnvm.h
>>> @@ -322,6 +322,7 @@ int lnvm_do_create_tgt(char *, char *, char *, int, int, int, int);
>>> int lnvm_do_remove_tgt(char *);
>>> int lnvm_do_factory_init(char *, int, int, int);
>>> int lnvm_do_id_ns(int, int, unsigned int);
>>> +int lnvm_do_chunk_log(int, __u32, __u32, void *, unsigned int);
>>> int lnvm_do_get_bbtbl(int, int, int, int, unsigned int);
>>> int lnvm_do_set_bbtbl(int, int, int, int, int, int, __u8);
>>> 
>>> diff --git a/plugins/lnvm/lnvm-nvme.c b/plugins/lnvm/lnvm-nvme.c
>>> index 754931a..6f2724a 100644
>>> --- a/plugins/lnvm/lnvm-nvme.c
>>> +++ b/plugins/lnvm/lnvm-nvme.c
>>> @@ -1,5 +1,7 @@
>>> #include <stdio.h>
>>> #include <errno.h>
>>> +#include <stdlib.h>
>>> +#include <unistd.h>
>>> 
>>> #include "nvme.h"
>>> #include "nvme-print.h"
>>> @@ -127,6 +129,88 @@ static int lnvm_id_ns(int argc, char **argv, struct command *cmd, struct plugin
>>> 	return lnvm_do_id_ns(fd, cfg.namespace_id, flags);
>>> }
>>> 
>>> +static int lnvm_chunk_log(int argc, char **argv, struct command *cmd, struct plugin *plugin)
>>> +{
>>> +	const char *desc = "Retrieve the chunk information log for the "\
>>> +		"specified given LightNVM device, returns in either "\
>>> +		"human-readable or binary format.\n"\
>>> +		"This will request Geometry first to get the "\
>>> +		"num_grp,num_pu,num_chk first to figure out the total size "\
>>> +		"of the log pages."\
>>> +		;
>>> +	const char *output_format = "Output format: normal|binary?;
>> 
>> normalbinary -> normal binary?
> 
> Oh It has "|" between them.  But I'm fine with that too.  It was just
> derieved from the nvme.c core module which builtin commands are taking
> now.

Oh, I didn?t see that. My mistake.

Javier
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Message signed with OpenPGP
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20190725/f4a438f8/attachment-0001.sig>

[PATCH 5/5] lnvm: introduce alias geometry for id-ns for lnvm

[Minwoo Im]

Hi Matias,

On 19-07-24 14:29:38, Matias Bj?rling wrote:
> On 23/07/2019 20.36, Minwoo Im wrote:
> > Now we have 2.0 OCSSD spec which introudces Geometry command instead of
> > Identity or something else.  This patch just adds an alias for this
> > command for the given NVMe namespace with backward compatibility.
> > 
> > Cc: Keith Busch <kbusch at kernel.org>
> > Cc: Matias Bjorling <mb at lightnvm.io>
> > Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
> > ---
> >   plugins/lnvm/lnvm-nvme.h | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/plugins/lnvm/lnvm-nvme.h b/plugins/lnvm/lnvm-nvme.h
> > index f091f7b..2879a0b 100644
> > --- a/plugins/lnvm/lnvm-nvme.h
> > +++ b/plugins/lnvm/lnvm-nvme.h
> > @@ -11,7 +11,7 @@ PLUGIN(NAME("lnvm", "LightNVM specific extensions"),
> >   	COMMAND_LIST(
> >   		ENTRY("list", "List available LightNVM devices", lnvm_list)
> >   		ENTRY("info", "List general information and available target engines", lnvm_info)
> > -		ENTRY("id-ns", "List geometry for LightNVM device", lnvm_id_ns)
> > +		ENTRY("id-ns", "List geometry for LightNVM device", lnvm_id_ns, "geometry")
> >   		ENTRY("chunk-log", "Chunk information by Get Log Page", lnvm_chunk_log)
> >   		ENTRY("init", "Initialize media manager on LightNVM device", lnvm_init)
> >   		ENTRY("create", "Create target on top of a LightNVM device", lnvm_create_tgt)
> 
> Hi Minwoo,
> 
> How about also rewording the text to:
> 
> List geometry structure for LightNVM device?

No problem with that.  Not a native speaker so always welcome this kind
of review :)

> Also, do we want to use a short hand (instead of writing geometry in full)?

You mean like "geo" or something like this?

> How is it done elsewhere in the nvme-cli code?

Actually only place that has this kind of _alias_ is fw-commit
subcommand:

```c
	ENTRY("fw-commit", "Verify and commit firmware to a specific slot (fw-activate in old version < 1.2)", fw_commit, "fw-activate")
```

The firmware commit command has been renamed to "fw-activate" so that
this kind of alias has been applied.

But in this patch's case, I just wanted to make sure that the "id-ns"
subcommand is for Geometry command that what the SPEC exactly says.

You have any other words in your mind?  It would be great if you can
propose here :)  OR do you want it as it is with "id-ns" only?

> Regards,
> 
> Matias
> 

Thanks!

[PATCH 4/5] lnvm: introduce chunk-log command for chunk info

[Minwoo Im]

On 19-07-24 14:27:10, Matias Bj?rling wrote:
> Could you squash patch 2, 3 and 4 together (4 being the main patch) - They
> belong together to implement one feature.
> 
> As a side-note, we have the same command for ZNS (that will be pushed when
> the ZNS TP is ratified) - In that, we've also added support for supplying
> start lba and number of chunks (zones) to return. Could you add that as well
> to this? Then there is coherency between the two?

Matias,

I just had a deep look at your slide about the ZNS which can be found at
[1].  Sorry, I misunderstood your meaning here.

This patch already introduced the SLBA in its chunk, but no nr_chunks in
a single summary line.  Do you want me to print it like:

	root at host:~/nvme-cli.git# nvme lnvm chunk-log /dev/nvme0n1
	nr_chunks: 480
	 [    0] { cs: 0x2, ct: 0x1, wli: 2, slba: 0x0000000000000000, cnlb: 0x0000000000001000, wp: 0x0000000000001000 }
	 [    1] { cs: 0x2, ct: 0x1, wli: 1, slba: 0x0000000000001000, cnlb: 0x0000000000001000, wp: 0x0000000000001000 }
	 [    2] { cs: 0x1, ct: 0x1, wli: 0, slba: 0x0000000000002000, cnlb: 0x0000000000001000, wp: 0x0000000000000000 }
	 [    3] { cs: 0x1, ct: 0x1, wli: 0, slba: 0x0000000000003000, cnlb: 0x0000000000001000, wp: 0x0000000000000000 }
	 [    4] { cs: 0x1, ct: 0x1, wli: 0, slba: 0x0000000000004000, cnlb: 0x0000000000001000, wp: 0x0000000000000000 }
	 [    5] { cs: 0x1, ct: 0x1, wli: 0, slba: 0x0000000000005000, cnlb: 0x0000000000001000, wp: 0x0000000000000000 }
	 [    6] { cs: 0x1, ct: 0x1, wli: 0, slba: 0x0000000000006000, cnlb: 0x0000000000001000, wp: 0x0000000000000000 }
	...
	 [  478] { cs: 0x1, ct: 0x1, wli: 0, slba: 0x00000000001fa000, cnlb: 0x0000000000001000, wp: 0x0000000000000000 }
	 [  479] { cs: 0x1, ct: 0x1, wli: 0, slba: 0x00000000001fb000, cnlb: 0x0000000000001000, wp: 0x0000000000000000 }

Thanks!

[1] https://nvmexpress.org/wp-content/uploads/NVMeAnnualMeeting2019-ZNSv3.pptx

[PATCH 4/5] lnvm: introduce chunk-log command for chunk info

[Matias Bjørling]

Hi Minwoo,

Something like this would do:

Total chunks in namespace: %d
...
SLBA: 0xee40000  WP: 0x12345678  CNLB: 0x12345 WLI: XXX   State: FREE
      Type: SEQWRITE_REQ   Attrs: 0x00
...

Thank you,
Matias

On Thu, Jul 25, 2019@5:29 PM Minwoo Im <minwoo.im.dev@gmail.com> wrote:
>
> On 19-07-24 14:27:10, Matias Bj?rling wrote:
> > Could you squash patch 2, 3 and 4 together (4 being the main patch) - They
> > belong together to implement one feature.
> >
> > As a side-note, we have the same command for ZNS (that will be pushed when
> > the ZNS TP is ratified) - In that, we've also added support for supplying
> > start lba and number of chunks (zones) to return. Could you add that as well
> > to this? Then there is coherency between the two?
>
> Matias,
>
> I just had a deep look at your slide about the ZNS which can be found at
> [1].  Sorry, I misunderstood your meaning here.
>
> This patch already introduced the SLBA in its chunk, but no nr_chunks in
> a single summary line.  Do you want me to print it like:
>
>         root at host:~/nvme-cli.git# nvme lnvm chunk-log /dev/nvme0n1
>         nr_chunks: 480
>          [    0] { cs: 0x2, ct: 0x1, wli: 2, slba: 0x0000000000000000, cnlb: 0x0000000000001000, wp: 0x0000000000001000 }
>          [    1] { cs: 0x2, ct: 0x1, wli: 1, slba: 0x0000000000001000, cnlb: 0x0000000000001000, wp: 0x0000000000001000 }
>          [    2] { cs: 0x1, ct: 0x1, wli: 0, slba: 0x0000000000002000, cnlb: 0x0000000000001000, wp: 0x0000000000000000 }
>          [    3] { cs: 0x1, ct: 0x1, wli: 0, slba: 0x0000000000003000, cnlb: 0x0000000000001000, wp: 0x0000000000000000 }
>          [    4] { cs: 0x1, ct: 0x1, wli: 0, slba: 0x0000000000004000, cnlb: 0x0000000000001000, wp: 0x0000000000000000 }
>          [    5] { cs: 0x1, ct: 0x1, wli: 0, slba: 0x0000000000005000, cnlb: 0x0000000000001000, wp: 0x0000000000000000 }
>          [    6] { cs: 0x1, ct: 0x1, wli: 0, slba: 0x0000000000006000, cnlb: 0x0000000000001000, wp: 0x0000000000000000 }
>         ...
>          [  478] { cs: 0x1, ct: 0x1, wli: 0, slba: 0x00000000001fa000, cnlb: 0x0000000000001000, wp: 0x0000000000000000 }
>          [  479] { cs: 0x1, ct: 0x1, wli: 0, slba: 0x00000000001fb000, cnlb: 0x0000000000001000, wp: 0x0000000000000000 }
>
> Thanks!
>
> [1] https://nvmexpress.org/wp-content/uploads/NVMeAnnualMeeting2019-ZNSv3.pptx

[PATCH 4/5] lnvm: introduce chunk-log command for chunk info

[Minwoo Im]

On 19-07-27 17:25:24, Matias Bj?rling wrote:
> Hi Minwoo,
> 
> Something like this would do:
> 
> Total chunks in namespace: %d
> ...
> SLBA: 0xee40000  WP: 0x12345678  CNLB: 0x12345 WLI: XXX   State: FREE
>       Type: SEQWRITE_REQ   Attrs: 0x00
> ...

Hi Matias,

Okay with that.  But I'm not sure what the "Attrs:" means here.  I'll
prepare the next version series with that format of print without Attrs
first.

Thanks,