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

RFC: xnvme backend support #1873

Closed
wants to merge 1 commit into from

Conversation

vikash-k5
Copy link
Contributor

Extended “struct nvme_dev” with the following device-handle types:

  • struct dev_handle
  • enum nvme_dev_type

The ‘dev_handle' contains ‘struct xnvme_dev*’, file-descripter and ‘nvme_dev_type’ to manage library-side dispatch.

Adding ‘open_dev_xnvme(struct nvme_dev **devp, char *devname)’ function to open xNVMe library device handle and same being passed to ‘libnvme’ through ‘struct dev_handle’.

This xnvme plugin can work on different platform such as linux, freebsd and even windows. As well as this can work with different driver on linux(kernel and userspace driver such as SPDK)

Note: this will require libnvme changes, which can be found here
linux-nvme/libnvme#600

…rivers, io_uring command on Linux. As well as using nvme-cli natively on FreeBSD. This is done by dispatching commands to xNVMe.

Signed-off-by: Vikash kumar <[email protected]>
Copy link
Collaborator

@igaw igaw left a comment

Choose a reason for hiding this comment

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

It looks more of the transport abstraction should be in libnvme.

@@ -49,6 +49,7 @@ libnvme_dep = dependency('libnvme', version: '>=1.3', required: true,
fallback : ['libnvme', 'libnvme_dep'])
libnvme_mi_dep = dependency('libnvme-mi', required: true,
fallback : ['libnvme', 'libnvme_mi_dep'])
libxnvme_dep = dependency('xnvme', version: '>=0.2.0', required: true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be guarded by meson option setup option to disable it completely, see json-c as example. Some folks really don't like if we add hard dependencies.

@@ -20,8 +20,8 @@
*/
#define do_admin_op(op, d, ...) ({ \
int __rc; \
if (d->type == NVME_DEV_DIRECT) \
__rc = nvme_ ## op(d->direct.fd, __VA_ARGS__); \
if (d->type == NVME_DEV_DIRECT || d->type == NVME_DEV_XNVME) \
Copy link
Collaborator

Choose a reason for hiding this comment

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

The complete nvme-wrap.c file shouldn't be needed after introducing the transport abstraction.

}



Copy link
Collaborator

Choose a reason for hiding this comment

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

Please avoid doing whitespace changes or adding too many newlines here.

}



static int get_dev(struct nvme_dev **dev, int argc, char **argv, int flags)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we are going to use the newly introduce transport abstraction from the library through out nvme-cli? Ideally we have this open device in libnvme, no?

@@ -5535,8 +5575,8 @@ static int set_property(int argc, char **argv, struct command *cmd, struct plugi
}

struct nvme_set_property_args args = {
.hnd = dev_fd(dev),
Copy link
Collaborator

Choose a reason for hiding this comment

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

dev_fd should be renamed of course.

@@ -69,15 +66,15 @@ struct nvme_dev {

#define dev_fd(d) __dev_fd(d, __func__, __LINE__)

static inline int __dev_fd(struct nvme_dev *dev, const char *func, int line)
static inline struct dev_handle* __dev_fd(struct nvme_dev *dev, const char *func, int line)
Copy link
Collaborator

Choose a reason for hiding this comment

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

as previously said, this function should be renamed.

@vikash-k5
Copy link
Contributor Author

Created new PR addressing review comments

@vikash-k5 vikash-k5 closed this Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants