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

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

[-- Attachment #1: Type: text/plain, Size: 1555 bytes --]

On Sat, 28 Aug 2021 18:41:02 +0900
Takashi Yano wrote:
> On Sat, 28 Aug 2021 10:43:27 +0200
> Corinna Vinschen wrote:
> > On Aug 28 02:21, Takashi Yano via Cygwin wrote:
> > > On Fri, 27 Aug 2021 12:00:50 -0400
> > > Ken Brown wrote:
> > > > Two years ago I thought I needed nt_create to avoid problems when calling 
> > > > set_pipe_non_blocking.  Are you saying that's not an issue?  Is 
> > > > set_pipe_non_blocking unnecessary?  Is that the point of your modification to 
> > > > raw_read?
> > > 
> > > Yes. Instead of making windows read function itself non-blocking,
> > > it is possible to check if the pipe can be read before read using
> > > PeekNamedPipe(). If the pipe cannot be read right now, EAGAIN is
> > > returned.
> > 
> > The problem is this:
> > 
> >   if (PeekNamedPipe())
> >     ReadFile(blocking);
> > 
> > is not atomic.  I. e., if PeekNamedPipe succeeds, nothing keeps another
> > thread from draining the pipe between the PeekNamedPipe and the ReadFile
> > call.  And as soon as ReadFile runs, it hangs indefinitely and we can't
> > stop it via a signal.
> 
> Hmm, you are right. Mutex guard seems to be necessary like pty code
> if we go this way.

I have found that set_pipe_non_blocking() succeeds for both read and
write pipes if the write pipe is created by CreateNamedPipe() and the
read pipe is created by CreateFile() contrary to the current create()
code. Therefore, not only nt_create() but also PeekNamedPipe() become
unnecessary.

Please see the revised patch attached.

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

[-- Attachment #2: v2-0002-Cygwin-pipe-Revert-to-create-rather-than-nt_creat.patch --]
[-- Type: application/octet-stream, Size: 8881 bytes --]

From 3f4ed10f86c88e1194a5f11201a8e088ce53d127 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Sun, 29 Aug 2021 17:23:57 +0900
Subject: [PATCH v2 2/2] Cygwin: pipe: Revert to create() rather than
 nt_create().

---
 winsup/cygwin/fhandler_pipe.cc | 192 ++++-----------------------------
 1 file changed, 21 insertions(+), 171 deletions(-)

diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
index 2dec0a848..a6ece17a6 100644
--- a/winsup/cygwin/fhandler_pipe.cc
+++ b/winsup/cygwin/fhandler_pipe.cc
@@ -463,13 +463,13 @@ fhandler_pipe::create (LPSECURITY_ATTRIBUTES sa_ptr, PHANDLE r, PHANDLE w,
   if (name)
     len += __small_sprintf (pipename + len, "%s", name);
 
-  open_mode |= PIPE_ACCESS_INBOUND | FILE_FLAG_FIRST_PIPE_INSTANCE;
+  open_mode |= PIPE_ACCESS_OUTBOUND | FILE_FLAG_FIRST_PIPE_INSTANCE;
 
   /* Retry CreateNamedPipe as long as the pipe name is in use.
      Retrying will probably never be necessary, but we want
      to be as robust as possible.  */
   DWORD err = 0;
-  while (r && !*r)
+  while (w && !*w)
     {
       static volatile ULONG pipe_unique_id;
       if (!name)
@@ -498,12 +498,12 @@ fhandler_pipe::create (LPSECURITY_ATTRIBUTES sa_ptr, PHANDLE r, PHANDLE w,
 	 definitely required for pty handling since fhandler_pty_master
 	 writes to the pipe in chunks, terminated by newline when CANON mode
 	 is specified.  */
-      *r = CreateNamedPipe (pipename, open_mode, pipe_mode, 1, psize,
+      *w = CreateNamedPipe (pipename, open_mode, pipe_mode, 1, psize,
 			   psize, NMPWAIT_USE_DEFAULT_WAIT, sa_ptr);
 
-      if (*r != INVALID_HANDLE_VALUE)
+      if (*w != INVALID_HANDLE_VALUE)
 	{
-	  debug_printf ("pipe read handle %p", *r);
+	  debug_printf ("pipe write handle %p", *w);
 	  err = 0;
 	  break;
 	}
@@ -516,14 +516,14 @@ fhandler_pipe::create (LPSECURITY_ATTRIBUTES sa_ptr, PHANDLE r, PHANDLE w,
 	     Pick a new name and retry.  */
 	  debug_printf ("pipe busy", !name ? ", retrying" : "");
 	  if (!name)
-	    *r = NULL;
+	    *w = NULL;
 	  break;
 	case ERROR_ACCESS_DENIED:
 	  /* The pipe is already open with incompatible parameters.
 	     Pick a new name and retry.  */
 	  debug_printf ("pipe access denied%s", !name ? ", retrying" : "");
 	  if (!name)
-	    *r = NULL;
+	    *w = NULL;
 	  break;
 	default:
 	  {
@@ -535,60 +535,42 @@ fhandler_pipe::create (LPSECURITY_ATTRIBUTES sa_ptr, PHANDLE r, PHANDLE w,
 
   if (err)
     {
-      *r = NULL;
+      *w = NULL;
       return err;
     }
 
-  if (!w)
-    debug_printf ("pipe write handle NULL");
+  if (!r)
+    debug_printf ("pipe read handle NULL");
   else
     {
       debug_printf ("CreateFile: name %s", pipename);
 
-      /* Open the named pipe for writing.
-	 Be sure to permit FILE_READ_ATTRIBUTES access.  */
-      DWORD access = GENERIC_WRITE | FILE_READ_ATTRIBUTES;
+      /* Open the named pipe for reading.
+	 Be sure to permit FILE_WRITE_ATTRIBUTES access.  */
+      DWORD access = GENERIC_READ | FILE_WRITE_ATTRIBUTES;
       if ((open_mode & PIPE_ACCESS_DUPLEX) == PIPE_ACCESS_DUPLEX)
-	access |= GENERIC_READ | FILE_WRITE_ATTRIBUTES;
-      *w = CreateFile (pipename, access, 0, sa_ptr, OPEN_EXISTING,
+	access |= GENERIC_WRITE | FILE_READ_ATTRIBUTES;
+      *r = CreateFile (pipename, access, 0, sa_ptr, OPEN_EXISTING,
 		      open_mode & FILE_FLAG_OVERLAPPED, 0);
 
-      if (!*w || *w == INVALID_HANDLE_VALUE)
+      if (!*r || *r == INVALID_HANDLE_VALUE)
 	{
 	  /* Failure. */
 	  DWORD err = GetLastError ();
 	  debug_printf ("CreateFile failed, r %p, %E", r);
-	  if (r)
-	    CloseHandle (*r);
-	  *w = NULL;
+	  if (w)
+	    CloseHandle (*w);
+	  *r = NULL;
 	  return err;
 	}
 
-      debug_printf ("pipe write handle %p", *w);
+      debug_printf ("pipe read handle %p", *r);
     }
 
   /* Success. */
   return 0;
 }
 
-/* The next version of fhandler_pipe::create used to call the previous
-   version.  But the read handle created by the latter doesn't have
-   FILE_WRITE_ATTRIBUTES access unless the pipe is created with
-   PIPE_ACCESS_DUPLEX, and it doesn't seem possible to add that access
-   right.  This causes set_pipe_non_blocking to fail.
-
-   To fix this we will define a helper function 'nt_create' that is
-   similar to the above fhandler_pipe::create but uses
-   NtCreateNamedPipeFile instead of CreateNamedPipe; this gives more
-   flexibility in setting the access rights.  We will use this helper
-   function in the version of fhandler_pipe::create below, which
-   suffices for all of our uses of set_pipe_non_blocking.  For
-   simplicity, nt_create will omit the 'open_mode' and 'name'
-   parameters, which aren't needed for our purposes.  */
-
-static int nt_create (LPSECURITY_ATTRIBUTES, PHANDLE, PHANDLE, DWORD,
-		      int64_t *);
-
 int
 fhandler_pipe::create (fhandler_pipe *fhs[2], unsigned psize, int mode)
 {
@@ -597,7 +579,7 @@ fhandler_pipe::create (fhandler_pipe *fhs[2], unsigned psize, int mode)
   int res = -1;
   int64_t unique_id;
 
-  int ret = nt_create (sa, &r, &w, psize, &unique_id);
+  int ret = create (sa, &r, &w, psize, NULL, 0, &unique_id);
   if (ret)
     __seterrno_from_win_error (ret);
   else if ((fhs[0] = (fhandler_pipe *) build_fh_dev (*piper_dev)) == NULL)
@@ -624,138 +606,6 @@ fhandler_pipe::create (fhandler_pipe *fhs[2], unsigned psize, int mode)
   return res;
 }
 
-static int
-nt_create (LPSECURITY_ATTRIBUTES sa_ptr, PHANDLE r, PHANDLE w,
-		DWORD psize, int64_t *unique_id)
-{
-  NTSTATUS status;
-  HANDLE npfsh;
-  ACCESS_MASK access;
-  OBJECT_ATTRIBUTES attr;
-  IO_STATUS_BLOCK io;
-  LARGE_INTEGER timeout;
-
-  /* Default to error. */
-  if (r)
-    *r = NULL;
-  if (w)
-    *w = NULL;
-
-  status = fhandler_base::npfs_handle (npfsh);
-  if (!NT_SUCCESS (status))
-    {
-      __seterrno_from_nt_status (status);
-      return GetLastError ();
-    }
-
-  /* Ensure that there is enough pipe buffer space for atomic writes.  */
-  if (!psize)
-    psize = DEFAULT_PIPEBUFSIZE;
-
-  UNICODE_STRING pipename;
-  WCHAR pipename_buf[MAX_PATH];
-  size_t len = __small_swprintf (pipename_buf, L"%S-%u-",
-				 &cygheap->installation_key,
-				 GetCurrentProcessId ());
-
-  access = GENERIC_READ | FILE_WRITE_ATTRIBUTES;
-
-  ULONG pipe_type = pipe_byte ? FILE_PIPE_BYTE_STREAM_TYPE
-    : FILE_PIPE_MESSAGE_TYPE;
-
-  /* Retry NtCreateNamedPipeFile as long as the pipe name is in use.
-     Retrying will probably never be necessary, but we want
-     to be as robust as possible.  */
-  DWORD err = 0;
-  while (r && !*r)
-    {
-      static volatile ULONG pipe_unique_id;
-      LONG id = InterlockedIncrement ((LONG *) &pipe_unique_id);
-      __small_swprintf (pipename_buf + len, L"pipe-nt-%p", id);
-      if (unique_id)
-	*unique_id = ((int64_t) id << 32 | GetCurrentProcessId ());
-
-      debug_printf ("name %W, size %u, mode %s", pipename_buf, psize,
-		    (pipe_type & FILE_PIPE_MESSAGE_TYPE)
-		    ? "PIPE_TYPE_MESSAGE" : "PIPE_TYPE_BYTE");
-
-      RtlInitUnicodeString (&pipename, pipename_buf);
-
-      InitializeObjectAttributes (&attr, &pipename,
-				  sa_ptr->bInheritHandle ? OBJ_INHERIT : 0,
-				  npfsh, sa_ptr->lpSecurityDescriptor);
-
-      timeout.QuadPart = -500000;
-      status = NtCreateNamedPipeFile (r, access, &attr, &io,
-				      FILE_SHARE_READ | FILE_SHARE_WRITE,
-				      FILE_CREATE, 0, pipe_type,
-				      FILE_PIPE_BYTE_STREAM_MODE,
-				      0, 1, psize, psize, &timeout);
-
-      if (NT_SUCCESS (status))
-	{
-	  debug_printf ("pipe read handle %p", *r);
-	  err = 0;
-	  break;
-	}
-
-      switch (status)
-	{
-	case STATUS_PIPE_BUSY:
-	case STATUS_INSTANCE_NOT_AVAILABLE:
-	case STATUS_PIPE_NOT_AVAILABLE:
-	  /* The pipe is already open with compatible parameters.
-	     Pick a new name and retry.  */
-	  debug_printf ("pipe busy, retrying");
-	  *r = NULL;
-	  break;
-	case STATUS_ACCESS_DENIED:
-	  /* The pipe is already open with incompatible parameters.
-	     Pick a new name and retry.  */
-	  debug_printf ("pipe access denied, retrying");
-	  *r = NULL;
-	  break;
-	default:
-	  {
-	    __seterrno_from_nt_status (status);
-	    err = GetLastError ();
-	    debug_printf ("failed, %E");
-	    *r = INVALID_HANDLE_VALUE;
-	  }
-	}
-    }
-
-  if (err)
-    {
-      *r = NULL;
-      return err;
-    }
-
-  if (!w)
-    debug_printf ("pipe write handle NULL");
-  else
-    {
-      debug_printf ("NtOpenFile: name %S", &pipename);
-
-      access = GENERIC_WRITE | FILE_READ_ATTRIBUTES;
-      status = NtOpenFile (w, access, &attr, &io, 0, 0);
-      if (!NT_SUCCESS (status))
-	{
-	  DWORD err = GetLastError ();
-	  debug_printf ("NtOpenFile failed, r %p, %E", r);
-	  if (r)
-	    CloseHandle (*r);
-	  *w = NULL;
-	  return err;
-	}
-
-      debug_printf ("pipe write handle %p", *w);
-    }
-
-  /* Success. */
-  return 0;
-}
-
 int
 fhandler_pipe::ioctl (unsigned int cmd, void *p)
 {
-- 
2.33.0