| Message ID | 20190725172335.6825-5-logang@deltatee.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nvmet: add target passthru commands support | expand |
On Thu, Jul 25, 2019 at 11:23:23AM -0600, Logan Gunthorpe wrote: > nvme_get_by_path() is analagous to blkdev_get_by_path() except it > gets a struct nvme_ctrl from the path to its char dev (/dev/nvme0). > > The purpose of this function is to support NVMe-OF target passthru. I can't find anywhere that you use this in this patchset.
On 2019-07-25 11:50 a.m., Matthew Wilcox wrote: > On Thu, Jul 25, 2019 at 11:23:23AM -0600, Logan Gunthorpe wrote: >> nvme_get_by_path() is analagous to blkdev_get_by_path() except it >> gets a struct nvme_ctrl from the path to its char dev (/dev/nvme0). >> >> The purpose of this function is to support NVMe-OF target passthru. > > I can't find anywhere that you use this in this patchset. > Oh sorry, the commit message is out of date the function was actually called nvme_ctrl_get_by_path() and it's used in Patch 10. I'll fix the commit message for the next version. Logan
On Thu, Jul 25, 2019 at 11:54:18AM -0600, Logan Gunthorpe wrote: > > > On 2019-07-25 11:50 a.m., Matthew Wilcox wrote: > > On Thu, Jul 25, 2019 at 11:23:23AM -0600, Logan Gunthorpe wrote: > >> nvme_get_by_path() is analagous to blkdev_get_by_path() except it > >> gets a struct nvme_ctrl from the path to its char dev (/dev/nvme0). > >> > >> The purpose of this function is to support NVMe-OF target passthru. > > > > I can't find anywhere that you use this in this patchset. > > > > Oh sorry, the commit message is out of date the function was actually > called nvme_ctrl_get_by_path() and it's used in Patch 10. Instead of by path, could we have configfs take something else, like the unique controller instance or serial number? I know that's different than how we handle blocks and files, but that way nvme core can lookup the cooresponding controller without adding new cdev dependencies.
>>>> nvme_get_by_path() is analagous to blkdev_get_by_path() except it >>>> gets a struct nvme_ctrl from the path to its char dev (/dev/nvme0). >>>> >>>> The purpose of this function is to support NVMe-OF target passthru. >>> >>> I can't find anywhere that you use this in this patchset. >>> >> >> Oh sorry, the commit message is out of date the function was actually >> called nvme_ctrl_get_by_path() and it's used in Patch 10. > > Instead of by path, could we have configfs take something else, like > the unique controller instance or serial number? I know that's different > than how we handle blocks and files, but that way nvme core can lookup > the cooresponding controller without adding new cdev dependencies. We could... but did we find sufficient justification to have the user handle passthru devices differently than any other backend? once we commit to an interface its very hard to change.
On 2019-07-25 1:58 p.m., Keith Busch wrote: > On Thu, Jul 25, 2019 at 11:54:18AM -0600, Logan Gunthorpe wrote: >> >> >> On 2019-07-25 11:50 a.m., Matthew Wilcox wrote: >>> On Thu, Jul 25, 2019 at 11:23:23AM -0600, Logan Gunthorpe wrote: >>>> nvme_get_by_path() is analagous to blkdev_get_by_path() except it >>>> gets a struct nvme_ctrl from the path to its char dev (/dev/nvme0). >>>> >>>> The purpose of this function is to support NVMe-OF target passthru. >>> >>> I can't find anywhere that you use this in this patchset. >>> >> >> Oh sorry, the commit message is out of date the function was actually >> called nvme_ctrl_get_by_path() and it's used in Patch 10. > > Instead of by path, could we have configfs take something else, like > the unique controller instance or serial number? I know that's different > than how we handle blocks and files, but that way nvme core can lookup > the cooresponding controller without adding new cdev dependencies. Well the previous version of the patchset just used the ctrl name ("nvme1") and looped through all the controllers to find a match. But this sucks because of the inconsistency and the fact that the name can change if hardware changes and the number changes. Allowing the user to make use of standard udev rules seems important to me. Logan
On Thu, Jul 25, 2019 at 02:28:28PM -0600, Logan Gunthorpe wrote: > > > On 2019-07-25 1:58 p.m., Keith Busch wrote: > > On Thu, Jul 25, 2019 at 11:54:18AM -0600, Logan Gunthorpe wrote: > >> > >> > >> On 2019-07-25 11:50 a.m., Matthew Wilcox wrote: > >>> On Thu, Jul 25, 2019 at 11:23:23AM -0600, Logan Gunthorpe wrote: > >>>> nvme_get_by_path() is analagous to blkdev_get_by_path() except it > >>>> gets a struct nvme_ctrl from the path to its char dev (/dev/nvme0). > >>>> > >>>> The purpose of this function is to support NVMe-OF target passthru. > >>> > >>> I can't find anywhere that you use this in this patchset. > >>> > >> > >> Oh sorry, the commit message is out of date the function was actually > >> called nvme_ctrl_get_by_path() and it's used in Patch 10. > > > > Instead of by path, could we have configfs take something else, like > > the unique controller instance or serial number? I know that's different > > than how we handle blocks and files, but that way nvme core can lookup > > the cooresponding controller without adding new cdev dependencies. > > Well the previous version of the patchset just used the ctrl name > ("nvme1") and looped through all the controllers to find a match. But > this sucks because of the inconsistency and the fact that the name can > change if hardware changes and the number changes. Allowing the user to > make use of standard udev rules seems important to me. Should we then create a new udev rule for persistent controller names? /dev/nvme1 may not be the same controller each time you refer to it.
On 2019-07-25 2:31 p.m., Keith Busch wrote: > On Thu, Jul 25, 2019 at 02:28:28PM -0600, Logan Gunthorpe wrote: >> >> >> On 2019-07-25 1:58 p.m., Keith Busch wrote: >>> On Thu, Jul 25, 2019 at 11:54:18AM -0600, Logan Gunthorpe wrote: >>>> >>>> >>>> On 2019-07-25 11:50 a.m., Matthew Wilcox wrote: >>>>> On Thu, Jul 25, 2019 at 11:23:23AM -0600, Logan Gunthorpe wrote: >>>>>> nvme_get_by_path() is analagous to blkdev_get_by_path() except it >>>>>> gets a struct nvme_ctrl from the path to its char dev (/dev/nvme0). >>>>>> >>>>>> The purpose of this function is to support NVMe-OF target passthru. >>>>> >>>>> I can't find anywhere that you use this in this patchset. >>>>> >>>> >>>> Oh sorry, the commit message is out of date the function was actually >>>> called nvme_ctrl_get_by_path() and it's used in Patch 10. >>> >>> Instead of by path, could we have configfs take something else, like >>> the unique controller instance or serial number? I know that's different >>> than how we handle blocks and files, but that way nvme core can lookup >>> the cooresponding controller without adding new cdev dependencies. >> >> Well the previous version of the patchset just used the ctrl name >> ("nvme1") and looped through all the controllers to find a match. But >> this sucks because of the inconsistency and the fact that the name can >> change if hardware changes and the number changes. Allowing the user to >> make use of standard udev rules seems important to me. > > Should we then create a new udev rule for persistent controller > names? /dev/nvme1 may not be the same controller each time you refer > to it. Udev can only create symlinks from /dev/nvme0 to /dev/nvme-persistent-name and users can do this as they need now. No changes needed. My point was if we use the ctrl name (nvme0) as a reference then the kernel can't make use of these symlinks or anything udev does seeing that name is internal to the kernel only. If we use cdev_get_by_path()/nvme_ctrl_get_by_path() then this isn't a problem as we can open a symlink to /dev/nvme0 without any issues. Logan
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 00399c3536fa..61e7b016ec36 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -2816,6 +2816,29 @@ static const struct file_operations nvme_dev_fops = { .compat_ioctl = nvme_dev_ioctl, }; +struct nvme_ctrl *nvme_ctrl_get_by_path(const char *path) +{ + struct nvme_ctrl *ctrl; + struct cdev *cdev; + + cdev = cdev_get_by_path(path); + if (IS_ERR(cdev)) + return ERR_CAST(cdev); + + if (cdev->ops != &nvme_dev_fops) { + ctrl = ERR_PTR(-EINVAL); + goto out_cdev_put; + } + + ctrl = container_of(cdev, struct nvme_ctrl, cdev); + nvme_get_ctrl(ctrl); + +out_cdev_put: + cdev_put(cdev); + return ctrl; +} +EXPORT_SYMBOL_GPL(nvme_ctrl_get_by_path); + static ssize_t nvme_sysfs_reset(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index da3d130773c4..a19bd0091de6 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -484,6 +484,8 @@ int nvme_get_log(struct nvme_ctrl *ctrl, u32 nsid, u8 log_page, u8 lsp, extern const struct attribute_group *nvme_ns_id_attr_groups[]; extern const struct block_device_operations nvme_ns_head_ops; +struct nvme_ctrl *nvme_ctrl_get_by_path(const char *path); + #ifdef CONFIG_NVME_MULTIPATH bool nvme_ctrl_use_ana(struct nvme_ctrl *ctrl); void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns,
nvme_get_by_path() is analagous to blkdev_get_by_path() except it gets a struct nvme_ctrl from the path to its char dev (/dev/nvme0). The purpose of this function is to support NVMe-OF target passthru. Signed-off-by: Logan Gunthorpe <logang@deltatee.com> --- drivers/nvme/host/core.c | 23 +++++++++++++++++++++++ drivers/nvme/host/nvme.h | 2 ++ 2 files changed, 25 insertions(+)