[PATCH] Revert "nvme: create 'slaves' and 'holders' entries for hidden controllers"

[PATCH] Revert "nvme: create 'slaves' and 'holders' entries for hidden controllers"

[Christoph Hellwig]

This reverts commit e9a48034d7d1318ece7d4a235838a86c94db9d68.

The slaves and holders link for the hidden gendisks confuse lsblk so that
it errors out on, or doesn't report the nvme multipath devices.  Given
that we don't need holder relationships for something that can't even be
directly accessed we should just stop creating those links.

Signed-off-by: Christoph Hellwig <hch at lst.de>
Reported-by: Potnuri Bharat Teja <bharat at chelsio.com>
Cc: stable at vger.kernel.org
---
 drivers/nvme/host/core.c      |  2 --
 drivers/nvme/host/multipath.c | 30 ------------------------------
 drivers/nvme/host/nvme.h      |  8 --------
 3 files changed, 40 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 817e5e2766da..7aeca5db7916 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3033,7 +3033,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 			ns->disk->disk_name);
 
 	nvme_mpath_add_disk(ns->head);
-	nvme_mpath_add_disk_links(ns);
 	return;
  out_unlink_ns:
 	mutex_lock(&ctrl->subsys->lock);
@@ -3053,7 +3052,6 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 		return;
 
 	if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
-		nvme_mpath_remove_disk_links(ns);
 		sysfs_remove_group(&disk_to_dev(ns->disk)->kobj,
 					&nvme_ns_id_attr_group);
 		if (ns->ndev)
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index b7e5c6db4d92..060f69e03427 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -210,25 +210,6 @@ void nvme_mpath_add_disk(struct nvme_ns_head *head)
 	mutex_unlock(&head->subsys->lock);
 }
 
-void nvme_mpath_add_disk_links(struct nvme_ns *ns)
-{
-	struct kobject *slave_disk_kobj, *holder_disk_kobj;
-
-	if (!ns->head->disk)
-		return;
-
-	slave_disk_kobj = &disk_to_dev(ns->disk)->kobj;
-	if (sysfs_create_link(ns->head->disk->slave_dir, slave_disk_kobj,
-			kobject_name(slave_disk_kobj)))
-		return;
-
-	holder_disk_kobj = &disk_to_dev(ns->head->disk)->kobj;
-	if (sysfs_create_link(ns->disk->part0.holder_dir, holder_disk_kobj,
-			kobject_name(holder_disk_kobj)))
-		sysfs_remove_link(ns->head->disk->slave_dir,
-			kobject_name(slave_disk_kobj));
-}
-
 void nvme_mpath_remove_disk(struct nvme_ns_head *head)
 {
 	if (!head->disk)
@@ -243,14 +224,3 @@ void nvme_mpath_remove_disk(struct nvme_ns_head *head)
 	blk_cleanup_queue(head->disk->queue);
 	put_disk(head->disk);
 }
-
-void nvme_mpath_remove_disk_links(struct nvme_ns *ns)
-{
-	if (!ns->head->disk)
-		return;
-
-	sysfs_remove_link(ns->disk->part0.holder_dir,
-			kobject_name(&disk_to_dev(ns->head->disk)->kobj));
-	sysfs_remove_link(ns->head->disk->slave_dir,
-			kobject_name(&disk_to_dev(ns->disk)->kobj));
-}
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 0521e4707d1c..d733b14ede9d 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -410,9 +410,7 @@ bool nvme_req_needs_failover(struct request *req, blk_status_t error);
 void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl);
 int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl,struct nvme_ns_head *head);
 void nvme_mpath_add_disk(struct nvme_ns_head *head);
-void nvme_mpath_add_disk_links(struct nvme_ns *ns);
 void nvme_mpath_remove_disk(struct nvme_ns_head *head);
