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

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

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

On Thu, 16 Sep 2021 18:09:05 +0900
Takashi Yano wrote:
> I encountered a problem with current git head.
> 
> To reproduce the problem:
> 1) Make the CPU hight load state (100% load for all cores).
> 2) Run '/bin/echo AAAAAAAAAAA 2>&1 |cat' several times.
> Sometimes, no output is shown.
> 
> This seems to be a race issue between multiple pipe writers.
> Now I am investigating the problem.

I would like to propose three additional patches, two are for
the issue above (0001 and 0003), and the other one (0002) fixes
the error handling.

Please have a look at patches attached.

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

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

From a57ad3e77a5b4760fc161406c411cd9d5084c39a 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 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: 0002-Cygwin-pipe-Fix-error-handling-in-fhandler_pip-creat.patch --]
[-- Type: application/octet-stream, Size: 2315 bytes --]

From c967493799fe261c6bc0d3d36a5664763ec29f31 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Thu, 16 Sep 2021 21:27:24 +0900
Subject: [PATCH 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_pipe.cc | 43 ++++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 12 deletions(-)

diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
index c2f4353f6..fd2ab87af 100644
--- a/winsup/cygwin/fhandler_pipe.cc
+++ b/winsup/cygwin/fhandler_pipe.cc
@@ -831,32 +831,51 @@ fhandler_pipe::create (fhandler_pipe *fhs[2], unsigned psize, int mode)
 	  NtClose (r);
 	  delete fhs[1];
 	  NtClose (w);
+	  goto out;
 	}
-      else
+      fhs[0]->set_read_mutex (mtx);
+
+      fhs[0]->select_sem = CreateSemaphore (sa, 0, INT32_MAX, NULL);
+      if (!fhs[0]->select_sem)
 	{
-	  fhs[0]->set_read_mutex (mtx);
-	  res = 0;
+	  CloseHandle (fhs[0]->read_mtx);
+	  delete fhs[0];
+	  NtClose (r);
+	  delete fhs[1];
+	  NtClose (w);
+	  goto out;
 	}
-      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 (), fhs[0]->select_sem,
+			    GetCurrentProcess (), &fhs[1]->select_sem,
+			    0, sa->bInheritHandle, DUPLICATE_SAME_ACCESS))
+	{
+	  CloseHandle (fhs[0]->read_mtx);
+	  CloseHandle (fhs[0]->select_sem);
+	  delete fhs[0];
+	  NtClose (r);
+	  delete fhs[1];
+	  NtClose (w);
+	  goto out;
+	}
+
       if (!DuplicateHandle (GetCurrentProcess (), r,
 			    GetCurrentProcess (), &fhs[1]->query_hdl,
 			    FILE_READ_DATA, sa->bInheritHandle, 0))
 	{
+	  CloseHandle (fhs[0]->read_mtx);
 	  CloseHandle (fhs[0]->select_sem);
 	  delete fhs[0];
-	  CloseHandle (r);
+	  NtClose (r);
 	  CloseHandle (fhs[1]->select_sem);
 	  delete fhs[1];
-	  CloseHandle (w);
+	  NtClose (w);
+	  goto out;
 	}
-      else
-	res = 0;
+
+      res = 0;
     }
 
+out:
   debug_printf ("%R = pipe([%p, %p], %d, %y)", res, fhs[0], fhs[1], psize, mode);
   return res;
 }
-- 
2.33.0


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

From 8c21f43b181aa24dd4dde91d6d3f45f3f86f4d81 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Thu, 16 Sep 2021 21:44:09 +0900
Subject: [PATCH 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 | 74 +++++++++++++++++++++++++++++++---
 2 files changed, 69 insertions(+), 6 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 919655012..e0a2dee09 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 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);
   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;
 }
 
@@ -554,11 +563,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");
+      cygwait (hdl_cnt_mtx);
+    }
   if (read_mtx)
     fork_fixup (parent, read_mtx, "read_mtx");
   if (select_sem)
@@ -567,6 +583,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 +594,7 @@ fhandler_pipe::dup (fhandler_base *child, int flags)
   ftp->set_popen_pid (0);
 
   int res = 0;
+  cygwait (hdl_cnt_mtx);
   if (fhandler_base::dup (child, flags))
     res = -1;
   else if (read_mtx &&
@@ -589,8 +608,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 +617,23 @@ 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;
     }
+  ReleaseMutex (hdl_cnt_mtx);
 
   debug_printf ("res %d", res);
   return res;
@@ -620,9 +649,13 @@ fhandler_pipe::close ()
     }
   if (read_mtx)
     CloseHandle (read_mtx);
+  cygwait (hdl_cnt_mtx);
   if (query_hdl)
     CloseHandle (query_hdl);
-  return fhandler_base::close ();
+  int ret = fhandler_base::close ();
+  ReleaseMutex (hdl_cnt_mtx);
+  CloseHandle (hdl_cnt_mtx);
+  return ret;
 }
 
 #define PIPE_INTRO "\\\\.\\pipe\\cygwin-"
@@ -872,6 +905,35 @@ fhandler_pipe::create (fhandler_pipe *fhs[2], unsigned psize, int mode)
 	  goto out;
 	}
 
+      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;
     }
 
-- 
2.33.0