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

[Corinna Vinschen]

Hi Takashi,

On Sep  2 17:15, Takashi Yano wrote:
> On Wed, 1 Sep 2021 10:46:24 +0200
> Corinna Vinschen wrote:
> > On Sep  1 17:23, Takashi Yano wrote:
> > > One idea is:
> > > 
> > > Count read handle and write handle opned using NtQueryObject().
> > > If the numbers of opened handle are equal each other, only
> > > the write side (pair of write handle and query_hdl) is alive.
> > > In this case, write() returns error.
> > > If read side is alive, number of read handles is greater than
> > > number of write handles. 
> > 
> > Interesting idea.  But where do you do the count?  The event object
> > will not get signalled, so WFMO will not return when performing a
> > blocking write.
> 
> I imagined something like attached patch.
> 
> Unfortunately, the attached patch seems to have bug that
> occasionally causes the following error while building
> cygwin1.dll.
> 
> <command-line>: error: "GCC_VERSION" redefined [-Werror]
> <command-line>: note: this is the location of the previous definition
> cc1plus: all warnings being treated as errors
> make[1]: *** [Makefile:1942: fhandler_proc.o] Error 1

> diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
> index 1f0f28077..38390848f 100644
> --- a/winsup/cygwin/fhandler.h
> +++ b/winsup/cygwin/fhandler.h
> @@ -1172,6 +1172,7 @@ class fhandler_pipe: public fhandler_base
>  {
>  private:
>    HANDLE query_hdl;
> +  HANDLE reader_evt;
>    pid_t popen_pid;
>    size_t max_atomic_write;
>    void set_pipe_non_blocking (bool nonblocking);
> @@ -1181,6 +1182,7 @@ public:
>    bool ispipe() const { return true; }
>  
>    HANDLE get_query_handle () const { return query_hdl; }
> +  bool reader_closed ();
>  
>    void set_popen_pid (pid_t pid) {popen_pid = pid;}
>    pid_t get_popen_pid () const {return popen_pid;}
> diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
> index 2d9e87bb3..4773d04da 100644
> --- a/winsup/cygwin/fhandler_pipe.cc
> +++ b/winsup/cygwin/fhandler_pipe.cc
> @@ -51,6 +51,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 (get_device () == FH_PIPEW)
> +    fpi.CompletionMode = FILE_PIPE_COMPLETE_OPERATION;

Ok, so  the write side is always non-blocking...


>    status = NtSetInformationFile (get_handle (), &io, &fpi, sizeof fpi,
>  				 FilePipeInformation);
>    if (!NT_SUCCESS (status))
> @@ -308,6 +310,22 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
>      }
>    else if (status == STATUS_THREAD_CANCELED)
>      pthread::static_cancel_self ();
> +
> +  if ((ssize_t)len > 0)
> +    SetEvent (reader_evt);
> +}

...and every successful read sets the event object to signalled.

Sounds good.

> +    }
> +
>    if (len <= max_atomic_write)
>      chunk = len;
>    else if (is_nonblocking ())
> @@ -400,10 +425,24 @@ fhandler_pipe::raw_write (const void *ptr, size_t len)
>  	  /* NtWriteFile returns success with # of bytes written == 0
>  	     if writing on a non-blocking pipe fails because the pipe
>  	     buffer doesn't have sufficient space. */
> -	  if (nbytes_now == 0 && nbytes == 0)
> +	  if (nbytes_now == 0 && nbytes == 0 && is_nonblocking ())
>  	    set_errno (EAGAIN);
> -	  ptr = ((char *) ptr) + chunk;
> +	  ptr = ((char *) ptr) + nbytes_now;
>  	  nbytes += nbytes_now;
> +	  if (reader_closed () && nbytes == 0)
> +	    {
> +	      set_errno(EPIPE);
> +	      raise(SIGPIPE);
> +	    }
> +	  if (!is_nonblocking () && nbytes < len)
> +	    {
> +	      if (nbytes_now == 0)
> +		{
> +		  cygwait (reader_evt);
> +		  ResetEvent (reader_evt);

But what if a read calls SetEvent between cygwait and ResetEvent?
This looks like a potential deadlock issue, no?

> +	    }
>  	}
>        else if (STATUS_PIPE_IS_CLOSED (status))
>  	{
> @@ -430,9 +469,14 @@ fhandler_pipe::raw_write (const void *ptr, size_t len)
>  void
>  fhandler_pipe::fixup_after_fork (HANDLE parent)
>  {
> +  if (close_on_exec() && query_hdl)
> +    CloseHandle (query_hdl);

Why do you close the handle here?  It gets already created with
inheritence settings according to the O_CLOEXEC flag.

>    if (query_hdl)

This is broken.  If you close query_hdl above, it's still != NULL
and fork_fixup will be called.

>      fork_fixup (parent, query_hdl, "query_hdl");
>    fhandler_base::fixup_after_fork (parent);
> +  if (!close_on_exec ())
> +    DuplicateHandle (parent, reader_evt, GetCurrentProcess (), &reader_evt,
> +		     0, 1, DUPLICATE_SAME_ACCESS);

Uhm... this is fixup_after_fork.  I'm a bit puzzled.  You create the
event object with inheritence set to TRUE unconditionally.  So the
forked process will have this handle anyway.  If you duplicate the
handle here, you'll have a handle leak.  What about creating and
duplicating with inheritance == !(flags & O_CLOEXEC) and just call
fork_fixup?

> @@ -463,7 +510,11 @@ fhandler_pipe::close ()
>  {
>    if (query_hdl)
>      NtClose (query_hdl);
> -  return fhandler_base::close ();
> +  int ret = fhandler_base::close ();
> +  if (get_device () == FH_PIPER)
> +    SetEvent (reader_evt);
> +  CloseHandle (reader_evt);
> +  return ret;
>  }

What do you do if the reader process gets killed or crashes?  I fear
this solution has the same problem as a solution using a self-implemented
counter.


Corinna