Re: [PATCH] nvme: add namespace paths links

Re: [PATCH] nvme: add namespace paths links

[Christoph Hellwig]

On Fri, Apr 01, 2022 at 11:28:55AM +0200, Hannes Reinecke wrote:
> It is really annoying to always have to loop through the entire
> /sys/block directory just to find the namespace path links for
> a single namespace in libnvme/nvme-cli.
> So provide links to the namespace paths for efficient lookup.

I think having some form of links would be useful.  Quite a while ago
Thadeu looked into adding some form of relationship for lsblk and
friends.  Maybe it would be good to sync up and make sure whatever
links we are adding would be useful for all users?

> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/nvme/host/core.c      | 3 +++
>  drivers/nvme/host/multipath.c | 8 ++++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index f204c6f78b5b..df4d89aa061f 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4054,6 +4054,9 @@ static void nvme_ns_remove(struct nvme_ns *ns)
>  
>  	if (!nvme_ns_head_multipath(ns->head))
>  		nvme_cdev_del(&ns->cdev, &ns->cdev_device);
> +	else
> +		sysfs_remove_link(&disk_to_dev(ns->head->disk)->kobj,
> +				  ns->disk->disk_name);
>  	del_gendisk(ns->disk);
>  	blk_cleanup_queue(ns->queue);
>  
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 7fc58e1f6b09..dc9a4018f90d 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -801,6 +801,8 @@ static int nvme_lookup_ana_group_desc(struct nvme_ctrl *ctrl,
>  
>  void nvme_mpath_add_disk(struct nvme_ns *ns, struct nvme_id_ns *id)
>  {
> +	int ret;
> +
>  	if (nvme_ctrl_use_ana(ns->ctrl)) {
>  		struct nvme_ana_group_desc desc = {
>  			.grpid = id->anagrpid,
> @@ -831,6 +833,12 @@ void nvme_mpath_add_disk(struct nvme_ns *ns, struct nvme_id_ns *id)
>  	if (blk_queue_is_zoned(ns->queue) && ns->head->disk)
>  		ns->head->disk->queue->nr_zones = ns->queue->nr_zones;
>  #endif
> +	ret = sysfs_create_link(&disk_to_dev(ns->head->disk)->kobj,
> +				&disk_to_dev(ns->disk)->kobj,
> +				ns->disk->disk_name);
> +	if (ret)
> +		dev_warn(ns->ctrl->device,
> +			 "failed to create namespace path link\n");
>  }
>  
>  void nvme_mpath_shutdown_disk(struct nvme_ns_head *head)
> -- 
> 2.29.2
---end quoted text---

Re: [PATCH] nvme: add namespace paths links

[Hannes Reinecke]

On 4/5/22 07:42, Christoph Hellwig wrote:
> On Fri, Apr 01, 2022 at 11:28:55AM +0200, Hannes Reinecke wrote:
>> It is really annoying to always have to loop through the entire
>> /sys/block directory just to find the namespace path links for
>> a single namespace in libnvme/nvme-cli.
>> So provide links to the namespace paths for efficient lookup.
> 
> I think having some form of links would be useful.  Quite a while ago
> Thadeu looked into adding some form of relationship for lsblk and
> friends.  Maybe it would be good to sync up and make sure whatever
> links we are adding would be useful for all users?
> 
Care to elaborate?
Using the block device name is the most straightforward way here; we 
don't have a good enumeration to leverage as the paths are essentially 
just a linked list.

We sure can have a subdirectory 'paths', and stick the links in there.
If that helps with lsblk ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH] nvme: add namespace paths links

[Christoph Hellwig]

On Tue, Apr 05, 2022 at 07:46:40AM +0200, Hannes Reinecke wrote:
> On 4/5/22 07:42, Christoph Hellwig wrote:
>> On Fri, Apr 01, 2022 at 11:28:55AM +0200, Hannes Reinecke wrote:
>>> It is really annoying to always have to loop through the entire
>>> /sys/block directory just to find the namespace path links for
>>> a single namespace in libnvme/nvme-cli.
>>> So provide links to the namespace paths for efficient lookup.
>>
>> I think having some form of links would be useful.  Quite a while ago
>> Thadeu looked into adding some form of relationship for lsblk and
>> friends.  Maybe it would be good to sync up and make sure whatever
>> links we are adding would be useful for all users?
>>
> Care to elaborate?
> Using the block device name is the most straightforward way here; we don't 
> have a good enumeration to leverage as the paths are essentially just a 
> linked list.
>
> We sure can have a subdirectory 'paths', and stick the links in there.
> If that helps with lsblk ...

I personally don't care.  But I've added Thadeu and the util-linux list
to get everyone on board.

Re: [PATCH] nvme: add namespace paths links

[Sagi Grimberg]

>>>> It is really annoying to always have to loop through the entire
>>>> /sys/block directory just to find the namespace path links for
>>>> a single namespace in libnvme/nvme-cli.
>>>> So provide links to the namespace paths for efficient lookup.
>>>
>>> I think having some form of links would be useful.  Quite a while ago
>>> Thadeu looked into adding some form of relationship for lsblk and
>>> friends.  Maybe it would be good to sync up and make sure whatever
>>> links we are adding would be useful for all users?
>>>
>> Care to elaborate?
>> Using the block device name is the most straightforward way here; we don't
>> have a good enumeration to leverage as the paths are essentially just a
>> linked list.
>>
>> We sure can have a subdirectory 'paths', and stick the links in there.
>> If that helps with lsblk ...
> 
> I personally don't care.  But I've added Thadeu and the util-linux list
> to get everyone on board.

Why not stick to slaves/ like the device mapper?

Re: [PATCH] nvme: add namespace paths links

[Hannes Reinecke]

On 4/5/22 10:34, Sagi Grimberg wrote:
> 
>>>>> It is really annoying to always have to loop through the entire
>>>>> /sys/block directory just to find the namespace path links for
>>>>> a single namespace in libnvme/nvme-cli.
>>>>> So provide links to the namespace paths for efficient lookup.
>>>>
>>>> I think having some form of links would be useful.  Quite a while ago
>>>> Thadeu looked into adding some form of relationship for lsblk and
>>>> friends.  Maybe it would be good to sync up and make sure whatever
>>>> links we are adding would be useful for all users?
>>>>
>>> Care to elaborate?
>>> Using the block device name is the most straightforward way here; we 
>>> don't
>>> have a good enumeration to leverage as the paths are essentially just a
>>> linked list.
>>>
>>> We sure can have a subdirectory 'paths', and stick the links in there.
>>> If that helps with lsblk ...
>>
>> I personally don't care.  But I've added Thadeu and the util-linux list
>> to get everyone on board.
> 
> Why not stick to slaves/ like the device mapper?

Because:

/**
  * bd_link_disk_holder - create symlinks between holding disk and slave 
bdev
  * @bdev: the claimed slave bdev
  * @disk: the holding disk
  *
  * DON'T USE THIS UNLESS YOU'RE ALREADY USING IT.
  *

Not sure _why_ it's there, but it is there.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH] nvme: add namespace paths links

[Sagi Grimberg]

>> Why not stick to slaves/ like the device mapper?
> 
> Because:
> 
> /**
>   * bd_link_disk_holder - create symlinks between holding disk and slave 
> bdev
>   * @bdev: the claimed slave bdev
>   * @disk: the holding disk
>   *
>   * DON'T USE THIS UNLESS YOU'RE ALREADY USING IT.
>   *
> 
> Not sure _why_ it's there, but it is there.

Fair enough

Re: [PATCH] nvme: add namespace paths links

[Karel Zak]

On Tue, Apr 05, 2022 at 07:48:36AM +0200, Christoph Hellwig wrote:
> On Tue, Apr 05, 2022 at 07:46:40AM +0200, Hannes Reinecke wrote:
> > On 4/5/22 07:42, Christoph Hellwig wrote:
> >> On Fri, Apr 01, 2022 at 11:28:55AM +0200, Hannes Reinecke wrote:
> >>> It is really annoying to always have to loop through the entire
> >>> /sys/block directory just to find the namespace path links for
> >>> a single namespace in libnvme/nvme-cli.
> >>> So provide links to the namespace paths for efficient lookup.
> >>
> >> I think having some form of links would be useful.  Quite a while ago
> >> Thadeu looked into adding some form of relationship for lsblk and
> >> friends.  Maybe it would be good to sync up and make sure whatever
> >> links we are adding would be useful for all users?
> >>
> > Care to elaborate?
> > Using the block device name is the most straightforward way here; we don't 
> > have a good enumeration to leverage as the paths are essentially just a 
> > linked list.
> >
> > We sure can have a subdirectory 'paths', and stick the links in there.
> > If that helps with lsblk ...
> 
> I personally don't care.  But I've added Thadeu and the util-linux list
> to get everyone on board.
 
lsblk internally can accept arbitrary relations between devices
(and libsmartcols can display it). If you introduce something new or
extend the current holders/slaves, it will probably be accessible for
lsblk to use it.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] nvme: add namespace paths links

[Hannes Reinecke]

On 4/25/22 15:54, Karel Zak wrote:
> On Tue, Apr 05, 2022 at 07:48:36AM +0200, Christoph Hellwig wrote:
>> On Tue, Apr 05, 2022 at 07:46:40AM +0200, Hannes Reinecke wrote:
>>> On 4/5/22 07:42, Christoph Hellwig wrote:
>>>> On Fri, Apr 01, 2022 at 11:28:55AM +0200, Hannes Reinecke wrote:
>>>>> It is really annoying to always have to loop through the entire
>>>>> /sys/block directory just to find the namespace path links for
>>>>> a single namespace in libnvme/nvme-cli.
>>>>> So provide links to the namespace paths for efficient lookup.
>>>>
>>>> I think having some form of links would be useful.  Quite a while ago
>>>> Thadeu looked into adding some form of relationship for lsblk and
>>>> friends.  Maybe it would be good to sync up and make sure whatever
>>>> links we are adding would be useful for all users?
>>>>
>>> Care to elaborate?
>>> Using the block device name is the most straightforward way here; we don't
>>> have a good enumeration to leverage as the paths are essentially just a
>>> linked list.
>>>
>>> We sure can have a subdirectory 'paths', and stick the links in there.
>>> If that helps with lsblk ...
>>
>> I personally don't care.  But I've added Thadeu and the util-linux list
>> to get everyone on board.
>   
> lsblk internally can accept arbitrary relations between devices
> (and libsmartcols can display it). If you introduce something new or
> extend the current holders/slaves, it will probably be accessible for
> lsblk to use it.
> 
Christoph, ping?

Shall I resend the patch or can you pick it up directly?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer