[PATCH v2] nvme: fix interpretation of dmrsl

[PATCH v2] nvme: fix interpretation of dmrsl

[Tom Yan]

dmrsl is in the unit of logical block, while max_discard_sectors is
in the unit of "linux sector".

Signed-off-by: Tom Yan <tom.ty89@gmail.com>
---
v2: ignore dmrsl if its sector-equivalent is larger than UINT_MAX
---
 drivers/nvme/host/core.c | 8 ++++++--
 drivers/nvme/host/nvme.h | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index efb85c6d8e2d..9d2f32739a15 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1622,6 +1622,11 @@ static void nvme_config_discard(struct gendisk *disk, struct nvme_ns *ns)
 	if (blk_queue_flag_test_and_set(QUEUE_FLAG_DISCARD, queue))
 		return;
 
+	int lts_shift = ilog2(size) - 9;
+
+	if (ctrl->dmrsl && ctrl->dmrsl <= UINT_MAX >> lts_shift)
+		ctrl->max_discard_sectors = ctrl->dmrsl << lts_shift;
+
 	blk_queue_max_discard_sectors(queue, ctrl->max_discard_sectors);
 	blk_queue_max_discard_segments(queue, ctrl->max_discard_segments);
 
@@ -2881,8 +2886,7 @@ static int nvme_init_non_mdts_limits(struct nvme_ctrl *ctrl)
 
 	if (id->dmrl)
 		ctrl->max_discard_segments = id->dmrl;
