[PATCH 4/4] nvme: enable char device per namespace
Javier González
javier at javigon.com
Tue Dec 1 13:57:32 EST 2020
On 01.12.2020 23:03, Minwoo Im wrote:
>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().
Good point. Will add that.
>
>> +
>> 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.
Agree. But as I understand it, Keith had a good argument to keep names
aligned with the hidden bdev. It is also true that in that comment he
suggested nesting the char device in /dev/nvme
Keith, would you mind looking over this? Thanks!
More information about the Linux-nvme
mailing list