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

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

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

Hi Corinna,

Thanks for reviewing the patches.

On Thu, 16 Sep 2021 15:25:46 +0200
Corinna Vinschen wrote:
> > diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
> > index fd2ab87af..7eedc010c 100644
> > --- a/winsup/cygwin/fhandler_pipe.cc
> > +++ b/winsup/cygwin/fhandler_pipe.cc
> > @@ -200,7 +200,14 @@ fhandler_pipe::open_setup (int flags)
> >        SECURITY_ATTRIBUTES *sa = sec_none_cloexec (flags);
> >        read_mtx = CreateMutex (sa, FALSE, NULL);
> >        if (!read_mtx)
> > -	debug_printf ("CreateMutex failed: %E");
> > +	debug_printf ("CreateMutex read_mtx failed: %E");
> > +    }
> > +  if (!hdl_cnt_mtx)
> > +    {
> > +      SECURITY_ATTRIBUTES *sa = sec_none_cloexec (flags);
> > +      hdl_cnt_mtx = CreateMutex (sa, FALSE, NULL);
> > +      if (!hdl_cnt_mtx)
> > +	debug_printf ("CreateMutex hdl_cnt_mtx failed: %E");
> >      }
> >    if (get_dev () == FH_PIPEW && !query_hdl)
> >      set_pipe_non_blocking (is_nonblocking ());
> > @@ -390,8 +397,10 @@ fhandler_pipe_fifo::reader_closed ()
> >  {
> >    if (!query_hdl)
> >      return false;
> > +  cygwait (hdl_cnt_mtx);
> 
> This is missing an `if (hdl_cnt_mtx)' check.

Done.

> Also, given you're using hdl_cnt_mtx only for short-lived blocking,
> you may want to reduce the overhead and just call WFSO with INFINITE
> timeout.
> 
> >    int n_reader = get_obj_handle_count (query_hdl);
> >    int n_writer = get_obj_handle_count (get_handle ());
> > +  ReleaseMutex (hdl_cnt_mtx);
> >    return n_reader == n_writer;
> >  }
> >  
> >  int
> > @@ -576,6 +594,7 @@ fhandler_pipe::dup (fhandler_base *child, int flags)
> >    ftp->set_popen_pid (0);
> >  
> >    int res = 0;
> > +  cygwait (hdl_cnt_mtx);
> 
> Same here and all the other cygwait(hdl_cnt_mtx) calls.

Done.

> It would be great if open_setup() would be converted to a method which
> is allowed to fail, rather than ignoring errors in sync object creation
> and having to test the handle throughout the code.  Given there's only a
> single caller of that function (dtable::init_std_file_from_handle), that
> shouldn't be much work.  But it's certainly better than ignoring creation
> failures in the long run.

I do not have confidence to do this correctly in a short time.
Leave it as a homework. Or may I leave it to you?

> > +      fhs[0]->hdl_cnt_mtx = CreateMutexW (sa, FALSE, NULL);
> > +      if (!fhs[0]->hdl_cnt_mtx)
> > +	{
> > +	  CloseHandle (fhs[0]->read_mtx);
> > +	  CloseHandle (fhs[0]->select_sem);
> > +	  delete fhs[0];
> > +	  NtClose (r);
> > +	  CloseHandle (fhs[1]->select_sem);
> > +	  CloseHandle (fhs[1]->query_hdl);
> > +	  delete fhs[1];
> > +	  NtClose (w);
> > +	  goto out;
> > +	}
> > +      if (!DuplicateHandle (GetCurrentProcess (), fhs[0]->hdl_cnt_mtx,
> > +			    GetCurrentProcess (), &fhs[1]->hdl_cnt_mtx,
> > +			    0, sa->bInheritHandle, DUPLICATE_SAME_ACCESS))
> > +	{
> > +	  CloseHandle (fhs[0]->read_mtx);
> > +	  CloseHandle (fhs[0]->select_sem);
> > +	  CloseHandle (fhs[0]->hdl_cnt_mtx);
> > +	  delete fhs[0];
> > +	  NtClose (r);
> > +	  CloseHandle (fhs[1]->select_sem);
> > +	  CloseHandle (fhs[1]->query_hdl);
> > +	  delete fhs[1];
> > +	  NtClose (w);
> > +	  goto out;
> > +	}
> > +
> >        res = 0;
> >      }
> 
> What about converting this to a goto error chain as in
> fhandler_pty_slave::setup_pseudoconsole?  This makes error handling
> much cleaner, IMHO.

Done.

I have attached the patches revised.

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

[-- Attachment #2: v2-0001-Cygwin-close_all_files-Do-not-duplicate-stderr-fo.patch --]
[-- Type: application/octet-stream, Size: 1091 bytes --]

From 4bbd03dc7d482966ca5ace2e130424a8ba2cbf9d Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Thu, 16 Sep 2021 20:50:54 +0900
Subject: [PATCH v2 1/3] Cygwin: close_all_files: Do not duplicate stderr for
 write pipe.

- Currently, the stderr handle is duplicated in close_all_files().
  This interferes the handle counting for detecting closure of read
  pipe, which is introduced by commit f79a4611. This patch stops
  duplicating stderr handle if it is write pipe.
---
 winsup/cygwin/syscalls.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 11af079cb..a3ffe3532 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -95,7 +95,7 @@ close_all_files (bool norelease)
       if (cfd >= 0)
 	{
 	  debug_only_printf ("closing fd %d", i);
-	  if (i == 2)
+	  if (i == 2 && cfd->get_dev () != FH_PIPEW)
 	    DuplicateHandle (GetCurrentProcess (), cfd->get_output_handle (),
 			     GetCurrentProcess (), &h,
 			     0, false, DUPLICATE_SAME_ACCESS);
-- 
2.33.0


[-- Attachment #3: v2-0002-Cygwin-pipe-Fix-error-handling-in-fhandler_pip-cr.patch --]
[-- Type: application/octet-stream, Size: 4410 bytes --]

From 4b784d72ecffe5293f10baee314ff892d5ddc59b Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Thu, 16 Sep 2021 23:15:17 +0900
Subject: [PATCH v2 2/3] Cygwin: pipe: Fix error handling in
 fhandler_pip::create().

- Currently, error handling in fhandler_pipe::create() is broken.
  This patch fixes that.
---
 winsup/cygwin/fhandler.h       |   1 -
 winsup/cygwin/fhandler_pipe.cc | 101 ++++++++++++++++-----------------
 2 files changed, 48 insertions(+), 54 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 919655012..eb312ccb6 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1206,7 +1206,6 @@ public:
   fhandler_pipe ();
 
   bool ispipe() const { return true; }
-  void set_read_mutex (HANDLE mtx) { read_mtx = mtx; }
   void set_pipe_buf_size ();
 
   void set_popen_pid (pid_t pid) {popen_pid = pid;}
diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
index c2f4353f6..582b4d564 100644
--- a/winsup/cygwin/fhandler_pipe.cc
+++ b/winsup/cygwin/fhandler_pipe.cc
@@ -803,61 +803,56 @@ fhandler_pipe::create (fhandler_pipe *fhs[2], unsigned psize, int mode)
 
   int ret = nt_create (sa, r, w, psize, &unique_id);
   if (ret)
-    __seterrno_from_win_error (ret);
-  else if ((fhs[0] = (fhandler_pipe *) build_fh_dev (*piper_dev)) == NULL)
     {
-      NtClose (r);
-      NtClose (w);
-    }
-  else if ((fhs[1] = (fhandler_pipe *) build_fh_dev (*pipew_dev)) == NULL)
-    {
-      delete fhs[0];
-      NtClose (r);
-      NtClose (w);
-    }
-  else
-    {
-      mode |= mode & O_TEXT ?: O_BINARY;
-      fhs[0]->init (r, FILE_CREATE_PIPE_INSTANCE | GENERIC_READ, mode,
-		    unique_id);
-      fhs[1]->init (w, FILE_CREATE_PIPE_INSTANCE | GENERIC_WRITE, mode,
-		    unique_id);
-      /* For the read side of the pipe, add a mutex.  See raw_read for the
-	 usage. */
-      HANDLE mtx = CreateMutexW (sa, FALSE, NULL);
-      if (!mtx)
-	{
-	  delete fhs[0];
-	  NtClose (r);
-	  delete fhs[1];
-	  NtClose (w);
-	}
-      else
-	{
-	  fhs[0]->set_read_mutex (mtx);
-	  res = 0;
-	}
-      fhs[0]->select_sem = CreateSemaphore (sa, 0, INT32_MAX, NULL);
-      if (fhs[0]->select_sem)
-	DuplicateHandle (GetCurrentProcess (), fhs[0]->select_sem,
-			 GetCurrentProcess (), &fhs[1]->select_sem,
-			 0, sa->bInheritHandle, DUPLICATE_SAME_ACCESS);
-      if (!DuplicateHandle (GetCurrentProcess (), r,
-			    GetCurrentProcess (), &fhs[1]->query_hdl,
-			    FILE_READ_DATA, sa->bInheritHandle, 0))
-	{
-	  CloseHandle (fhs[0]->select_sem);
-	  delete fhs[0];
-	  CloseHandle (r);
-	  CloseHandle (fhs[1]->select_sem);
-	  delete fhs[1];
-	  CloseHandle (w);
-	}
-      else
-	res = 0;
+      __seterrno_from_win_error (ret);
+      goto out;
     }
-
-  debug_printf ("%R = pipe([%p, %p], %d, %y)", res, fhs[0], fhs[1], psize, mode);
+  if ((fhs[0] = (fhandler_pipe *) build_fh_dev (*piper_dev)) == NULL)
+    goto err_close_rw_handle;
+  if ((fhs[1] = (fhandler_pipe *) build_fh_dev (*pipew_dev)) == NULL)
+    goto err_delete_fhs0;
+  mode |= mode & O_TEXT ?: O_BINARY;
+  fhs[0]->init (r, FILE_CREATE_PIPE_INSTANCE | GENERIC_READ, mode, unique_id);
+  fhs[1]->init (w, FILE_CREATE_PIPE_INSTANCE | GENERIC_WRITE, mode, unique_id);
+
+  /* For the read side of the pipe, add a mutex.  See raw_read for the
+     usage. */
+  fhs[0]->read_mtx = CreateMutexW (sa, FALSE, NULL);
+  if (!fhs[0]->read_mtx)
+    goto err_delete_fhs1;
+
+  fhs[0]->select_sem = CreateSemaphore (sa, 0, INT32_MAX, NULL);
+  if (!fhs[0]->select_sem)
+    goto err_close_read_mtx;
+  if (!DuplicateHandle (GetCurrentProcess (), fhs[0]->select_sem,
+			GetCurrentProcess (), &fhs[1]->select_sem,
+			0, sa->bInheritHandle, DUPLICATE_SAME_ACCESS))
+    goto err_close_select_sem0;
+
+  if (!DuplicateHandle (GetCurrentProcess (), r,
+			GetCurrentProcess (), &fhs[1]->query_hdl,
+			FILE_READ_DATA, sa->bInheritHandle, 0))
+    goto err_close_select_sem1;
+
+  res = 0;
+  goto out;
+
+err_close_select_sem1:
+  CloseHandle (fhs[1]->select_sem);
+err_close_select_sem0:
+  CloseHandle (fhs[0]->select_sem);
+err_close_read_mtx:
+  CloseHandle (fhs[0]->read_mtx);
+err_delete_fhs1:
+  delete fhs[1];
+err_delete_fhs0:
+  delete fhs[0];
+err_close_rw_handle:
+  NtClose (r);
+  NtClose (w);
+out:
+  debug_printf ("%R = pipe([%p, %p], %d, %y)",
+		res, fhs[0], fhs[1], psize, mode);
   return res;
 }
 
-- 
2.33.0


[-- Attachment #4: v2-0003-Cygwin-pipe-Fix-race-issue-regarding-handle-count.patch --]
[-- Type: application/octet-stream, Size: 5579 bytes --]

From bc1a2d65cb36b839c5e9246f65865abd62cb2587 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Thu, 16 Sep 2021 23:21:57 +0900
Subject: [PATCH v2 3/3] Cygwin: pipe: Fix race issue regarding handle count.

- This patch fixes the race issue in the handle counting to detect
  closure of read pipe, which is introduced by commit f79a4611.
  A mutex hdl_cnt_mtx is introduced for this issue.
---
 winsup/cygwin/fhandler.h       |  1 +
 winsup/cygwin/fhandler_pipe.cc | 65 ++++++++++++++++++++++++++++++----
 2 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index eb312ccb6..31edb1822 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1176,6 +1176,7 @@ class fhandler_pipe_fifo: public fhandler_base
  protected:
   size_t pipe_buf_size;
   HANDLE query_hdl;
+  HANDLE hdl_cnt_mtx;
   virtual void release_select_sem (const char *) {};
 
  public:
diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
index 582b4d564..2068a943e 100644
--- a/winsup/cygwin/fhandler_pipe.cc
+++ b/winsup/cygwin/fhandler_pipe.cc
@@ -200,7 +200,14 @@ fhandler_pipe::open_setup (int flags)
       SECURITY_ATTRIBUTES *sa = sec_none_cloexec (flags);
       read_mtx = CreateMutex (sa, FALSE, NULL);
       if (!read_mtx)
-	debug_printf ("CreateMutex failed: %E");
+	debug_printf ("CreateMutex read_mtx failed: %E");
+    }
+  if (!hdl_cnt_mtx)
+    {
+      SECURITY_ATTRIBUTES *sa = sec_none_cloexec (flags);
+      hdl_cnt_mtx = CreateMutex (sa, FALSE, NULL);
+      if (!hdl_cnt_mtx)
+	debug_printf ("CreateMutex hdl_cnt_mtx failed: %E");
     }
   if (get_dev () == FH_PIPEW && !query_hdl)
     set_pipe_non_blocking (is_nonblocking ());
@@ -390,8 +397,12 @@ fhandler_pipe_fifo::reader_closed ()
 {
   if (!query_hdl)
     return false;
+  if (hdl_cnt_mtx)
+    WaitForSingleObject (hdl_cnt_mtx, INFINITE);
   int n_reader = get_obj_handle_count (query_hdl);
   int n_writer = get_obj_handle_count (get_handle ());
+  if (hdl_cnt_mtx)
+    ReleaseMutex (hdl_cnt_mtx);
   return n_reader == n_writer;
 }
 
@@ -554,11 +565,18 @@ fhandler_pipe::set_close_on_exec (bool val)
     set_no_inheritance (select_sem, val);
   if (query_hdl)
     set_no_inheritance (query_hdl, val);
+  if (hdl_cnt_mtx)
+    set_no_inheritance (hdl_cnt_mtx, val);
 }
 
 void
 fhandler_pipe::fixup_after_fork (HANDLE parent)
 {
+  if (hdl_cnt_mtx)
+    {
+      fork_fixup (parent, hdl_cnt_mtx, "hdl_cnt_mtx");
+      WaitForSingleObject (hdl_cnt_mtx, INFINITE);
+    }
   if (read_mtx)
     fork_fixup (parent, read_mtx, "read_mtx");
   if (select_sem)
@@ -567,6 +585,8 @@ fhandler_pipe::fixup_after_fork (HANDLE parent)
     fork_fixup (parent, query_hdl, "query_hdl");
 
   fhandler_base::fixup_after_fork (parent);
+  if (hdl_cnt_mtx)
+    ReleaseMutex (hdl_cnt_mtx);
 }
 
 int
@@ -576,6 +596,8 @@ fhandler_pipe::dup (fhandler_base *child, int flags)
   ftp->set_popen_pid (0);
 
   int res = 0;
+  if (hdl_cnt_mtx)
+    WaitForSingleObject (hdl_cnt_mtx, INFINITE);
   if (fhandler_base::dup (child, flags))
     res = -1;
   else if (read_mtx &&
@@ -589,8 +611,8 @@ fhandler_pipe::dup (fhandler_base *child, int flags)
     }
   else if (select_sem &&
 	   !DuplicateHandle (GetCurrentProcess (), select_sem,
-			    GetCurrentProcess (), &ftp->select_sem,
-			    0, !(flags & O_CLOEXEC), DUPLICATE_SAME_ACCESS))
+			     GetCurrentProcess (), &ftp->select_sem,
+			     0, !(flags & O_CLOEXEC), DUPLICATE_SAME_ACCESS))
     {
       __seterrno ();
       ftp->close ();
@@ -598,13 +620,24 @@ fhandler_pipe::dup (fhandler_base *child, int flags)
     }
   else if (query_hdl &&
 	   !DuplicateHandle (GetCurrentProcess (), query_hdl,
-			    GetCurrentProcess (), &ftp->query_hdl,
-			    0, !(flags & O_CLOEXEC), DUPLICATE_SAME_ACCESS))
+			     GetCurrentProcess (), &ftp->query_hdl,
+			     0, !(flags & O_CLOEXEC), DUPLICATE_SAME_ACCESS))
+    {
+      __seterrno ();
+      ftp->close ();
+      res = -1;
+    }
+  else if (hdl_cnt_mtx &&
+	   !DuplicateHandle (GetCurrentProcess (), hdl_cnt_mtx,
+			     GetCurrentProcess (), &ftp->hdl_cnt_mtx,
+			     0, !(flags & O_CLOEXEC), DUPLICATE_SAME_ACCESS))
     {
       __seterrno ();
       ftp->close ();
       res = -1;
     }
+  if (hdl_cnt_mtx)
+    ReleaseMutex (hdl_cnt_mtx);
 
   debug_printf ("res %d", res);
   return res;
@@ -620,9 +653,17 @@ fhandler_pipe::close ()
     }
   if (read_mtx)
     CloseHandle (read_mtx);
+  if (hdl_cnt_mtx)
+    WaitForSingleObject (hdl_cnt_mtx, INFINITE);
   if (query_hdl)
     CloseHandle (query_hdl);
-  return fhandler_base::close ();
+  int ret = fhandler_base::close ();
+  if (hdl_cnt_mtx)
+    {
+      ReleaseMutex (hdl_cnt_mtx);
+      CloseHandle (hdl_cnt_mtx);
+    }
+  return ret;
 }
 
 #define PIPE_INTRO "\\\\.\\pipe\\cygwin-"
@@ -834,9 +875,21 @@ fhandler_pipe::create (fhandler_pipe *fhs[2], unsigned psize, int mode)
 			FILE_READ_DATA, sa->bInheritHandle, 0))
     goto err_close_select_sem1;
 
+  fhs[0]->hdl_cnt_mtx = CreateMutexW (sa, FALSE, NULL);
+  if (!fhs[0]->hdl_cnt_mtx)
+    goto err_close_query_hdl;
+  if (!DuplicateHandle (GetCurrentProcess (), fhs[0]->hdl_cnt_mtx,
+			GetCurrentProcess (), &fhs[1]->hdl_cnt_mtx,
+			0, sa->bInheritHandle, DUPLICATE_SAME_ACCESS))
+    goto err_close_hdl_cnt_mtx0;
+
   res = 0;
   goto out;
 
+err_close_hdl_cnt_mtx0:
+  CloseHandle (fhs[0]->hdl_cnt_mtx);
+err_close_query_hdl:
+  CloseHandle (fhs[1]->query_hdl);
 err_close_select_sem1:
   CloseHandle (fhs[1]->select_sem);
 err_close_select_sem0:
-- 
2.33.0