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

[Takashi Yano <takashi.yano@nifty.ne.jp>]

On Mon, 13 Sep 2021 20:32:33 +0200
Corinna Vinschen wrote:

> On Sep 12 20:04, 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.
> > [...]
> 
> What I miss is a bit more detailed commit message...

I am sorry, I will add more detail commit message.

> > diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
> > index 9b4255cfd..b051f5c03 100644
> > --- a/winsup/cygwin/fhandler_pipe.cc
> > +++ b/winsup/cygwin/fhandler_pipe.cc
> > @@ -56,6 +56,8 @@ fhandler_pipe::set_pipe_non_blocking (bool nonblocking)
> >    fpi.ReadMode = FILE_PIPE_BYTE_STREAM_MODE;
> >    fpi.CompletionMode = nonblocking ? FILE_PIPE_COMPLETE_OPERATION
> >      : FILE_PIPE_QUEUE_OPERATION;
> > +  if (query_hdl)
> > +    fpi.CompletionMode = FILE_PIPE_COMPLETE_OPERATION;
> 
> This should be a single expression, i.e.
> 
>    fpi.CompletionMode = nonblocking || query_hdl
>                         ? FILE_PIPE_COMPLETE_OPERATION
>                         : FILE_PIPE_QUEUE_OPERATION;
> 
> ideally combined with a comment.

Thanks. I'll do that.

> But then again... you're basically switching the write side of
> a pipe to nonblocking mode unconditionally.  The downside is a
> busy wait:
> 
> >  fhandler_pipe_fifo::raw_write (const void *ptr, size_t len)
> >  {
> > @@ -493,7 +490,20 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t len)
> >  			      get_obj_handle_count (select_sem), NULL);
> >  	  /* 0 bytes returned?  EAGAIN.  See above. */
> >  	  if (NT_SUCCESS (status) && nbytes == 0)
> > -	    set_errno (EAGAIN);
> > +	    {
> > +	      if (reader_closed ())
> > +		{
> > +		  set_errno (EPIPE);
> > +		  raise (SIGPIPE);
> > +		}
> > +	      else if (is_nonblocking ())
> > +		set_errno (EAGAIN);
> > +	      else
> > +		{
> > +		  cygwait (select_sem, 10);
> > +		  continue;
> 
> I'm a bit puzzled.  The cygwait branch neglects to check if select_sem
> is NULL (the preceeding ReleaseSemaphore expression does!)
> And then it doesn't matter if the caller got blocked or not, it will
> always perform a continue.  So why do it at all?  Worse, if this
> expression loops, it will eat up the semaphore, because each call will
> decrement the semaphore count until it blocks.  That sounds wrong to me.

It is by design. ReleaseSemaphore() releases maximum number of semaphore
which the waiter can exists. If only one writer and one reader exist,
ReleaseSemaphore releases 2 semaphores. Then cygwait here consume semaphore
two times and return to wait state.
This wait state is released by raw_read() or close().
 
> Btw., while looking into the current pipe code, I wonder what select_sem
> is doing in the pipe code at all so far.  It gets released, but it never
> gets waited on?!?  Am I missing something?

The semaphore is waited in select.cc.
But, wait. Wat happens if select() is not called? Released semaphore
can be accumulated up to INT32_MAX!!?

Let me consider.

> >  	{
> > @@ -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");
> 
> I don't understand why calling fork_fixup on query_hdl should depend
> on the handle count.  If you duplicate a writer, you always have to
> duplicate query_hdl as well to keep the count, no?  Inheritence is
> handled by the O_CLOEXEC flag and fork_fixup will do the right thing.

I thought so, however, counting query_hdl cannot work as expected
without this check. The number of query_hdl opend seems to exceed
the number of writer.

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

> 
> > +      if (!DuplicateHandle (GetCurrentProcess (), r,
> > +			    GetCurrentProcess (), &fhs[1]->query_hdl,
> > +			    GENERIC_READ, !(mode & O_CLOEXEC), 0))
> 
> This is a bug I introduced accidentally during testing.  This
> GENERIC_READ is actually supposed to be a FILE_READ_ATTRIBUTES.
> Sorry about that.

The PoC code uses PeekNamedPipe for query_hdl, so GENERIC_READ is
necessary I think.

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