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@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; --
Thanks Keith. I will send a new version today. Regarding nvme-cli: what are your thoughts?
