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

[Ken Brown <kbrown@cornell.edu>]

On 9/12/2021 7:04 AM, Takashi Yano wrote:
> On Sun, 12 Sep 2021 17:48:49 +0900
> Takashi Yano wrote:
>> Hmm. Then, what about PoC code attached? This returns to Corinna's
>> query_hdl, and counts read/write handles to detect closing reader side.
>>
>> If the number of read handles is equal to number of write handles,
>> only the pairs of write handle and query_hdl are alive. So, read pipe
>> supposed to be closed.
>>
>> This patch depends another patch I posted a few hours ago.
> 
> Revised a bit.

A few small comments/questions:

> diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
> index 13fba9a14..f09af2c37 100644
> --- a/winsup/cygwin/fhandler.h
> +++ b/winsup/cygwin/fhandler.h
> @@ -1176,10 +1176,15 @@ class fhandler_pipe_fifo: public fhandler_base
>  {
>   protected:
>    size_t pipe_buf_size;
> +  HANDLE query_hdl;
>  
>   public:
>    fhandler_pipe_fifo ();
>  
> +  HANDLE get_query_handle () const { return query_hdl; }
> +  void close_query_handle () { CloseHandle (query_hdl); query_hdl = NULL; }

Should you use if(query_hdl) here?  Or is it up to the caller to check that?

> @@ -522,6 +532,10 @@ fhandler_pipe::fixup_after_fork (HANDLE parent)
>      fork_fixup (parent, read_mtx, "read_mtx");
>    if (select_sem)
>      fork_fixup (parent, select_sem, "select_sem");
> +  /* Do not duplicate query_hdl if it has been already inherited. */
> +  if (query_hdl && !get_obj_handle_count (query_hdl))
> +    fork_fixup (parent, query_hdl, "query_hdl");


Why do you need to call get_obj_handle_count here?  Shouldn't fork_fixup take 
care of the case where the handle has been inherited?

> @@ -608,12 +608,43 @@ pipe_data_available (int fd, fhandler_base *fh, HANDLE h, bool writing)
>      }
>    if (writing)
>      {
> -      /* WriteQuotaAvailable is decremented by the number of bytes requested
> -	 by a blocking reader on the other side of the pipe.  Cygwin readers
> -	 are serialized and never request a number of bytes equivalent to the
> -	 full buffer size.  So WriteQuotaAvailable is 0 only if either the
> -	 read buffer on the other side is really full, or if we have non-Cygwin
> -	 readers. */
> +      /* If there is anything available in the pipe buffer then signal
> +        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().
> +
> +        Note that WriteQuotaAvailable is unreliable.
> +
> +        Usually WriteQuotaAvailable on the write side reflects the space
> +        available in the inbound buffer on the read side.  However, if a
> +        pipe read is currently pending, WriteQuotaAvailable on the write side
> +        is decremented by the number of bytes the read side is requesting.
> +        So it's possible (even likely) that WriteQuotaAvailable is 0, even
> +        if the inbound buffer on the read side is not full.  This can lead to
> +        a deadlock situation: The reader is waiting for data, but select
> +        on the writer side assumes that no space is available in the read
> +        side inbound buffer.
> +
> +        Consequentially, the only reliable information is available on the
> +        read side, so fetch info from the read side via the pipe-specific
> +        query handle.  Use fpli.WriteQuotaAvailable as storage for the actual
> +        interesting value, which is the OutboundQuote on the write side,

I thought Corinna's experiments showed that InboundQuota and OutboundQuota are 
the same on the read and write sides, and that InboundQuota is the one we should 
be using.

Ken