[PATCH 4/4] nvme: enable char device per namespace
Minwoo Im
minwoo.im.dev at gmail.com
Tue Dec 1 09:03:48 EST 2020
Hello,
On 20-12-01 13:56:10, javier at javigon.com wrote:
> From: Javier González <javier.gonz at samsung.com>
>
> Create a char device per NVMe namespace. This char device is always
> initialized, independently of whether thedeatures implemented by the
> device are supported by the kernel. User-space can therefore always
> issue IOCTLs to the NVMe driver using this char device.
>
> The char device is presented as /dev/nvmeXcYnZ to follow the hidden
> block device. This naming also aligns with nvme-cli filters, so the char
> device should be usable without tool changes.
>
> Signed-off-by: Javier González <javier.gonz at samsung.com>
> ---
> drivers/nvme/host/core.c | 144 +++++++++++++++++++++++++++++++++++----
> drivers/nvme/host/nvme.h | 3 +
> 2 files changed, 132 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 2c23ea6dc296..9c4acf2725f3 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -86,7 +86,9 @@ static DEFINE_MUTEX(nvme_subsystems_lock);
>
> static DEFINE_IDA(nvme_instance_ida);
> static dev_t nvme_ctrl_base_chr_devt;
> +static dev_t nvme_ns_base_chr_devt;
> static struct class *nvme_class;
> +static struct class *nvme_ns_class;
> static struct class *nvme_subsys_class;
>
> static void nvme_put_subsystem(struct nvme_subsystem *subsys);
> @@ -497,6 +499,7 @@ static void nvme_free_ns(struct kref *kref)
> if (ns->ndev)
> nvme_nvm_unregister(ns);
>
> + cdev_device_del(&ns->cdev, &ns->cdev_device);
> put_disk(ns->disk);
> nvme_put_ns_head(ns->head);
> nvme_put_ctrl(ns->ctrl);
> @@ -1696,15 +1699,15 @@ static int nvme_handle_ctrl_ioctl(struct nvme_ns *ns, unsigned int cmd,
> return ret;
> }
>
> -static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
> - unsigned int cmd, unsigned long arg)
> +static int __nvme_ns_ioctl(struct gendisk *disk, unsigned int cmd,
> + unsigned long arg)
> {
> struct nvme_ns_head *head = NULL;
> void __user *argp = (void __user *)arg;
> struct nvme_ns *ns;
> int srcu_idx, ret;
>
> - ns = nvme_get_ns_from_disk(bdev->bd_disk, &head, &srcu_idx);
> + ns = nvme_get_ns_from_disk(disk, &head, &srcu_idx);
> if (unlikely(!ns))
> return -EWOULDBLOCK;
>
> @@ -1741,6 +1744,18 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
> return ret;
> }
>
> +static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
> + unsigned int cmd, unsigned long arg)
> +{
> + return __nvme_ns_ioctl(bdev->bd_disk, cmd, arg);
> +}
> +
> +static long nvme_cdev_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + return __nvme_ns_ioctl((struct gendisk *)file->private_data, cmd, arg);
> +}
> +
> #ifdef CONFIG_COMPAT
> struct nvme_user_io32 {
> __u8 opcode;
> @@ -1782,10 +1797,8 @@ static int nvme_compat_ioctl(struct block_device *bdev, fmode_t mode,
> #define nvme_compat_ioctl NULL
> #endif /* CONFIG_COMPAT */
>
> -static int nvme_open(struct block_device *bdev, fmode_t mode)
> +static int __nvme_open(struct nvme_ns *ns)
> {
> - struct nvme_ns *ns = bdev->bd_disk->private_data;
> -
> #ifdef CONFIG_NVME_MULTIPATH
> /* should never be called due to GENHD_FL_HIDDEN */
> if (WARN_ON_ONCE(ns->head->disk))
> @@ -1804,12 +1817,24 @@ static int nvme_open(struct block_device *bdev, fmode_t mode)
> return -ENXIO;
> }
>
> +static void __nvme_release(struct nvme_ns *ns)
> +{
> + module_put(ns->ctrl->ops->module);
> + nvme_put_ns(ns);
> +}
> +
> +static int nvme_open(struct block_device *bdev, fmode_t mode)
> +{
> + struct nvme_ns *ns = bdev->bd_disk->private_data;
> +
> + return __nvme_open(ns);
> +}
> +
> static void nvme_release(struct gendisk *disk, fmode_t mode)
> {
> struct nvme_ns *ns = disk->private_data;
>
> - module_put(ns->ctrl->ops->module);
> - nvme_put_ns(ns);
> + __nvme_release(ns);
> }
>
> static int nvme_getgeo(struct block_device *bdev, struct hd_geometry *geo)
> @@ -1821,6 +1846,26 @@ static int nvme_getgeo(struct block_device *bdev, struct hd_geometry *geo)
> return 0;
> }
>
> +static int nvme_cdev_open(struct inode *inode, struct file *file)
> +{
> + struct nvme_ns *ns = container_of(inode->i_cdev, struct nvme_ns, cdev);
> + int ret;
> +
> + ret = __nvme_open(ns);
> + if (!ret)
> + file->private_data = ns->disk;
> +
> + return ret;
> +}
> +
> +static int nvme_cdev_release(struct inode *inode, struct file *file)
> +{
> + struct nvme_ns *ns = container_of(inode->i_cdev, struct nvme_ns, cdev);
> +
> + __nvme_release(ns);
> + return 0;
> +}
> +
> #ifdef CONFIG_BLK_DEV_INTEGRITY
> static void nvme_init_integrity(struct gendisk *disk, u16 ms, u8 pi_type,
> u32 max_integrity_segments)
> @@ -2303,6 +2348,14 @@ static const struct block_device_operations nvme_bdev_ops = {
> .pr_ops = &nvme_pr_ops,
> };
>
> +static const struct file_operations nvme_cdev_fops = {
> + .owner = THIS_MODULE,
> + .open = nvme_cdev_open,
> + .release = nvme_cdev_release,
> + .unlocked_ioctl = nvme_cdev_ioctl,
> + .compat_ioctl = compat_ptr_ioctl,
> +};
> +
> #ifdef CONFIG_NVME_MULTIPATH
> static int nvme_ns_head_open(struct block_device *bdev, fmode_t mode)
> {
> @@ -3301,6 +3354,9 @@ static inline struct nvme_ns_head *dev_to_ns_head(struct device *dev)
> {
> struct gendisk *disk = dev_to_disk(dev);
>
> + if (dev->class == nvme_ns_class)
> + return ((struct nvme_ns *)dev_get_drvdata(dev))->head;
I think it would be better if it can have inline function
nvme_get_ns_from_cdev() just like nvme_get_ns_from_dev().
> +
> if (disk->fops == &nvme_bdev_ops)
> return nvme_get_ns_from_dev(dev)->head;
> else
> @@ -3390,7 +3446,7 @@ static struct attribute *nvme_ns_id_attrs[] = {
> };
>
> static umode_t nvme_ns_id_attrs_are_visible(struct kobject *kobj,
> - struct attribute *a, int n)
> + struct attribute *a, int n)
Unrelated changes for this patch.
> {
> struct device *dev = container_of(kobj, struct device, kobj);
> struct nvme_ns_ids *ids = &dev_to_ns_head(dev)->ids;
> @@ -3432,6 +3488,11 @@ const struct attribute_group *nvme_ns_id_attr_groups[] = {
> NULL,
> };
>
> +const struct attribute_group *nvme_ns_char_id_attr_groups[] = {
> + &nvme_ns_id_attr_group,
> + NULL,
> +};
> +
> #define nvme_show_str_function(field) \
> static ssize_t field##_show(struct device *dev, \
> struct device_attribute *attr, char *buf) \
> @@ -3824,6 +3885,36 @@ struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid)
> }
> EXPORT_SYMBOL_NS_GPL(nvme_find_get_ns, NVME_TARGET_PASSTHRU);
>
> +static int nvme_alloc_chardev_ns(struct nvme_ctrl *ctrl, struct nvme_ns *ns)
> +{
> + char cdisk_name[DISK_NAME_LEN];
> + int ret = 0;
Unnecessary initialization for local variable.
> +
> + device_initialize(&ns->cdev_device);
> + ns->cdev_device.devt = MKDEV(MAJOR(nvme_ns_base_chr_devt),
> + ns->head->instance);
> + ns->cdev_device.class = nvme_ns_class;
> + ns->cdev_device.parent = ctrl->device;
> + ns->cdev_device.groups = nvme_ns_char_id_attr_groups;
> + dev_set_drvdata(&ns->cdev_device, ns);
> +
> + sprintf(cdisk_name, "nvme%dc%dn%d", ctrl->subsys->instance,
> + ctrl->instance, ns->head->instance);
In multi-path, private namespaces for a head are not in /dev, so I don't
think this will hurt private namespaces (e.g., nvme0c0n1), But it looks
like it will make a little bit confusions between chardev and hidden blkdev.
I don't against to update nvme-cli things also even naming conventions are
going to become different than nvmeXcYnZ.
More information about the Linux-nvme
mailing list