Re: nvme: enable char device per namespace

["Javier González" <javier@javigon.com>]

On 16.12.2020 08:26, Keith Busch wrote:
>On Wed, Dec 16, 2020 at 09:01:51AM +0100, Javier González wrote:
>> > On 15 Dec 2020, at 23.46, Keith Busch <kbusch@kernel.org> wrote:
>> > On Tue, Dec 15, 2020 at 08:55:57PM +0100, javier@javigon.com wrote:
>> >> +static int nvme_alloc_chardev_ns(struct nvme_ctrl *ctrl, struct nvme_ns *ns)
>> >> +{
>> >> +    char cdisk_name[DISK_NAME_LEN];
>> >> +    int ret;
>> >> +
>> >> +    device_initialize(&ns->cdev_device);
>> >> +    ns->cdev_device.devt = MKDEV(MAJOR(nvme_ns_base_chr_devt),
>> >> +                     ns->head->instance);
>> >
>> > Ah, I see now. We are making these generic handles for each path, but
>> > the ns->head->instance is the same for all paths to a namespace, so it's
>> > not unique for that. Further, that head->instance is allocated per
>> > subsystem, so it's not unique from namespace heads seen in other
>> > subsystems.
>> >
>> > So, I think you need to allocate a new dev_t for each subsystem rather
>> > than the global nvme_ns_base_chr_devt, and I guess we also need a new
>> > nvme_ns instance field assigned from yet another ida?
>>
>> Ok. I’ll look into it.
>
>The suggestion may be overkill as we don't need unique majors for each
>controller right now (that may change if people need more than a
>million generic handles, but I think we're a ways off from that reality).
>
>The following on top of your patch makes it all work for me. Also, I
>don't think we should abort adding the namespace if the generic handle
>fails, so that's included here too:
>
>---
>diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>index c1aa4bccdeb2..cc9eaf4eba32 100644
>--- a/drivers/nvme/host/core.c
>+++ b/drivers/nvme/host/core.c
>@@ -86,6 +86,8 @@ static DEFINE_MUTEX(nvme_subsystems_lock);
>
> static DEFINE_IDA(nvme_instance_ida);
> static dev_t nvme_ctrl_base_chr_devt;
>+
>+static DEFINE_IDA(nvme_gen_minor_ida);
> static dev_t nvme_ns_base_chr_devt;
> static struct class *nvme_class;
> static struct class *nvme_ns_class;
>@@ -539,7 +541,8 @@ static void nvme_free_ns(struct kref *kref)
>
> 	if (ns->ndev)
> 		nvme_nvm_unregister(ns);
>-
>+	if (ns->minor)
>+		ida_simple_remove(&nvme_gen_minor_ida, ns->minor - 1);
> 	cdev_device_del(&ns->cdev, &ns->cdev_device);
> 	put_disk(ns->disk);
> 	nvme_put_ns_head(ns->head);
>@@ -3932,9 +3935,13 @@ static int nvme_alloc_chardev_ns(struct nvme_ctrl *ctrl, struct nvme_ns *ns)
> 	char cdisk_name[DISK_NAME_LEN];
> 	int ret;
>
>+	ret = ida_simple_get(&nvme_gen_minor_ida, 0, 0, GFP_KERNEL);
>+	if (ret < 0)
>+		return ret;
>+
>+	ns->minor = ret + 1;
> 	device_initialize(&ns->cdev_device);
>-	ns->cdev_device.devt = MKDEV(MAJOR(nvme_ns_base_chr_devt),
>-				     ns->head->instance);
>+	ns->cdev_device.devt = MKDEV(MAJOR(nvme_ns_base_chr_devt), ret);
> 	ns->cdev_device.class = nvme_ns_class;
> 	ns->cdev_device.parent = ctrl->device;
> 	ns->cdev_device.groups = nvme_ns_char_id_attr_groups;
>@@ -3945,15 +3952,22 @@ static int nvme_alloc_chardev_ns(struct nvme_ctrl *ctrl, struct nvme_ns *ns)
>
> 	ret = dev_set_name(&ns->cdev_device, "%s", cdisk_name);
> 	if (ret)
>-		return ret;
>+		goto put_ida;
>
> 	cdev_init(&ns->cdev, &nvme_cdev_fops);
> 	ns->cdev.owner = ctrl->ops->module;
>
> 	ret = cdev_device_add(&ns->cdev, &ns->cdev_device);
> 	if (ret)
>-		kfree_const(ns->cdev_device.kobj.name);
>+		goto free_kobj;
>+
>+	return ret;
>
>+free_kobj:
>+	kfree_const(ns->cdev_device.kobj.name);
>+put_ida:
>+	ida_simple_remove(&nvme_gen_minor_ida, ns->minor - 1);
>+	ns->minor = 0;
> 	return ret;
> }
>
>@@ -4023,7 +4037,9 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
> 	nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name);
>
> 	if (nvme_alloc_chardev_ns(ctrl, ns))
>-		goto out_put_disk;
>+		dev_warn(ctrl->device,
>+			"failed to create generic handle for nsid:%d\n",
>+			nsid);
>
> 	kfree(id);
>
>diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
>index 168c7719cda4..ccfd49d2a030 100644
>--- a/drivers/nvme/host/nvme.h
>+++ b/drivers/nvme/host/nvme.h
>@@ -435,6 +435,7 @@ struct nvme_ns {
>
> 	struct device cdev_device;	/* char device */
> 	struct cdev cdev;
>+	int minor;
>
> 	int lba_shift;
> 	u16 ms;
>--

Thanks Keith. I will send a new version today.

Regarding nvme-cli: what are your thoughts?