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

Ken Brown kbrown@cornell.edu
Sun Sep 5 20:27:37 GMT 2021


On 9/5/2021 4:09 PM, Takashi Yano wrote:
> Hi Ken,
> 
> On Sun, 5 Sep 2021 14:47:26 -0400
> Ken Brown wrote:
>> 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;
> 
> Could you please try attached new patch?

LGTM.  And it applies cleanly.  Maybe I did something wrong when I thought it 
didn't apply.

Thanks.

Ken


More information about the Cygwin-developers mailing list