-void nvme_mpath_remove_disk_links(struct nvme_ns *ns);
 
 static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns)
 {
@@ -454,12 +452,6 @@ static inline void nvme_mpath_add_disk(struct nvme_ns_head *head)
 static inline void nvme_mpath_remove_disk(struct nvme_ns_head *head)
 {
 }
-static inline void nvme_mpath_add_disk_links(struct nvme_ns *ns)
-{
-}
-static inline void nvme_mpath_remove_disk_links(struct nvme_ns *ns)
-{
-}
 static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns)
 {
 }
-- 
2.14.2

[PATCH] Revert "nvme: create 'slaves' and 'holders' entries for hidden controllers"

[Hannes Reinecke]

On 03/07/2018 02:13 PM, Christoph Hellwig wrote:
> This reverts commit e9a48034d7d1318ece7d4a235838a86c94db9d68.
> 
> The slaves and holders link for the hidden gendisks confuse lsblk so that
> it errors out on, or doesn't report the nvme multipath devices.  Given
> that we don't need holder relationships for something that can't even be
> directly accessed we should just stop creating those links.
> 
> Signed-off-by: Christoph Hellwig <hch at lst.de>
> Reported-by: Potnuri Bharat Teja <bharat at chelsio.com>
> Cc: stable at vger.kernel.org
> ---
>  drivers/nvme/host/core.c      |  2 --
>  drivers/nvme/host/multipath.c | 30 ------------------------------
>  drivers/nvme/host/nvme.h      |  8 --------
>  3 files changed, 40 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 817e5e2766da..7aeca5db7916 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3033,7 +3033,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
>  			ns->disk->disk_name);
>  
>  	nvme_mpath_add_disk(ns->head);
> -	nvme_mpath_add_disk_links(ns);
>  	return;
>   out_unlink_ns:
>  	mutex_lock(&ctrl->subsys->lock);
> @@ -3053,7 +3052,6 @@ static void nvme_ns_remove(struct nvme_ns *ns)
>  		return;
>  
>  	if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
> -		nvme_mpath_remove_disk_links(ns);
>  		sysfs_remove_group(&disk_to_dev(ns->disk)->kobj,
>  					&nvme_ns_id_attr_group);
>  		if (ns->ndev)
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index b7e5c6db4d92..060f69e03427 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -210,25 +210,6 @@ void nvme_mpath_add_disk(struct nvme_ns_head *head)
>  	mutex_unlock(&head->subsys->lock);
>  }
>  
> -void nvme_mpath_add_disk_links(struct nvme_ns *ns)
> -{
> -	struct kobject *slave_disk_kobj, *holder_disk_kobj;
> -
> -	if (!ns->head->disk)
> -		return;
> -
> -	slave_disk_kobj = &disk_to_dev(ns->disk)->kobj;
> -	if (sysfs_create_link(ns->head->disk->slave_dir, slave_disk_kobj,
> -			kobject_name(slave_disk_kobj)))
> -		return;
> -
> -	holder_disk_kobj = &disk_to_dev(ns->head->disk)->kobj;
> -	if (sysfs_create_link(ns->disk->part0.holder_dir, holder_disk_kobj,
> -			kobject_name(holder_disk_kobj)))
> -		sysfs_remove_link(ns->head->disk->slave_dir,
> -			kobject_name(slave_disk_kobj));
> -}
> -
>  void nvme_mpath_remove_disk(struct nvme_ns_head *head)
>  {
>  	if (!head->disk)
> @@ -243,14 +224,3 @@ void nvme_mpath_remove_disk(struct nvme_ns_head *head)
>  	blk_cleanup_queue(head->disk->queue);
>  	put_disk(head->disk);
>  }
> -
> -void nvme_mpath_remove_disk_links(struct nvme_ns *ns)
> -{
> -	if (!ns->head->disk)
> -		return;
> -
> -	sysfs_remove_link(ns->disk->part0.holder_dir,
> -			kobject_name(&disk_to_dev(ns->head->disk)->kobj));
> -	sysfs_remove_link(ns->head->disk->slave_dir,
> -			kobject_name(&disk_to_dev(ns->disk)->kobj));
> -}
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 0521e4707d1c..d733b14ede9d 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -410,9 +410,7 @@ bool nvme_req_needs_failover(struct request *req, blk_status_t error);
>  void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl);
>  int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl,struct nvme_ns_head *head);
>  void nvme_mpath_add_disk(struct nvme_ns_head *head);
> -void nvme_mpath_add_disk_links(struct nvme_ns *ns);
>  void nvme_mpath_remove_disk(struct nvme_ns_head *head);
> -void nvme_mpath_remove_disk_links(struct nvme_ns *ns);
>  
>  static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns)
>  {
> @@ -454,12 +452,6 @@ static inline void nvme_mpath_add_disk(struct nvme_ns_head *head)
>  static inline void nvme_mpath_remove_disk(struct nvme_ns_head *head)
>  {
>  }
> -static inline void nvme_mpath_add_disk_links(struct nvme_ns *ns)
> -{
> -}
> -static inline void nvme_mpath_remove_disk_links(struct nvme_ns *ns)
> -{
> -}
>  static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns)
>  {
>  }
> 
The nice thing about the holders/slave is that one can discover the
topology, and potentially figure out any issues (like one path going
down etc).
How do we detect the topology now?
Does nvme cli has the functionality?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

