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

[Takashi Yano]

On Mon, 13 Sep 2021 13:42:46 -0400
Ken Brown wrote:
> > 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?

Right. I will fix it.

> > @@ -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?

I also thought so, however, counting query_hdl did not work
as expected without this check. I am not sure why...

There seems to be the case that handle is already inherited
here without fork_fixup.

> > @@ -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.

I have confirmed that behaviour. You are right. Using InboundQuota
is right thing. I'll fix it. Thanks.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>