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
0001-ssh-improve-the-logic-for-detecting-blocking-directi.patch
Description: application/mbox
------------------------------------------------------------------- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
