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

[Hannes Reinecke <hare@suse.de>]

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