nvme: make sure ns head inherits underlying device limits

PATCH Superseded Sagi Grimberg 2 participants 3 messages Nov 1, 2018 – Nov 2, 2018 NVMe

Share & tools

Sagi Grimberg

author maintainer

2018-11-01 16:04 UTC #0 lore raw

PATCH v3 1/1 nvme: make sure ns head inherits underlying device limits

Whenever we update ns_head info, we need to make sure it is still compatible with all underlying backing devices because although nvme multipath doesn't have any explicit use of these limits, other devices can still be stacked on top of it which may rely on the underlying limits. Start with unlimited stacking limits, and every info update iterate over siblings and adjust queue limits.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
Changes from v2:
- fixed missing ifdef config (multipathing)

Changes from v1:
- fix change log with better reasoning

 drivers/nvme/host/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/nvme/host/core.c +8 -0

index 4325f1aeea1f..66d46e1f8093 100644
@@ -1482,6 +1482,7 @@ static bool nvme_ns_ids_equal(struct nvme_ns_ids *a, struct nvme_ns_ids *b)
 static void nvme_update_disk_info(struct gendisk *disk,
 		struct nvme_ns *ns, struct nvme_id_ns *id)
 {
+	struct nvme_ns *n;
 	sector_t capacity = le64_to_cpup(&id->nsze) << (ns->lba_shift - 9);
 	unsigned short bs = 1 << ns->lba_shift;

@@ -1506,6 +1507,9 @@ static void nvme_update_disk_info(struct gendisk *disk,
 	else
 		set_disk_ro(disk, false);
 
+	list_for_each_entry_rcu(n, &ns->head->list, siblings)
+		blk_queue_stack_limits(disk->queue, n->queue);
+
 	blk_mq_unfreeze_queue(disk->queue);
 }

@@ -2960,6 +2964,10 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
 	if (ret)
 		goto out_cleanup_srcu;
 
+#ifdef CONFIG_NVME_MULTIPATH
+	blk_set_stacking_limits(&head->disk->queue->limits);
+#endif
+
 	list_add_tail(&head->entry, &ctrl->subsys->nsheads);
 
 	kref_get(&ctrl->subsys->ref);

Signature · 1 line

2.17.1

Christoph Hellwig

maintainer

2018-11-02 05:41 UTC #1 lore raw

PATCH v3 1/1 nvme: make sure ns head inherits underlying device limits

Quoted 9 lines

drivers/nvme/host/core.c +1 -0

@@ -1482,6 +1482,7 @@ static bool nvme_ns_ids_equal(struct nvme_ns_ids *a, struct nvme_ns_ids *b)
 static void nvme_update_disk_info(struct gendisk *disk,
 		struct nvme_ns *ns, struct nvme_id_ns *id)
 {
+	struct nvme_ns *n;
 	sector_t capacity = le64_to_cpup(&id->nsze) << (ns->lba_shift - 9);
 	unsigned short bs = 1 << ns->lba_shift;

drivers/nvme/host/core.c static void nvme_update_disk_info(struct gendisk *disk, · line 1511

Quoted 5 lines

drivers/nvme/host/core.c +2 -0

@@ -1506,6 +1507,9 @@ static void nvme_update_disk_info(struct gendisk *disk,
 	else
 		set_disk_ro(disk, false);
 
+	list_for_each_entry_rcu(n, &ns->head->list, siblings)
+		blk_queue_stack_limits(disk->queue, n->queue);

I think this should only be done for ns->head_disk, nor the disks attached to the namespaces.

drivers/nvme/host/core.c static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl, · line 2969

Quoted 7 lines

drivers/nvme/host/core.c +3 -0

@@ -2960,6 +2964,10 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
 	if (ret)
 		goto out_cleanup_srcu;
 
+#ifdef CONFIG_NVME_MULTIPATH
+	blk_set_stacking_limits(&head->disk->queue->limits);
+#endif

And this should probably done in nvme_mpath_alloc_disk.

Sagi Grimberg

author maintainer

2018-11-02 17:16 UTC #2 lore raw

PATCH v3 1/1 nvme: make sure ns head inherits underlying device limits

Quoted 12 lines

(quoted diff) +1 -0

@@ -1482,6 +1482,7 @@ static bool nvme_ns_ids_equal(struct nvme_ns_ids *a, struct nvme_ns_ids *b)
  static void nvme_update_disk_info(struct gendisk *disk,
  		struct nvme_ns *ns, struct nvme_id_ns *id)
  {
+	struct nvme_ns *n;
  	sector_t capacity = le64_to_cpup(&id->nsze) << (ns->lba_shift - 9);
  	unsigned short bs = 1 << ns->lba_shift;

@@ -1506,6 +1507,9 @@ static void nvme_update_disk_info(struct gendisk *disk,
  	else
  		set_disk_ro(disk, false);

Quoted 5 lines

Review static void nvme_update_disk_info(struct gendisk *disk, · line 1511
Quoted 2 lines
(quoted diff) +2 -0
+	list_for_each_entry_rcu(n, &ns->head->list, siblings)
+		blk_queue_stack_limits(disk->queue, n->queue);
I think this should only be done for ns->head_disk, nor the disks attached to the namespaces.

Well the siblings list would be empty for the namespaces themselves.

I could move this to the call site:
--

drivers/nvme/host/core.c +0 -0

index 4325f1aeea1f..ff5d543624a4 100644
@@ -1535,8 +1535,10 @@ static void __nvme_revalidate_disk(struct gendisk

*disk, struct nvme_id_ns *id)

         if (ns->ndev)
                 nvme_nvm_update_nvm_info(ns);
  #ifdef CONFIG_NVME_MULTIPATH
-       if (ns->head->disk)
+       if (ns->head->disk) {
                 nvme_update_disk_info(ns->head->disk, ns, id);

+ blk_queue_stack_limits(ns->head->disk->queue, ns->queue);
+ }
#endif
}
--

Quoted 9 lines

Review static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl, · line 2969
Quoted 7 lines
(quoted diff) +3 -0
@@ -2960,6 +2964,10 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
  	if (ret)
  		goto out_cleanup_srcu;
 
+#ifdef CONFIG_NVME_MULTIPATH
+	blk_set_stacking_limits(&head->disk->queue->limits);
+#endif
And this should probably done in nvme_mpath_alloc_disk.

Yes, I can do that (the original version had it there but can't remember why I decided to have it here instead).