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

[Ken Brown <kbrown@cornell.edu>]

On 8/29/2021 10:15 PM, Ken Brown via Cygwin wrote:
> On 8/29/2021 8:22 PM, Takashi Yano via Cygwin wrote:
>> On Mon, 30 Aug 2021 09:13:14 +0900
>> Takashi Yano wrote:
>>> On Sun, 29 Aug 2021 17:04:56 -0400
>>> Ken Brown wrote:
>>>> On 8/29/2021 5:07 AM, Takashi Yano via Cygwin wrote:
>>>>> On Sat, 28 Aug 2021 18:41:02 +0900
>>>>> Takashi Yano wrote:
>>>>>> On Sat, 28 Aug 2021 10:43:27 +0200
>>>>>> Corinna Vinschen wrote:
>>>>>>> On Aug 28 02:21, Takashi Yano via Cygwin wrote:
>>>>>>>> On Fri, 27 Aug 2021 12:00:50 -0400
>>>>>>>> Ken Brown wrote:
>>>>>>>>> Two years ago I thought I needed nt_create to avoid problems when calling
>>>>>>>>> set_pipe_non_blocking.  Are you saying that's not an issue?  Is
>>>>>>>>> set_pipe_non_blocking unnecessary?  Is that the point of your 
>>>>>>>>> modification to
>>>>>>>>> raw_read?
>>>>>>>>
>>>>>>>> Yes. Instead of making windows read function itself non-blocking,
>>>>>>>> it is possible to check if the pipe can be read before read using
>>>>>>>> PeekNamedPipe(). If the pipe cannot be read right now, EAGAIN is
>>>>>>>> returned.
>>>>>>>
>>>>>>> The problem is this:
>>>>>>>
>>>>>>>     if (PeekNamedPipe())
>>>>>>>       ReadFile(blocking);
>>>>>>>
>>>>>>> is not atomic.  I. e., if PeekNamedPipe succeeds, nothing keeps another
>>>>>>> thread from draining the pipe between the PeekNamedPipe and the ReadFile
>>>>>>> call.  And as soon as ReadFile runs, it hangs indefinitely and we can't
>>>>>>> stop it via a signal.
>>>>>>
>>>>>> Hmm, you are right. Mutex guard seems to be necessary like pty code
>>>>>> if we go this way.
>>>>>
>>>>> I have found that set_pipe_non_blocking() succeeds for both read and
>>>>> write pipes if the write pipe is created by CreateNamedPipe() and the
>>>>> read pipe is created by CreateFile() contrary to the current create()
>>>>> code. Therefore, not only nt_create() but also PeekNamedPipe() become
>>>>> unnecessary.
>>>>>
>>>>> Please see the revised patch attached.
>>>>
>>>> I haven't had a chance to test this myself yet, but occurs to me that we might
>>>> have a different problem after this patch: Does the write handle that we get
>>>> from CreateNamedPipe() have FILE_READ_ATTRIBUTES access?
>>>
>>> I have just checked this, and the answer is "No". Due to this problem,
>>> NtQueryInformationFile() call in select() fails on the write pipe.
>>>
>>> It seems that we need more consideration...
>>
>> We have two easy options:
>> 1) Configure the pipe with PIPE_ACCESS_DUPLEX.
>> 2) Use nt_create() again and forget C# program issue.
> 
> I vote for 2), but let's see what Corinna thinks.
> 
>> Even without this problem, select() for writing pipe has a bug
>> and does not wrok as expected. The following patch seems to be
>> needed.
>>
>> diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
>> index 83e1c00e0..ac2fd227e 100644
>> --- a/winsup/cygwin/select.cc
>> +++ b/winsup/cygwin/select.cc
>> @@ -612,7 +612,6 @@ pipe_data_available (int fd, fhandler_base *fh, HANDLE h, 
>> bool writing)
>>             that.  This means that a pipe could still block since you could
>>             be trying to write more to the pipe than is available in the
>>             buffer but that is the hazard of select().  */
>> -      fpli.WriteQuotaAvailable = fpli.OutboundQuota - fpli.ReadDataAvailable;
>>         if (fpli.WriteQuotaAvailable > 0)
>>          {
>>            paranoid_printf ("fd %d, %s, write: size %u, avail %u", fd,
>>
> 
> I agree.

Now I'm starting to wonder.  The use of fpli.OutboundQuota - 
fpli.ReadDataAvailable instead of fpli.WriteQuotaAvailable was introduced in 
commit a010e6abe, with no comment in the commit message or the code to explain 
why.  Corinna, does this make sense to you?  Is it related to the issues raised 
in the message

  https://cygwin.com/pipermail/cygwin-patches/2004q4/005002.html

that you cited elsewhere in this thread?

BTW, when I was working on the pipe approach to AF_UNIX sockets (topic/af_unix 
branch), I had occasion to step through select.cc:pipe_data_available in gdb, 
and the use of fpli.OutboundQuota - fpli.ReadDataAvailable definitely seemed 
wrong to me.  So when I wrote peek_socket_unix on that branch, I used 
fpli.WriteQuotaAvailable, as Takashi is suggesting now.

Ken