[PATCH] nvme: make sure ns head inherits underlying device limits

[PATCH] nvme: make sure ns head inherits underlying device limits

[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.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index bdfbbeb4b2f8..6194b3ee30ca 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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);
-- 
2.17.1

[PATCH] nvme: make sure ns head inherits underlying device limits

[Christoph Hellwig]

On Fri, Oct 19, 2018@03:24:50PM -0700, Sagi Grimberg wrote:
> 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.

[PATCH] nvme: make sure ns head inherits underlying device limits

[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.

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...

[PATCH] nvme: make sure ns head inherits underlying device limits

[Christoph Hellwig]

On Mon, Oct 22, 2018@05:22:15PM -0700, Sagi Grimberg wrote:
> 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!

[PATCH] nvme: make sure ns head inherits underlying device limits

[Sagi Grimberg]

Christoph,

>> 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.

[PATCH] nvme: make sure ns head inherits underlying device limits

[Christoph Hellwig]

On Tue, Oct 30, 2018@11:11:11AM -0700, Sagi Grimberg wrote:
> Christoph,
>
>>> 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.

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