All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Richard W.M. Jones" <rjones@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: qemu-devel@nongnu.org, jcody@redhat.com, qemu-stable@nongnu.org,
	qemu-block@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll
Date: Wed, 10 May 2017 16:57:00 +0100	[thread overview]
Message-ID: <20170510155700.GY16511@redhat.com> (raw)
In-Reply-To: <20170510143205.32013-1-pbonzini@redhat.com>

On Wed, May 10, 2017 at 04:31:58PM +0200, Paolo Bonzini wrote:
> Since the last patch in v1 didn't work, I bit the bullet and converted
> the whole thing to coroutines (patches 4-6).  This in turns allows a more
> elegant solution to wait for CURLStates to get free (patch 7).
> 
> I tested this by lowering CURL_NUM_STATES to 2.  With this change, the
> buggy case triggers a couple times while booting a Fedora netinst image.

This series fixes the original bug, so:

  Tested-by: Richard W.M. Jones <rjones@redhat.com>

I think the Reported-by in patch 3 should credit Kun Wei for finding
the bug, and we should probably mention the BZ too:

  Reported-by: Kun Wei <kuwei@redhat.com>
  Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1447590

A nit pick perhaps but in patch 5 you say "This was broken before for
disks > 2TB, but now it would break at 4GB.".
I understand after reading it a few times that you mean it would be
broken at 4GB, if you hadn't changed size_t -> uint64_t (on 32 bit
platforms).  Perhaps better to clarify that sentence.

---

I also ran some performance and stability testing.  I used virt-ls for
this.  The following command will iterate over every file in a remote
guest image and print an md5sum:

  LIBGUESTFS_BACKEND=direct \
  LIBGUESTFS_HV=~/d/qemu/x86_64-softmmu/qemu-system-x86_64 \
  virt-ls -a http://somehost/rhel-guest-image-7.1-20150224.0.x86_64.qcow2 \
          -lR --checksum /

I timed this with and without your patches, but there was no
significant difference (but note that virt-ls is a fundamentally
sequential program).

It didn't crash or hang at any time during my testing.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

  parent reply	other threads:[~2017-05-10 15:57 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-10 14:31 [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Paolo Bonzini
2017-05-10 14:31 ` [Qemu-devel] [PATCH 1/7] curl: strengthen assertion in curl_clean_state Paolo Bonzini
2017-05-10 16:33   ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-11 20:35   ` [Qemu-devel] " Jeff Cody
2017-05-10 14:32 ` [Qemu-devel] [PATCH 2/7] curl: never invoke callbacks with s->mutex held Paolo Bonzini
2017-05-10 16:33   ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-11 20:40   ` [Qemu-devel] " Jeff Cody
2017-05-10 14:32 ` [Qemu-devel] [PATCH 3/7] curl: avoid recursive locking of BDRVCURLState mutex Paolo Bonzini
2017-05-10 16:38   ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-11 20:56   ` [Qemu-devel] " Jeff Cody
2017-05-12 14:48     ` Paolo Bonzini
2017-05-10 14:32 ` [Qemu-devel] [PATCH 4/7] curl: split curl_find_state/curl_init_state Paolo Bonzini
2017-05-10 17:26   ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-11 13:49     ` [Qemu-devel] " Paolo Bonzini
2017-05-12 21:38   ` Jeff Cody
2017-05-10 14:32 ` [Qemu-devel] [PATCH 5/7] curl: convert CURLAIOCB to byte values Paolo Bonzini
2017-05-10 17:36   ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-10 18:37     ` Eric Blake
2017-05-12 21:38   ` [Qemu-devel] " Jeff Cody
2017-05-10 14:32 ` [Qemu-devel] [PATCH 6/7] curl: convert readv to coroutines Paolo Bonzini
2017-05-12 21:40   ` Jeff Cody
2017-05-10 14:32 ` [Qemu-devel] [PATCH 7/7] curl: do not do aio_poll when waiting for a free CURLState Paolo Bonzini
2017-05-10 17:54   ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-12 21:41   ` [Qemu-devel] " Jeff Cody
2017-05-10 15:11 ` [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll no-reply
2017-05-10 15:57 ` Richard W.M. Jones [this message]
2017-05-15 19:12   ` [Qemu-devel] [Qemu-block] " Max Reitz
2017-05-15 20:30     ` Richard W.M. Jones

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170510155700.GY16511@redhat.com \
    --to=rjones@redhat.com \
    --cc=jcody@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-stable@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.