From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout.kundenserver.de (mout.kundenserver.de [212.227.126.130]) by sourceware.org (Postfix) with ESMTPS id 57F263858437 for ; Mon, 30 Aug 2021 20:14:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 57F263858437 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 1MDyoW-1mArWF3z5I-00A1Ye for ; Mon, 30 Aug 2021 22:14:16 +0200 Received: by calimero.vinschen.de (Postfix, from userid 500) id 7DDB2A80D72; Mon, 30 Aug 2021 22:14:15 +0200 (CEST) Date: Mon, 30 Aug 2021 22:14:15 +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: <20210828184102.f2206a8a9e5fe5cf24bf5e45@nifty.ne.jp> <20210829180729.48b4e877f773cb3980c5766d@nifty.ne.jp> <20210830091314.f9a2cb71794d0f68cdb5eba7@nifty.ne.jp> <20210830092259.52f7d54fc3fa340738373af4@nifty.ne.jp> <529d7dd7-d876-ca51-cc1f-e414d3c24f71@cornell.edu> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="l/kEWt9gI8KVzZbS" Content-Disposition: inline In-Reply-To: X-Provags-ID: V03:K1:rm/3P3nyl87k4d0kq+33XExzhFXA1cqpBHK/I2AID5Xa76B8D+i X3Dh69xA1ppZ2vQ1m0BLuprrILUicyCSNoerRZRs80CO56LxJqmwYv9oEKXx7NS8+dxn79p 2bYgM6glIEOhILz5fz8AS7ZCRlxGgRvyEfLQaOiZaaP4DEaWOkeGVTPNQeFBqsxLYlCuwSA e6sHv8caQMDlptBDUcH+g== X-UI-Out-Filterresults: notjunk:1;V03:K0:1sEodsFmDs8=:w/axoHeJLzNlebhwupLnbM c46DykmXNKsMMQBWWQJfRxnqypM19LLwF25a9Y9FOzTeHMGaUpNGiSDGfnuNTjMNXpN2I0WcP YjPhF/8IebYj8depBzQuz2V6NjIa3D52gVUMMnKQEMrM4AFNcngPveHvn2qn0j0l5IKECIDNy j++XM6Ne35QmiUumqtg2IjFUoTRQS8+bl58mxhE9eY+oT76b4AEKjDP7OK1Tx+ymlECOjRRxo g/Ok85KKaoBM1PXTa6QeLEPhUTKb+Vi7aVaTSWnKb2NKObNeScHVCQdyaTzyBYoQxq0j1VG1Z csKPNKMrgd9MWOeC06AwCG/Rq1Iu+F0WiXe5v/gx+zih72b8tkn3wV6ansYV7P21oyn/atJeg rQ0/Ms3Cjv3mEwVaJFNlup7eqOwr0dsViRS1df0aUWzOi27C5MsCqmR5vp/l8AE4joXnOuUx1 bS89cnabniTlz5tby0n3eKJSQI2ahdXs/fLgGVjeH236av2m/W8eBYiCazR03n4zR/bA6mpjf 9ipBNLAQrKuZXiNsnhKm30qMTjwN+W9TjfJ9UDC/ZqsgaWlyK0wDW2xncvi/STe7uZdOFpC6Z srVqBvp+Qb3QtnMBH1eDKPw1HPRlQyuMm7ARh5W4kgvgya/TA5WTNdze6lfD1/bdT85ZeBo9v GkDkACg5pYSlEgkXgx6ZXuF4hoM0VYAhpZI4fA0Se3fO6zJuizCONciE0p0GrSVl9Db7FTKjY 9haU6k8Zq0RT0cDx+E2E4K+Bhpw/33huAtvvo/ez6fHDFiUuzlGKRfJrVBU= 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, 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: Mon, 30 Aug 2021 20:14:28 -0000 --l/kEWt9gI8KVzZbS Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Hi Ken, Hi Takashi, On Aug 30 19:00, Corinna Vinschen wrote: > Ok, let's discuss this. I added more code to my testcase and here's > what I see. I dropped all data from the output which doesn't change. > [...] > - InboundQuota and OutboundQuota are always constant values and > do not depend on the side the information has been queried on. > That certainly makes sense. > > - WriteQuotaAvailable does not depend on the OutboundQuota, but on > the InboundQuota, and very likely on the InboundQuota of the read > side. The OutboundQuota *probably* only makes sense when using > named pipes with remote clients, which we never do anyway. > > The preceeding output shows that ReadDataAvailable on the read side and > WriteQuotaAvailable on the write side are connected. If we write 20 > bytes, ReadDataAvailable is incremented by 20 and WriteQuotaAvailable is > decremented by 20. > > So: write.WriteQuotaAvailable == InboundQuota - read.ReadDataAvailable. > > Except when a ReadFile is pending on the read side. It's as if the > running ReadFile already reserved write quota. So the write side > WriteQuotaAvailable is the number of bytes we can write without blocking, > after all pending ReadFiles have been satisfied. > > Unfortunately that doesn't really make sense when looked at it from the > user space. > > What that means in the first place is that WriteQuotaAvailable on the > write side is unreliable. What we really need is InboundQuota - > read.ReadDataAvailable. The problem with that is that the write side > usually has no access to the read side of the pipe. > > Long story short, I have no idea how to fix that ATM. Well, what about keeping a duplicate of the read side handle on the write side just for calling NtQueryInformationFile? Attached is an untested patch, can you have a look if that makes sense? Btw., I think I found a bug in the new fhandler_pipe::create. If the function fails to create the write side fhandler, it deletes the read side fhandler, but neglects to close the read handle. My patch fixes that. While looking into this I found a problem in fhandler_disk_file in terms of handle inheritance of the special handle for pread/pwrite. I already force pushed this onto topic/pipe. Thanks, Corinna --l/kEWt9gI8KVzZbS Content-Type: text/plain; charset=utf-8 Content-Disposition: attachment; filename="pipe.diff" diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h index 132e6002133b..a2de4301521b 100644 --- a/winsup/cygwin/fhandler.h +++ b/winsup/cygwin/fhandler.h @@ -462,6 +462,7 @@ public: virtual HANDLE& get_output_handle_nat () { return io_handle; } virtual HANDLE get_stat_handle () { return pc.handle () ?: io_handle; } virtual HANDLE get_echo_handle () const { return NULL; } + virtual HANDLE get_query_handle () const { return NULL; } virtual bool hit_eof () {return false;} virtual select_record *select_read (select_stuff *); virtual select_record *select_write (select_stuff *); @@ -1171,6 +1172,7 @@ class fhandler_socket_unix : public fhandler_socket class fhandler_pipe: public fhandler_base { private: + HANDLE query_hdl; pid_t popen_pid; size_t max_atomic_write; void set_pipe_non_blocking (bool nonblocking); @@ -1179,6 +1181,12 @@ public: bool ispipe() const { return true; } + HANDLE get_query_handle () + { + return (get_device () == FH_PIPEW) ? query_hdl : get_handle (); + } + void set_query_handle (HANDLE qh) { query_hdl = qh; } + void set_popen_pid (pid_t pid) {popen_pid = pid;} pid_t get_popen_pid () const {return popen_pid;} off_t lseek (off_t offset, int whence); @@ -1187,7 +1195,9 @@ public: select_record *select_except (select_stuff *); char *get_proc_fd_name (char *buf); int open (int flags, mode_t mode = 0); + void fixup_after_fork (HANDLE); int dup (fhandler_base *child, int); + int close (); void __reg3 raw_read (void *ptr, size_t& len); ssize_t __reg3 raw_write (const void *ptr, size_t len); int ioctl (unsigned int cmd, void *); diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc index b99f00c099f8..f698f9063207 100644 --- a/winsup/cygwin/fhandler_pipe.cc +++ b/winsup/cygwin/fhandler_pipe.cc @@ -405,22 +405,45 @@ fhandler_pipe::raw_write (const void *ptr, size_t len) return ret; } +void +fhandler_pipe::fixup_after_fork (HANDLE parent) +{ + if (query_hdl) + fork_fixup (parent, query_hdl, "query_hdl"); + fhandler_base::fixup_after_fork (parent); +} + int fhandler_pipe::dup (fhandler_base *child, int flags) { fhandler_pipe *ftp = (fhandler_pipe *) child; ftp->set_popen_pid (0); - int res; - if (get_handle () && fhandler_base::dup (child, flags)) + int res = 0; + if (fhandler_base::dup (child, flags)) res = -1; - else - res = 0; + else if (query_hdl && + !DuplicateHandle (GetCurrentProcess (), query_hdl, + GetCurrentProcess (), &ftp->query_hdl, + 0, !(flags & O_CLOEXEC), DUPLICATE_SAME_ACCESS)) + { + __seterrno (); + ftp->close (); + res = -1; + } debug_printf ("res %d", res); return res; } +int +fhandler_pipe::close () +{ + if (query_hdl) + NtClose (query_hdl); + return fhandler_base::close (); +} + #define PIPE_INTRO "\\\\.\\pipe\\cygwin-" /* Create a pipe, and return handles to the read and write ends, @@ -608,6 +631,7 @@ fhandler_pipe::create (fhandler_pipe *fhs[2], unsigned psize, int mode) else if ((fhs[1] = (fhandler_pipe *) build_fh_dev (*pipew_dev)) == NULL) { delete fhs[0]; + CloseHandle (r); CloseHandle (w); } else @@ -617,7 +641,17 @@ fhandler_pipe::create (fhandler_pipe *fhs[2], unsigned psize, int mode) unique_id); fhs[1]->init (w, FILE_CREATE_PIPE_INSTANCE | GENERIC_WRITE, mode, unique_id); - res = 0; + if (!DuplicateHandle (GetCurrentProcess (), r, GetCurrentProcess (), + &fhs[1]->query_hdl, FILE_READ_ATTRIBUTES, + !(mode & O_CLOEXEC), 0)) + { + delete fhs[0]; + CloseHandle (r); + delete fhs[1]; + CloseHandle (w); + } + else + res = 0; } debug_printf ("%R = pipe([%p, %p], %d, %y)", res, fhs[0], fhs[1], psize, mode); diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc index 83e1c00e0ac7..dc0563a45729 100644 --- a/winsup/cygwin/select.cc +++ b/winsup/cygwin/select.cc @@ -608,11 +608,14 @@ pipe_data_available (int fd, fhandler_base *fh, HANDLE h, bool writing) } if (writing) { - /* If there is anything available in the pipe buffer then signal - that. This means that a pipe could still block since you could - be trying to write more to the pipe than is available in the - buffer but that is the hazard of select(). */ - fpli.WriteQuotaAvailable = fpli.OutboundQuota - fpli.ReadDataAvailable; + /* If there is anything available in the pipe buffer then signal + that. This means that a pipe could still block since you could + be trying to write more to the pipe than is available in the + buffer but that is the hazard of select(). + Note that WriteQuotaAvailable is unreliable. The only reliable + information is available on the read side, which is why we fetch + the info from the read side via the pipe-specific query handle. */ + fpli.WriteQuotaAvailable = fpli.InboundQuota - fpli.ReadDataAvailable; if (fpli.WriteQuotaAvailable > 0) { paranoid_printf ("fd %d, %s, write: size %u, avail %u", fd, @@ -718,7 +721,7 @@ out: fhandler_pty_master *fhm = (fhandler_pty_master *) fh; fhm->set_mask_flusho (s->read_ready); } - h = fh->get_output_handle (); + h = fh->get_query_handle (); if (s->write_selected && dev != FH_PIPER) { gotone += s->write_ready = pipe_data_available (s->fd, fh, h, true); --l/kEWt9gI8KVzZbS--