Patchwork [v6,00/12] curl: fix curl read

login
register
mail settings
Submitter Richard W.M. Jones
Date May 28, 2013, 11:32 a.m.
Message ID <20130528113215.GD5105@redhat.com>
Download mbox | patch
Permalink /patch/246839/
State New
Headers show

Comments

Richard W.M. Jones - May 28, 2013, 11:32 a.m.
This fixes the obvious bug.

I wonder if it should be even larger?  One use for curl is to install
guests using ISOs from websites without having to download the ISO,
and I imagine that even a 30 second timeout could be conservative for
that task.

Rich.
Fam Zheng - May 29, 2013, 1:07 a.m.
On Tue, 05/28 12:32, Richard W.M. Jones wrote:
> 
> This fixes the obvious bug.

Thanks for figuring out this. Mainline had this 5s timeout so I kept it,
but you don't experience this bug, right? Since master doesn't setup a
timer to get curl notified about the timing, the option is just not
effective.

> I wonder if it should be even larger?  One use for curl is to install
> guests using ISOs from websites without having to download the ISO,
> and I imagine that even a 30 second timeout could be conservative for
> that task.
> 

Long latency network is common in practice, as well as low bandwidth,
the meaning of the timeout is to complete the request, in extreme cases
if it is a 1Kbps link, downloading 256k takes minutes. Anyway, I think
making it larger won't hurt.
Richard W.M. Jones - May 29, 2013, 9:25 a.m.
On Wed, May 29, 2013 at 09:07:25AM +0800, Fam Zheng wrote:
> On Tue, 05/28 12:32, Richard W.M. Jones wrote:
> > 
> > This fixes the obvious bug.
> 
> Thanks for figuring out this. Mainline had this 5s timeout so I kept it,
> but you don't experience this bug, right? Since master doesn't setup a
> timer to get curl notified about the timing, the option is just not
> effective.

Indeed, qemu master has:

    curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, 5);

but I don't encounter the bug when using master, and I'm pretty
certain about that because I've tested it a lot.

It could be that qemu master manages to recover from / restart these
incomplete reads, and doesn't deliver EIO up to the guest.

> > I wonder if it should be even larger?  One use for curl is to install
> > guests using ISOs from websites without having to download the ISO,
> > and I imagine that even a 30 second timeout could be conservative for
> > that task.
> > 
> 
> Long latency network is common in practice, as well as low bandwidth,
> the meaning of the timeout is to complete the request, in extreme cases
> if it is a 1Kbps link, downloading 256k takes minutes. Anyway, I think
> making it larger won't hurt.

I tried playing around with timeouts yesterday.

Inside the guest there is a SCSI timeout (30 seconds default, see
[1]).  This has to be made larger if we make the qemu timeout larger
than 30 seconds.

With upstream qemu I can increase this timeout to 180 seconds and so
read ISOs from slow public websites.  (Try changing the test script so
the 'disk' variable points to an ISO such as [2]).

With the v6 patch, adjusting the SCSI timeout in the guest doesn't
have any effect -- the SCSI disk "aborts" after ~40 seconds whatever I
do.

Rich.

[1] http://kb.vmware.com/kb/1009465
[2] http://mirrorservice.org/sites/dl.fedoraproject.org/pub/fedora/linux/releases/18/Live/x86_64/

Patch

diff --git a/block/curl.c b/block/curl.c
index 3e330b6..759f7cb 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -332,7 +332,7 @@  static CURLState *curl_init_state(BDRVCURLState *s)
         goto out;
     }
     curl_easy_setopt(state->curl, CURLOPT_URL, s->url);
-    curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, 5);
+    curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, 30);
     curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION, (void *)curl_read_cb);
     curl_easy_setopt(state->curl, CURLOPT_WRITEDATA, (void *)state);
     curl_easy_setopt(state->curl, CURLOPT_PRIVATE, (void *)state);