diff mbox series

[03/24] nvme: let set_capacity_revalidate_and_notify update the bdev size

Message ID 20201106190337.1973127-4-hch@lst.de (mailing list archive)
State Superseded
Headers show
Series [01/24] block: remove the call to __invalidate_device in check_disk_size_change | expand

Commit Message

Christoph Hellwig Nov. 6, 2020, 7:03 p.m. UTC
There is no good reason to call revalidate_disk_size separately.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/core.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Comments

Hannes Reinecke Nov. 9, 2020, 7:53 a.m. UTC | #1
On 11/6/20 8:03 PM, Christoph Hellwig wrote:
> There is no good reason to call revalidate_disk_size separately.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/nvme/host/core.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 376096bfc54a83..4e86c9aafd88a7 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2053,7 +2053,7 @@ static void nvme_update_disk_info(struct gendisk *disk,
>   			capacity = 0;
>   	}
>   
> -	set_capacity_revalidate_and_notify(disk, capacity, false);
> +	set_capacity_revalidate_and_notify(disk, capacity, true);
>   
>   	nvme_config_discard(disk, ns);
>   	nvme_config_write_zeroes(disk, ns);
> @@ -2136,7 +2136,6 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
>   		blk_stack_limits(&ns->head->disk->queue->limits,
>   				 &ns->queue->limits, 0);
>   		blk_queue_update_readahead(ns->head->disk->queue);
> -		nvme_update_bdev_size(ns->head->disk);
>   		blk_mq_unfreeze_queue(ns->head->disk->queue);
>   	}
>   #endif

Hold on.
This, at the very least, should be a separate patch.
With this you are changing the behaviour of nvme multipath.

Originally nvme multipath would update/change the size of the multipath 
device according to the underlying path devices.
With this patch the size of the multipath device will _not_ change if 
there is a change on the underlying devices.

While personally I would _love_ to have this patch, we should at least 
document this by making it a separate patch.
And we possibly should check if both sizes are the same, and think of 
what we should be doing if they are not.

Cheers,

Hannes
Christoph Hellwig Nov. 9, 2020, 8:53 a.m. UTC | #2
On Mon, Nov 09, 2020 at 08:53:58AM +0100, Hannes Reinecke wrote:
>> index 376096bfc54a83..4e86c9aafd88a7 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -2053,7 +2053,7 @@ static void nvme_update_disk_info(struct gendisk *disk,
>>   			capacity = 0;
>>   	}
>>   -	set_capacity_revalidate_and_notify(disk, capacity, false);
>> +	set_capacity_revalidate_and_notify(disk, capacity, true);
>>     	nvme_config_discard(disk, ns);
>>   	nvme_config_write_zeroes(disk, ns);
>> @@ -2136,7 +2136,6 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
>>   		blk_stack_limits(&ns->head->disk->queue->limits,
>>   				 &ns->queue->limits, 0);
>>   		blk_queue_update_readahead(ns->head->disk->queue);
>> -		nvme_update_bdev_size(ns->head->disk);
>>   		blk_mq_unfreeze_queue(ns->head->disk->queue);
>>   	}
>>   #endif
>
> Hold on.
> This, at the very least, should be a separate patch.
> With this you are changing the behaviour of nvme multipath.
>
> Originally nvme multipath would update/change the size of the multipath 
> device according to the underlying path devices.
> With this patch the size of the multipath device will _not_ change if there 
> is a change on the underlying devices.

