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

[Corinna Vinschen <corinna-cygwin@cygwin.com>]

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

> 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; }
> +  bool reader_closed ();
> +
>    ssize_t __reg3 raw_write (const void *ptr, size_t len);
>  
>  };
> 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.

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.

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?

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

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


Corinna