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