nvme: add dry-run NVME_ARGS default option by ikegami-t · Pull Request #2751 · linux-nvme/nvme-cli

ikegami-t · 2025-03-27T18:17:41Z

The option is to show command instead of sending.

igaw · 2025-04-01T12:05:36Z

 		OPT_FMT("output-format", 'o', &nvme_cfg.output_format, output_format), \
 		##__VA_ARGS__,                                                         \
 		OPT_UINT("timeout",      't', &nvme_cfg.timeout,       timeout),       \
+		OPT_FLAG("dry-run",      'd', &nvme_cfg.dry_run,       dry_run),       \


Let's use only the long option name for it. It's likely one command is already using -d and I'd like to avoid further confusion with the short options name.

Agree so fixed as mentioned. Note: To make sure let me describe the changes as below.

The admin-passthru and io-passthru command using passthru() deleted the short option -d for the option --dry-run.

The comapre, read and write commands using submit_io() delete the short option -w for the option --dry-run.

Other commands using NVME_ARGS added the option --dry-run withtout the short option -d.

igaw · 2025-04-01T12:09:05Z

+	int err = 0;

-	if (log_level >= LOG_DEBUG)
+	if (log_level >= LOG_DEBUG || dry_run)


I'd rather not mix the log level with the --dry-run options. I suggest to update the documentation instead.

Or if you don't agree, the --dry-run should then really only print the command nothing else.

Yes right so fixed to output only command input data only without the result and err outputs for the --dry-run option.

ikegami-t · 2025-04-01T16:13:35Z

Fixed as mentioned and just rebased. Thank you.

igaw · 2025-04-02T07:07:33Z

@@ -8295,7 +8293,7 @@ static int submit_io(int opcode, char *command, const char *desc, int argc, char
 		printf("pif             : %02x\n", pif);


Wouldn't it make sense to move this part to logging.c eventually? I would love to see that we extend the logging there to print more of the commands. But this is just an idea for a new side project :)

Right so okay I will do this by a separate patch. Thank you.

igaw · 2025-04-02T07:10:23Z

 		  OPT_SHRT("dir-spec",          'S', &cfg.dspec,             dspec),
 		  OPT_BYTE("dsm",               'D', &cfg.dsmgmt,            dsm),
 		  OPT_FLAG("show-command",      'V', &cfg.show,              show),
-		  OPT_FLAG("dry-run",           'w', &cfg.dry_run,           dry),


I think we should keep the -w short hand here, to stay compatible.

Fixed as mentioned.

igaw · 2025-04-02T07:10:53Z

 	struct timeval start;
 	struct timeval end;
-	int err;
+	int err = 0;


This is an unrelated change, not really needed.

Yes so fixed.

The option is to show command instead of sending. Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>

Also latency output moved into logging as same. Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>

igaw · 2025-04-14T09:08:39Z

+	printf("sts             : %02x\n", args->sts);
+}
+
+int nvme_submit_io(struct nvme_io_args *args, __u8 opcode, bool show, bool latency)


I suggest instead adding more indirect code for the logging, we could just look at the opcode in nvme_show_command, e.g.

static void nvme_show_command(struct nvme_passthru_cmd *cmd, int err) { switch (cmd->opcode) { case nvme_cmd_read: case nvme_cmd_write: case nvme_cmd_compare: nvme_show_io_command(cmd); break; default: nvme_show_common(cmd); } printf("result : %08x\n", cmd->result); printf("err : %d\n", err); }

igaw · 2025-04-14T09:17:09Z

+	if (show || dry_run)
+		nvme_show_io_command(args, opcode);
+
+	if (dry_run)


and then the dry_run maybe should be

int nvme_submit_passthru(int fd, unsigned long ioctl_cmd, struct nvme_passthru_cmd *cmd, __u32 *result) { struct timeval start; struct timeval end; int err = 0; if (log_level >= LOG_DEBUG) gettimeofday(&start, NULL); if (!dry_run) err = ioctl(fd, ioctl_cmd, cmd); if (log_level >= LOG_DEBUG) { gettimeofday(&end, NULL); nvme_show_command(cmd, err); nvme_show_latency(start, end); } if (err >= 0 && result) *result = cmd->result; return err; }

ikegami-t · 2025-04-14T13:01:49Z

@igaw Thanks for your review comments. Basically look good but let me confirm below before the fix.

