Skip to contentSkip to conversation

nvme: make sure ns head inherits underlying device limits

PATCH New Sagi Grimberg 2 participants 6 messages Oct 19, 2018 – Oct 31, 2018 NVMe

lore
Share & tools
lore

Sagi Grimberg

author maintainer
#0 lore raw

PATCH 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. 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>
---
 drivers/nvme/host/core.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/nvme/host/core.c +6 -0
index bdfbbeb4b2f8..6194b3ee30ca 100644
@@ -1480,6 +1480,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;
 

@@ -1504,6 +1505,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);
 }
 

@@ -2958,6 +2962,8 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
 	if (ret)
 		goto out_cleanup_srcu;
 
+	blk_set_stacking_limits(&head->disk->queue->limits);
+
 	list_add_tail(&head->entry, &ctrl->subsys->nsheads);
 
 	kref_get(&ctrl->subsys->ref);
Signature · 1 line
2.17.1
maintainer
#1 lore raw

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

Quoted 4 lines · Sagi Grimberg

Whenever we update ns_head info, we need to make sure it is still compatible with all underlying backing devices. Start with unlimited stacking limits, and every info update iterate over siblings and adjust queue limits.

Why? Neither the block layer, nor the nvme multipath code should even look at this values.

author maintainer
#2 lore raw

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

Quoted 8 lines
Quoted 5 lines

Whenever we update ns_head info, we need to make sure it is still compatible with all underlying backing devices. Start with unlimited stacking limits, and every info update iterate over siblings and adjust queue limits.

Why? Neither the block layer, nor the nvme multipath code should even look at this values.

Without this patch at least max_segments and max_hw_sectors of the head device node are degenerated to defaults that have nothing to do with the underlying device limits.

This can result in unnecessary bio splitting for the head device node...

Although I did not check, same holds for discard sectors/segments.

Maybe this is an overkill? but thought this would be a better idea than simply adjusting limits one-by-one depending on what I find...

maintainer
#3 lore raw

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

Quoted 6 lines · Sagi Grimberg

Without this patch at least max_segments and max_hw_sectors of the head device node are degenerated to defaults that have nothing to do with the underlying device limits.

This can result in unnecessary bio splitting for the head device node...

But we never split bios for the multipath queue!

author maintainer
#4 lore raw

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

Christoph,

Quoted 8 lines
Quoted 6 lines

Without this patch at least max_segments and max_hw_sectors of the head device node are degenerated to defaults that have nothing to do with the underlying device limits.

This can result in unnecessary bio splitting for the head device node...

But we never split bios for the multipath queue!

Yes of course we don't, but we can still stack other devices on top of the multipath device that can split bios and in fact rely on the underlying queue limits.

maintainer
#5 lore raw

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

Quoted 6 lines · Sagi Grimberg

Christoph,

Quoted 4 lines
Quoted 4 lines

Without this patch at least max_segments and max_hw_sectors of the head device node are degenerated to defaults that have nothing to do with the underlying device limits.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

This can result in unnecessary bio splitting for the head device node...

But we never split bios for the multipath queue!

Yes of course we don't, but we can still stack other devices on top of the multipath device that can split bios and in fact rely on the underlying queue limits.

Ok, that makes sense. Can you resubmit it with that in a comment?