[v2,4/5] nvme-fc: Make initial connect attempt synchronous

  Commit 4c984154efa1 ("nvme-fc: change controllers first connect to use
reconnect path") made the connection attempt asynchronous in order to
make the connection attempt from autoconnect/boot via udev/systemd up
case a bit more reliable.

Unfortunately, one side effect of this is that any wrong parameters
provided from userspace will not be directly reported as invalid, e.g.
auth keys.

So instead having the policy code inside the kernel it's better to
address this in userspace, for example in nvme-cli or nvme-stas.

This aligns the fc transport with tcp and rdma.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 drivers/nvme/host/fc.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)
  

Hi Daniel,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Daniel-Wagner/nvme-fc-Do-not-wait-in-vain-when-unloading-module/20230620-213849
base:   linus/master
patch link:    https://lore.kernel.org/r/20230620133711.22840-5-dwagner%40suse.de
patch subject: [PATCH v2 4/5] nvme-fc: Make initial connect attempt synchronous
config: openrisc-randconfig-m041-20230622 (https://download.01.org/0day-ci/archive/20230624/202306240125.U2jdrjAY-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230624/202306240125.U2jdrjAY-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202306240125.U2jdrjAY-lkp@intel.com/

smatch warnings:
drivers/nvme/host/fc.c:3590 nvme_fc_init_ctrl() warn: passing zero to 'ERR_PTR'

vim +/ERR_PTR +3590 drivers/nvme/host/fc.c

61bff8ef008845 James Smart       2017-04-23  3533  	ret = nvme_init_ctrl(&ctrl->ctrl, dev, &nvme_fc_ctrl_ops, 0);
61bff8ef008845 James Smart       2017-04-23  3534  	if (ret)
98e3528012cd57 Ross Lagerwall    2023-01-20  3535  		goto out_free_queues;
e399441de9115c James Smart       2016-12-02  3536  
61bff8ef008845 James Smart       2017-04-23  3537  	/* at this point, teardown path changes to ref counting on nvme ctrl */
e399441de9115c James Smart       2016-12-02  3538  
98e3528012cd57 Ross Lagerwall    2023-01-20  3539  	ret = nvme_alloc_admin_tag_set(&ctrl->ctrl, &ctrl->admin_tag_set,
98e3528012cd57 Ross Lagerwall    2023-01-20  3540  			&nvme_fc_admin_mq_ops,
98e3528012cd57 Ross Lagerwall    2023-01-20  3541  			struct_size((struct nvme_fcp_op_w_sgl *)NULL, priv,
98e3528012cd57 Ross Lagerwall    2023-01-20  3542  				    ctrl->lport->ops->fcprqst_priv_sz));
98e3528012cd57 Ross Lagerwall    2023-01-20  3543  	if (ret)
98e3528012cd57 Ross Lagerwall    2023-01-20  3544  		goto fail_ctrl;
98e3528012cd57 Ross Lagerwall    2023-01-20  3545  
e399441de9115c James Smart       2016-12-02  3546  	spin_lock_irqsave(&rport->lock, flags);
e399441de9115c James Smart       2016-12-02  3547  	list_add_tail(&ctrl->ctrl_list, &rport->ctrl_list);
e399441de9115c James Smart       2016-12-02  3548  	spin_unlock_irqrestore(&rport->lock, flags);
e399441de9115c James Smart       2016-12-02  3549  
ac881fd1288ca6 Daniel Wagner     2023-06-20  3550  	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING)) {
4c984154efa131 James Smart       2018-06-13  3551  		dev_err(ctrl->ctrl.device,
4c984154efa131 James Smart       2018-06-13  3552  			"NVME-FC{%d}: failed to init ctrl state\n", ctrl->cnum);
4c984154efa131 James Smart       2018-06-13  3553  		goto fail_ctrl;

No error code on this path.  Originally it didn't matter because it was
hardcoded to return ERR_PTR(-EIO);

17c4dc6eb7e1b2 James Smart       2017-10-09  3554  	}
17c4dc6eb7e1b2 James Smart       2017-10-09  3555  
ac881fd1288ca6 Daniel Wagner     2023-06-20  3556  	ret = nvme_fc_create_association(ctrl);
ac881fd1288ca6 Daniel Wagner     2023-06-20  3557  	if (ret)
4c984154efa131 James Smart       2018-06-13  3558  		goto fail_ctrl;
4c984154efa131 James Smart       2018-06-13  3559  
4c984154efa131 James Smart       2018-06-13  3560  	dev_info(ctrl->ctrl.device,
4c984154efa131 James Smart       2018-06-13  3561  		"NVME-FC{%d}: new ctrl: NQN \"%s\"\n",
e5ea42faa773c6 Hannes Reinecke   2021-09-22  3562  		ctrl->cnum, nvmf_ctrl_subsysnqn(&ctrl->ctrl));
4c984154efa131 James Smart       2018-06-13  3563  
4c984154efa131 James Smart       2018-06-13  3564  	return &ctrl->ctrl;
4c984154efa131 James Smart       2018-06-13  3565  
4c984154efa131 James Smart       2018-06-13  3566  fail_ctrl:
19fce0470f0503 James Smart       2020-12-01  3567  	cancel_work_sync(&ctrl->ioerr_work);
cf25809bec2c7d James Smart       2018-03-13  3568  	cancel_work_sync(&ctrl->ctrl.reset_work);
cf25809bec2c7d James Smart       2018-03-13  3569  	cancel_delayed_work_sync(&ctrl->connect_work);
cf25809bec2c7d James Smart       2018-03-13  3570  
de41447aac034c Ewan D. Milne     2017-04-24  3571  	ctrl->ctrl.opts = NULL;
17c4dc6eb7e1b2 James Smart       2017-10-09  3572  
61bff8ef008845 James Smart       2017-04-23  3573  	/* initiate nvme ctrl ref counting teardown */
e399441de9115c James Smart       2016-12-02  3574  	nvme_uninit_ctrl(&ctrl->ctrl);
61bff8ef008845 James Smart       2017-04-23  3575  
0b5a7669a457dd James Smart       2017-06-15  3576  	/* Remove core ctrl ref. */
0b5a7669a457dd James Smart       2017-06-15  3577  	nvme_put_ctrl(&ctrl->ctrl);
0b5a7669a457dd James Smart       2017-06-15  3578  
61bff8ef008845 James Smart       2017-04-23  3579  	/* as we're past the point where we transition to the ref
61bff8ef008845 James Smart       2017-04-23  3580  	 * counting teardown path, if we return a bad pointer here,
61bff8ef008845 James Smart       2017-04-23  3581  	 * the calling routine, thinking it's prior to the
61bff8ef008845 James Smart       2017-04-23  3582  	 * transition, will do an rport put. Since the teardown
61bff8ef008845 James Smart       2017-04-23  3583  	 * path also does a rport put, we do an extra get here to
61bff8ef008845 James Smart       2017-04-23  3584  	 * so proper order/teardown happens.
61bff8ef008845 James Smart       2017-04-23  3585  	 */
61bff8ef008845 James Smart       2017-04-23  3586  	nvme_fc_rport_get(rport);
61bff8ef008845 James Smart       2017-04-23  3587  
ac881fd1288ca6 Daniel Wagner     2023-06-20  3588  	if (ret > 0)
ac881fd1288ca6 Daniel Wagner     2023-06-20  3589  		ret = -EIO;
ac881fd1288ca6 Daniel Wagner     2023-06-20 @3590  	return ERR_PTR(ret);
e399441de9115c James Smart       2016-12-02  3591  
61bff8ef008845 James Smart       2017-04-23  3592  out_free_queues:
61bff8ef008845 James Smart       2017-04-23  3593  	kfree(ctrl->queues);
e399441de9115c James Smart       2016-12-02  3594  out_free_ida:
61bff8ef008845 James Smart       2017-04-23  3595  	put_device(ctrl->dev);
3dd83f4013f0e8 Sagi Grimberg     2022-02-14  3596  	ida_free(&nvme_fc_ctrl_cnt, ctrl->cnum);
e399441de9115c James Smart       2016-12-02  3597  out_free_ctrl:
e399441de9115c James Smart       2016-12-02  3598  	kfree(ctrl);
e399441de9115c James Smart       2016-12-02  3599  out_fail:
e399441de9115c James Smart       2016-12-02  3600  	/* exit via here doesn't follow ctlr ref points */
e399441de9115c James Smart       2016-12-02  3601  	return ERR_PTR(ret);
e399441de9115c James Smart       2016-12-02  3602  }
  

