From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout.kundenserver.de (mout.kundenserver.de [212.227.126.187]) by sourceware.org (Postfix) with ESMTPS id 9C8043857C50 for ; Thu, 2 Sep 2021 18:54:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9C8043857C50 Authentication-Results: sourceware.org; dmarc=fail (p=none dis=none) header.from=cygwin.com Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=cygwin.com Received: from calimero.vinschen.de ([24.134.7.25]) by mrelayeu.kundenserver.de (mreue009 [212.227.15.167]) with ESMTPSA (Nemesis) id 1MsI0I-1nFc4M39eE-00tiXO for ; Thu, 02 Sep 2021 20:54:12 +0200 Received: by calimero.vinschen.de (Postfix, from userid 500) id 6E111A80D84; Thu, 2 Sep 2021 20:54:11 +0200 (CEST) Date: Thu, 2 Sep 2021 20:54:11 +0200 From: Corinna Vinschen To: cygwin-developers@cygwin.com Subject: Re: cygrunsrv + sshd + rsync = 20 times too slow -- throttled? Message-ID: Reply-To: cygwin-developers@cygwin.com Mail-Followup-To: cygwin-developers@cygwin.com References: <583ca127-02e7-6b3c-3732-6478c0f862e3@cornell.edu> <20210901080220.ee4a5bfbea62cc1ae0a9598e@nifty.ne.jp> <20210901091652.6bf3cccbcaed4a22f6ffa6b0@nifty.ne.jp> <20210901172339.1039604b7067e0492534a20f@nifty.ne.jp> <20210902171549.72910cf8e01c822233bfc56a@nifty.ne.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210902171549.72910cf8e01c822233bfc56a@nifty.ne.jp> X-Provags-ID: V03:K1:5CGvi4T58ypqj9rFP8uL4SsUGjPw7LmGpAjBCSwshsFC+pUSr1H TZHt0votW0ldsB92F79T5gJEs8OY9/rbBe3+xqN8TiqWYOamTg4EKWCZMqFDu02NQ0cbMz4 iObGCafEcFJzKC7pYdnZGBFsdIyr208jb2UvAFNmy2FODKzsiV/kLuWA1FVpgFtKuXGvtrW ISmvSi0Ui/99SkFZytUhQ== X-UI-Out-Filterresults: notjunk:1;V03:K0:wtwHvSgw1t4=:B7m/HKAOZ09N7A7U86Rb17 teBimBx107ocuSDtM5plMELHkFGKuTbrqSCjDiMnGGYbgHeNeIg0JQ9qwJjfMA/8FywpIIJyF ar4qb3qVPjk+eHzRumr16SOG9I4xHoDo+uH/OJA3boqs4qEldz2mcvbujJbsjAeToJtss8cR9 GvWCApeXxd/xKxw6YyHTIX36yvQrokzq2z9x55IZh16lNQ6YY7k3WuJp0yJ3AivE6WfOwOYzJ 9BGolq6EJAlWQXykQX/WqEGUVS8CAoQPQFX8DLBWBwVmb1810OzInoGdmPaYDVMzHoR8/XLIX QwNDxwHpFDCcex8CTur0ewHxaU08riRhIXovk9fZMSVQtNi34QsWREHr9YAWNmaH3tdz8cM7a HsHnebUiiBiJJhWF4iGT1gJ8INumQusB17Bbp+WLz3T4xLp4NNZmVMXH0r+xDxKFnJkRua+rd U+YXseUv33dOQbgAzq5aUBTWjcg2a+9uMH5/nnIpi6kgQqSFN4pQQaq3YBGgYawkoERhdoLhD acXWeuAm5V0JXPlcCreUu3QPm1AkwbRBXsCIPLGu1TUEisNtgl9/LGIFcG65KhzmIY1YqFcj7 8reCpyKETwmswy3Bll1Fcaq0UPNOCBT1y4mV6zL9MoztuINUKYBqP9RiQ5I9GyN2gebTZnUA1 +AHU542+4YN7QUVA40ok6bhIfd4wHlpGkE6D8MNZ+c2FEv4hPg0Q7O+7LYV6mLHYiT+qImWZz sh9vGSdglz2ksQEqn3waimWaIi0KWa/ZDYpjyfEZhx6a/ir1ig50Hthu738= X-Spam-Status: No, score=-106.0 required=5.0 tests=BAYES_00, GIT_PATCH_0, GOOD_FROM_CORINNA_CYGWIN, KAM_DMARC_NONE, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NEUTRAL, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: cygwin-developers@cygwin.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Cygwin core component developers mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Sep 2021 18:54:16 -0000 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. > > : error: "GCC_VERSION" redefined [-Werror] > : 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