Re: [PATCH] nvme/tcp: handle tls partially sent records in write_space()

[Hannes Reinecke <hare@suse.de>]

On 10/8/25 04:11, Wilfred Mallawa wrote:
> On Tue, 2025-10-07 at 11:51 +0200, Hannes Reinecke wrote:
>> On 10/7/25 11:24, Wilfred Mallawa wrote:
>>> On Tue, 2025-10-07 at 07:19 +0200, Hannes Reinecke wrote:
>>>> On 10/7/25 02:46, Wilfred Mallawa wrote:
>>>>> From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
>>>>>
>>>>
>>> [...]
>>>> I wonder: Do we really need to check for a partially assembled
>>>> record,
>>>> or wouldn't it be easier to call queue->write_space() every time
>>>> here?
>>>> We sure would end up with executing the callback more often, but
>>>> if
>>>> no
>>>> data is present it shouldn't do any harm.
>>>>
>>>> IE just use
>>>>
>>>> if (nvme_tcp_queue_tls(queue)
>>>>        queue->write_space(sk);
>>>
>>> Hey Hannes,
>>>
>>> This was my initial approach, but I figured using
>>> tls_is_partially_sent_record() might be slightly more efficient.
>>> But if
>>> we think that's negligible, happy to go with this approach
>>> (omitting
>>> the partial record check).
>>>
>> Please do.
>> Performance testing on NVMe-TCP is notoriously tricky, so for now we
>> really should not assume anything here.
>> And it's making the patch _vastly_ simpler, _and_ we don't have to
>> involve the networking folks here.
> 
> Okay, will send a V2 with this approach.
> 
>> We have a similar patch for the data_ready() function in nvmet_tcp(),
>> and that seemed to work, too.
>> Nit: we don't unset the 'NOSPACE' flag there. Can you check if that's
>> really required?
>> And, if it is, fixup nvmet_tcp() to unset it?
>> Or, if not, modify your patch to not clear it?
> 
> I don't see why we would need to clear the NOSPACE flag in
> data_ready()? My understanding is that this flag is used when the send
> buffer is full.
> 
> I would think the clear_bit() is necessary in write_space() since it
> would typically get done in something like sk_stream_write_space()?
> However, running some quick FIOs with the clear_bit() removed, things
> seem to work. Not sure if removing it has any further implications
> though...
> 
I am not sure, either. Code analysis suggests that we don't need to
do that, but then we're the first ever to explore that area.
So I would think we don't need to worry (as nvmet-tcp doesn't do that,
either). Sounds like a question for LPC.
So let's drop the 'NOSPACE' flag handling to get the
partial records fixed, and address the NOSPACE issue separately.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich