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-discover: lookup existing persistent controllers #942

Closed
wants to merge 1 commit into from

Conversation

hreinecke
Copy link
Collaborator

If persistent controller connections are present they should be
preferred even if no --device option is given on the commandline.
To avoid selecting a temporary non-persistent controller the
'kato' attribute is checked; any controller which does not have
this attribute or for which the attribute is '0' will be skipped.

I have submitted a patch for adding the 'kato' sysfs controller attribute to linux-nvme.

Signed-off-by: Hannes Reinecke hare@suse.de

If persistent controller connections are present they should be
preferred even if no --device option is given on the commandline.
To avoid selecting a temporary non-persistent controller the
'kato' attribute is checked; any controller which does not have
this attribute or for which the attribute is '0' will be skipped.

Signed-off-by: Hannes Reinecke <hare@suse.de>
@@ -320,6 +320,27 @@ static bool ctrl_matches_connectargs(char *name, struct connect_args *args)
cargs.trsvcid = parse_conn_arg(addr, ' ', conarg_trsvcid);
cargs.host_traddr = parse_conn_arg(addr, ' ', conarg_host_traddr);

if (!strcmp(cargs.subsysnqn, NVME_DISC_SUBSYS_NAME)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest moving this test further down after the !strcmp(cargs.subsysnqn, args->subsysnqn). We don't need to test this if we aren't looking for a discovery controller.

*
* The 'kato' attribute might not be present; assume a
* non-persistent controller for these installations.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the device string was set by the user on the command line and there is no kato attribute, we should assume that the controller is usable (i.e. will persist at least until we try to retrieve the discovery page). Otherwise you basically disable the --device options on kernels that don't support kato.

So, this logic only makes sense to me if this function is called from find_ctrl_with_connectargs(). But OTOH, why not generally assume that the controller is persistent if there is no kato attribute? The worst thing that can happen is an error attempting to connect to it, and this error will be transient by definition.

@mwilck
Copy link
Contributor

mwilck commented Mar 5, 2021

There's another issue with this patch: referrals. Checking a given cfg.device against the connect args makes sense if do_discover() is called recursively from connect_ctrl(). If we don't do this, the discovery is almost guaranteed to fail.

Forget this comment. I misread your patch. Sorry.

@mwilck
Copy link
Contributor

mwilck commented Mar 5, 2021

I wanted to send a PR to your branch, but github won't let me do that. So please just have a look at the two topmost commit on https://github.com/mwilck/nvme-cli/tree/persistent-discovery

@mwilck
Copy link
Contributor

mwilck commented Apr 1, 2021

#992 should supersede this PR. @hreinecke, please double-check.

@hreinecke
Copy link
Collaborator Author

#992 should supersede this PR. @hreinecke, please double-check.

Yes, it does. And if not I'll rebase it on top of the (now-merged) #992.

@hreinecke hreinecke closed this Apr 2, 2021
@hreinecke
Copy link
Collaborator Author

Superseded by #992

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.

None yet

2 participants