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

[Ken Brown]

On 9/20/2021 5:09 PM, Ken Brown wrote:
> On 9/20/2021 3:14 PM, Ken Brown wrote:
>> Hi Takashi,
>>
>> On 9/20/2021 8:52 AM, Takashi Yano wrote:
>>> On Mon, 13 Sep 2021 11:07:42 +0200
>>> Corinna Vinschen wrote:
>>>> On Sep 11 11:35, Takashi Yano wrote:
>>>>> Keeping read handle in write pipe (Corinna's query_hdl) causes problem
>>>>> that write side cannot detect close on read side.
>>>>> Is it possible to open read handle temporally when pipe_data_available()
>>>>> is called?
>>>>
>>>> 1. You would have to know which process keeps the other side of the pipe.
>>>> 2. You would have to have the permission to open the other process to
>>>>     duplicate the pipe into your own process
>>>> 3. You would have to know the HANDLE value of the read side of your pipe
>>>>     in that other process.
>>>>
>>>> Point 1 is (kind of) doable using GetNamedPipeClientProcessId or
>>>> GetNamedPipeServerProcessId.  ZIt's not clear how reliable these
>>>> functions are, given that both pipe sides are created by the same
>>>> process and then usually inherited by two child processes communicating
>>>> over that pipe.
>>>>
>>>> Point 2 is most of the time the case, especially when talking with
>>>> native processes.
>>>>
>>>> Point 3 requires some sort of IPC.
>>>>
>>>> Having said that, I think this is too complicated.
>>>
>>> In the current implementation, there is a potential risk that
>>> write side fails to detect the closure of read side if more than
>>> one writers exist and one of them is non-cygwin process.
>>>
>>> Therefore, I had tried to implement the above idea (tentative
>>> query handle). Finally, I could convince myself to some extent,
>>> so I attach a patch for that.
>>>
>>> As for point 1 and 3, I used NtQuerySystemInformation() with
>>> SystemHandleInformation. I also used NtQueryObject() with
>>> ObjectNameInformation in order to know which handle is the
>>> other side of the pipe. As for point 2, it is not possible to
>>> open the process which is running as a service, so the current
>>> query_hdl is used for such process as an exception.
>>>
>>> Ken, WDYT of this implementation?
>>
>> This looks like a great idea!  I'll review it in detail later today or tomorrow.
> 
> I only saw one thing that doesn't look right to me:
> 
>> +      /* Check object name */
>> +      status = NtQueryObject (h, ObjectNameInformation, ntfn, 65536, &len);
>> +      if (!NT_SUCCESS (status))
>> +    goto close_handle;
>> +      ntfn->Name.Buffer[ntfn->Name.Length / sizeof (WCHAR)] = L'\0';
>> +      if (wcscmp (name, ntfn->Name.Buffer) == 0)
>> +    {
>> +      query_hdl_proc = proc;
>> +      query_hdl_value = (HANDLE)(intptr_t) shi->Handles[i].HandleValue;
>> +      qh = h;
>> +      break;
>> +    }
>> +close_handle:
>> +      CloseHandle (h);
>> +close_proc:
>> +      CloseHandle (proc);
> 
> Doesn't this mean that query_hdl_proc is not a valid handle any more?  So the 
> attempt to reduce overhead at the beginning of tentative_query_hdl() will never 
> work.

Never mind.  I see my stupid mistake.

> Other than that, I still think it looks great.  But it's complicated enough that 
> I think Corinna should review it too when she returns.  (It also uses Windows 
> functions that I have no experience with.)  Nevertheless, I'm tempted to push it 
> so that it can get testing, even if Corinna changes it or reverts it later.
> 
> Ken