-	if (id->dmrsl)
-		ctrl->max_discard_sectors = le32_to_cpu(id->dmrsl);
+	ctrl->dmrsl = le32_to_cpu(id->dmrsl);
 	if (id->wzsl)
 		ctrl->max_zeroes_sectors = nvme_mps_to_sectors(ctrl, id->wzsl);
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 1393bbf82d71..1c85edcf7dbb 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -279,6 +279,7 @@ struct nvme_ctrl {
 #endif
 	u16 crdt[3];
 	u16 oncs;
+	u32 dmrsl;
 	u16 oacs;
 	u16 sqsize;
 	u32 max_namespaces;
-- 
2.35.2

Re: [PATCH v2] nvme: fix interpretation of dmrsl

[Christoph Hellwig]

This doesn't even compile:

drivers/nvme/host/core.c: In function ‘nvme_config_discard’:
drivers/nvme/host/core.c:1638:2: error: ISO C90 forbids mixed
declarations and code [-Werror=declaration-after-statement]
 1638 |  int lts_shift = ilog2(size) - 9;
      |  ^~~

[PATCH v3] nvme: fix interpretation of dmrsl

[Tom Yan]

dmrsl is in the unit of logical block, while max_discard_sectors is
in the unit of "linux sector".

Signed-off-by: Tom Yan <tom.ty89@gmail.com>
---
v2: ignore dmrsl if its sector-equivalent is larger than UINT_MAX
v3: put the lts_shift declaration before code
---
 drivers/nvme/host/core.c | 7 +++++--
 drivers/nvme/host/nvme.h | 1 +
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index efb85c6d8e2d..8bee0d139fd6 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1606,6 +1606,7 @@ static void nvme_config_discard(struct gendisk *disk, struct nvme_ns *ns)
 	struct nvme_ctrl *ctrl = ns->ctrl;
 	struct request_queue *queue = disk->queue;
 	u32 size = queue_logical_block_size(queue);
+	int lts_shift = ilog2(size) - 9;
 
 	if (ctrl->max_discard_sectors == 0) {
 		blk_queue_flag_clear(QUEUE_FLAG_DISCARD, queue);
@@ -1622,6 +1623,9 @@ static void nvme_config_discard(struct gendisk *disk, struct nvme_ns *ns)
 	if (blk_queue_flag_test_and_set(QUEUE_FLAG_DISCARD, queue))
 		return;
 
+	if (ctrl->dmrsl && ctrl->dmrsl <= UINT_MAX >> lts_shift)
+		ctrl->max_discard_sectors = ctrl->dmrsl << lts_shift;
+
 	blk_queue_max_discard_sectors(queue, ctrl->max_discard_sectors);
 	blk_queue_max_discard_segments(queue, ctrl->max_discard_segments);
 
@@ -2881,8 +2885,7 @@ static int nvme_init_non_mdts_limits(struct nvme_ctrl *ctrl)
 
 	if (id->dmrl)
 		ctrl->max_discard_segments = id->dmrl;
-	if (id->dmrsl)
-		ctrl->max_discard_sectors = le32_to_cpu(id->dmrsl);
+	ctrl->dmrsl = le32_to_cpu(id->dmrsl);
 	if (id->wzsl)
 		ctrl->max_zeroes_sectors = nvme_mps_to_sectors(ctrl, id->wzsl);
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 1393bbf82d71..1c85edcf7dbb 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -279,6 +279,7 @@ struct nvme_ctrl {
 #endif
 	u16 crdt[3];
 	u16 oncs;
+	u32 dmrsl;
 	u16 oacs;
 	u16 sqsize;
 	u32 max_namespaces;
-- 
2.36.0

Re: [PATCH v3] nvme: fix interpretation of dmrsl

[Keith Busch]

On Fri, Apr 29, 2022 at 12:50:51AM +0800, Tom Yan wrote:
> @@ -1606,6 +1606,7 @@ static void nvme_config_discard(struct gendisk *disk, struct nvme_ns *ns)
>  	struct nvme_ctrl *ctrl = ns->ctrl;
>  	struct request_queue *queue = disk->queue;
>  	u32 size = queue_logical_block_size(queue);
> +	int lts_shift = ilog2(size) - 9;
>  
>  	if (ctrl->max_discard_sectors == 0) {
>  		blk_queue_flag_clear(QUEUE_FLAG_DISCARD, queue);
> @@ -1622,6 +1623,9 @@ static void nvme_config_discard(struct gendisk *disk, struct nvme_ns *ns)
>  	if (blk_queue_flag_test_and_set(QUEUE_FLAG_DISCARD, queue))
>  		return;
>  
> +	if (ctrl->dmrsl && ctrl->dmrsl <= UINT_MAX >> lts_shift)
> +		ctrl->max_discard_sectors = ctrl->dmrsl << lts_shift;
> +
>  	blk_queue_max_discard_sectors(queue, ctrl->max_discard_sectors);

I think the only change you really need is just changing the above line to:

  	blk_queue_max_discard_sectors(queue, nvme_lba_to_sect(ns, ctrl->max_discard_sectors));

[PATCH v4] nvme: fix interpretation of dmrsl

[Tom Yan]

dmrsl is in the unit of logical block, while max_discard_sectors is
in the unit of "linux sector".

Signed-off-by: Tom Yan <tom.ty89@gmail.com>
---
v2: ignore dmrsl if its sector-equivalent is larger than UINT_MAX
v3: put the lts_shift declaration before code
v4: make use of nvme_sect_to_lba() and nvme_lba_to_sect()
---
 drivers/nvme/host/core.c | 6 ++++--
 drivers/nvme/host/nvme.h | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index efb85c6d8e2d..8e06a6d675ef 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1622,6 +1622,9 @@ static void nvme_config_discard(struct gendisk *disk, struct nvme_ns *ns)
 	if (blk_queue_flag_test_and_set(QUEUE_FLAG_DISCARD, queue))
 		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);
 
@@ -2881,8 +2884,7 @@ static int nvme_init_non_mdts_limits(struct nvme_ctrl *ctrl)
 
 	if (id->dmrl)
 		ctrl->max_discard_segments = id->dmrl;
-	if (id->dmrsl)
-		ctrl->max_discard_sectors = le32_to_cpu(id->dmrsl);
+	ctrl->dmrsl = le32_to_cpu(id->dmrsl);
 	if (id->wzsl)
 		ctrl->max_zeroes_sectors = nvme_mps_to_sectors(ctrl, id->wzsl);
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 1393bbf82d71..1c85edcf7dbb 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -279,6 +279,7 @@ struct nvme_ctrl {
 #endif
 	u16 crdt[3];
 	u16 oncs;
+	u32 dmrsl;
 	u16 oacs;
 	u16 sqsize;
 	u32 max_namespaces;
-- 
2.36.0

Re: [PATCH v4] nvme: fix interpretation of dmrsl

[Tom Yan]

Hi Keith,

IIRC I was once told that 0xffffffff should be set as the queue's max
discard sectors, regardless of logical block size, to signify that the
drive does not pose any limit. Besides I don't think it's a good idea
to put a value that is not a count of sectors into a variable which
expects that (as it would be confusing).

I also kept the overflow check. Let me know if there's a good reason not to.

Regards,
Tom

On Fri, 29 Apr 2022 at 12:53, Tom Yan <tom.ty89@gmail.com> wrote:
>
> dmrsl is in the unit of logical block, while max_discard_sectors is
> in the unit of "linux sector".
>
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
> ---
> v2: ignore dmrsl if its sector-equivalent is larger than UINT_MAX
> v3: put the lts_shift declaration before code
> v4: make use of nvme_sect_to_lba() and nvme_lba_to_sect()
> ---
>  drivers/nvme/host/core.c | 6 ++++--
>  drivers/nvme/host/nvme.h | 1 +
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index efb85c6d8e2d..8e06a6d675ef 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1622,6 +1622,9 @@ static void nvme_config_discard(struct gendisk *disk, struct nvme_ns *ns)
>         if (blk_queue_flag_test_and_set(QUEUE_FLAG_DISCARD, queue))
>                 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);
>
> @@ -2881,8 +2884,7 @@ static int nvme_init_non_mdts_limits(struct nvme_ctrl *ctrl)
>
>         if (id->dmrl)
>                 ctrl->max_discard_segments = id->dmrl;
> -       if (id->dmrsl)
> -               ctrl->max_discard_sectors = le32_to_cpu(id->dmrsl);
> +       ctrl->dmrsl = le32_to_cpu(id->dmrsl);
>         if (id->wzsl)
>                 ctrl->max_zeroes_sectors = nvme_mps_to_sectors(ctrl, id->wzsl);
>
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 1393bbf82d71..1c85edcf7dbb 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -279,6 +279,7 @@ struct nvme_ctrl {
>  #endif
>         u16 crdt[3];
>         u16 oncs;
> +       u32 dmrsl;
>         u16 oacs;
>         u16 sqsize;
>         u32 max_namespaces;
> --
> 2.36.0
>

Re: [PATCH v4] nvme: fix interpretation of dmrsl

[Christoph Hellwig]

Thanks,

applied to nvme-5.19.