cygrunsrv + sshd + rsync = 20 times too slow -- throttled?
Takashi Yano
takashi.yano@nifty.ne.jp
Sun Sep 5 19:42:33 GMT 2021
On Sun, 5 Sep 2021 14:47:26 -0400
Ken Brown wrote:
> Hi Takashi,
>
> On 9/5/2021 9:50 AM, Takashi Yano wrote:
> > On Sun, 5 Sep 2021 22:40:59 +0900
> > Takashi Yano wrote:
> >> On Sun, 5 Sep 2021 08:15:23 +0900
> >> Takashi Yano wrote:
> >>> Hi Ken,
> >>>
> >>> On Sat, 4 Sep 2021 10:04:12 -0400
> >>> Ken Brown wrote:
> >>>> On 9/4/2021 8:37 AM, Takashi Yano wrote:
> >>>>> On Sat, 4 Sep 2021 21:02:58 +0900
> >>>>> Takashi Yano wrote:
> >>>>>> Hi Corinna, Ken,
> >>>>>>
> >>>>>> On Fri, 3 Sep 2021 09:27:37 -0400
> >>>>>> Ken Brown wrote:
> >>>>>>> On 9/3/2021 8:22 AM, Takashi Yano wrote:
> >>>>>>>> POSIX says:
> >>>>>>>> The value returned may be less than nbyte if the number of bytes left
> >>>>>>>> in the file is less than nbyte, if the read() request was interrupted
> >>>>>>>> by a signal, or if the file is a pipe or FIFO or special file and has
> >>>>>>>> ~~~
> >>>>>>>> fewer than nbyte bytes immediately available for reading.
> >>>>>>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>>>>>>
> >>>>>>>> https://pubs.opengroup.org/onlinepubs/009604599/functions/read.html
> >>>>>>>>
> >>>>>>>> If it is turned over, read() should read all data immediately available,
> >>>>>>>> I think.
> >>>>>>>
> >>>>>>> I understand the reasoning now, but I think your patch isn't quite right. As it
> >>>>>>> stands, if the call to NtQueryInformationFile fails but total_length != 0,
> >>>>>>> you're trying to read again without knowing that there's data in the pipe.
> >>>>>>>
> >>>>>>> Also, I think you need the following:
> >>>>>>>
> >>>>>>> diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
> >>>>>>> index ef7823ae5..46bb96961 100644
> >>>>>>> --- a/winsup/cygwin/fhandler_pipe.cc
> >>>>>>> +++ b/winsup/cygwin/fhandler_pipe.cc
> >>>>>>> @@ -348,8 +348,13 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
> >>>>>>> CloseHandle (evt);
> >>>>>>> if (status == STATUS_THREAD_SIGNALED)
> >>>>>>> {
> >>>>>>> - set_errno (EINTR);
> >>>>>>> - len = (size_t) -1;
> >>>>>>> + if (total_len == 0)
> >>>>>>> + {
> >>>>>>> + set_errno (EINTR);
> >>>>>>> + len = (size_t) -1;
> >>>>>>> + }
> >>>>>>> + else
> >>>>>>> + len = total_len;
> >>>>>>> }
> >>>>>>> else if (status == STATUS_THREAD_CANCELED)
> >>>>>>> pthread::static_cancel_self ();
> >>>>>>
> >>>>>> Thanks for your advice. I fixed the issue and attached new patch.
> >>>>>>
> >>>>>> On Fri, 3 Sep 2021 17:37:13 +0200
> >>>>>> Corinna Vinschen wrote:
> >>>>>>> Hmm, I see the point, but we might have another problem with that.
> >>>>>>>
> >>>>>>> We can't keep the mutex while waiting on the pending read, and there
> >>>>>>> could be more than one pending read running at the time. if so,
> >>>>>>> chances are extremly high, that the data written to the buffer gets
> >>>>>>> split like this:
> >>>>>>>
> >>>>>>> reader 1 reader 2
> >>>>>>>
> >>>>>>> calls read(65536) calls read(65536)
> >>>>>>>
> >>>>>>> calls NtReadFile(16384 bytes)
> >>>>>>> calls NtReadFile(16384 bytes)
> >>>>>>>
> >>>>>>> writer writes 65536 bytes
> >>>>>>>
> >>>>>>> wakes up and gets 16384 bytes
> >>>>>>> wakes up and gets 16384 bytes
> >>>>>>> gets the mutex, calls
> >>>>>>> NtReadFile(32768) which
> >>>>>>> returns immediately with
> >>>>>>> 32768 bytes added to the
> >>>>>>> caller's buffer.
> >>>>>>>
> >>>>>>> so the buffer returned to reader 1 is 49152 bytes, with 16384 bytes
> >>>>>>> missing in the middle of it, *without* the reader knowing about that
> >>>>>>> fact. If reader 1 gets the first 16384 bytes, the 16384 bytes have
> >>>>>>> been read in a single call, at least, so the byte order is not
> >>>>>>> unknowingly broken on the application level.
> >>>>>>>
> >>>>>>> Does that make sense?
> >>>>>>
> >>>>>> Why can't we keep the mutex while waiting on the pending read?
> >>>>>> If we can keep the mutex, the issue above mentioned does not
> >>>>>> happen, right?
> >>>>>>
> >>>>>> What about the patch attached? This keeps the mutex while read()
> >>>>>> but I do not see any defects so far.
> >>>>
> >>>> LGTM.
> >>>>
> >>>> If Corinna agrees, I have a couple of suggestions.
> >>>>
> >>>> 1. With this patch, we can no longer have more than one pending ReadFile. So
> >>>> there's no longer a need to count read handles, and the problem with select is
> >>>> completely fixed as long as the number of bytes requested is less than the pipe
> >>>> buffer size.
> >>>>
> >>>> 2. raw_read is now reading in chunks, like raw_write. For readability of the
> >>>> code, I think it would be better to make the two functions as similar as
> >>>> possible. For example, you could replace the do/while loop by a
> >>>> while(total_len<orig_len) loop. And you could even use similar names for the
> >>>> variables, e.g., nbytes instead of total_len, or vice versa.
> >>>
> >>> Thanks for the suggestion. I have rebuilt the patch.
> >>> Please see the patch attached.
> >>
> >> This patch seems to fail to adopt to current git head of topic/pipe
> >> branch. I rebuilt the patch to fit current top/pipe.
> >>
> >> Please see the patch attached.
> >
> > Small typo.
> >
> > - pipe buffer size. Every pending read lowers WriteQuotaAvailable
> > + pipe buffer size. pending read lowers WriteQuotaAvailable
> >
> > should be:
> >
> > - pipe buffer size. Every pending read lowers WriteQuotaAvailable
> > + pipe buffer size. Pending read lowers WriteQuotaAvailable
>
> The patch looks great to me. Two minor nits:
>
> 1. The patch doesn't apply cleanly. Could you rebase it against the current
> HEAD of topic/pipe?
>
> 2. There's no need for chunk to be less than the number of bytes requested if we
> know there's data in the pipe. So maybe something like this (untested) would be
> better:
>
> ULONG chunk;
> status = NtQueryInformationFile (get_handle (), &io,
> &fpli, sizeof (fpli),
> FilePipeLocalInformation);
> if (NT_SUCCESS (status))
> {
> if (fpli.ReadDataAvailable > 0)
> chunk = left;
> else if (nbytes != 0)
> break;
> else
> chunk = fpli.InboundQuota / 2;
> }
> else if (nbytes != 0)
> break;
> else
> chunk = max_atomic_write / 2;
>
> if (chunk < left)
> len1 = chunk;
Thanks for the advice.
As for 1., is not the current git head 866a62037e235d558584e821a11d60d848e06234?
In my environment, patch can apply cleanly by git am.
--
Takashi Yano <takashi.yano@nifty.ne.jp>
More information about the Cygwin-developers
mailing list