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

[Corinna Vinschen]

On Sep  6 20:16, Takashi Yano wrote:
> Hi Corinna,
> 
> On Mon, 6 Sep 2021 10:13:10 +0200
> Corinna Vinschen wrote:
> > If you have multiple readers, all but one of them will hang in this
> > WFSO.  They will block here without a chance to kill or Ctrl-C them
> > and thread cancellation won't work.
> > 
> > To fix that you have to use cygwait and handle signals and thread
> > cancellation the same way as in the below code following the NtReadFile.
> 
> OK. Thanks for the advice. Then what about the patch attached?
> 
> > >  	  /* If the pipe is empty, don't request more bytes than half the
> > > -	     pipe buffer size.  Every pending read lowers WriteQuotaAvailable
> > > -	     on the write side and thus affects select's ability to return
> > > -	     more or less reliable info whether a write succeeds or not.
> > > -
> > > -	     Let the size of the request depend on the number of readers
> > > -	     at the time. */
> > > +	     pipe buffer size. Pending read lowers WriteQuotaAvailable on
> > > +	     the write side and thus affects select's ability to return
> > > +	     more or less reliable info whether a write succeeds or not. */
> > > +	  ULONG chunk = max_atomic_write / 2;
> > >  	  status = NtQueryInformationFile (get_handle (), &io,
> > >  					   &fpli, sizeof (fpli),
> > >  					   FilePipeLocalInformation);
> > > -	  if (NT_SUCCESS (status) && fpli.ReadDataAvailable == 0)
> > 
> > If the readers are serialized anyway, why fetch only half the remaining
> > buffer size?  In that case fetching fpli.InboundQuota - 1 is as good
> > as fetching just the half of it, isn't it?
> 
> It sounds reasonable. Adopted in the attached patch.

Patch looks ok.

I added one more patches:

- It occured to me that the code is lacking a CancelIo in case cygwait
  is waiting for NtReadFile/NtWriteFile.  Actually, calling cygwait
  without "mask" parameter will result in cygwait performing the thread
  cancellation by itself, but cancelling a thread does not cancel the
  async IO started by that thread.  So I fixed the cygwait calls in
  raw_read/raw_write to return to the caller and then call CancelIo
  before cancelling the thread.

I planned to push one more patch:

- Drop max_atomic_write, it's == DEFAULT_PIPEBUFSIZE anyway

But then some things were coming to mind, which we still have to discuss.

- I think setting chunk to DEFAULT_PIPEBUFSIZE - 1 in the read case and
  DEFAULT_PIPEBUFSIZE in the write case by default is dangerous.
  Assuming the pipe has been created by a non-Cygwin process, the values
  may be way too high.

  Suggestion: Actually set max_atomic_write to something useful.
  Set max_atomic_write to DEFAULT_PIPEBUFSIZE in fhandler_pipe::create.
  In case of stdio handles inherited from non-Cygwin processes, fetch
  the pipe buffer size via NtQueryInformationFile in
  dtable::init_std_file_from_handle().  Better, in a matching
  fhandler_pipe method called from init_std_file_from_handle().

- What about calling select for writing on pipes read by non-Cygwin
  processes?  In that case, we still can't rely on WriteQuotaAvailable,
  just as before.

  I have a vague idea that we might want to count readers in that case,
  but I have to think about it some more.


Corinna