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

[Ken Brown <kbrown@cornell.edu>]

Hi Takashi,

On 9/5/2021 9:50 AM, Takashi Yano wrote:
> On Sun, 5 Sep 2021 22:40:59 +0900
> Takashi Yano wrote:
>> On Sun, 5 Sep 2021 08:15:23 +0900
>> Takashi Yano wrote:
>>> Hi Ken,
>>>
>>> On Sat, 4 Sep 2021 10:04:12 -0400
>>> Ken Brown wrote:
>>>> On 9/4/2021 8:37 AM, Takashi Yano wrote:
>>>>> On Sat, 4 Sep 2021 21:02:58 +0900
>>>>> Takashi Yano wrote:
>>>>>> Hi Corinna, Ken,
>>>>>>
>>>>>> On Fri, 3 Sep 2021 09:27:37 -0400
>>>>>> Ken Brown wrote:
>>>>>>> On 9/3/2021 8:22 AM, Takashi Yano wrote:
>>>>>>>> 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 ();
>>>>>>
>>>>>> Thanks for your advice. I fixed the issue and attached new patch.
>>>>>>
>>>>>> On Fri, 3 Sep 2021 17:37:13 +0200
>>>>>> Corinna Vinschen wrote:
>>>>>>> Hmm, I see the point, but we might have another problem with that.
>>>>>>>
>>>>>>> We can't keep the mutex while waiting on the pending read, and there
>>>>>>> could be more than one pending read running at the time.  if so,
>>>>>>> chances are extremly high, that the data written to the buffer gets
>>>>>>> split like this:
>>>>>>>
>>>>>>>      reader 1		               reader 2
>>>>>>>
>>>>>>>      calls read(65536)                   calls read(65536)
>>>>>>>
>>>>>>>      calls NtReadFile(16384 bytes)
>>>>>>>                                          calls NtReadFile(16384 bytes)
>>>>>>>
>>>>>>> writer writes 65536 bytes
>>>>>>>
>>>>>>>      wakes up and gets 16384 bytes
>>>>>>>                                          wakes up and gets 16384 bytes
>>>>>>>      gets the mutex, calls
>>>>>>>      NtReadFile(32768) which
>>>>>>>      returns immediately with
>>>>>>>      32768 bytes added to the
>>>>>>>      caller's buffer.
>>>>>>>
>>>>>>> so the buffer returned to reader 1 is 49152 bytes, with 16384 bytes
>>>>>>> missing in the middle of it, *without* the reader knowing about that
>>>>>>> fact.  If reader 1 gets the first 16384 bytes, the 16384 bytes have
>>>>>>> been read in a single call, at least, so the byte order is not
>>>>>>> unknowingly broken on the application level.
>>>>>>>
>>>>>>> Does that make sense?
>>>>>>
>>>>>> Why can't we keep the mutex while waiting on the pending read?
>>>>>> If we can keep the mutex, the issue above mentioned does not
>>>>>> happen, right?
>>>>>>
>>>>>> What about the patch attached? This keeps the mutex while read()
>>>>>> but I do not see any defects so far.
>>>>
>>>> LGTM.
>>>>
>>>> If Corinna agrees, I have a couple of suggestions.
>>>>
>>>> 1. With this patch, we can no longer have more than one pending ReadFile.  So
>>>> there's no longer a need to count read handles, and the problem with select is
>>>> completely fixed as long as the number of bytes requested is less than the pipe
>>>> buffer size.
>>>>
>>>> 2. raw_read is now reading in chunks, like raw_write.  For readability of the
>>>> code, I think it would be better to make the two functions as similar as
>>>> possible.  For example, you could replace the do/while loop by a
>>>> while(total_len<orig_len) loop.  And you could even use similar names for the
>>>> variables, e.g., nbytes instead of total_len, or vice versa.
>>>
>>> Thanks for the suggestion. I have rebuilt the patch.
>>> Please see the patch attached.
>>
>> This patch seems to fail to adopt to current git head of topic/pipe
>> branch. I rebuilt the patch to fit current top/pipe.
>>
>> Please see the patch attached.
> 
> Small typo.
> 
> -	     pipe buffer size.  Every pending read lowers WriteQuotaAvailable
> +	     pipe buffer size. pending read lowers WriteQuotaAvailable
> 
> should be:
> 
> -	     pipe buffer size.  Every pending read lowers WriteQuotaAvailable
> +	     pipe buffer size. Pending read lowers WriteQuotaAvailable

The patch looks great to me.  Two minor nits:

1. The patch doesn't apply cleanly.  Could you rebase it against the current 
HEAD of topic/pipe?

2. There's no need for chunk to be less than the number of bytes requested if we 
know there's data in the pipe.  So maybe something like this (untested) would be 
better:

           ULONG chunk;
           status = NtQueryInformationFile (get_handle (), &io,
                                            &fpli, sizeof (fpli),
                                            FilePipeLocalInformation);
           if (NT_SUCCESS (status))
             {
               if (fpli.ReadDataAvailable > 0)
                 chunk = left;
               else if (nbytes != 0)
                 break;
               else
                 chunk = fpli.InboundQuota / 2;
             }
           else if (nbytes != 0)
             break;
           else
             chunk = max_atomic_write / 2;

           if (chunk < left)
             len1 = chunk;

Ken