The suggestion will change the some logging behaviors so are the changes okay for now? Or still should we keep the logging behavior basically?
Basically the struct nvme_passthru_cmd values opcode and cdw, etc. can be converted to nvme_show_io_command() outputs values nlb and slba, etc. but only the value storage_tag needed pif and sts values to pass and also 'args_size' needed probably. The nvme-cli converts struct config to struct nvme_io_args and the libnvme converts struct nvme_io_args to struct nvme_passthru_cmd then the logging feature converts the struct nvme_passthru_cmd to struct nvme_io_args again. Is this really required?

igaw · 2025-04-28T18:29:24Z

I think this would be a very good thing to have in place for being able to add more tests and make the libnvme2 development a bit more less eventful. My idea is to tests for a bunch of command, e.g.

nvme id-ctrl --dry-run --verbose /dev/nvme0 | nvme-check-passthru test-data/id-ctrl.log

and nvme-check-passthru would compare if the command words are identically and obviously filter out the dynamic values such pointers.

I am fine with changing some of the logging behavior. This is something I don't feel it is so important to be stable. Let be give a go with your patches as I want to figure out how to get the testframe work done.

igaw · 2025-04-28T18:44:57Z

I think the -V (show command) and -t (show latency) could just be mapped to the verbosity levels. This is not 100% the same output but as long these two options still do something it should be fine.

igaw · 2025-04-28T19:45:21Z

I've got something here:
igaw@607f130

Not sure if it is possible to write get a parser for getting the storage tag etc back from the command words, e.g. STS, PIF, etc. Would be nice if this is possible.

igaw · 2025-04-28T20:01:45Z

On the other hand we could leave the submit_io logging in place as it shows the higher level view (e.g. storage tag, sts, etc) and the low level output for the command dwords could also be there... let me play with this.

ikegami-t · 2025-04-29T09:29:35Z

Thanks for your explanations. I can agree with you.

I've got something here: igaw@607f130

Yes the commit looks good.

Not sure if it is possible to write get a parser for getting the storage tag etc back from the command words, e.g. STS, PIF, etc. Would be nice if this is possible.
On the other hand we could leave the submit_io logging in place as it shows the higher level view (e.g. storage tag, sts, etc) and the low level output for the command dwords could also be there... let me play with this.

Right and good suggetion to leave the higher level output also.

igaw · 2025-04-29T12:28:05Z

I think we can leave the submit_io code where it is. One thing we might consider is to make it a bit more useful by "beautifying" the output. But this is a different task.

I'll update my version and submit it properly for review.

ikegami-t · 2025-04-29T13:48:18Z

Thank you. Noted it. Let me close this PR.

(Added)
Since the commit: igaw@607f130 can be applied directly but if needed I can reopen the PR again so please let me know then.

igaw reviewed Apr 1, 2025

View reviewed changes

ikegami-t force-pushed the nvme-dry-run branch from 405f649 to e331184 Compare April 1, 2025 16:01

igaw reviewed Apr 2, 2025

View reviewed changes

ikegami-t force-pushed the nvme-dry-run branch from e331184 to 41de986 Compare April 2, 2025 13:26

nvme: add dry-run NVME_ARGS default option

e1882cd

The option is to show command instead of sending. Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>

ikegami-t force-pushed the nvme-dry-run branch 3 times, most recently from 648bf0a to 8bc4f5b Compare April 13, 2025 11:42

nvme: move submit_io show-command and dry-run outputs into logging

e1667bd

Also latency output moved into logging as same. Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>

ikegami-t force-pushed the nvme-dry-run branch from 8bc4f5b to e1667bd Compare April 13, 2025 11:45

igaw reviewed Apr 14, 2025

View reviewed changes

ikegami-t mentioned this pull request Apr 22, 2025

Mi admin passthru linux-nvme/libnvme#983

Merged

ikegami-t closed this Apr 29, 2025

		@@ -8295,7 +8293,7 @@ static int submit_io(int opcode, char command, const char desc, int argc, char
		printf("pif : %02x\n", pif);

Conversation

ikegami-t commented Mar 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikegami-t commented Apr 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igaw Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikegami-t commented Apr 14, 2025

Uh oh!

igaw commented Apr 28, 2025

Uh oh!

igaw commented Apr 28, 2025

Uh oh!

igaw commented Apr 28, 2025

Uh oh!

igaw commented Apr 28, 2025

Uh oh!

ikegami-t commented Apr 29, 2025

Uh oh!

igaw commented Apr 29, 2025

Uh oh!

ikegami-t commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

igaw Apr 14, 2025 •

edited

Loading

ikegami-t commented Apr 29, 2025 •

edited

Loading