cygrunsrv + sshd + rsync = 20 times too slow -- throttled?

[Ken Brown]

On 9/3/2021 8:22 AM, Takashi Yano wrote:
> On Fri, 3 Sep 2021 13:41:45 +0200
> Corinna Vinschen wrote:
>> On Sep  3 13:31, Corinna Vinschen wrote:
>>> On Sep  3 19:13, Takashi Yano wrote:
>>>> On Fri, 3 Sep 2021 19:00:46 +0900
>>>> Takashi Yano wrote:
>>>>> On Thu, 2 Sep 2021 21:35:21 +0200
>>>>> Corinna Vinschen wrote:
>>>>>> On Sep  2 21:00, Corinna Vinschen wrote:
>>>>>> It's getting too late again.  I drop off for tonight, but I attached
>>>>>> my POC code I have so far.  It also adds the snippets from my previous
>>>>>> patch which fixes stuff Takashi found during testing.  It also fixes
>>>>>> something which looks like a bug in raw_write:
>>>>>>
>>>>>> -	  ptr = ((char *) ptr) + chunk;
>>>>>> +	  ptr = ((char *) ptr) + nbytes_now;
>>>>>>
>>>>>> Incrementing ptr by chunk bytes while only nbytes_now have been written
>>>>>> looks incorrect.
>>>>>>
>>>>>> As for the reader, it makes the # of bytes to read dependent on the
>>>>>> number of reader handles.  I don't know if that's such a bright idea,
>>>>>> but this can be changed easily.
>>>>>>
>>>>>> Anyway, this runs all my testcases successfully but they are anything
>>>>>> but thorough.
>>>>>>
>>>>>> Patch relativ to topic/pipe attached.  Would you both mind to take a
>>>>>> scrutinizing look?
>>>>>
>>>>> Thanks.
>>>>>
>>>>> Your code seems that read() returns only the partial data even
>>>>> if the pipe stil has more data. Is this by design?
>>>>>
>>>>> This happes in both blocking and non-blocking case.
>>>>
>>>> The patch attached seems to fix the issue.
>>>
>>> I'm sorry, but I don't see what your patch is supposed to do in the
>>> first place.  What I see is that it now calls NtQueryInformationFile
>>> even in the non-blocking case, which is not supposed to happen.
>>>
>>> I'm a bit puzzled what the actual bug is.
>>>
>>> The code changing len is only called if there's no data in the pipe.
>>> In that case we only request a partial buffer so as not to block
>>> the writer on select.
>>>
>>> If there *is* data in the pipe, it will just go straight to the
>>> NtReadFile code without changing len.
>>>
>>> Where's the mistake?
>>
>> Oh, wait.  Do you mean, if we only request less than len bytes, but
>> after NtReadFile there's still data in the buffer, we should try to
>> deplete the buffer up to len bytes in a subsequent NtReadFile?
> 
> Yes. I am sorry, my intent was not clear because I did more than
> necessary in the previous patch. Please see attached patch revised.
> 
>> I thought this is unnecessary, actually, because of POSIX:
>>
>>     The standard developers considered adding atomicity requirements  to  a
>>     pipe  or FIFO, but recognized that due to the nature of pipes and FIFOs
>>     there could be no guarantee of atomicity of reads of {PIPE_BUF} or  any
>>     other size that would be an aid to applications portability.
> 
> POSIX says:
>      The value returned may be less than nbyte if the number of bytes left
>      in the file is less than nbyte, if the read() request was interrupted
>      by a signal, or if the file is a pipe or FIFO or special file and has
>                                                                        ~~~
>      fewer than nbyte bytes immediately available for reading.
>      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> https://pubs.opengroup.org/onlinepubs/009604599/functions/read.html
> 
> If it is turned over, read() should read all data immediately available,
> I think.

I understand the reasoning now, but I think your patch isn't quite right.  As it 
stands, if the call to NtQueryInformationFile fails but total_length != 0, 
you're trying to read again without knowing that there's data in the pipe.

Also, I think you need the following:

diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
index ef7823ae5..46bb96961 100644
--- a/winsup/cygwin/fhandler_pipe.cc
+++ b/winsup/cygwin/fhandler_pipe.cc
@@ -348,8 +348,13 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
      CloseHandle (evt);
    if (status == STATUS_THREAD_SIGNALED)
      {
-      set_errno (EINTR);
-      len = (size_t) -1;
+      if (total_len == 0)
+       {
+         set_errno (EINTR);
+         len = (size_t) -1;
+       }
+      else
+       len = total_len;
      }
    else if (status == STATUS_THREAD_CANCELED)
      pthread::static_cancel_self ();

Ken