Re: [PATCHv2 2/4] tree: add queue-depth attribute for nvme path object

[Hannes Reinecke <hare@suse.de>]

On 4/17/25 15:59, Nilay Shroff wrote:
> Add a new attribute named "queue_depth" under the NVMe path object. This
> attribute is used by the iopolicy "queue-depth", which was introduced in
> kernel v6.11. However, the corresponding sysfs attribute for queue depth
> was only added in kernel v6.14.
> 
> The queue_depth value can be useful for observing which paths are selected
> for I/O forwarding, based on the depth of each path. To support this,
> export the attribute in libnvme.map so it can be accessed via nvme-cli.
> 
> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
> ---
>   src/libnvme.map    |  1 +
>   src/nvme/private.h |  1 +
>   src/nvme/tree.c    | 12 +++++++++++-
>   src/nvme/tree.h    |  8 ++++++++
>   4 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libnvme.map b/src/libnvme.map
> index 4314705f..e53fad6b 100644
> --- a/src/libnvme.map
> +++ b/src/libnvme.map
> @@ -317,6 +317,7 @@ LIBNVME_1_0 {
>   		nvme_path_get_ctrl;
>   		nvme_path_get_name;
>   		nvme_path_get_ns;
> +		nvme_path_get_queue_depth;
>   		nvme_path_get_sysfs_dir;
>   		nvme_paths_filter;
>   		nvme_read_config;
> diff --git a/src/nvme/private.h b/src/nvme/private.h
> index f45c5823..f94276e2 100644
> --- a/src/nvme/private.h
> +++ b/src/nvme/private.h
> @@ -34,6 +34,7 @@ struct nvme_path {
>   	char *sysfs_dir;
>   	char *ana_state;
>   	int grpid;
> +	int queue_depth;
>   };
>   
>   struct nvme_ns_head {
> diff --git a/src/nvme/tree.c b/src/nvme/tree.c
> index bd7fb53e..b7a38a07 100644
> --- a/src/nvme/tree.c
> +++ b/src/nvme/tree.c
> @@ -903,6 +903,11 @@ const char *nvme_path_get_name(nvme_path_t p)
>   	return p->name;
>   }
>   
> +int nvme_path_get_queue_depth(nvme_path_t p)
> +{
> +	return p->queue_depth;
> +}
> +
>   const char *nvme_path_get_ana_state(nvme_path_t p)
>   {
>   	return p->ana_state;
> @@ -921,7 +926,7 @@ void nvme_free_path(struct nvme_path *p)
>   static int nvme_ctrl_scan_path(nvme_root_t r, struct nvme_ctrl *c, char *name)
>   {
>   	struct nvme_path *p;
> -	_cleanup_free_ char *path = NULL, *grpid = NULL;
> +	_cleanup_free_ char *path = NULL, *grpid = NULL, *queue_depth = NULL;
>   	int ret;
>   
>   	nvme_msg(r, LOG_DEBUG, "scan controller %s path %s\n",
> @@ -955,6 +960,11 @@ static int nvme_ctrl_scan_path(nvme_root_t r, struct nvme_ctrl *c, char *name)
>   		sscanf(grpid, "%d", &p->grpid);
>   	}
>   
> +	queue_depth = nvme_get_path_attr(p, "queue_depth");
> +	if (queue_depth) {
> +		sscanf(queue_depth, "%d", &p->queue_depth);
> +	}
> +
>   	list_node_init(&p->nentry);
>   	list_node_init(&p->entry);
>   	list_add_tail(&c->paths, &p->entry);
> diff --git a/src/nvme/tree.h b/src/nvme/tree.h
> index 9f382e9c..a9082f8e 100644
> --- a/src/nvme/tree.h
> +++ b/src/nvme/tree.h
> @@ -867,6 +867,14 @@ const char *nvme_path_get_sysfs_dir(nvme_path_t p);
>    */
>   const char *nvme_path_get_ana_state(nvme_path_t p);
>   
> +/**
> + * nvme_path_get_queue_depth() - Queue depth of an nvme_path_t object
> + * @p: &nvme_path_t object
> + *
> + * Return: Queue depth of @p
> + */
> +int nvme_path_get_queue_depth(nvme_path_t p);
> +
>   /**
>    * nvme_path_get_ctrl() - Parent controller of an nvme_path_t object
>    * @p:	&nvme_path_t object

As discussed: saving the 'queue_depth' attribute is a bad idea, as it
changes frequently.
I'd rather read the attribute value from sysfs whenever
'nvme_path_get_queue_depth' is called, and use the tree structure only
to hold the pathname.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@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