[PATCH V2] nvme: set non-mdts limits in nvme_scan_work

[PATCH V2] nvme: set non-mdts limits in nvme_scan_work

[Chaitanya Kulkarni]

In current implementation we set the non-mdts limits by calling
nvme_init_non_mdts_limits() from nvme_init_ctrl_finish().
This also tries to set the limits for the discovery controller which
has no I/O queues resulting in the warning message reported by the
nvme_log_error() when running blktest nvme/002: -

[ 2005.155946] run blktests nvme/002 at 2022-04-09 16:57:47
[ 2005.192223] loop: module loaded
[ 2005.196429] nvmet: adding nsid 1 to subsystem blktests-subsystem-0
[ 2005.200334] nvmet: adding nsid 1 to subsystem blktests-subsystem-1

<------------------------------SNIP---------------------------------->

[ 2008.958108] nvmet: adding nsid 1 to subsystem blktests-subsystem-997
[ 2008.962082] nvmet: adding nsid 1 to subsystem blktests-subsystem-998
[ 2008.966102] nvmet: adding nsid 1 to subsystem blktests-subsystem-999
[ 2008.973132] nvmet: creating discovery controller 1 for subsystem nqn.2014-08.org.nvmexpress.discovery for NQN testhostnqn.
*[ 2008.973196] nvme1: Identify(0x6), Invalid Field in Command (sct 0x0 / sc 0x2) MORE DNR*
[ 2008.974595] nvme nvme1: new ctrl: "nqn.2014-08.org.nvmexpress.discovery"
[ 2009.103248] nvme nvme1: Removing ctrl: NQN "nqn.2014-08.org.nvmexpress.discovery"

Move the call of nvme_init_non_mdts_limits() to nvme_scan_work() after
we verify that I/O queues are created since that is a converging point
for each transport where these limits are actually used.

1. FC :
nvme_fc_create_association()
 ...
 nvme_fc_create_io_queues()
 ...
 nvme_start_ctrl()
  nvme_scan_queue()
   nvme_scan_work()

2. PCIe:-
nvme_reset_work()
 ...
 nvme_setup_io_queues()
  nvme_create_io_queues()
   nvme_alloc_queue()
 ...
 nvme_start_ctrl()
  nvme_scan_queue()
   nvme_scan_work()

3. RDMA :-
nvme_rdma_setup_ctrl()
 ...
 nvme_rdma_configure_io_queues()
 ...
 nvme_start_ctrl()
  nvme_scan_queue()
   nvme_scan_work()

4. TCP :-
nvme_tcp_setup_ctrl()
 ...
 nvme_tcp_configure_io_queues()
 ...
 nvme_start_ctrl()
  nvme_scan_queue()
   nvme_scan_work()

* nvme_scan_work()
...
nvme_validate_or_alloc_ns()
  nvme_alloc_ns()
   nvme_update_ns_info()
    nvme_update_disk_info()
     nvme_config_discard() <---
     blk_queue_max_write_zeroes_sectors() <---

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
* changes from V1:-

1. Instead of duplicating the call for nvme_init_non_mdts_limits()
   move it to the nvme_scan_work().

---
 drivers/nvme/host/core.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 42f9772abc4d..8cbd70606f09 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3082,10 +3082,6 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl)
 	if (ret)
 		return ret;
 
-	ret = nvme_init_non_mdts_limits(ctrl);
-	if (ret < 0)
-		return ret;
-
 	ret = nvme_configure_apst(ctrl);
 	if (ret < 0)
 		return ret;
