Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nvme: fix verbose logging for certain commands #2406

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

martin-gpy
Copy link
Contributor

Verbose logging for certain nvme commands like id-ctrl, id-ns, smart-log, sanitize-log, etc. does not print anything additional or verbose compared to the regular output. These commands already implement a human-readable option which works fine, but it is the verbose option that is broken. So fix the verbose option here to ensure it is on par with the human-readable option. Later if required, we can eliminate any one of these options for these commands to avoid redundancy.

@ikegami-t
Copy link
Contributor

The verbose option shows detail information if twice input as -vv for example. (If only once specificed the command latency only ouput.) Some commands options may be duplicated with the human-readable and verbose options but basically the verbose option is for the command detail information output but not human readable I think.

@martin-gpy
Copy link
Contributor Author

martin-gpy commented Jul 9, 2024

The verbose option shows detail information if twice input as -vv for example. (If only once specificed the command latency only ouput.)

Yes, I know. But the current -v invocation for these commands is practically a no-op since it only prints latency info along with the regular output, and that is what this patch attempts to address here. If -vv is specified, it will continue to display additional ioctl structure values as before. This patch doesn't hinder that. So we are still good here.

Some commands options may be duplicated with the human-readable and verbose options but basically the verbose option is for the command detail information output but not human readable I think.

As described above, this patch only fixes the -v invocation which is currently similar to a no-op for these commands. Doesn't alter anything else like the -vv option. And it fixes this -v option by making it on par with the human readable option. As already described in the commit, this does have the downside that you now end up having two redundant options but that anyways is better than having a no-op -v option in the first place, isn't it? And that could be cleaned up later if desired.

@igaw
Copy link
Collaborator

igaw commented Jul 9, 2024

I like the idea to use -v as synonym for -H. If we go this route I think we should move the latency output to the next level of verbose (-vv). TBH, I've never liked it to have it split away from the debug output.

@martin-gpy
Copy link
Contributor Author

I like the idea to use -v as synonym for -H. If we go this route I think we should move the latency output to the next level of verbose (-vv). TBH, I've never liked it to have it split away from the debug output.

Ah, ok. So basically you are asking to:

  • Make -v synonymous with -H (which this patch already does)
  • Additionally display the latency & ioctl command values in the debug (i.e. -vv) output.

Will give that a try here.

@igaw
Copy link
Collaborator

igaw commented Jul 9, 2024

Yes. I agree with your reasoning to retire -H eventually and use the more generic -v flag for all commands.

IIRC, the latency output is printed at INFO level instead of DEBUG.

@martin-gpy martin-gpy force-pushed the fix_verbose_logging branch from 9c4b119 to 64349b2 Compare July 9, 2024 17:41
The -v verbose option for certain nvme commands like id-ctrl,
id-ns, smart-log, sanitize-log, etc. is practically a no-op since
it only prints latency info in addition to the regular output.
But at the same time, these commands already implement a -H human
readable option which prints additional useful info. So fix the
-v option to make it synonymous with the -H option here. And while
we are at it, move the latency info from current INFO level (i.e.
-v logging) to DEBUG level (i.e. -vv logging) since it is better
suited there.

Signed-off-by: Martin George <[email protected]>
@martin-gpy martin-gpy force-pushed the fix_verbose_logging branch from 64349b2 to 86b9464 Compare July 9, 2024 17:59
@igaw igaw merged commit 5cbfb18 into linux-nvme:master Jul 10, 2024
17 checks passed
@igaw
Copy link
Collaborator

igaw commented Jul 10, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants