On Tuesday, October 08, 2013 14:01:58 Kamil Dudka wrote:
> On Monday, October 07, 2013 22:19:19 Daniel Stenberg wrote:
> > On Mon, 7 Oct 2013, Kamil Dudka wrote:
> > > ... if there is nothing to wait for. This fixes a regression introduced
> > > by
> > > commit 0feeab78 limiting the speed of SCP upload to 16384 B/s on a fast
> > > connection (such as localhost).
> >
> > Thanks for the patch. I'm just a little concerned that this fix corrects
> > the symptom rather than actually addresses the underlying problem.
>
> Thanks for the reply. I hoped the patch would help to understand the issue.
> > Let me explain what I suspect and once given some research we can decide
> > on
> > how to act on it.
> >
> > The change 0feeab78 you mention was a change that would prevent libcurl
> > from busy-looping like crazy when it found nothing to wait on. (That code
> > was later adjusted by d529f3882b.)
> >
> > The key here is the "nothing to wait on" part. When there's a transfer
> > going on, even SCP, there should be a file descriptor to wait for[*] and
> > the previously mentioned logic should not kick in. If that logic works, I
> > don't think this patch is necessary.
> >
> > Since your patch clearly helps your case (as otherwise you wouldn't have
> > posted it)
>
> Indeed. The patch increased the SCP upload speed from ~16 KB/s to ~48 MB/s,
> so we are definitely not dealing with a micro-optimization :-)
>
> > I can only deduct that curl_multi_wait() in this case returns a
> > zero in its fifth argument to signal that there's no file descriptor to
> > wait for, and yet there should be as there's a transfer in progress!
> >
> > Did I understand this correctly?
>
> Exactly!
>
> > If so, we need to dig further why libcurl wrongly drops the file
> > descriptor, right?
>
> Then the problem seems to be in ssh_block2waitfor() as it explicitly clears
> conn->waitfor unless it gets EAGAIN from the libssh2 layer. What is the
> purpose of this logic? Perhaps a left-over from the legacy easy interface?
>
> The logic pretends to work for SFTP, where each packet gets acknowledged by
> server, which likely results in EAGAIN returned to the libcurl level in each
> iteration. In case of SCP, libssh2 successfully writes the packet
> _without_ getting EAGAIN in each iteration. Consequently, conn->waitfor
> never gets a non-zero value, curl_multi_wait() keeps returning zero file
> descriptors, and the value without_fds in easy_transfer() keeps
> incrementing indefinitely.
> > [*] = it will also return faster when there's a timeout that has expired
> > but that shouldn't happen in multiple fast invokes so I don't think
> > that's what you see happening.
>
> Correct.

The attached patch, restricted to the ssh.c module, appears to fix the problem
for me. Is it the (more) correct way to address the issue?

Kamil
Received on 2013-10-08