@@ -4239,11 +4235,19 @@ static void nvme_scan_work(struct work_struct *work)
 {
 	struct nvme_ctrl *ctrl =
 		container_of(work, struct nvme_ctrl, scan_work);
+	int ret;
 
 	/* No tagset on a live ctrl means IO queues could not created */
 	if (ctrl->state != NVME_CTRL_LIVE || !ctrl->tagset)
 		return;
 
+	ret = nvme_init_non_mdts_limits(ctrl);
+	if (ret < 0) {
+		dev_warn(ctrl->device,
+			"reading non-mdts-limits failed: %d\n", ret);
+		return;
+	}
+
 	if (test_and_clear_bit(NVME_AER_NOTICE_NS_CHANGED, &ctrl->events)) {
 		dev_info(ctrl->device, "rescanning namespaces.\n");
 		nvme_clear_changed_ns_log(ctrl);
-- 
2.29.0

Re: [PATCH V2] nvme: set non-mdts limits in nvme_scan_work

[Christoph Hellwig]

This re-reads the limits everytime a namespace is added or deleted,
is that really such a good idea?

On Tue, May 17, 2022 at 11:49:44PM -0700, Chaitanya Kulkarni wrote:
> In current implementation we set the non-mdts limits by calling
> nvme_init_non_mdts_limits() from nvme_init_ctrl_finish().
> This also tries to set the limits for the discovery controller which
> has no I/O queues resulting in the warning message reported by the
> nvme_log_error() when running blktest nvme/002: -
> 
> [ 2005.155946] run blktests nvme/002 at 2022-04-09 16:57:47
> [ 2005.192223] loop: module loaded
> [ 2005.196429] nvmet: adding nsid 1 to subsystem blktests-subsystem-0
> [ 2005.200334] nvmet: adding nsid 1 to subsystem blktests-subsystem-1
> 
> <------------------------------SNIP---------------------------------->
> 
> [ 2008.958108] nvmet: adding nsid 1 to subsystem blktests-subsystem-997
> [ 2008.962082] nvmet: adding nsid 1 to subsystem blktests-subsystem-998
> [ 2008.966102] nvmet: adding nsid 1 to subsystem blktests-subsystem-999
> [ 2008.973132] nvmet: creating discovery controller 1 for subsystem nqn.2014-08.org.nvmexpress.discovery for NQN testhostnqn.
> *[ 2008.973196] nvme1: Identify(0x6), Invalid Field in Command (sct 0x0 / sc 0x2) MORE DNR*
> [ 2008.974595] nvme nvme1: new ctrl: "nqn.2014-08.org.nvmexpress.discovery"
> [ 2009.103248] nvme nvme1: Removing ctrl: NQN "nqn.2014-08.org.nvmexpress.discovery"
> 
> Move the call of nvme_init_non_mdts_limits() to nvme_scan_work() after
> we verify that I/O queues are created since that is a converging point
> for each transport where these limits are actually used.
> 
> 1. FC :
> nvme_fc_create_association()
>  ...
>  nvme_fc_create_io_queues()
>  ...
>  nvme_start_ctrl()
>   nvme_scan_queue()
>    nvme_scan_work()
> 
> 2. PCIe:-
> nvme_reset_work()
>  ...
>  nvme_setup_io_queues()
>   nvme_create_io_queues()
>    nvme_alloc_queue()
>  ...
>  nvme_start_ctrl()
>   nvme_scan_queue()
>    nvme_scan_work()
> 
> 3. RDMA :-
> nvme_rdma_setup_ctrl()
>  ...
>  nvme_rdma_configure_io_queues()
>  ...
>  nvme_start_ctrl()
>   nvme_scan_queue()
>    nvme_scan_work()
> 
> 4. TCP :-
> nvme_tcp_setup_ctrl()
>  ...
>  nvme_tcp_configure_io_queues()
>  ...
>  nvme_start_ctrl()
>   nvme_scan_queue()
>    nvme_scan_work()
> 
> * nvme_scan_work()
> ...
> nvme_validate_or_alloc_ns()
>   nvme_alloc_ns()
>    nvme_update_ns_info()
>     nvme_update_disk_info()
>      nvme_config_discard() <---
>      blk_queue_max_write_zeroes_sectors() <---
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
> * changes from V1:-
> 
> 1. Instead of duplicating the call for nvme_init_non_mdts_limits()
>    move it to the nvme_scan_work().
> 
> ---
>  drivers/nvme/host/core.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 42f9772abc4d..8cbd70606f09 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3082,10 +3082,6 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl)
>  	if (ret)
>  		return ret;
>  
> -	ret = nvme_init_non_mdts_limits(ctrl);
> -	if (ret < 0)
> -		return ret;
> -
>  	ret = nvme_configure_apst(ctrl);
>  	if (ret < 0)
>  		return ret;
> @@ -4239,11 +4235,19 @@ static void nvme_scan_work(struct work_struct *work)
>  {
>  	struct nvme_ctrl *ctrl =
>  		container_of(work, struct nvme_ctrl, scan_work);
> +	int ret;
>  
>  	/* No tagset on a live ctrl means IO queues could not created */
>  	if (ctrl->state != NVME_CTRL_LIVE || !ctrl->tagset)
>  		return;
>  
> +	ret = nvme_init_non_mdts_limits(ctrl);
> +	if (ret < 0) {
> +		dev_warn(ctrl->device,
> +			"reading non-mdts-limits failed: %d\n", ret);
> +		return;
> +	}
> +
>  	if (test_and_clear_bit(NVME_AER_NOTICE_NS_CHANGED, &ctrl->events)) {
>  		dev_info(ctrl->device, "rescanning namespaces.\n");
>  		nvme_clear_changed_ns_log(ctrl);
> -- 
> 2.29.0
---end quoted text---

Re: [PATCH V2] nvme: set non-mdts limits in nvme_scan_work

[Keith Busch]

On Wed, May 18, 2022 at 10:56:02AM +0200, Christoph Hellwig wrote:
> This re-reads the limits everytime a namespace is added or deleted,
> is that really such a good idea?

The limits are reported in logical blocks, which is dependent on the formats
that attached namespaces have.

Re: [PATCH V2] nvme: set non-mdts limits in nvme_scan_work

[Christoph Hellwig]

On Wed, May 18, 2022 at 08:42:02AM -0600, Keith Busch wrote:
> On Wed, May 18, 2022 at 10:56:02AM +0200, Christoph Hellwig wrote:
> > This re-reads the limits everytime a namespace is added or deleted,
> > is that really such a good idea?
> 
> The limits are reported in logical blocks, which is dependent on the formats
> that attached namespaces have.

Well, the Identify controller limits aren't really supposed to
change randomly.  But ok, maybe that is a reason to frequently
rescan due work around this broken design.  Can we add a comment
to explain that?

Re: [PATCH V2] nvme: set non-mdts limits in nvme_scan_work

[Chaitanya Kulkarni]

On 5/18/22 07:51, Christoph Hellwig wrote:
> On Wed, May 18, 2022 at 08:42:02AM -0600, Keith Busch wrote:
>> On Wed, May 18, 2022 at 10:56:02AM +0200, Christoph Hellwig wrote:
>>> This re-reads the limits everytime a namespace is added or deleted,
>>> is that really such a good idea?
>>
>> The limits are reported in logical blocks, which is dependent on the formats
>> that attached namespaces have.
> 
> Well, the Identify controller limits aren't really supposed to
> change randomly.  But ok, maybe that is a reason to frequently
> rescan due work around this broken design.  Can we add a comment
> to explain that?

sent out V3 with added comment.

-ck