Re: [PATCH 08/18] nvme-tcp: do not set MSG_SENDPAGE_NOTLAST

Hannes Reinecke
2023-04-17T15:35:48Z

On 4/17/23 17:28, Sagi Grimberg wrote:
>
>>> nak from me.
>>>
>>>> MSG_SENDPAGE_NOTLAST was introduced with commit 35f9c09fe9c7
>>>> ("tcp: tcp_sendpages() should call tcp_push() once") to fix issues
>>>> with the ->sendfile() call, and the implication seems to be that
>>>> the flag should be strictly internal to sendfile.
>>>
>>> no, the use case was to allow sendpage consumers to build larger
>>> tso packets for large payloads (splice being the primary one), nvme-tcp
>>> also benefits from it (as do others), just like sendfile.
>>>
>>>> Hence we shouldn't be setting it on nvme-tcp, and anyway doing
>>>> so will cause TLS sending invalid data.
>>>
>>> That is something else. I don't recall the full details on the
>>> discussion with Jakub, but it was not my intention to strike
>>> it altogether, just remove it if !sendpage_ok.
>>>
>> But then we're running afoul with tls_sw_do_sendpage(), which uses
>> MSG_SENDPAGE_NOTLAST as a record indicator. Forwarding
>> MSG_SENDPAGE_NOTLAST to tls results in incorrect TLS records on the
>> wire and a transmission stall.
>>
>> Took me days to figure that out.
>>
>> If you have a better idea, I'm all ears.
>
> So sendfile is unsupported over kTLS? Or is there some special
> case for it in the upper layer?
>
Dunno. Seems to work, at least according to commit d452d48b9f8b
("tls: prevent oversized sendfile() hangs by ignoring MSG_MORE")

Which is also where I got the notion about sendfile() from.
If MSG_SENDPAGE_NOTLAST is valid for non-sendfile users then
tls_sw_do_sendpage() has a bug.

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