Re: [PATCH 2/2] nvme: create slaves/holder entries for multipath devices
On Thu, Dec 06, 2018 at 04:44:32PM +0100, Christoph Hellwig wrote: > On Wed, Nov 28, 2018 at 06:38:17AM -0200, Thadeu Lima de Souza Cascardo wrote: > > I will send a followup that includes your two patches, fixing up this one, > > plus > > another two because that is simply reverting the revert of Hanne's patch. > > Which > > means it still has the lsblk bug. > > > > Then, we have two options: do not use GENHD_FL_HIDDEN on multipath nvme > > disks, > > or expose devt for those disks. I am sending a patch for the second option. > > Can you please send it out so we can look at it before the 4.21 merge > window opens? Just sent. Sorry about the delay. Thanks. Cascardo.
[PATCH 4/4] block: expose devt for GENHD_FL_HIDDEN disks
Without this exposure, lsblk will fail as it tries to find out the
device's dev_t numbers. This causes a real problem for nvme multipath
devices, as their slaves are hidden.
Exposing them fixes the problem, even though trying to open the devices
returns an error in the case of nvme multipath. So, right now, it's the
driver's responsibility to return a failure to open hidden devices.
Signed-off-by: Thadeu Lima de Souza Cascardo
---
block/genhd.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/block/genhd.c b/block/genhd.c
index 7674fce32fca..65a7fa664188 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -687,6 +687,7 @@ static void __device_add_disk(struct device *parent, struct
gendisk *disk,
disk_alloc_events(disk);
+ disk_to_dev(disk)->devt = devt;
if (disk->flags & GENHD_FL_HIDDEN) {
/*
* Don't let hidden disks show up in /proc/partitions,
@@ -698,13 +699,12 @@ static void __device_add_disk(struct device *parent,
struct gendisk *disk,
int ret;
/* Register BDI before referencing it from bdev */
- disk_to_dev(disk)->devt = devt;
ret = bdi_register_owner(disk->queue->backing_dev_info,
disk_to_dev(disk));
WARN_ON(ret);
- blk_register_region(disk_devt(disk), disk->minors, NULL,
- exact_match, exact_lock, disk);
}
+ blk_register_region(disk_devt(disk), disk->minors, NULL,
+ exact_match, exact_lock, disk);
register_disk(parent, disk, groups);
if (register_queue)
blk_register_queue(disk);
@@ -776,8 +776,7 @@ void del_gendisk(struct gendisk *disk)
WARN_ON(1);
}
- if (!(disk->flags & GENHD_FL_HIDDEN))
- blk_unregister_region(disk_devt(disk), disk->minors);
+ blk_unregister_region(disk_devt(disk), disk->minors);
kobject_put(disk->part0.holder_dir);
kobject_put(disk->slave_dir);
--
2.19.1
[PATCH 3/4] nvme: Should not warn when a disk path is opened
As we hide the disks used for nvme multipath, their open functions should be allowed to be called, as their devices are not exposed. However, not exposing the devices cause problems with lsblk, which won't find the devices and show them, which is even more of a problem when holders/slaves relationships are exposed. Not warning here allow us to expose the disks without creating spurious warnings. A failure should still be returned, which is the case here, and still allows lsblk to work. Signed-off-by: Thadeu Lima de Souza Cascardo --- drivers/nvme/host/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 1dc29795f1ee..22424b2adfad 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -1327,8 +1327,8 @@ static int nvme_open(struct block_device *bdev, fmode_t mode) struct nvme_ns *ns = bdev->bd_disk->private_data; #ifdef CONFIG_NVME_MULTIPATH - /* should never be called due to GENHD_FL_HIDDEN */ - if (WARN_ON_ONCE(ns->head->disk)) + /* Should fail as it's hidden */ + if (ns->head->disk) goto fail; #endif if (!kref_get_unless_zero(>kref)) -- 2.19.1
[PATCH 1/4] block: move holder tracking from struct block_device to hd_struct
From: Christoph Hellwig
We'd like to track the slaves and holder for nvme multipath devices
in the same standard fashion as all the other stacked block devices
to make the life for things like distro installers easy.
But struct block_device only exists while we have open instances,
which we never have for the underlying devices of a nvme-multipath
setup. But we can easily move the older list into struct hd_struct
which exists all the time the block device exists, the only interesting
bit is that we need a new mutex for it.
Signed-off-by: Christoph Hellwig
---
block/genhd.c| 4 +++
block/partition-generic.c| 4 +++
drivers/block/drbd/drbd_nl.c | 4 +--
drivers/md/bcache/super.c| 8 +++---
drivers/md/dm.c | 4 +--
drivers/md/md.c | 4 +--
fs/block_dev.c | 48
include/linux/fs.h | 11 +++--
include/linux/genhd.h| 4 +++
9 files changed, 47 insertions(+), 44 deletions(-)
diff --git a/block/genhd.c b/block/genhd.c
index 0145bcb0cc76..7674fce32fca 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1468,6 +1468,10 @@ struct gendisk *__alloc_disk_node(int minors, int
node_id)
disk->minors = minors;
rand_initialize_disk(disk);
+#ifdef CONFIG_SYSFS
+ INIT_LIST_HEAD(>part0.holder_disks);
+ mutex_init(>part0.holder_mutex);
+#endif
disk_to_dev(disk)->class = _class;
disk_to_dev(disk)->type = _type;
device_initialize(disk_to_dev(disk));
diff --git a/block/partition-generic.c b/block/partition-generic.c
index 5f8db5c5140f..83edc23ff1e8 100644
--- a/block/partition-generic.c
+++ b/block/partition-generic.c
@@ -333,6 +333,10 @@ struct hd_struct *add_partition(struct gendisk *disk, int
partno,
}
seqcount_init(>nr_sects_seq);
+#ifdef CONFIG_SYSFS
+ INIT_LIST_HEAD(>holder_disks);
+ mutex_init(>holder_mutex);
+#endif
pdev = part_to_dev(p);
p->start_sect = start;
diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
index d15703b1ffe8..42354e34b565 100644
--- a/drivers/block/drbd/drbd_nl.c
+++ b/drivers/block/drbd/drbd_nl.c
@@ -1670,7 +1670,7 @@ static struct block_device *open_backing_dev(struct
drbd_device *device,
if (!do_bd_link)
return bdev;
- err = bd_link_disk_holder(bdev, device->vdisk);
+ err = bd_link_disk_holder(bdev->bd_part, device->vdisk);
if (err) {
blkdev_put(bdev, FMODE_READ | FMODE_WRITE | FMODE_EXCL);
drbd_err(device, "bd_link_disk_holder(\"%s\", ...) failed with
%d\n",
@@ -1719,7 +1719,7 @@ static void close_backing_dev(struct drbd_device *device,
struct block_device *b
if (!bdev)
return;
if (do_bd_unlink)
- bd_unlink_disk_holder(bdev, device->vdisk);
+ bd_unlink_disk_holder(bdev->bd_part, device->vdisk);
blkdev_put(bdev, FMODE_READ | FMODE_WRITE | FMODE_EXCL);
}
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 7bbd670a5a84..e49b177b1c60 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -677,7 +677,7 @@ static void bcache_device_unlink(struct bcache_device *d)
sysfs_remove_link(>kobj, "cache");
for_each_cache(ca, d->c, i)
- bd_unlink_disk_holder(ca->bdev, d->disk);
+ bd_unlink_disk_holder(ca->bdev->bd_part, d->disk);
}
}
@@ -688,7 +688,7 @@ static void bcache_device_link(struct bcache_device *d,
struct cache_set *c,
struct cache *ca;
for_each_cache(ca, d->c, i)
- bd_link_disk_holder(ca->bdev, d->disk);
+ bd_link_disk_holder(ca->bdev->bd_part, d->disk);
snprintf(d->name, BCACHEDEVNAME_SIZE,
"%s%u", name, d->id);
@@ -936,7 +936,7 @@ void bch_cached_dev_run(struct cached_dev *dc)
}
add_disk(d->disk);
- bd_link_disk_holder(dc->bdev, dc->disk.disk);
+ bd_link_disk_holder(dc->bdev->bd_part, dc->disk.disk);
/*
* won't show up in the uevent file, use udevadm monitor -e instead
* only class / kset properties are persistent
@@ -1199,7 +1199,7 @@ static void cached_dev_free(struct closure *cl)
kthread_stop(dc->status_update_thread);
if (atomic_read(>running))
- bd_unlink_disk_holder(dc->bdev, dc->disk.disk);
+ bd_unlink_disk_holder(dc->bdev->bd_part, dc->disk.disk);
bcache_device_free(>disk);
list_del(>list);
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index ab72d79775ee..823b592f066b 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -770,7 +770,7 @@ static int open_table_device(struct table_device *td, dev_t
dev,
if (IS_ERR(bdev))
return PTR_ERR(bdev);
- r = bd_link_disk_holder(bdev,
[PATCH 2/4] nvme: create slaves/holder entries for multipath devices
From: Christoph Hellwig
This allows tools like distro installers easily track the relationship.
Signed-off-by: Christoph Hellwig
[cascardo: only add disk when there is ns->head]
Signed-off-by: Thadeu Lima de Souza Cascardo
---
drivers/nvme/host/core.c | 5 +++--
drivers/nvme/host/multipath.c | 13 +++--
drivers/nvme/host/nvme.h | 12
3 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 3ce7fc9df378..1dc29795f1ee 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -371,7 +371,7 @@ static void nvme_free_ns_head(struct kref *ref)
struct nvme_ns_head *head =
container_of(ref, struct nvme_ns_head, ref);
- nvme_mpath_remove_disk(head);
+ nvme_mpath_remove_nshead(head);
ida_simple_remove(>subsys->ns_ida, head->instance);
list_del_init(>entry);
cleanup_srcu_struct_quiesced(>srcu);
@@ -3120,7 +3120,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl,
unsigned nsid)
device_add_disk(ctrl->device, ns->disk, nvme_ns_id_attr_groups);
- nvme_mpath_add_disk(ns, id);
+ nvme_mpath_add_ns(ns, id);
nvme_fault_inject_init(ns);
kfree(id);
@@ -3144,6 +3144,7 @@ static void nvme_ns_remove(struct nvme_ns *ns)
nvme_fault_inject_fini(ns);
if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
+ nvme_mpath_remove_ns(ns);
del_gendisk(ns->disk);
blk_cleanup_queue(ns->queue);
if (blk_get_integrity(ns->disk))
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index ec310b1b9267..174c64643592 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -500,7 +500,7 @@ static int nvme_set_ns_ana_state(struct nvme_ctrl *ctrl,
return 0;
}
-void nvme_mpath_add_disk(struct nvme_ns *ns, struct nvme_id_ns *id)
+void nvme_mpath_add_ns(struct nvme_ns *ns, struct nvme_id_ns *id)
{
if (nvme_ctrl_use_ana(ns->ctrl)) {
mutex_lock(>ctrl->ana_lock);
@@ -513,9 +513,18 @@ void nvme_mpath_add_disk(struct nvme_ns *ns, struct
nvme_id_ns *id)
nvme_mpath_set_live(ns);
mutex_unlock(>head->lock);
}
+
+ if (ns->head->disk)
+ bd_link_disk_holder(>disk->part0, ns->head->disk);
+}
+
+void nvme_mpath_remove_ns(struct nvme_ns *ns)
+{
+ if (ns->head->disk)
+ bd_unlink_disk_holder(>disk->part0, ns->head->disk);
}
-void nvme_mpath_remove_disk(struct nvme_ns_head *head)
+void nvme_mpath_remove_nshead(struct nvme_ns_head *head)
{
if (!head->disk)
return;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index ae77eb16fd1f..365262d11e53 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -470,8 +470,9 @@ void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns,
void nvme_failover_req(struct request *req);
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 *ns, struct nvme_id_ns *id);
-void nvme_mpath_remove_disk(struct nvme_ns_head *head);
+void nvme_mpath_add_ns(struct nvme_ns *ns, struct nvme_id_ns *id);
+void nvme_mpath_remove_ns(struct nvme_ns *ns);
+void nvme_mpath_remove_nshead(struct nvme_ns_head *head);
int nvme_mpath_init(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id);
void nvme_mpath_uninit(struct nvme_ctrl *ctrl);
void nvme_mpath_stop(struct nvme_ctrl *ctrl);
@@ -515,11 +516,14 @@ static inline int nvme_mpath_alloc_disk(struct nvme_ctrl
*ctrl,
{
return 0;
}
-static inline void nvme_mpath_add_disk(struct nvme_ns *ns,
+static inline void nvme_mpath_add_ns(struct nvme_ns *ns,
struct nvme_id_ns *id)
{
}
-static inline void nvme_mpath_remove_disk(struct nvme_ns_head *head)
+static inline void nvme_mpath_remove_ns(struct nvme_ns *ns)
+{
+}
+static inline void nvme_mpath_remove_nshead(struct nvme_ns_head *head)
{
}
static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns)
--
2.19.1
[PATCH 0/4] nvme multipath: expose slaves/holders
Exposing slaves/holders is necessary in order to find out the real PCI device and its driver for the root filesystem when generating an initramfs with initramfs-tools. That fails right now for nvme multipath devices, which this patchset fixes. However, because the slave devices are hidden, lsblk fails without some extra patches, as it can't find the device numbers for the slave devices, and exits. Christoph Hellwig (2): block: move holder tracking from struct block_device to hd_struct nvme: create slaves/holder entries for multipath devices Thadeu Lima de Souza Cascardo (2): nvme: Should not warn when a disk path is opened block: expose devt for GENHD_FL_HIDDEN disks block/genhd.c | 13 ++ block/partition-generic.c | 4 +++ drivers/block/drbd/drbd_nl.c | 4 +-- drivers/md/bcache/super.c | 8 +++--- drivers/md/dm.c | 4 +-- drivers/md/md.c | 4 +-- drivers/nvme/host/core.c | 9 --- drivers/nvme/host/multipath.c | 13 -- drivers/nvme/host/nvme.h | 12 ++--- fs/block_dev.c| 48 +++ include/linux/fs.h| 11 +++- include/linux/genhd.h | 4 +++ 12 files changed, 75 insertions(+), 59 deletions(-) -- 2.19.1
Re: [PATCH 2/2] nvme: create slaves/holder entries for multipath devices
On Fri, Nov 16, 2018 at 02:08:05PM +0100, Christoph Hellwig wrote:
> This allows tools like distro installers easily track the relationship.
>
> Signed-off-by: Christoph Hellwig
> ---
> drivers/nvme/host/core.c | 5 +++--
> drivers/nvme/host/multipath.c | 12 ++--
> drivers/nvme/host/nvme.h | 12
> 3 files changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index f172d63db2b5..ff0ce5ef1040 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -371,7 +371,7 @@ static void nvme_free_ns_head(struct kref *ref)
> struct nvme_ns_head *head =
> container_of(ref, struct nvme_ns_head, ref);
>
> - nvme_mpath_remove_disk(head);
> + nvme_mpath_remove_nshead(head);
> ida_simple_remove(>subsys->ns_ida, head->instance);
> list_del_init(>entry);
> cleanup_srcu_struct_quiesced(>srcu);
> @@ -3118,7 +3118,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl,
> unsigned nsid)
>
> device_add_disk(ctrl->device, ns->disk, nvme_ns_id_attr_groups);
>
> - nvme_mpath_add_disk(ns, id);
> + nvme_mpath_add_ns(ns, id);
> nvme_fault_inject_init(ns);
> kfree(id);
>
> @@ -3142,6 +3142,7 @@ static void nvme_ns_remove(struct nvme_ns *ns)
>
> nvme_fault_inject_fini(ns);
> if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
> + nvme_mpath_remove_ns(ns);
> del_gendisk(ns->disk);
> blk_cleanup_queue(ns->queue);
> if (blk_get_integrity(ns->disk))
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index b82b0d3ca39a..457d5ad7cfe9 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -515,7 +515,7 @@ static int nvme_set_ns_ana_state(struct nvme_ctrl *ctrl,
> return 0;
> }
>
> -void nvme_mpath_add_disk(struct nvme_ns *ns, struct nvme_id_ns *id)
> +void nvme_mpath_add_ns(struct nvme_ns *ns, struct nvme_id_ns *id)
> {
> if (nvme_ctrl_use_ana(ns->ctrl)) {
> mutex_lock(>ctrl->ana_lock);
> @@ -528,9 +528,17 @@ void nvme_mpath_add_disk(struct nvme_ns *ns, struct
> nvme_id_ns *id)
> nvme_mpath_set_live(ns);
> mutex_unlock(>head->lock);
> }
> +
> + bd_link_disk_holder(>disk->part0, ns->head->disk);
> +}
> +
That will oops because there is no test for ns->head->disk.
I will send a followup that includes your two patches, fixing up this one, plus
another two because that is simply reverting the revert of Hanne's patch. Which
means it still has the lsblk bug.
Then, we have two options: do not use GENHD_FL_HIDDEN on multipath nvme disks,
or expose devt for those disks. I am sending a patch for the second option.
Thanks.
Cascardo.
> +void nvme_mpath_remove_ns(struct nvme_ns *ns)
> +{
> + if (ns->head->disk)
> + bd_unlink_disk_holder(>disk->part0, ns->head->disk);
> }
>
> -void nvme_mpath_remove_disk(struct nvme_ns_head *head)
> +void nvme_mpath_remove_nshead(struct nvme_ns_head *head)
> {
> if (!head->disk)
> return;
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 32a1f1cfdfb4..e20353bbf6a8 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -470,8 +470,9 @@ void nvme_set_disk_name(char *disk_name, struct nvme_ns
> *ns,
> void nvme_failover_req(struct request *req);
> 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 *ns, struct nvme_id_ns *id);
> -void nvme_mpath_remove_disk(struct nvme_ns_head *head);
> +void nvme_mpath_add_ns(struct nvme_ns *ns, struct nvme_id_ns *id);
> +void nvme_mpath_remove_ns(struct nvme_ns *ns);
> +void nvme_mpath_remove_nshead(struct nvme_ns_head *head);
> int nvme_mpath_init(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id);
> void nvme_mpath_uninit(struct nvme_ctrl *ctrl);
> void nvme_mpath_stop(struct nvme_ctrl *ctrl);
> @@ -515,11 +516,14 @@ static inline int nvme_mpath_alloc_disk(struct
> nvme_ctrl *ctrl,
> {
> return 0;
> }
> -static inline void nvme_mpath_add_disk(struct nvme_ns *ns,
> +static inline void nvme_mpath_add_ns(struct nvme_ns *ns,
> struct nvme_id_ns *id)
> {
> }
> -static inline void nvme_mpath_remove_disk(struct nvme_ns_head *head)
> +static inline void nvme_mpath_remove_ns(struct nvme_ns *ns)
> +{
> +}
> +static inline void nvme_mpath_remove_nshead(struct nvme_ns_head *head)
> {
> }
> static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns)
> --
> 2.19.1
>
Re: [PATCH 5/6] platform/x86: make device_attribute const
For classmate-laptop.c Acked-by: Thadeu Lima de Souza Cascardo <casca...@holoscopio.com>