On Tue, Jun 20, 2023 at 03:37:10PM +0200, Daniel Wagner wrote:
> Commit 4c984154efa1 ("nvme-fc: change controllers first connect to use
> reconnect path") made the connection attempt asynchronous in order to
> make the connection attempt from autoconnect/boot via udev/systemd up
> case a bit more reliable.
> 
> Unfortunately, one side effect of this is that any wrong parameters
> provided from userspace will not be directly reported as invalid, e.g.
> auth keys.
> 
> So instead having the policy code inside the kernel it's better to
> address this in userspace, for example in nvme-cli or nvme-stas.
> 
> This aligns the fc transport with tcp and rdma.
> 
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>  drivers/nvme/host/fc.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 472ed285fd45..aa2911f07c6c 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -2943,6 +2943,8 @@ nvme_fc_create_io_queues(struct nvme_fc_ctrl *ctrl)
>  	/* force put free routine to ignore io queues */
>  	ctrl->ctrl.tagset = NULL;
>  
> +	if (ret > 0)
> +		ret = -EIO;

All these checks for ret > 0 make me unhappy.  I don't understand how
they are a part of the commit.

I have tried to look at the context and I think maybe you are working
around the fact that qla_nvme_ls_req() returns QLA_FUNCTION_FAILED on
error.

Also the qla_nvme_ls_req() function EINVAL on error.  I just wrote a
commit message saying that none of the callers cared but I missed that
apparently gets returned to nvme_fc_init_ctrl().  :/
https://lore.kernel.org/all/49866d28-4cfe-47b0-842b-78f110e61aab@moroto.mountain/

Let's just fix qla_nvme_ls_req() instead of working around it here.

And let's add a WARN_ON_ONCE() somewhere to prevent future bugs.

regards,
dan carpenter
  

On Mon, Jun 26, 2023 at 02:33:18PM +0300, Dan Carpenter wrote:
> > @@ -2943,6 +2943,8 @@ nvme_fc_create_io_queues(struct nvme_fc_ctrl *ctrl)
> >  	/* force put free routine to ignore io queues */
> >  	ctrl->ctrl.tagset = NULL;
> >  
> > +	if (ret > 0)
> > +		ret = -EIO;
> 
> All these checks for ret > 0 make me unhappy.  I don't understand how
> they are a part of the commit.

We have two types of error message types in the nvme subsystem. The negative
values are the usual ones and positive ones are nvme protocol errors.

For example if the authentication fails because of invalid credentials when
doing the authentication nvmf_connect_admin_queue() will return a value of
NVME_SC_AUTH_REQUIRED. This is also the value which gets propagated to this
point here. The problem is any positive error code is interpreted as a valid
pointer later in the code, which results in a crash.

> I have tried to look at the context and I think maybe you are working
> around the fact that qla_nvme_ls_req() returns QLA_FUNCTION_FAILED on
> error.

The auth blktests are exercising the error path here and that's why I added
this check. BTW, we already use in other places, this is not completely new in
this subsystem.

> Also the qla_nvme_ls_req() function EINVAL on error.  I just wrote a
> commit message saying that none of the callers cared but I missed that
> apparently gets returned to nvme_fc_init_ctrl().  :/
> https://lore.kernel.org/all/49866d28-4cfe-47b0-842b-78f110e61aab@moroto.mountain/

Thank!

> Let's just fix qla_nvme_ls_req() instead of working around it here.
>
> And let's add a WARN_ON_ONCE() somewhere to prevent future bugs.

This makes sense for the driver APIs. Though for the core nvme subsystem this
needs to be discusses/redesigned how to handle the protocol errors first.

Thanks,
Daniel
  

On 6/27/23 08:18, Daniel Wagner wrote:
> On Mon, Jun 26, 2023 at 02:33:18PM +0300, Dan Carpenter wrote:
>>> @@ -2943,6 +2943,8 @@ nvme_fc_create_io_queues(struct nvme_fc_ctrl *ctrl)
>>>   	/* force put free routine to ignore io queues */
>>>   	ctrl->ctrl.tagset = NULL;
>>>   
>>> +	if (ret > 0)
>>> +		ret = -EIO;
>>
>> All these checks for ret > 0 make me unhappy.  I don't understand how
>> they are a part of the commit.
> 
> We have two types of error message types in the nvme subsystem. The negative
> values are the usual ones and positive ones are nvme protocol errors.
> 
> For example if the authentication fails because of invalid credentials when
> doing the authentication nvmf_connect_admin_queue() will return a value of
> NVME_SC_AUTH_REQUIRED. This is also the value which gets propagated to this
> point here. The problem is any positive error code is interpreted as a valid
> pointer later in the code, which results in a crash.
> 
>> I have tried to look at the context and I think maybe you are working
>> around the fact that qla_nvme_ls_req() returns QLA_FUNCTION_FAILED on
>> error.
> 
> The auth blktests are exercising the error path here and that's why I added
> this check. BTW, we already use in other places, this is not completely new in
> this subsystem.
> 
>> Also the qla_nvme_ls_req() function EINVAL on error.  I just wrote a
>> commit message saying that none of the callers cared but I missed that
>> apparently gets returned to nvme_fc_init_ctrl().  :/
>> https://lore.kernel.org/all/49866d28-4cfe-47b0-842b-78f110e61aab@moroto.mountain/
> 
> Thank!
> 
>> Let's just fix qla_nvme_ls_req() instead of working around it here.
>>
>> And let's add a WARN_ON_ONCE() somewhere to prevent future bugs.
> 
> This makes sense for the driver APIs. Though for the core nvme subsystem this
> needs to be discusses/redesigned how to handle the protocol errors first.
> 
I would stick with the 'normal' nvme syntax of having negative errors as 
internal errors (ie errnos), '0' for no error, and positive numbers as 
NVMe protocol errors.
As such I would also advocate to not map NVMe protocol errors onto error 
numbers but rather fix the callers to not do a pointer conversion.

Cheers,

Hannes
  

On 6/27/23 08:39, Hannes Reinecke wrote:
> On 6/27/23 08:18, Daniel Wagner wrote:
>> On Mon, Jun 26, 2023 at 02:33:18PM +0300, Dan Carpenter wrote:
>>>> @@ -2943,6 +2943,8 @@ nvme_fc_create_io_queues(struct nvme_fc_ctrl 
>>>> *ctrl)
>>>>       /* force put free routine to ignore io queues */
>>>>       ctrl->ctrl.tagset = NULL;
>>>> +    if (ret > 0)
>>>> +        ret = -EIO;
>>>
>>> All these checks for ret > 0 make me unhappy.  I don't understand how
>>> they are a part of the commit.
>>
>> We have two types of error message types in the nvme subsystem. The 
>> negative
>> values are the usual ones and positive ones are nvme protocol errors.
>>
>> For example if the authentication fails because of invalid credentials 
>> when
>> doing the authentication nvmf_connect_admin_queue() will return a 
>> value of
>> NVME_SC_AUTH_REQUIRED. This is also the value which gets propagated to 
>> this
>> point here. The problem is any positive error code is interpreted as a 
>> valid
>> pointer later in the code, which results in a crash.
>>
>>> I have tried to look at the context and I think maybe you are working
>>> around the fact that qla_nvme_ls_req() returns QLA_FUNCTION_FAILED on
>>> error.
>>
>> The auth blktests are exercising the error path here and that's why I 
>> added
>> this check. BTW, we already use in other places, this is not 
>> completely new in
>> this subsystem.
>>
>>> Also the qla_nvme_ls_req() function EINVAL on error.  I just wrote a
>>> commit message saying that none of the callers cared but I missed that
>>> apparently gets returned to nvme_fc_init_ctrl().  :/
>>> https://lore.kernel.org/all/49866d28-4cfe-47b0-842b-78f110e61aab@moroto.mountain/
>>
>> Thank!
>>
>>> Let's just fix qla_nvme_ls_req() instead of working around it here.
>>>
>>> And let's add a WARN_ON_ONCE() somewhere to prevent future bugs.
>>
>> This makes sense for the driver APIs. Though for the core nvme 
>> subsystem this
>> needs to be discusses/redesigned how to handle the protocol errors first.
>>
> I would stick with the 'normal' nvme syntax of having negative errors as 
> internal errors (ie errnos), '0' for no error, and positive numbers as 
> NVMe protocol errors.
> As such I would also advocate to not map NVMe protocol errors onto error 
> numbers but rather fix the callers to not do a pointer conversion.
> 
Aw. Now I see it.

It's the ->create_ctrl() callback which will always return a controller 
pointer or an error value.
If we were to return a protocol error we would need to stick it into the 
controller structure itself. But if we doing that then we'll be ending 
up with a non-existing controller, ie we'd be returning a structure for 
a dead controller. Not exactly pretty, but it would allow us to improve
the userland API to return the NVMe protocol error by reading from the
fabrics device; the controller structure itself would be cleaned up when 
closing that device.

Hmm.

Cheers,

Hannes
  

On 6/20/2023 6:37 AM, Daniel Wagner wrote:
> Commit 4c984154efa1 ("nvme-fc: change controllers first connect to use
> reconnect path") made the connection attempt asynchronous in order to
> make the connection attempt from autoconnect/boot via udev/systemd up
> case a bit more reliable.
> 
> Unfortunately, one side effect of this is that any wrong parameters
> provided from userspace will not be directly reported as invalid, e.g.
> auth keys.
> 
> So instead having the policy code inside the kernel it's better to
> address this in userspace, for example in nvme-cli or nvme-stas.
> 
> This aligns the fc transport with tcp and rdma.

As much as you want to make this change to make transports "similar", I 
am dead set against it unless you are completing a long qualification of 
the change on real FC hardware and FC-NVME devices. There is probably 
1.5 yrs of testing of different race conditions that drove this change. 
You cannot declare success from a simplistic toy tool such as fcloop for 
validation.

The original issues exist, probably have even morphed given the time 
from the original change, and this will seriously disrupt the transport 
and any downstream releases.  So I have a very strong NACK on this change.

Yes - things such as the connect failure results are difficult to return 
back to nvme-cli. I have had many gripes about the nvme-cli's behavior 
over the years, especially on negative cases due to race conditions 
which required retries. It still fails this miserably.  The async 
reconnect path solved many of these issues for fc.

For the auth failure, how do we deal with things if auth fails over time 
as reconnects fail due to a credential changes ?  I would think 
commonality of this behavior drives part of the choice.

-- james
  

Hi James,

On Sat, Jul 01, 2023 at 05:11:11AM -0700, James Smart wrote:
> As much as you want to make this change to make transports "similar", I am
> dead set against it unless you are completing a long qualification of the
> change on real FC hardware and FC-NVME devices. There is probably 1.5 yrs of
> testing of different race conditions that drove this change. You cannot
> declare success from a simplistic toy tool such as fcloop for validation.
> 
> The original issues exist, probably have even morphed given the time from
> the original change, and this will seriously disrupt the transport and any
> downstream releases.  So I have a very strong NACK on this change.
> 
> Yes - things such as the connect failure results are difficult to return
> back to nvme-cli. I have had many gripes about the nvme-cli's behavior over
> the years, especially on negative cases due to race conditions which
> required retries. It still fails this miserably.  The async reconnect path
> solved many of these issues for fc.
> 
> For the auth failure, how do we deal with things if auth fails over time as
> reconnects fail due to a credential changes ?  I would think commonality of
> this behavior drives part of the choice.

Alright, what do you think about the idea to introduce a new '--sync' option to
nvme-cli which forwards this info to the kernel that we want to wait for the
initial connect to succeed or fail? Obviously, this needs to handle signals too.

From what I understood this is also what Ewan would like to have.

Hannes thought it would make sense to use the same initial connect logic in
tcp/rdma, because there could also be transient erros (e.g. spanning tree
protocol). In short making the tcp/rdma do the same thing as fc?

So let's drop the final patch from this series for the time. Could you give some
feedback on the rest of the patches?

Thanks,
Daniel
  

On 7/6/2023 5:07 AM, Daniel Wagner wrote:
> Hi James,
> 
> On Sat, Jul 01, 2023 at 05:11:11AM -0700, James Smart wrote:
>> As much as you want to make this change to make transports "similar", I am
>> dead set against it unless you are completing a long qualification of the
>> change on real FC hardware and FC-NVME devices. There is probably 1.5 yrs of
>> testing of different race conditions that drove this change. You cannot
>> declare success from a simplistic toy tool such as fcloop for validation.
>>
>> The original issues exist, probably have even morphed given the time from
>> the original change, and this will seriously disrupt the transport and any
>> downstream releases.  So I have a very strong NACK on this change.
>>
>> Yes - things such as the connect failure results are difficult to return
>> back to nvme-cli. I have had many gripes about the nvme-cli's behavior over
>> the years, especially on negative cases due to race conditions which
>> required retries. It still fails this miserably.  The async reconnect path
>> solved many of these issues for fc.
>>
>> For the auth failure, how do we deal with things if auth fails over time as
>> reconnects fail due to a credential changes ?  I would think commonality of
>> this behavior drives part of the choice.
> 
> Alright, what do you think about the idea to introduce a new '--sync' option to
> nvme-cli which forwards this info to the kernel that we want to wait for the
> initial connect to succeed or fail? Obviously, this needs to handle signals too.
> 
>  From what I understood this is also what Ewan would like to have
To me this is not sync vs non-sync option, it's a max_reconnects value 
tested for in nvmf_should_reconnect(). Which, if set to 0 (or 1), should 
fail if the initial connect fails.

Right now max_reconnects is calculated by the ctrl_loss_tmo and 
reconnect_delay. So there's already a way via the cli to make sure 
there's only 1 connect attempt. I wouldn't mind seeing an exact cli 
option that sets it to 1 connection attempt w/o the user calculation and 
2 value specification.

I also assume that this is not something that would be set by default in 
the auto-connect scripts or automated cli startup scripts.


> 
> Hannes thought it would make sense to use the same initial connect logic in
> tcp/rdma, because there could also be transient erros (e.g. spanning tree
> protocol). In short making the tcp/rdma do the same thing as fc?

I agree that the same connect logic makes sense for tcp/rdma. Certainly 
one connect/teardown path vs one at create and one at reconnect makes 
sense. The transient errors during 1st connect was the why FC added it 
and I would assume tcp/rdma has it's own transient errors or timing 
relationships at initial connection setups, etc.

For FC, we're trying to work around errors to transport commands (FC 
NVME ELS's) that fail (dropped or timeout) or commands used to 
initialize the controller which may be dropped/timeout thus fail 
controller init. Although NVMe-FC does have a retransmission option, it 
generally doesn't apply to the FC NVME LS's, and few of the FC devices 
have yet to turn on the retransmission option to deal with the errors. 
So the general behavior is connection termination and/or association 
termination which then depends on the reconnect path to retry. It's also 
critical as connection requests are automated on FC based on 
connectivity events. If we fail out to the cli due to the fabric 
dropping some up front command, there's no guarantee there will be 
another connectivity event to restart the controller create and we end 
up without device connectivity. The other issue we had to deal with was 
how long sysadm hung out waiting for the auto-connect script to 
complete. We couldn't wait the entire multiple retry case, and returning 
before the 1st attempt was complete was against the spirit of the cli - 
so we waited for the 1st attempt to try, released sysadm and let the 
reconnect go on in the background.


> 
> So let's drop the final patch from this series for the time. Could you give some
> feedback on the rest of the patches?
> 
> Thanks,
> Daniel

I'll look at them.

-- james
  

On 7/12/23 00:47, James Smart wrote:
> On 7/6/2023 5:07 AM, Daniel Wagner wrote:
>> Hi James,
>>
>> On Sat, Jul 01, 2023 at 05:11:11AM -0700, James Smart wrote:
>>> As much as you want to make this change to make transports "similar", 
>>> I am dead set against it unless you are completing a long qualification
>>> of the change on real FC hardware and FC-NVME devices. There is probably 
>>> 1.5 yrs of testing of different race conditions that drove this change.
>>> You cannot declare success from a simplistic toy tool such as fcloop for 
>>> validation.
>>>
>>> The original issues exist, probably have even morphed given the time 
>>> from
>>> the original change, and this will seriously disrupt the transport 
>>> and any
>>> downstream releases.  So I have a very strong NACK on this change.
>>>
>>> Yes - things such as the connect failure results are difficult to return
>>> back to nvme-cli. I have had many gripes about the nvme-cli's 
>>> behavior over
>>> the years, especially on negative cases due to race conditions which
>>> required retries. It still fails this miserably.  The async reconnect 
>>> path
>>> solved many of these issues for fc.
>>>
>>> For the auth failure, how do we deal with things if auth fails over 
>>> time as
>>> reconnects fail due to a credential changes ?  I would think 
>>> commonality of
>>> this behavior drives part of the choice.
>>
>> Alright, what do you think about the idea to introduce a new '--sync' 
>> option to
>> nvme-cli which forwards this info to the kernel that we want to wait 
>> for the
>> initial connect to succeed or fail? Obviously, this needs to handle 
>> signals too.
>>
>>  From what I understood this is also what Ewan would like to have
> To me this is not sync vs non-sync option, it's a max_reconnects value 
> tested for in nvmf_should_reconnect(). Which, if set to 0 (or 1), should 
> fail if the initial connect fails.
> 
Well, this is more a technical detail while we continue to harp about 
'sync' vs 'non-sync'.
Currently all instances of ->create_ctrl() are running asynchronously,
ie ->create_ctrl() returns a 'ctrl' object which is still in the process
of establishing the connection.
(And there it doesn't really matter whether it's FC or TCP/RDMA; FC is 
kicking of a workqueue for the 'reconnect' call, whereas TCP/RDMA is 
creating the association and issues the actual 'connect' NVMe SQE via
an I/O workqueue; net result is identical).
And when we talk about 'sync' connect we are planning to _wait_ until
this asynchronous operation reaches a steady state, ie either after the 
connect attempts succeeded or after the connect retries are exhausted.

And yes, we _are_ aware that this might be a quite long time.

> Right now max_reconnects is calculated by the ctrl_loss_tmo and 
> reconnect_delay. So there's already a way via the cli to make sure 
> there's only 1 connect attempt. I wouldn't mind seeing an exact cli 
> option that sets it to 1 connection attempt w/o the user calculation and 
> 2 value specification.
> 
Again, we do _not_ propose to change any of the default settings.
The 'sync' option will not modify the reconnect settings, it will just 
wait until a steady state it reached.

> I also assume that this is not something that would be set by default in 
> the auto-connect scripts or automated cli startup scripts.
> 
You assume correctly. That's why it'll be an additional option.

Cheers,

Hannes
  

The basic issue I am trying to solve with NVMe/FC is that we cannot get systems
to boot reliably from NVMe/FC (boot from SAN) because we don't know how long
the connect is going to take, and we have seen spurious failures in our testing.

In general, I think the whole business of a userspace syscall() -> asynchronous
work in the kernel is problematic because the syscall can return a good status
even if the connect is never going to work due to a condition that is
not going to
clear.  It is difficult and cumbersome to script around this reliably.

So what I am saying is the current mechanism doesn't work completely
right anyway.

We also encounter the problem Daniel is addressing trying to get
blktests and other
internal tests to work, for similar reasons.

-Ewan

On Thu, Jul 6, 2023 at 8:07 AM Daniel Wagner <dwagner@suse.de> wrote:
>
> Hi James,
>
> On Sat, Jul 01, 2023 at 05:11:11AM -0700, James Smart wrote:
> > As much as you want to make this change to make transports "similar", I am
> > dead set against it unless you are completing a long qualification of the
> > change on real FC hardware and FC-NVME devices. There is probably 1.5 yrs of
> > testing of different race conditions that drove this change. You cannot
> > declare success from a simplistic toy tool such as fcloop for validation.
> >
> > The original issues exist, probably have even morphed given the time from
> > the original change, and this will seriously disrupt the transport and any
> > downstream releases.  So I have a very strong NACK on this change.
> >
> > Yes - things such as the connect failure results are difficult to return
> > back to nvme-cli. I have had many gripes about the nvme-cli's behavior over
> > the years, especially on negative cases due to race conditions which
> > required retries. It still fails this miserably.  The async reconnect path
> > solved many of these issues for fc.
> >
> > For the auth failure, how do we deal with things if auth fails over time as
> > reconnects fail due to a credential changes ?  I would think commonality of
> > this behavior drives part of the choice.
>
> Alright, what do you think about the idea to introduce a new '--sync' option to
> nvme-cli which forwards this info to the kernel that we want to wait for the
> initial connect to succeed or fail? Obviously, this needs to handle signals too.
>
> From what I understood this is also what Ewan would like to have.
>
> Hannes thought it would make sense to use the same initial connect logic in
> tcp/rdma, because there could also be transient erros (e.g. spanning tree
> protocol). In short making the tcp/rdma do the same thing as fc?
>
> So let's drop the final patch from this series for the time. Could you give some
> feedback on the rest of the patches?
>
> Thanks,
> Daniel
>