[PATCH V3] nvme: enable char device per namespace

[Keith Busch]

On Wed, Dec 16, 2020 at 09:01:51AM +0100, Javier González wrote:
> > On 15 Dec 2020, at 23.46, Keith Busch <kbusch at kernel.org> wrote:
> > On Tue, Dec 15, 2020 at 08:55:57PM +0100, javier at 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;
--