[PATCH] nvme-fabrics: check max outstanding commands

[PATCH] nvme-fabrics: check max outstanding commands

[Guixin Liu]

Maxcmd is mandatory for fabrics, check it early to identify the root
cause instead of waiting for it to propagate to "sqsize" and "allocing
queue".

By the way, change nvme_check_ctrl_fabric_info() to
nvmf_validate_identify_ctrl().

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 drivers/nvme/host/core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 0a96362912ce..5cdd22f591f9 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3084,7 +3084,8 @@ static int nvme_init_effects(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 	return 0;
 }
 
-static int nvme_check_ctrl_fabric_info(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
+static int nvmf_validate_identify_ctrl(struct nvme_ctrl *ctrl,
+				       struct nvme_id_ctrl *id)
 {
 	/*
 	 * In fabrics we need to verify the cntlid matches the
@@ -3117,6 +3118,11 @@ static int nvme_check_ctrl_fabric_info(struct nvme_ctrl *ctrl, struct nvme_id_ct
 		return -EINVAL;
 	}
 
+	if (!ctrl->maxcmd) {
+		dev_err(ctrl->device, "Maximum outstanding commands is 0\n");
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
@@ -3232,7 +3238,7 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
 		ctrl->iorcsz = le32_to_cpu(id->iorcsz);
 		ctrl->maxcmd = le16_to_cpu(id->maxcmd);
 
-		ret = nvme_check_ctrl_fabric_info(ctrl, id);
+		ret = nvmf_validate_identify_ctrl(ctrl, id);
 		if (ret)
 			goto out_free;
 	} else {
-- 
2.43.0

Re: [PATCH] nvme-fabrics: check max outstanding commands

[Guixin Liu]

Hi,

     Gentle ping...

Best Regards,

Guixin Liu

在 2024/2/23 11:30, Guixin Liu 写道:
> Maxcmd is mandatory for fabrics, check it early to identify the root
> cause instead of waiting for it to propagate to "sqsize" and "allocing
> queue".
>
> By the way, change nvme_check_ctrl_fabric_info() to
> nvmf_validate_identify_ctrl().
>
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> ---
>   drivers/nvme/host/core.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 0a96362912ce..5cdd22f591f9 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3084,7 +3084,8 @@ static int nvme_init_effects(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>   	return 0;
>   }
>   
> -static int nvme_check_ctrl_fabric_info(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
> +static int nvmf_validate_identify_ctrl(struct nvme_ctrl *ctrl,
> +				       struct nvme_id_ctrl *id)
>   {
>   	/*
>   	 * In fabrics we need to verify the cntlid matches the
> @@ -3117,6 +3118,11 @@ static int nvme_check_ctrl_fabric_info(struct nvme_ctrl *ctrl, struct nvme_id_ct
>   		return -EINVAL;
>   	}
>   
> +	if (!ctrl->maxcmd) {
> +		dev_err(ctrl->device, "Maximum outstanding commands is 0\n");
> +		return -EINVAL;
> +	}
> +
>   	return 0;
>   }
>   
> @@ -3232,7 +3238,7 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
>   		ctrl->iorcsz = le32_to_cpu(id->iorcsz);
>   		ctrl->maxcmd = le16_to_cpu(id->maxcmd);
>   
> -		ret = nvme_check_ctrl_fabric_info(ctrl, id);
> +		ret = nvmf_validate_identify_ctrl(ctrl, id);
>   		if (ret)
>   			goto out_free;
>   	} else {

Re: [PATCH] nvme-fabrics: check max outstanding commands

[Keith Busch]

On Fri, Feb 23, 2024 at 11:30:04AM +0800, Guixin Liu wrote:
> Maxcmd is mandatory for fabrics, check it early to identify the root
> cause instead of waiting for it to propagate to "sqsize" and "allocing
> queue".
> 
> By the way, change nvme_check_ctrl_fabric_info() to
> nvmf_validate_identify_ctrl().
> 
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> ---
>  drivers/nvme/host/core.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 0a96362912ce..5cdd22f591f9 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3084,7 +3084,8 @@ static int nvme_init_effects(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>  	return 0;
>  }
>  
> -static int nvme_check_ctrl_fabric_info(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
> +static int nvmf_validate_identify_ctrl(struct nvme_ctrl *ctrl,
> +				       struct nvme_id_ctrl *id)
>  {
>  	/*
>  	 * In fabrics we need to verify the cntlid matches the
> @@ -3117,6 +3118,11 @@ static int nvme_check_ctrl_fabric_info(struct nvme_ctrl *ctrl, struct nvme_id_ct
>  		return -EINVAL;
>  	}
>  
> +	if (!ctrl->maxcmd) {
> +		dev_err(ctrl->device, "Maximum outstanding commands is 0\n");
> +		return -EINVAL;
> +	}

This part seems fine, but I don't think you need to change the name of
the function for this addition.

Re: [PATCH] nvme-fabrics: check max outstanding commands

[Guixin Liu]

在 2024/2/28 01:46, Keith Busch 写道:
> On Fri, Feb 23, 2024 at 11:30:04AM +0800, Guixin Liu wrote:
>> Maxcmd is mandatory for fabrics, check it early to identify the root
>> cause instead of waiting for it to propagate to "sqsize" and "allocing
>> queue".
>>
>> By the way, change nvme_check_ctrl_fabric_info() to
>> nvmf_validate_identify_ctrl().
>>
>> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
>> ---
>>   drivers/nvme/host/core.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 0a96362912ce..5cdd22f591f9 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -3084,7 +3084,8 @@ static int nvme_init_effects(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>>   	return 0;
>>   }
>>   
>> -static int nvme_check_ctrl_fabric_info(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>> +static int nvmf_validate_identify_ctrl(struct nvme_ctrl *ctrl,
>> +				       struct nvme_id_ctrl *id)
>>   {
>>   	/*
>>   	 * In fabrics we need to verify the cntlid matches the
>> @@ -3117,6 +3118,11 @@ static int nvme_check_ctrl_fabric_info(struct nvme_ctrl *ctrl, struct nvme_id_ct
>>   		return -EINVAL;
>>   	}
>>   
>> +	if (!ctrl->maxcmd) {
>> +		dev_err(ctrl->device, "Maximum outstanding commands is 0\n");
>> +		return -EINVAL;
>> +	}
> This part seems fine, but I don't think you need to change the name of
> the function for this addition.

OK, this will be removed in v2.

Best Regards,

Guixin Liu

Re: Re: [PATCH] nvme-fabrics: check max outstanding commands

[Daniel Wagner]

On Wed, Feb 28, 2024 at 10:23:24AM +0800, Guixin Liu wrote:
> 
> 在 2024/2/28 01:46, Keith Busch 写道:
> > On Fri, Feb 23, 2024 at 11:30:04AM +0800, Guixin Liu wrote:
> > > Maxcmd is mandatory for fabrics, check it early to identify the root
> > > cause instead of waiting for it to propagate to "sqsize" and "allocing
> > > queue".
> > > 
> > > By the way, change nvme_check_ctrl_fabric_info() to
> > > nvmf_validate_identify_ctrl().
> > > 
> > > Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> > > ---
> > >   drivers/nvme/host/core.c | 10 ++++++++--
> > >   1 file changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > > index 0a96362912ce..5cdd22f591f9 100644
> > > --- a/drivers/nvme/host/core.c
> > > +++ b/drivers/nvme/host/core.c
> > > @@ -3084,7 +3084,8 @@ static int nvme_init_effects(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
> > >   	return 0;
> > >   }
> > > -static int nvme_check_ctrl_fabric_info(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
> > > +static int nvmf_validate_identify_ctrl(struct nvme_ctrl *ctrl,
> > > +				       struct nvme_id_ctrl *id)
> > >   {
> > >   	/*
> > >   	 * In fabrics we need to verify the cntlid matches the
> > > @@ -3117,6 +3118,11 @@ static int nvme_check_ctrl_fabric_info(struct nvme_ctrl *ctrl, struct nvme_id_ct
> > >   		return -EINVAL;
> > >   	}
> > > +	if (!ctrl->maxcmd) {
> > > +		dev_err(ctrl->device, "Maximum outstanding commands is 0\n");
> > > +		return -EINVAL;
> > > +	}
> > This part seems fine, but I don't think you need to change the name of
> > the function for this addition.
> 
> OK, this will be removed in v2.

And please run blktests with it. Last time, we had some fallouts when a
new check was added. Thanks.

Re: [PATCH] nvme-fabrics: check max outstanding commands

[Guixin Liu]

> And please run blktests with it. Last time, we had some fallouts when a
> new check was added. Thanks.

I am sorry for the last time's mistake, I've already run the blktests 
this time,

the maxcmd is mandatory for all controllers.

Best Regards,

Guixin Liu


Blktests log:

nvme/002 (create many subsystems and test discovery)         [passed]
     runtime  16.481s  ...  16.716s
nvme/003 (test if we're sending keep-alives to a discovery controller) 
[passed]
     runtime  11.130s  ...  11.160s
nvme/004 (test nvme and nvmet UUID NS descriptors) [passed]
     runtime  0.535s  ...  0.615s
nvme/005 (reset local loopback target) [passed]
     runtime  0.969s  ...  0.973s
nvme/006 (create an NVMeOF target with a block device-backed ns) [passed]
     runtime  0.046s  ...  0.046s
nvme/007 (create an NVMeOF target with a file-backed ns) [passed]
     runtime  0.029s  ...  0.028s
nvme/008 (create an NVMeOF host with a block device-backed ns) [passed]
     runtime  0.540s  ...  0.519s
nvme/009 (create an NVMeOF host with a file-backed ns) [passed]
     runtime  0.539s  ...  0.536s
nvme/010 (run data verification fio job on NVMeOF block device-backed 
ns) [passed]
     runtime  3.388s  ...  3.356s
nvme/011 (run data verification fio job on NVMeOF file-backed ns) [passed]
     runtime  259.255s  ...  265.053s
nvme/012 (run mkfs and data verification fio job on NVMeOF block 
device-backed ns) [passed]
     runtime  11.734s  ...  11.623s
nvme/013 (run mkfs and data verification fio job on NVMeOF file-backed 
ns) [passed]
     runtime  228.180s  ...  231.981s
nvme/014 (flush a NVMeOF block device-backed ns) [passed]
     runtime  12.468s  ...  14.417s
nvme/015 (unit test for NVMe flush for file backed ns) [passed]
     runtime  11.854s  ...  11.849s
nvme/016 (create/delete many NVMeOF block device-backed ns and test 
discovery) [passed]
     runtime  8.017s  ...  8.201s
nvme/017 (create/delete many file-ns and test discovery) [passed]
     runtime  8.419s  ...  8.596s
nvme/018 (unit test NVMe-oF out of range access on a file backend) [passed]
     runtime  0.511s  ...  0.529s
nvme/019 (test NVMe DSM Discard command on NVMeOF block-device ns) [passed]
     runtime  0.535s  ...  0.547s
nvme/020 (test NVMe DSM Discard command on NVMeOF file-backed ns) [passed]
     runtime  0.529s  ...  0.526s
nvme/021 (test NVMe list command on NVMeOF file-backed ns) [passed]
     runtime  0.529s  ...  0.525s
nvme/022 (test NVMe reset command on NVMeOF file-backed ns) [passed]
     runtime  0.949s  ...  0.951s
nvme/023 (test NVMe smart-log command on NVMeOF block-device ns) [passed]
     runtime  0.527s  ...  0.557s
nvme/024 (test NVMe smart-log command on NVMeOF file-backed ns) [passed]
     runtime  0.526s  ...  0.523s
nvme/025 (test NVMe effects-log command on NVMeOF file-backed ns) [passed]
     runtime  0.539s  ...  0.526s
nvme/026 (test NVMe ns-descs command on NVMeOF file-backed ns) [passed]
     runtime  0.531s  ...  0.544s
nvme/027 (test NVMe ns-rescan command on NVMeOF file-backed ns) [passed]
     runtime  0.529s  ...  0.512s
nvme/028 (test NVMe list-subsys command on NVMeOF file-backed ns) [passed]
     runtime  0.520s  ...  0.523s
nvme/029 (test userspace IO via nvme-cli read/write interface) [passed]
     runtime  0.600s  ...  0.595s
nvme/030 (ensure the discovery generation counter is updated 
appropriately) [passed]
     runtime  0.183s  ...  0.160s
nvme/031 (test deletion of NVMeOF controllers immediately after setup) 
[passed]
     runtime  5.149s  ...  4.988s
nvme/038 (test deletion of NVMeOF subsystem without enabling) [passed]
     runtime  0.006s  ...  0.006s
nvme/040 (test nvme fabrics controller reset/disconnect operation during 
I/O) [passed]
     runtime  7.054s  ...  7.077s
nvme/041 (Create authenticated connections)                  [not run]
     kernel 6.8.0-rc5+ config not found
     kernel 6.8.0-rc5+ config not found
nvme/042 (Test dhchap key types for authenticated connections) [not run]
     kernel 6.8.0-rc5+ config not found
     kernel 6.8.0-rc5+ config not found
nvme/043 (Test hash and DH group variations for authenticated 
connections) [not run]
     kernel 6.8.0-rc5+ config not found
     kernel 6.8.0-rc5+ config not found
nvme/044 (Test bi-directional authentication)                [not run]
     kernel 6.8.0-rc5+ config not found
     kernel 6.8.0-rc5+ config not found
nvme/045 (Test re-authentication)                            [not run]
     kernel 6.8.0-rc5+ config not found
     kernel 6.8.0-rc5+ config not found
nvme/047 (test different queue types for fabric transports)  [not run]
     runtime  2.730s  ...
     nvme_trtype=loop is not supported in this test
nvme/048 (Test queue count changes on reconnect)             [not run]
     runtime  7.073s  ...
     nvme_trtype=loop is not supported in this test

Re: [PATCH] nvme-fabrics: check max outstanding commands

[Daniel Wagner]

On Wed, Feb 28, 2024 at 05:54:41PM +0800, Guixin Liu wrote:
> 
> > And please run blktests with it. Last time, we had some fallouts when a
> > new check was added. Thanks.
> 
> I am sorry for the last time's mistake, I've already run the blktests this
> time,

No worries.

> the maxcmd is mandatory for all controllers.

Sure, not disputing this, it's just we should at least make sure that
our soft target implementation is following the rules.

> Blktests log:

Excellent. Thanks!