[PATCH] nvme: fix discard support without oncs

[PATCH] nvme: fix discard support without oncs

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The device can report discard support without setting the ONCS DSM bit.
When not set, the driver clears max_discard_size expecting it to be set
later. We don't know the size until we have the namespace format,
though, so setting it is deferred until configuring one, but the driver
was abandoning the discard settings due to that initial clearing.

Move the max_discard_size calculation above the check for a '0' discard
size.

Fixes: 1a86924e4f46475 ("nvme: fix interpretation of DMRSL")
Cc: Tom Yan <tom.ty89@gmail.com>
Reported-by: Laurence Oberman <loberman@redhat.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 53ef028596c61..d6a9bac91a4cd 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1674,6 +1674,9 @@ static void nvme_config_discard(struct gendisk *disk, struct nvme_ns *ns)
 	struct request_queue *queue = disk->queue;
 	u32 size = queue_logical_block_size(queue);
 
+	if (ctrl->dmrsl && ctrl->dmrsl <= nvme_sect_to_lba(ns, UINT_MAX))
+		ctrl->max_discard_sectors = nvme_lba_to_sect(ns, ctrl->dmrsl);
+
 	if (ctrl->max_discard_sectors == 0) {
 		blk_queue_max_discard_sectors(queue, 0);
 		return;
@@ -1688,9 +1691,6 @@ static void nvme_config_discard(struct gendisk *disk, struct nvme_ns *ns)
 	if (queue->limits.max_discard_sectors)
 		return;
 
-	if (ctrl->dmrsl && ctrl->dmrsl <= nvme_sect_to_lba(ns, UINT_MAX))
-		ctrl->max_discard_sectors = nvme_lba_to_sect(ns, ctrl->dmrsl);
-
 	blk_queue_max_discard_sectors(queue, ctrl->max_discard_sectors);
 	blk_queue_max_discard_segments(queue, ctrl->max_discard_segments);
 
-- 
2.34.1

Re: [PATCH] nvme: fix discard support without oncs

[Laurence Oberman]

On Mon, 2023-04-03 at 13:09 -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The device can report discard support without setting the ONCS DSM
> bit.
> When not set, the driver clears max_discard_size expecting it to be
> set
> later. We don't know the size until we have the namespace format,
> though, so setting it is deferred until configuring one, but the
> driver
> was abandoning the discard settings due to that initial clearing.
> 
> Move the max_discard_size calculation above the check for a '0'
> discard
> size.
> 
> Fixes: 1a86924e4f46475 ("nvme: fix interpretation of DMRSL")
> Cc: Tom Yan <tom.ty89@gmail.com>
> Reported-by: Laurence Oberman <loberman@redhat.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/nvme/host/core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 53ef028596c61..d6a9bac91a4cd 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1674,6 +1674,9 @@ static void nvme_config_discard(struct gendisk
> *disk, struct nvme_ns *ns)
>         struct request_queue *queue = disk->queue;
>         u32 size = queue_logical_block_size(queue);
>  
> +       if (ctrl->dmrsl && ctrl->dmrsl <= nvme_sect_to_lba(ns,
> UINT_MAX))
> +               ctrl->max_discard_sectors = nvme_lba_to_sect(ns,
> ctrl->dmrsl);
> +
>         if (ctrl->max_discard_sectors == 0) {
>                 blk_queue_max_discard_sectors(queue, 0);
>                 return;
> @@ -1688,9 +1691,6 @@ static void nvme_config_discard(struct gendisk
> *disk, struct nvme_ns *ns)
>         if (queue->limits.max_discard_sectors)
>                 return;
>  
> -       if (ctrl->dmrsl && ctrl->dmrsl <= nvme_sect_to_lba(ns,
> UINT_MAX))
> -               ctrl->max_discard_sectors = nvme_lba_to_sect(ns,
> ctrl->dmrsl);
> -
>         blk_queue_max_discard_sectors(queue, ctrl-
> >max_discard_sectors);
>         blk_queue_max_discard_segments(queue, ctrl-
> >max_discard_segments);
>  

Many Thanks Keith for the quick ressponse

I can confirm this patch fixes the issue and I have discard support
again on the Infinidat device.

[   33.240602] nvme nvme1: creating 8 I/O queues.
[   33.330643] nvme nvme1: mapped 8/0/0 default/read/poll queues.
[   33.348589] nvme nvme1: new ctrl: NQN "nqn.2020-
01.com.infinidat:36000-subsystem-696", addr 192.168.1.2:4420
[   37.590766] XFS (nvme1n1): Mounting V5 Filesystem 6763a33f-18cc-
4a26-894b-8b0f8d79a98a
[   40.762363] XFS (nvme1n1): Ending clean mount

Tested-by: Laurence Oberman <loberman@redhat.com>

Re: [PATCH] nvme: fix discard support without oncs

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH] nvme: fix discard support without oncs

[Niklas Cassel]

On Mon, Apr 03, 2023 at 01:09:25PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The device can report discard support without setting the ONCS DSM bit.
> When not set, the driver clears max_discard_size expecting it to be set
> later. We don't know the size until we have the namespace format,
> though, so setting it is deferred until configuring one, but the driver
> was abandoning the discard settings due to that initial clearing.
> 
> Move the max_discard_size calculation above the check for a '0' discard
> size.
> 
> Fixes: 1a86924e4f46475 ("nvme: fix interpretation of DMRSL")
> Cc: Tom Yan <tom.ty89@gmail.com>
> Reported-by: Laurence Oberman <loberman@redhat.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/nvme/host/core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 53ef028596c61..d6a9bac91a4cd 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1674,6 +1674,9 @@ static void nvme_config_discard(struct gendisk *disk, struct nvme_ns *ns)
>  	struct request_queue *queue = disk->queue;
>  	u32 size = queue_logical_block_size(queue);
>  
> +	if (ctrl->dmrsl && ctrl->dmrsl <= nvme_sect_to_lba(ns, UINT_MAX))
> +		ctrl->max_discard_sectors = nvme_lba_to_sect(ns, ctrl->dmrsl);
> +
>  	if (ctrl->max_discard_sectors == 0) {
>  		blk_queue_max_discard_sectors(queue, 0);
>  		return;
> @@ -1688,9 +1691,6 @@ static void nvme_config_discard(struct gendisk *disk, struct nvme_ns *ns)
>  	if (queue->limits.max_discard_sectors)
>  		return;
>  
> -	if (ctrl->dmrsl && ctrl->dmrsl <= nvme_sect_to_lba(ns, UINT_MAX))
> -		ctrl->max_discard_sectors = nvme_lba_to_sect(ns, ctrl->dmrsl);
> -
>  	blk_queue_max_discard_sectors(queue, ctrl->max_discard_sectors);
>  	blk_queue_max_discard_segments(queue, ctrl->max_discard_segments);
>  
> -- 
> 2.34.1
> 
> 

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

Re: [PATCH] nvme: fix discard support without oncs

[Christoph Hellwig]

Thanks,

applied to nvme-6.3.