Messages in this thread | | | Date | Tue, 27 Jun 2023 08:51:49 +0200 | Subject | Re: [PATCH v2 4/5] nvme-fc: Make initial connect attempt synchronous | From | Hannes Reinecke <> |
| |
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 -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew Myers, Andrew McDonald, Martje Boudien Moerman
| |