Re: [PATCH v5 3/6] nvme-tcp: short-circuit reconnect retries

[Daniel Wagner]

On Tue, Apr 09, 2024 at 11:40:03PM +0300, Sagi Grimberg wrote:
> > +/*
> > + * Evaluate the status information returned by the LLDD
> 
> We usually say transport, less so LLDD.

Updated the documentation accordingly.

> > + * 1) The LLDD reports an negative status. There was an error (e.g. no
> > + *    memory) on the host side and thus abort the operation.
> > + *    Note, there are exception such as ENOTCONN which is
> > + *    not an internal driver error, thus we filter these errors
> > + *    out and retry later.
>
> What is this ENOTCONN here? From what I see its just a random
> escape value to bypass the status check. I think that 0 would do the same
> wouldn't it?

ENOTCONN is issued by the reset handler. Sure, 0 will work as well, so
if the consense is that the reset handler should do that. see below.

> > -	if (nvmf_should_reconnect(ctrl)) {
> > +	if (recon && nvmf_should_reconnect(ctrl)) {
> 
> its weird to have a condition that checks _and_ condition
> of two different "should reconnect" conditions.
> 
> Why don't we simply pass the status to nvmf_should_reconnect()
> and evaluate there?

Sure, makes sense.

> >   static void nvme_tcp_error_recovery_work(struct work_struct *work)
> > @@ -2295,7 +2300,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
> >   		return;
> >   	}
> > -	nvme_tcp_reconnect_or_remove(ctrl);
> > +	nvme_tcp_reconnect_or_remove(ctrl, -ENOTCONN);

Here is the source of ENOTCONN.