Yes, it will.  Take a close look at nvme_update_disk_info and how it is
called.
Hannes Reinecke Nov. 9, 2020, 9:25 a.m. UTC | #3
On 11/9/20 9:53 AM, Christoph Hellwig wrote:
> On Mon, Nov 09, 2020 at 08:53:58AM +0100, Hannes Reinecke wrote:
>>> index 376096bfc54a83..4e86c9aafd88a7 100644
>>> --- a/drivers/nvme/host/core.c
>>> +++ b/drivers/nvme/host/core.c
>>> @@ -2053,7 +2053,7 @@ static void nvme_update_disk_info(struct gendisk *disk,
>>>    			capacity = 0;
[ .. ]
>> Originally nvme multipath would update/change the size of the multipath
>> device according to the underlying path devices.
>> With this patch the size of the multipath device will _not_ change if there
>> is a change on the underlying devices.
> 
> Yes, it will.  Take a close look at nvme_update_disk_info and how it is
> called.
> 
Okay, then: What would be the correct way of handling a size update for 
NVMe multipath?
Assuming we're getting an AEN for each path signalling the size change
(or a controller reset leading to a size change).
So if we're updating the size of the multipath device together with the 
path device at the first AEN/reset we'll end up with the other paths 
having a different size than the multipath device (and the path we've 
just been updating).
- Do we care, or cross fingers and hope for the best?
- Shouldn't we detect the case where we won't get a size update for the 
other paths, or, indeed, we have a genuine device size mismatch due to a 
misconfiguration on the target?

IE shouldn't we have a flag 'size update pending' for the other paths,, 
to take them out ouf use temporarily until the other AENs/resets have 
been processed?

Cheers,

Hannes
Sagi Grimberg Nov. 9, 2020, 11:28 p.m. UTC | #4
> [ .. ]
>>> Originally nvme multipath would update/change the size of the multipath
>>> device according to the underlying path devices.
>>> With this patch the size of the multipath device will _not_ change if 
>>> there
>>> is a change on the underlying devices.
>>
>> Yes, it will.  Take a close look at nvme_update_disk_info and how it is
>> called.
>>
> Okay, then: What would be the correct way of handling a size update for 
> NVMe multipath?
> Assuming we're getting an AEN for each path signalling the size change
> (or a controller reset leading to a size change).
> So if we're updating the size of the multipath device together with the 
> path device at the first AEN/reset we'll end up with the other paths 
> having a different size than the multipath device (and the path we've 
> just been updating).
> - Do we care, or cross fingers and hope for the best?
> - Shouldn't we detect the case where we won't get a size update for the 
> other paths, or, indeed, we have a genuine device size mismatch due to a 
> misconfiguration on the target?
> 
> IE shouldn't we have a flag 'size update pending' for the other paths,, 
> to take them out ouf use temporarily until the other AENs/resets have 
> been processed?

the mpath device will take the minimum size from all the paths, that is
what blk_stack_limits does. When the AEN for all the paths will arrive
the mpath size will update.

Not sure how this is different than what we have today...
Hannes Reinecke Nov. 10, 2020, 7 a.m. UTC | #5
On 11/10/20 12:28 AM, Sagi Grimberg wrote:
> 
>> [ .. ]
>>>> Originally nvme multipath would update/change the size of the multipath
>>>> device according to the underlying path devices.
>>>> With this patch the size of the multipath device will _not_ change 
>>>> if there
>>>> is a change on the underlying devices.
>>>
>>> Yes, it will.  Take a close look at nvme_update_disk_info and how it is
>>> called.
>>>
>> Okay, then: What would be the correct way of handling a size update 
>> for NVMe multipath?
>> Assuming we're getting an AEN for each path signalling the size change
>> (or a controller reset leading to a size change).
>> So if we're updating the size of the multipath device together with 
>> the path device at the first AEN/reset we'll end up with the other 
>> paths having a different size than the multipath device (and the path 
>> we've just been updating).
>> - Do we care, or cross fingers and hope for the best?
>> - Shouldn't we detect the case where we won't get a size update for 
>> the other paths, or, indeed, we have a genuine device size mismatch 
>> due to a misconfiguration on the target?
>>
>> IE shouldn't we have a flag 'size update pending' for the other 
>> paths,, to take them out ouf use temporarily until the other 
>> AENs/resets have been processed?
> 
> the mpath device will take the minimum size from all the paths, that is
> what blk_stack_limits does. When the AEN for all the paths will arrive
> the mpath size will update.
> 
But that's precisely my point; there won't be an AEN for _all_ paths, 
but rather one AEN per path. Which will be processed separately, leading 
to the issue described above.

> Not sure how this is different than what we have today...

Oh, that is a problem even today.
So we should probably move it to a different thread...

Cheers,

Hannes
diff mbox series

Patch

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 376096bfc54a83..4e86c9aafd88a7 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2053,7 +2053,7 @@  static void nvme_update_disk_info(struct gendisk *disk,
 			capacity = 0;
 	}
 
-	set_capacity_revalidate_and_notify(disk, capacity, false);
+	set_capacity_revalidate_and_notify(disk, capacity, true);
 
 	nvme_config_discard(disk, ns);
 	nvme_config_write_zeroes(disk, ns);
@@ -2136,7 +2136,6 @@  static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
 		blk_stack_limits(&ns->head->disk->queue->limits,
 				 &ns->queue->limits, 0);
 		blk_queue_update_readahead(ns->head->disk->queue);
-		nvme_update_bdev_size(ns->head->disk);
 		blk_mq_unfreeze_queue(ns->head->disk->queue);
 	}
 #endif
@@ -3965,8 +3964,6 @@  static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids)
 	 */
 	if (ret && ret != -ENOMEM && !(ret > 0 && !(ret & NVME_SC_DNR)))
 		nvme_ns_remove(ns);
-	else
-		revalidate_disk_size(ns->disk, true);
 }
 
 static void nvme_validate_or_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)