Re: [PATCH 06/18] nvme-tcp: call 'queue->data_ready()' in nvme_tcp_data_ready()

[Hannes Reinecke <hare@suse.de>]

On 3/22/23 01:18, Chris Leech wrote:
> On Tue, Mar 21, 2023 at 03:09:06PM +0100, Hannes Reinecke wrote:
>> On 3/21/23 14:44, Sagi Grimberg wrote:
>>>
>>>> Call the original data_ready() callback in nvme_tcp_data_ready()
>>>> to avoid a receive stall.
>>>
>>> Can you please improve the description to include what is the stall?
>>> For example, does the stall exist today? If it is, I would like to
>>> separate such patches from this set and include them asap.
>>>
>> That is actually particular to the TLS implementation, as it uses the
>> 'data_ready' callback to produce the data which can be read by eg recvmsg().
>>
>> Without this call there's no data to peruse for recvmsg().
>>
>> But I'm not _that_ deep into networking details to know whether this is TLS
>> specific or an issue with any data_ready callback.
>> I assume the latter, but then again, who knows.
>>
>> Hence the slightly vague description.
> 
> This looks like the socket callbacks end up hooked in the wrong order.
> Ideally it would be tcp -> tls -> nvme_tcp, while this currently looks
> like tcp -> nvme_tcp and then this call back to tls for decryption.
> 
Well, problem is that I need not one but two sets the callbacks.
One callback is for waking up userspace (took me weeks to figure that 
out), and needs to added before calling the userspace helper.
The other is the 'normal' nvme-tcp callback:

tcp->nvme-upcall->tls->nvme-tcp

So really the problem is not so much an inversion, but rather the fact
that the nvme-upcall callback is really only needed for the duration
of the handshake. And hence I thought that we should remove the callback
once we're done with the upcall.
Turns out that we can't, and the best we can do is to disable the 
functionality, leaving the callback itself in place.

> I'm not quite sure how to untangle this; nvme_tcp can't just set it's
> own callbacks before initializing kTLS, becuse that's being done by
> tlshd which is going to need the userspace socket API callbacks working.
> 
Correct.
So for now I'll leave the callbacks in place, even though they are 
pointless after the upcall.

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