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

[Ken Brown]

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.

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