cygrunsrv + sshd + rsync = 20 times too slow -- throttled?
Corinna Vinschen
corinna-cygwin@cygwin.com
Tue Aug 31 10:18:57 GMT 2021
On Aug 31 12:05, Corinna Vinschen wrote:
> On Aug 31 18:25, Takashi Yano wrote:
> > On Tue, 31 Aug 2021 11:08:42 +0200
> > Corinna Vinschenwrote:
> > > On Aug 31 17:55, Takashi Yano wrote:
> > > > On Mon, 30 Aug 2021 22:14:15 +0200
> > > > Corinna Vinschen wrote:
> > > > > Hi Ken, Hi Takashi,
> > > > >
> > > > > On Aug 30 19:00, Corinna Vinschen wrote:
> > > > > 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.
> > > >
> > > > I tested your patch attached. Unfortunately, select() does not work
> > > > as expected for write pipe. Even if the select reports write pipe
> > > > is available, writing to pipe fails. It seems that your patch fails
> > > > to detect pipe full.
> > >
> > > Bummer. Is that with byte mode pipes or with message mode pipes? If
> > > the latter, if you try to write more data than available in the buffer,
> > > it's bound to fail.
> >
> > Both message pipe and byte pipe.
> >
> > > Did you add debug output to pipe_data_available to see how the
> > > information looks like? Or do you have a simple, self-contained
> > > testcase in plain C?
> >
> > The test case is attached. If select() works as expected, the program
> > does not show "r" or "w". However, with your patch, the program prints
> > many "w" (means write() fails with EAGAIN).
>
> Thanks! I found th culprit, but we have another problem. Even if
> select returns correct info, A write, trying to write more bytes
> than are available in the buffer, hangs. This shouldn't happen.
> Still digging...
That's, of course, correct behaviour for pipes in blocking mode. D'oh!
Please try the attached patch on top of topic/pipe.
Thanks,
Corinna
-------------- next part --------------
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 132e6002133b..1f0f28077a7c 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1171,6 +1171,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 +1180,8 @@ public:
bool ispipe() const { return true; }
+ HANDLE get_query_handle () const { return query_hdl; }
+
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 +1190,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 2dec0a84817c..479b62bbd4aa 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,10 +641,23 @@ 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;
+ /* For the write side of the pipe, duplicate the handle to the read side
+ into query_hdl just for calling NtQueryInformationFile. See longish
+ comment in select.cc, pipe_data_available() for the reasoning. */
+ if (!DuplicateHandle (GetCurrentProcess (), r, GetCurrentProcess (),
+ &fhs[1]->query_hdl, GENERIC_READ,
+ !(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);
+ debug_printf ("%R = pipe(%d, %y)", res, psize, mode);
return res;
}
@@ -661,7 +698,7 @@ nt_create (LPSECURITY_ATTRIBUTES sa_ptr, PHANDLE r, PHANDLE w,
access = GENERIC_READ | FILE_WRITE_ATTRIBUTES;
ULONG pipe_type = pipe_byte ? FILE_PIPE_BYTE_STREAM_TYPE
- : FILE_PIPE_MESSAGE_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
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 83e1c00e0ac7..7e69ad834d9a 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -608,15 +608,33 @@ 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.
+
+ Usually WriteQuotaAvailable on the write side reflects the space
+ available in the inbound buffer on the read side. However, if a
+ pipe read is currently pending, WriteQuotaAvailable on the write side
+ is decremented by the number of bytes the read side is requesting.
+ So it's possible (even likely) that WriteQuotaAvailable is 0, even
+ if the inbound buffer on the read side is not full. This can lead to
+ a deadlock situation: The reader is waiting for data, but select
+ on the writer side assumes that no space is available in the read
+ side inbound buffer.
+
+ Consequentially, the only reliable information is available on the
+ read side, so fetch info from the read side via the pipe-specific
+ query handle. Use fpli.WriteQuotaAvailable as storage for the actual
+ interesting value, which is the InboundQuote on the read side,
+ decremented by the number of bytes of data in that buffer. */
+ fpli.WriteQuotaAvailable = fpli.InboundQuota - fpli.ReadDataAvailable;
if (fpli.WriteQuotaAvailable > 0)
{
paranoid_printf ("fd %d, %s, write: size %u, avail %u", fd,
- fh->get_name (), fpli.OutboundQuota,
+ fh->get_name (), fpli.InboundQuota,
fpli.WriteQuotaAvailable);
return 1;
}
@@ -718,9 +736,14 @@ out:
fhandler_pty_master *fhm = (fhandler_pty_master *) fh;
fhm->set_mask_flusho (s->read_ready);
}
- h = fh->get_output_handle ();
if (s->write_selected && dev != FH_PIPER)
{
+ /* For the write side of a pipe, fetch the handle to the read side.
+ See the longish comment in pipe_data_available for the reasoning. */
+ if (dev == FH_PIPEW)
+ h = ((fhandler_pipe *) fh)->get_query_handle ();
+ else
+ h = fh->get_output_handle ();
gotone += s->write_ready = pipe_data_available (s->fd, fh, h, true);
select_printf ("write: %s, gotone %d", fh->get_name (), gotone);
}
More information about the Cygwin-developers
mailing list