Re: [PATCH 4/4] nvme: enable char device per namespace

[Javier González <javier@xxxxxxxxxxx>]

On 01.12.2020 23:03, Minwoo Im wrote:
> Hello,
>
> On 20-12-01 13:56:10, javier@xxxxxxxxxxx wrote:
> > From: Javier González <javier.gonz@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > ---
> >  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!