[PATCH] Revert "nvme: create 'slaves' and 'holders' entries for hidden controllers"

[Keith Busch]

On Wed, Mar 07, 2018@02:40:39PM +0100, Hannes Reinecke wrote:
> The nice thing about the holders/slave is that one can discover the
> topology, and potentially figure out any issues (like one path going
> down etc).

I also thought the sysfs hierarchy was useful, but it sounds like causes
more problems than it solves. :(

> How do we detect the topology now?
> Does nvme cli has the functionality?

Not in the way you'd probably like. It can provide all the information
you'd need to put it together, but it doesn't provide a convenient
method to get or visualize it. I'll add a note on the project to fix
that.

[PATCH] Revert "nvme: create 'slaves' and 'holders' entries for hidden controllers"

[Sitsofe Wheeler]

On 7 March 2018@15:03, Keith Busch <keith.busch@intel.com> wrote:
> On Wed, Mar 07, 2018@02:40:39PM +0100, Hannes Reinecke wrote:
>> The nice thing about the holders/slave is that one can discover the
>> topology, and potentially figure out any issues (like one path going
>> down etc).
>
> I also thought the sysfs hierarchy was useful, but it sounds like causes
> more problems than it solves. :(

The problem is that it's the first of its kind in regard to virtual
slaves/devices and many of the consumers of block device sysfs simply
weren't ready for it to appear where it did. Perhaps if it had been a
different part of sysfs hierarchy and/or linked up a different way
(virtual-slaves/?) it would have been invisible to programs that
weren't ready to consume it...

-- 
Sitsofe | http://sucs.org/~sits/

[PATCH] Revert "nvme: create 'slaves' and 'holders' entries for hidden controllers"

[Christoph Hellwig]

On Wed, Mar 07, 2018@03:56:36PM +0000, Sitsofe Wheeler wrote:
> The problem is that it's the first of its kind in regard to virtual
> slaves/devices and many of the consumers of block device sysfs simply
> weren't ready for it to appear where it did. Perhaps if it had been a
> different part of sysfs hierarchy and/or linked up a different way
> (virtual-slaves/?) it would have been invisible to programs that
> weren't ready to consume it...

Yes, I think we should simply creates a paths/ directory.  Not sure
there is much point in the backlink.

[PATCH] Revert "nvme: create 'slaves' and 'holders' entries for hidden controllers"

[Keith Busch]

On Wed, Mar 07, 2018@06:18:05PM +0100, Christoph Hellwig wrote:
> 
> Yes, I think we should simply creates a paths/ directory.  Not sure
> there is much point in the backlink.

That sounds good to me. For now, looks like we need the revert for the
current rc and stable; applied to nvme-4.16-rc5.

[PATCH] Revert "nvme: create 'slaves' and 'holders' entries for hidden controllers"

[Sitsofe Wheeler]

On 7 March 2018@17:38, Keith Busch <keith.busch@intel.com> wrote:
> On Wed, Mar 07, 2018@06:18:05PM +0100, Christoph Hellwig wrote:
>>
>> Yes, I think we should simply creates a paths/ directory.  Not sure
>> there is much point in the backlink.
>
> That sounds good to me. For now, looks like we need the revert for the
> current rc and stable; applied to nvme-4.16-rc5.

The only thing that concerns me is where do the stats live? Over in
https://github.com/axboe/fio/pull/526#issuecomment-369521772 we found
the main device doesn't record stats. Will that still be the case?

-- 
Sitsofe | http://sucs.org/~sits/

[PATCH] Revert "nvme: create 'slaves' and 'holders' entries for hidden controllers"

[Keith Busch]

On Wed, Mar 07, 2018@05:47:34PM +0000, Sitsofe Wheeler wrote:
> The only thing that concerns me is where do the stats live? Over in
> https://github.com/axboe/fio/pull/526#issuecomment-369521772 we found
> the main device doesn't record stats. Will that still be the case?

The stats still live in the same place at
/sys/block/nvmeXcYnZ/queue/stats. This patch just removes symlinks to the
each path block device in the multipath block device's slave directory,
so 'fio' won't see them at the moment.

[PATCH] Revert "nvme: create 'slaves' and 'holders' entries for hidden controllers"

[Sitsofe Wheeler]

On 7 March 2018@20:37, Keith Busch <keith.busch@intel.com> wrote:
>
> The stats still live in the same place at
> /sys/block/nvmeXcYnZ/queue/stats. This patch just removes symlinks to the
> each path block device in the multipath block device's slave directory,
> so 'fio' won't see them at the moment.

Fair enough - things can be reworked after if/when a replacement link lands.

-- 
Sitsofe | http://sucs.org/~sits/

[PATCH] Revert "nvme: create 'slaves' and 'holders' entries for hidden controllers"

[Sagi Grimberg]

>> The problem is that it's the first of its kind in regard to virtual
>> slaves/devices and many of the consumers of block device sysfs simply
>> weren't ready for it to appear where it did. Perhaps if it had been a
>> different part of sysfs hierarchy and/or linked up a different way
>> (virtual-slaves/?) it would have been invisible to programs that
>> weren't ready to consume it...
> 
> Yes, I think we should simply creates a paths/ directory.  Not sure
> there is much point in the backlink.

I'm good with this.

[PATCH] Revert "nvme: create 'slaves' and 'holders' entries for hidden controllers"

[Keith Busch]

On Wed, Mar 07, 2018@08:03:45AM -0700, Keith Busch wrote:
> On Wed, Mar 07, 2018@02:40:39PM +0100, Hannes Reinecke wrote:
> > How do we detect the topology now?
> > Does nvme cli has the functionality?
> 
> Not in the way you'd probably like. It can provide all the information
> you'd need to put it together, but it doesn't provide a convenient
> method to get or visualize it. I'll add a note on the project to fix
> that.

Actually, nvmecli already has something kind of close, thanks
to Johannes! The 'list-subsys' command is most of the way there. It
currently looks like this, and just needs to go one level deeper to
append the namespaces to the output:

# nvme list-subsys -o json
{
  "Subsystems" : [
    {
      "Name" : "nvme-subsys0",
      "NQN" : "nqn.2014.08.org.nvmexpress:8086108ePHLE7200015N6P4B7335943:ICDPC5ED2ORA6.4T"
    },
    {
      "Paths" : [
        {
          "Name" : "nvme0",
          "Transport" : "pcie",
          "Address" : "0000:03:00.0"
        },
        {
          "Name" : "nvme1",
          "Transport" : "pcie",
          "Address" : "0000:04:00.0"
        }
      ]
    }
  ]
}

While this should readily work for fabrics, I needed to add a new kernel
driver patch to export the PCIe address for the above (patch staged
for 4.17).

[PATCH] Revert "nvme: create 'slaves' and 'holders' entries for hidden controllers"

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850