cygrunsrv + sshd + rsync = 20 times too slow -- throttled?
Ken Brown
kbrown@cornell.edu
Sun Sep 5 18:47:26 GMT 2021
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
More information about the Cygwin-developers
mailing list