[RFC-PATCH 2/2] nvme: use the namespace id for block device names

[Hannes Reinecke]

On 3/2/26 23:25, Keith Busch wrote:
> From: Keith Busch <kbusch at kernel.org>
> 
> We can now assume the ns_id provides a unique number within a subsystem
> when creating a namespace kobject. Use that rather than pulling an
> available bit from an ida, which becomes unnecessary once we use the
> controller's reported identifier.
> 
> The user observable change is that the suffix of the nvme namespace
> device handle name will always be consistent regardless of which
> controller is scanned first or what the namespace attachment state is
> for a given controller being scanned. The prefix will still be dependent
> on the order the controllers were connected/probed.
> 
> Signed-off-by: Keith Busch <kbusch at kernel.org>
> ---
>   drivers/nvme/host/core.c      | 19 +++++--------------
>   drivers/nvme/host/multipath.c |  4 ++--
>   drivers/nvme/host/nvme.h      |  2 --
>   3 files changed, 7 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 7a558d5103a21..3f2f9b2be87c2 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -667,7 +667,6 @@ static void nvme_free_ns_head(struct kref *ref)
>   		container_of(ref, struct nvme_ns_head, ref);
>   
>   	nvme_mpath_put_disk(head);
> -	ida_free(&head->subsys->ns_ida, head->instance);
>   	cleanup_srcu_struct(&head->srcu);
>   	nvme_put_subsystem(head->subsys);
>   	kfree(head->plids);
> @@ -3113,7 +3112,6 @@ static void nvme_destroy_subsystem(struct kref *ref)
>   	list_del(&subsys->entry);
>   	mutex_unlock(&nvme_subsystems_lock);
>   
> -	ida_destroy(&subsys->ns_ida);
>   	device_del(&subsys->dev);
>   	put_device(&subsys->dev);
>   }
> @@ -3257,7 +3255,6 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>   			put_device(&subsys->dev);
>   			goto out_unlock;
>   		}
> -		ida_init(&subsys->ns_ida);
>   		list_add_tail(&subsys->entry, &nvme_subsystems);
>   	}
>   
> @@ -3868,7 +3865,7 @@ static int nvme_add_ns_cdev(struct nvme_ns *ns)
>   
>   	ns->cdev_device.parent = ns->ctrl->device;
>   	ret = dev_set_name(&ns->cdev_device, "ng%dn%d",
> -			   ns->ctrl->instance, ns->head->instance);
> +			   ns->ctrl->instance, ns->head->ns_id);
>   	if (ret)
>   		return ret;
>   
> @@ -3890,14 +3887,10 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
>   	head = kzalloc(size, GFP_KERNEL);
>   	if (!head)
>   		goto out;
> -	ret = ida_alloc_min(&ctrl->subsys->ns_ida, 1, GFP_KERNEL);
> -	if (ret < 0)
> -		goto out_free_head;
> -	head->instance = ret;
>   	INIT_LIST_HEAD(&head->list);
>   	ret = init_srcu_struct(&head->srcu);
>   	if (ret)
> -		goto out_ida_remove;
> +		goto out_free_head;
>   	head->subsys = ctrl->subsys;
>   	head->ns_id = info->nsid;
>   	head->ids = info->ids;
> @@ -3925,8 +3918,6 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
>   	return head;
>   out_cleanup_srcu:
>   	cleanup_srcu_struct(&head->srcu);
> -out_ida_remove:
> -	ida_free(&ctrl->subsys->ns_ida, head->instance);
>   out_free_head:
>   	kfree(head);
>   out:
> @@ -4162,14 +4153,14 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, struct nvme_ns_info *info)
>   	 */
>   	if (nvme_ns_head_multipath(ns->head)) {
>   		sprintf(disk->disk_name, "nvme%dc%dn%d", ctrl->subsys->instance,
> -			ctrl->instance, ns->head->instance);
> +			ctrl->instance, ns->head->ns_id);
>   		disk->flags |= GENHD_FL_HIDDEN;
>   	} else if (multipath) {
>   		sprintf(disk->disk_name, "nvme%dn%d", ctrl->subsys->instance,
> -			ns->head->instance);
> +			ns->head->ns_id);
>   	} else {
>   		sprintf(disk->disk_name, "nvme%dn%d", ctrl->instance,
> -			ns->head->instance);
> +			ns->head->ns_id);
>   	}
>   
>   	if (nvme_update_ns_info(ns, info))
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 02a50181d1dd6..8715b04ff92a8 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -640,7 +640,7 @@ static int nvme_add_ns_head_cdev(struct nvme_ns_head *head)
>   
>   	head->cdev_device.parent = &head->subsys->dev;
>   	ret = dev_set_name(&head->cdev_device, "ng%dn%d",
> -			   head->subsys->instance, head->instance);
> +			   head->subsys->instance, head->ns_id);
>   	if (ret)
>   		return ret;
>   	ret = nvme_cdev_add(&head->cdev, &head->cdev_device,
> @@ -767,7 +767,7 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
>   	 */
>   	set_bit(GD_SUPPRESS_PART_SCAN, &head->disk->state);
>   	sprintf(head->disk->disk_name, "nvme%dn%d",
> -			ctrl->subsys->instance, head->instance);
> +			ctrl->subsys->instance, head->ns_id);
>   	nvme_tryget_ns_head(head);
>   	return 0;
>   }
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index ed8ce356c363e..142a5684205a8 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -499,7 +499,6 @@ struct nvme_subsystem {
>   	u8			cmic;
>   	enum nvme_subsys_type	subtype;
>   	u16			vendor_id;
> -	struct ida		ns_ida;
>   #ifdef CONFIG_NVME_MULTIPATH
>   	enum nvme_iopolicy	iopolicy;
>   #endif
> @@ -540,7 +539,6 @@ struct nvme_ns_head {
>   	struct nvme_effects_log *effects;
>   	u64			nuse;
>   	unsigned		ns_id;
> -	int			instance;
>   #ifdef CONFIG_BLK_DEV_ZONED
>   	u64			zsze;
>   #endif

The idea is nice, and I would love to go into that
direction.
But have you checked how this holds up under
rescan/remapping (eg things like blktest/nvme/058)?
Removal of the sysfs nodes might be delayed, and we cannot
create new entries with the same name until then.
So if that is taken care of, fine, but I don't see that in
the patch ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare at suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich