Re: [PATCH 2/3] nvme-tcp: Check for write space before queueing requests

[Hannes Reinecke <hare@suse.de>]

On 5/20/22 11:17, Sagi Grimberg wrote:
> 
>> The current model of always queue incoming requests lead to
>> write stalls as we easily overload the network device under
>> high I/O load.
>> To avoid unlimited queueing we should rather check if write
>> space is available before accepting new requests.
> 
> I'm somewhat on the fence with this one... On one end, we
> are checking the sock write space, but don't check the queued
> requests. And, this is purely advisory and not really a check
> we rely on.
> 
> The merit of doing something like this is that we don't start
> the request timer, but we can just as easily queue the request
> and have it later queued for long due to sock being overloaded.
> 
> Can you explain your thoughts to why this is a good solution?
> 
Request timeouts.
As soon as we call 'blk_mq_start_request()' the I/O timer is called, and 
given that we (currently) queue _every_ request irrespective of the 
underlying device status we might end up queueing for a _loooong_ time.

Timeouts while still in the queue are being handled by the first patch, 
but the underlying network might also be busy with retries and whatnot.
So again, queuing requests when we _know_ there'll be a congestion is 
just asking for trouble (or, rather, spurious I/O timeouts).

If one is worried about performance one can always increase the wmem 
size :-), but really it means that either your testcase or your network 
is misdesigned.
And I'm perfectly fine with increasing the latency in these cases.
What I don't like is timeouts, as these will show up to the user and we 
get all the supportcalls telling us that the kernel is broken.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer