Re: [Qemu-block] [Qemu-devel] [PATCH v5 0/3] file-posix: Simplifications on image locking
On Thu, 10/11 15:21, Fam Zheng wrote: > v5: Address Max's comments (Thanks for reviewing): > - Clean up after test done. > - Add rev-by to patch 1 and 2. Ping? Fam
Re: [Qemu-block] [PATCH] tests: Fix Python 3 detection on older GNU make versions
On 11/7/18 1:05 AM, Markus Armbruster wrote: > Eduardo Habkost writes: > >> The $(SHELLSTATUS) variable requires GNU make >= 4.2, but Travis >> seems to provide an older version. Change the existing rules to >> use command output instead of exit code, to make it compatible >> with older GNU make versions. >> >> Signed-off-by: Eduardo Habkost >> --- >> I think that's the cause of the Travis failures. I have >> submitted a test job right now, at: >> https://travis-ci.org/ehabkost/qemu-hacks/jobs/451387962 >> Let's see if it fixes the issue. >> --- >> tests/Makefile.include | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/tests/Makefile.include b/tests/Makefile.include >> index d2e577eabb..074eece558 100644 >> --- a/tests/Makefile.include >> +++ b/tests/Makefile.include >> @@ -913,8 +913,8 @@ TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results >> # information please refer to "avocado --help". >> AVOCADO_SHOW=none >> >> -$(shell $(PYTHON) -c 'import sys; assert sys.version_info >= (3,0)' >> >/dev/null 2>&1) >> -ifeq ($(.SHELLSTATUS),0) >> +PYTHON3 = $(shell $(PYTHON) -c 'import sys; print(1 if sys.version_info >= >> (3, 0) else 0)') >> +ifeq ($(PYTHON3), 1) >> $(TESTS_VENV_DIR): $(TESTS_VENV_REQ) >> $(call quiet-command, \ >> $(PYTHON) -m venv --system-site-packages $@, \ > > PEP 394 recommends software distributions install Python 3 into the > default path as python3, and users use that instead of python, except > for programs that are source compatible with both 2 and 3. So, is > finding out whether python is a Python 3 really appropriate? Why can't > we just use python3 and be done with it? > I mentioned that before, when you pointed out the issue you fix here, that configure may be the best place to get the Python version (not only the major version) and make it available elsewhere. Even if not used for other purposes, it is IMO important information to show on the resulting "configure" output. I'm sending patches to do that in a few. > If we can't: isn't this a configure problem? > I believe adhering to PEP394 is a good thing, but even that document recognizes that the real world is not a perfect place: "however, end users should be aware that python refers to python3 on at least Arch Linux". And it only covers *nix systems, so if we hope to care for non-*nix systems, we have to check the Python version manually. So, I guess the safest approach from QEMU's side is to check for the version indeed. - Cleber.
Re: [Qemu-block] [PATCH 3/7] qcow2: split out reading normal clusters from qcow2_co_preadv
(Broken quoting in text/plain again)
Am 01.11.2018 um 13:17 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 27.09.2018 20:35, Max Reitz wrote:
>
> On 07.08.18 19:43, Vladimir Sementsov-Ogievskiy wrote:
>
> Memory allocation may become less efficient for encrypted case. It's a
> payment for further asynchronous scheme.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/qcow2.c | 114
> --
> 1 file changed, 64 insertions(+), 50 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 65e3ca00e2..5e7f2ee318 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1808,6 +1808,67 @@ out:
> return ret;
> }
>
> +static coroutine_fn int qcow2_co_preadv_normal(BlockDriverState *bs,
> + uint64_t
> file_cluster_offset,
> + uint64_t offset,
> + uint64_t bytes,
> + QEMUIOVector *qiov,
> + uint64_t qiov_offset)
> +{
> +int ret;
> +BDRVQcow2State *s = bs->opaque;
> +void *crypt_buf = NULL;
> +QEMUIOVector hd_qiov;
> +int offset_in_cluster = offset_into_cluster(s, offset);
> +
> +if ((file_cluster_offset & 511) != 0) {
> +return -EIO;
> +}
> +
> +qemu_iovec_init(_qiov, qiov->niov);
>
> So you're not just re-allocating the bounce buffer for every single
> call, but also this I/O vector. Hm.
And this one is actually at least a little more serious, I think.
Decryption and decompression (including copying between the original
qiov and the bounce buffer) are already very slow, so allocating a
bounce buffer or not shouldn't make any noticable difference.
But I'd like to keep allocations out of the fast path as much as we can.
> +if (bs->encrypted) {
> +assert(s->crypto);
> +assert(bytes <= QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size);
> +
> +crypt_buf = qemu_try_blockalign(bs->file->bs, bytes);
>
> 1. Why did you remove the comment?
>
> 2. The check for whether crypt_buf was successfully allocated is missing.
>
> 3. Do you have any benchmarks for encrypted images? Having to allocate
> the bounce buffer for potentially every single cluster sounds rather bad
> to me.
Actually, benchmarks for normal, fully allocated images would be a
little more interesting. Though I'm not sure if qcow2 actually performs
well enough that we'll see any difference even there (when we measured
memory allocation overhead etc. for raw images in the context of
comparing coroutines to AIO callback styes, the difference was already
hard to see).
Kevin
Re: [Qemu-block] Change in qemu 2.12 causes qemu-img convert to NBD to write more data
On Wed, Nov 7, 2018 at 7:27 PM Kevin Wolf wrote: > Am 07.11.2018 um 15:56 hat Nir Soffer geschrieben: > > Wed, Nov 7, 2018 at 4:36 PM Richard W.M. Jones > wrote: > > > > > Another thing I tried was to change the NBD server (nbdkit) so that it > > > doesn't advertise zero support to the client: > > > > > > $ nbdkit --filter=log --filter=nozero memory size=6G > logfile=/tmp/log \ > > > --run './qemu-img convert ./fedora-28.img -n $nbd' > > > $ grep '\.\.\.$' /tmp/log | sed 's/.*\([A-Z][a-z]*\).*/\1/' | uniq -c > > >2154 Write > > > > > > Not surprisingly no zero commands are issued. The size of the write > > > commands is very uneven -- it appears to be send one command per block > > > of zeroes or data. > > > > > > Nir: If we could get information from imageio about whether zeroing is > > > implemented efficiently or not by the backend, we could change > > > virt-v2v / nbdkit to advertise this back to qemu. > > > > There is no way to detect the capability, ioctl(BLKZEROOUT) always > > succeeds, falling back to manual zeroing in the kernel silently > > > > Even if we could, sending zero on the wire from qemu may be even > > slower, and it looks like qemu send even more requests in this case > > (2154 vs ~1300). > > > > Looks like this optimization in qemu side leads to worse performance, > > so it should not be enabled by default. > > Well, that's overgeneralising your case a bit. If the backend does > support efficient zero writes (which file systems, the most common case, > generally do), doing one big write_zeroes request at the start can > improve performance quite a bit. > > It seems the problem is that we can't really know whether the operation > will be efficient because the backends generally don't tell us. Maybe > NBD could introduce a flag for this, but in the general case it appears > to me that we'll have to have a command line option. > > However, I'm curious what your exact use case and the backend used in it > is? Can something be improved there to actually get efficient zero > writes and get even better performance than by just disabling the big > zero write? The backend is some NetApp storage connected via FC. I don't have more info on this. We get zero rate of about 1G/s on this storage, which is quite slow compared with other storage we tested. One option we check now is if this is the kernel silent fallback to manual zeroing when the server advertise wrong value of write_same_max_bytes. Having a command line option to control this behavior sounds good. I don't have enough data to tell what should be the default, but I think the safe way would be to keep old behavior. Nir
Re: [Qemu-block] Change in qemu 2.12 causes qemu-img convert to NBD to write more data
Am 07.11.2018 um 15:56 hat Nir Soffer geschrieben: > Wed, Nov 7, 2018 at 4:36 PM Richard W.M. Jones wrote: > > > Another thing I tried was to change the NBD server (nbdkit) so that it > > doesn't advertise zero support to the client: > > > > $ nbdkit --filter=log --filter=nozero memory size=6G logfile=/tmp/log \ > > --run './qemu-img convert ./fedora-28.img -n $nbd' > > $ grep '\.\.\.$' /tmp/log | sed 's/.*\([A-Z][a-z]*\).*/\1/' | uniq -c > >2154 Write > > > > Not surprisingly no zero commands are issued. The size of the write > > commands is very uneven -- it appears to be send one command per block > > of zeroes or data. > > > > Nir: If we could get information from imageio about whether zeroing is > > implemented efficiently or not by the backend, we could change > > virt-v2v / nbdkit to advertise this back to qemu. > > There is no way to detect the capability, ioctl(BLKZEROOUT) always > succeeds, falling back to manual zeroing in the kernel silently > > Even if we could, sending zero on the wire from qemu may be even > slower, and it looks like qemu send even more requests in this case > (2154 vs ~1300). > > Looks like this optimization in qemu side leads to worse performance, > so it should not be enabled by default. Well, that's overgeneralising your case a bit. If the backend does support efficient zero writes (which file systems, the most common case, generally do), doing one big write_zeroes request at the start can improve performance quite a bit. It seems the problem is that we can't really know whether the operation will be efficient because the backends generally don't tell us. Maybe NBD could introduce a flag for this, but in the general case it appears to me that we'll have to have a command line option. However, I'm curious what your exact use case and the backend used in it is? Can something be improved there to actually get efficient zero writes and get even better performance than by just disabling the big zero write? Kevin
Re: [Qemu-block] Change in qemu 2.12 causes qemu-img convert to NBD to write more data
On 11/7/18 6:13 AM, Richard W.M. Jones wrote: (I'm not going to claim this is a bug, but it causes a large, easily measurable performance regression in virt-v2v). I haven't closely looked at at this email thread yet, but a quick first impression: In qemu 2.12 this behaviour changed: $ nbdkit --filter=log memory size=6G logfile=/tmp/log \ --run './qemu-img convert ./fedora-28.img -n $nbd' $ grep '\.\.\.$' /tmp/log | sed 's/.*\([A-Z][a-z]*\).*/\1/' | uniq -c 193 Zero 1246 Write It now zeroes the whole disk up front and then writes data over the top of the zeroed blocks. The reason for the performance regression is that in the first case we write 6G in total. In the second case we write 6G of zeroes up front, followed by the amount of data in the disk image (in this case the test disk image contains 1G of non-sparse data, so we write about 7G in total). There was talk on the NBD list a while ago about the idea of letting the server advertise to the client when the image is known to start in an all-zero state, so that the client doesn't have to waste time writing zeroes (or relying on repeated NBD_CMD_BLOCK_STATUS calls to learn the same). This may be justification for reviving that topic. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-block] [Qemu-devel] [PATCH] tests: Fix Python 3 detection on older GNU make versions
Eduardo Habkost writes: > On Wed, Nov 07, 2018 at 01:45:35PM +, Peter Maydell wrote: >> On 7 November 2018 at 12:49, Eduardo Habkost wrote: >> > Now, why do we need --with-python, and why do we need to use >> > $(PYTHON) when running tests? If somebody wants to use a >> > different Python binary when running tests, they can already use >> > $PATH for that. >> > >> > (That's the same argument I used for iotests a while ago: >> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg566631.html) >> >> I'm not a great fan of requiring the user to mess with their PATH >> to get configure to work. Also, the first python on the path >> might be the wrong one, and we don't pass PATH from configure >> to make so you end up having to make sure you specify it >> right in both places. > > You're assuming that this will actually require some people to > mess with their $PATH because they currently don't have Python on > their $PATH. I don't see any evidence that this is expected to > happen. Do you? What makes Python so special we must provide special means to find it off the PATH? Why not other tools? >> Plus we already have --with-python, so if you want to drop >> it you need to deprecate it first, and you need a justification >> that's strong enough to outweigh breaking users' existing >> build/packaging setups and scripts... > > I would really like to remove the option as soon as we start > requiring Python 3. Let's stop reinventing solutions to problems > already addressed by PEP 394. Concur. We should use python3 wherever we need a Python 3. There's no point in letting configure check whether python3 is actually Python 3. We should use python2 wherever we need a Python 2. This case needs to go away (if it isn't gone already). We may use any of python3, python, python2 wherever we can work with both Python 3 and 2. Simply using python3 works on sufficiently recent hosts. For maximum portability, we can let configure try python3, then python. I wouldn't bother trying python2 as well.
Re: [Qemu-block] [PATCH] tests: Fix Python 3 detection on older GNU make versions
On Wed, Nov 07, 2018 at 01:45:35PM +, Peter Maydell wrote: > On 7 November 2018 at 12:49, Eduardo Habkost wrote: > > Now, why do we need --with-python, and why do we need to use > > $(PYTHON) when running tests? If somebody wants to use a > > different Python binary when running tests, they can already use > > $PATH for that. > > > > (That's the same argument I used for iotests a while ago: > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg566631.html) > > I'm not a great fan of requiring the user to mess with their PATH > to get configure to work. Also, the first python on the path > might be the wrong one, and we don't pass PATH from configure > to make so you end up having to make sure you specify it > right in both places. You're assuming that this will actually require some people to mess with their $PATH because they currently don't have Python on their $PATH. I don't see any evidence that this is expected to happen. Do you? > > Plus we already have --with-python, so if you want to drop > it you need to deprecate it first, and you need a justification > that's strong enough to outweigh breaking users' existing > build/packaging setups and scripts... I would really like to remove the option as soon as we start requiring Python 3. Let's stop reinventing solutions to problems already addressed by PEP 394. -- Eduardo
Re: [Qemu-block] Change in qemu 2.12 causes qemu-img convert to NBD to write more data
On Wed, Nov 07, 2018 at 04:56:48PM +0200, Nir Soffer wrote: > Wed, Nov 7, 2018 at 4:36 PM Richard W.M. Jones wrote: > > > Another thing I tried was to change the NBD server (nbdkit) so that it > > doesn't advertise zero support to the client: > > > > $ nbdkit --filter=log --filter=nozero memory size=6G logfile=/tmp/log \ > > --run './qemu-img convert ./fedora-28.img -n $nbd' > > $ grep '\.\.\.$' /tmp/log | sed 's/.*\([A-Z][a-z]*\).*/\1/' | uniq -c > >2154 Write > > > > Not surprisingly no zero commands are issued. The size of the write > > commands is very uneven -- it appears to be send one command per block > > of zeroes or data. > > > > Nir: If we could get information from imageio about whether zeroing is > > implemented efficiently or not by the backend, we could change > > virt-v2v / nbdkit to advertise this back to qemu. > > > > There is no way to detect the capability, ioctl(BLKZEROOUT) always > succeeds, falling back to manual zeroing in the kernel silently > > Even if we could, sending zero on the wire from qemu may be even > slower, Yes this is a very good point. Sending zeroes would be terrible. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
Re: [Qemu-block] Change in qemu 2.12 causes qemu-img convert to NBD to write more data
Wed, Nov 7, 2018 at 4:36 PM Richard W.M. Jones wrote: > Another thing I tried was to change the NBD server (nbdkit) so that it > doesn't advertise zero support to the client: > > $ nbdkit --filter=log --filter=nozero memory size=6G logfile=/tmp/log \ > --run './qemu-img convert ./fedora-28.img -n $nbd' > $ grep '\.\.\.$' /tmp/log | sed 's/.*\([A-Z][a-z]*\).*/\1/' | uniq -c >2154 Write > > Not surprisingly no zero commands are issued. The size of the write > commands is very uneven -- it appears to be send one command per block > of zeroes or data. > > Nir: If we could get information from imageio about whether zeroing is > implemented efficiently or not by the backend, we could change > virt-v2v / nbdkit to advertise this back to qemu. > There is no way to detect the capability, ioctl(BLKZEROOUT) always succeeds, falling back to manual zeroing in the kernel silently Even if we could, sending zero on the wire from qemu may be even slower, and it looks like qemu send even more requests in this case (2154 vs ~1300). Looks like this optimization in qemu side leads to worse performance, so it should not be enabled by default. Nir
Re: [Qemu-block] Change in qemu 2.12 causes qemu-img convert to NBD to write more data
Another thing I tried was to change the NBD server (nbdkit) so that it doesn't advertise zero support to the client: $ nbdkit --filter=log --filter=nozero memory size=6G logfile=/tmp/log \ --run './qemu-img convert ./fedora-28.img -n $nbd' $ grep '\.\.\.$' /tmp/log | sed 's/.*\([A-Z][a-z]*\).*/\1/' | uniq -c 2154 Write Not surprisingly no zero commands are issued. The size of the write commands is very uneven -- it appears to be send one command per block of zeroes or data. Nir: If we could get information from imageio about whether zeroing is implemented efficiently or not by the backend, we could change virt-v2v / nbdkit to advertise this back to qemu. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
Re: [Qemu-block] [PATCH 3/7] qcow2: split out reading normal clusters from qcow2_co_preadv
On 01.11.18 13:17, Vladimir Sementsov-Ogievskiy wrote:
> 27.09.2018 20:35, Max Reitz wrote:
>> On 07.08.18 19:43, Vladimir Sementsov-Ogievskiy wrote:
>>> Memory allocation may become less efficient for encrypted case. It's a
>>> payment for further asynchronous scheme.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>> ---
>>> block/qcow2.c | 114
>>> --
>>> 1 file changed, 64 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 65e3ca00e2..5e7f2ee318 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -1808,6 +1808,67 @@ out:
>>> return ret;
>>> }
>>>
>>> +static coroutine_fn int qcow2_co_preadv_normal(BlockDriverState *bs,
>>> + uint64_t
>>> file_cluster_offset,
>>> + uint64_t offset,
>>> + uint64_t bytes,
>>> + QEMUIOVector *qiov,
>>> + uint64_t qiov_offset)
>>> +{
>>> +int ret;
>>> +BDRVQcow2State *s = bs->opaque;
>>> +void *crypt_buf = NULL;
>>> +QEMUIOVector hd_qiov;
>>> +int offset_in_cluster = offset_into_cluster(s, offset);
>>> +
>>> +if ((file_cluster_offset & 511) != 0) {
>>> +return -EIO;
>>> +}
>>> +
>>> +qemu_iovec_init(_qiov, qiov->niov);
>> So you're not just re-allocating the bounce buffer for every single
>> call, but also this I/O vector. Hm.
>>
>>> +if (bs->encrypted) {
>>> +assert(s->crypto);
>>> +assert(bytes <= QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size);
>>> +
>>> +crypt_buf = qemu_try_blockalign(bs->file->bs, bytes);
>> 1. Why did you remove the comment?
>>
>> 2. The check for whether crypt_buf was successfully allocated is missing.
>>
>> 3. Do you have any benchmarks for encrypted images? Having to allocate
>> the bounce buffer for potentially every single cluster sounds rather bad
>> to me.
>
> Hmm, about this.
>
> Now we have compress threads to do several compress operations in
> parallel. And I plan to do the same thing for decompression, encryption
> and decryption. So, we'll definitely need several cluster-size buffers
> to do all these operations. How many? The first thought is just as many
> as maximum number of threads (or twice as many, to have in and out
> buffers for compression). But it's wrong, consider for example
> encryption on write:
>
> 1. get free thread for encryption (may be yield if maximum thread number
> achieved, waiting until all threads are busy)
> 2. get buffer (per thread)
> 3. thread handles encryption
> a. free thread here?
> 4. write encrypted data to underlying file
> b. free thread here?
>
> Of course, we want free thread as soon as possible, in "a." not in "b.".
> And this mean, that we should free buffer in "a." too, so we should copy
> data to locally allocated buffer, or, better, we just use local buffer
> from the beginning, and thread don't own it's own buffer..
>
> So, what are the options we have?
>
> 1. the most simple thing: just allocate buffers for each request
> locally, like it is already done for compression.
> pros: simple
> cons: allocate/free every time may influence performance, as you noticed
>
> 2. keep a pool of such buffers, so when buffer freed, it's just queued
> to the list of free buffers
> pros:
> reduce number of real alloc/free calls
> we can limit the pool maximum size
> we can allocate new buffers on demand, not the whole pool at start
> cons:
> hmm, so, it looks like custom allocator. Is it really needed? Is it
> really faster than just use system allocator and call alloc/free every
> time we need a buffer?
>
> 3. should not we use g_slice_alloc instead of inventing it in "2."
>
> So, I've decided to do some tests, and here are results (memcpy is for
> 32K, all other things allocates 64K in a loop, list_alloc is a simple
> realization of "2.":
>
> nothing: 2 it / 0.301361 sec = 663655696.202532 it/s
> memcpy: 200 it / 1.633015 sec = 1224728.554970 it/s
> g_malloc: 2 it / 5.346707 sec = 37406202.540530 it/s
> g_slice_alloc: 2 it / 7.812036 sec = 25601520.402792 it/s
> list_alloc: 2 it / 5.432771 sec = 36813626.268630 it/s
> posix_memalign: 2000 it / 1.025543 sec = 19501864.376084 it/s
> aligned_alloc: 2000 it / 1.035639 sec = 19311752.149739 it/s
>
> So, you see that yes, list_alloc is twice as fast as posix_memalign, but
> on the other hand, simple memcpy is a lot slower (16 times slower) (and
> I don't say about real disk IO which will be even more slower), so,
> should we care about allocation, what do you thing?
> test is attached.
Agreed. Thanks for testing.
I am a bit worried about the maximum memory usage still. With 16
workers, having bounce buffers can mean up to 32 MB of memory usage with
the default cluster size (and 1
Re: [Qemu-block] [PATCH] tests: Fix Python 3 detection on older GNU make versions
On 7 November 2018 at 12:49, Eduardo Habkost wrote: > Now, why do we need --with-python, and why do we need to use > $(PYTHON) when running tests? If somebody wants to use a > different Python binary when running tests, they can already use > $PATH for that. > > (That's the same argument I used for iotests a while ago: > https://www.mail-archive.com/qemu-devel@nongnu.org/msg566631.html) I'm not a great fan of requiring the user to mess with their PATH to get configure to work. Also, the first python on the path might be the wrong one, and we don't pass PATH from configure to make so you end up having to make sure you specify it right in both places. Plus we already have --with-python, so if you want to drop it you need to deprecate it first, and you need a justification that's strong enough to outweigh breaking users' existing build/packaging setups and scripts... thanks -- PMM
Re: [Qemu-block] [PATCH v2] block: Make more block drivers compile-time configurable
On 07.11.18 07:36, Markus Armbruster wrote: > From: Jeff Cody > > This adds configure options to control the following block drivers: > > * Bochs > * Cloop > * Dmg > * Qcow (V1) > * Vdi > * Vvfat > * qed > * parallels > * sheepdog > > Each of these defaults to being enabled. > > Signed-off-by: Jeff Cody > Signed-off-by: Markus Armbruster > --- > v2: Fix handling of dmg-bz2.o [Max] > > block/Makefile.objs | 22 --- > configure | 91 + > 2 files changed, 107 insertions(+), 6 deletions(-) Thanks, applied to my block branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block Max signature.asc Description: OpenPGP digital signature
[Qemu-block] [PATCH v4 13/15] block: Stop passing flags to bdrv_reopen_queue_child()
Now that all callers are passing the new options using the QDict we no
longer need the 'flags' parameter.
This patch makes the following changes:
1) The update_options_from_flags() call is no longer necessary
so it can be removed.
2) The update_flags_from_options() call is now used in all cases,
and is moved down a few lines so it happens after the options
QDict contains the final set of values.
3) The flags parameter is removed. Now the flags are initialized
using the current value (for the top-level node) or the parent
flags (after inherit_options()). In both cases the initial
values are updated to reflect the new options in the QDict. This
happens in bdrv_reopen_queue_child() (as explained above) and in
bdrv_reopen_prepare().
Signed-off-by: Alberto Garcia
---
block.c | 48 +++-
1 file changed, 19 insertions(+), 29 deletions(-)
diff --git a/block.c b/block.c
index 8b401e1cf4..8bc808d6f3 100644
--- a/block.c
+++ b/block.c
@@ -2912,7 +2912,6 @@ BlockDriverState *bdrv_open(const char *filename, const
char *reference,
static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
BlockDriverState *bs,
QDict *options,
- int flags,
const BdrvChildRole *role,
QDict *parent_options,
int parent_flags)
@@ -2921,7 +2920,9 @@ static BlockReopenQueue
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
BlockReopenQueueEntry *bs_entry;
BdrvChild *child;
-QDict *old_options, *explicit_options;
+QDict *old_options, *explicit_options, *options_copy;
+int flags;
+QemuOpts *opts;
/* Make sure that the caller remembered to use a drained section. This is
* important to avoid graph changes between the recursive queuing here and
@@ -2947,22 +2948,11 @@ static BlockReopenQueue
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
/*
* Precedence of options:
* 1. Explicitly passed in options (highest)
- * 2. Set in flags (only for top level)
- * 3. Retained from explicitly set options of bs
- * 4. Inherited from parent node
- * 5. Retained from effective options of bs
+ * 2. Retained from explicitly set options of bs
+ * 3. Inherited from parent node
+ * 4. Retained from effective options of bs
*/
-if (!parent_options) {
-/*
- * Any setting represented by flags is always updated. If the
- * corresponding QDict option is set, it takes precedence. Otherwise
- * the flag is translated into a QDict option. The old setting of bs is
- * not considered.
- */
-update_options_from_flags(options, flags);
-}
-
/* Old explicitly set values (don't overwrite by inherited value) */
if (bs_entry) {
old_options = qdict_clone_shallow(bs_entry->state.explicit_options);
@@ -2976,16 +2966,10 @@ static BlockReopenQueue
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
/* Inherit from parent node */
if (parent_options) {
-QemuOpts *opts;
-QDict *options_copy;
-assert(!flags);
+flags = 0;
role->inherit_options(, options, parent_flags, parent_options);
-options_copy = qdict_clone_shallow(options);
-opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
-qemu_opts_absorb_qdict(opts, options_copy, NULL);
-update_flags_from_options(, opts);
-qemu_opts_del(opts);
-qobject_unref(options_copy);
+} else {
+flags = bdrv_get_flags(bs);
}
/* Old values are used for options that aren't set yet */
@@ -2993,6 +2977,14 @@ static BlockReopenQueue
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
bdrv_join_options(bs, options, old_options);
qobject_unref(old_options);
+/* We have the final set of options so let's update the flags */
+options_copy = qdict_clone_shallow(options);
+opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
+qemu_opts_absorb_qdict(opts, options_copy, NULL);
+update_flags_from_options(, opts);
+qemu_opts_del(opts);
+qobject_unref(options_copy);
+
/* bdrv_open_inherit() sets and clears some additional flags internally */
flags &= ~BDRV_O_PROTOCOL;
if (flags & BDRV_O_RDWR) {
@@ -3032,7 +3024,7 @@ static BlockReopenQueue
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
qdict_extract_subqdict(options, _child_options, child_key_dot);
g_free(child_key_dot);
-bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options, 0,
+bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options,
[Qemu-block] [PATCH v4 12/15] block: Remove flags parameter from bdrv_reopen_queue()
Now that all callers are passing all flag changes as QDict options,
the flags parameter is no longer necessary, so we can get rid of it.
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
block.c | 5 +++--
block/replication.c | 6 ++
include/block/block.h | 3 +--
qemu-io-cmds.c| 2 +-
4 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/block.c b/block.c
index 31de7b2299..8b401e1cf4 100644
--- a/block.c
+++ b/block.c
@@ -3041,8 +3041,9 @@ static BlockReopenQueue
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
BlockDriverState *bs,
-QDict *options, int flags)
+QDict *options)
{
+int flags = bdrv_get_flags(bs);
return bdrv_reopen_queue_child(bs_queue, bs, options, flags,
NULL, NULL, 0);
}
@@ -3116,7 +3117,7 @@ int bdrv_reopen_set_read_only(BlockDriverState *bs, bool
read_only,
qdict_put_bool(opts, BDRV_OPT_READ_ONLY, read_only);
bdrv_subtree_drained_begin(bs);
-queue = bdrv_reopen_queue(NULL, bs, opts, bdrv_get_flags(bs));
+queue = bdrv_reopen_queue(NULL, bs, opts);
ret = bdrv_reopen_multiple(bdrv_get_aio_context(bs), queue, errp);
bdrv_subtree_drained_end(bs);
diff --git a/block/replication.c b/block/replication.c
index 481a225924..fdbfe47fa4 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -393,19 +393,17 @@ static void reopen_backing_file(BlockDriverState *bs,
bool writable,
bdrv_subtree_drained_begin(s->secondary_disk->bs);
if (s->orig_hidden_read_only) {
-int flags = bdrv_get_flags(s->hidden_disk->bs);
QDict *opts = qdict_new();
qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs,
- opts, flags);
+ opts);
}
if (s->orig_secondary_read_only) {
-int flags = bdrv_get_flags(s->secondary_disk->bs);
QDict *opts = qdict_new();
qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
reopen_queue = bdrv_reopen_queue(reopen_queue, s->secondary_disk->bs,
- opts, flags);
+ opts);
}
if (reopen_queue) {
diff --git a/include/block/block.h b/include/block/block.h
index de72c7a093..f70a843b72 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -299,8 +299,7 @@ BlockDriverState *bdrv_open(const char *filename, const
char *reference,
BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
int flags, Error **errp);
BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
-BlockDriverState *bs,
-QDict *options, int flags);
+BlockDriverState *bs, QDict *options);
int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error
**errp);
int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
Error **errp);
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index c9ae09d574..2c39124036 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -2075,7 +2075,7 @@ static int reopen_f(BlockBackend *blk, int argc, char
**argv)
}
bdrv_subtree_drained_begin(bs);
-brq = bdrv_reopen_queue(NULL, bs, opts, flags);
+brq = bdrv_reopen_queue(NULL, bs, opts);
bdrv_reopen_multiple(bdrv_get_aio_context(bs), brq, _err);
bdrv_subtree_drained_end(bs);
--
2.11.0
[Qemu-block] [PATCH v4 09/15] block: Drop bdrv_reopen()
No one is using this function anymore, so we can safely remove it.
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
block.c | 21 -
include/block/block.h | 1 -
2 files changed, 22 deletions(-)
diff --git a/block.c b/block.c
index cfa53f7114..31de7b2299 100644
--- a/block.c
+++ b/block.c
@@ -3106,27 +3106,6 @@ cleanup:
return ret;
}
-
-/* Reopen a single BlockDriverState with the specified flags. */
-int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
-{
-int ret = -1;
-Error *local_err = NULL;
-BlockReopenQueue *queue;
-
-bdrv_subtree_drained_begin(bs);
-
-queue = bdrv_reopen_queue(NULL, bs, NULL, bdrv_flags);
-ret = bdrv_reopen_multiple(bdrv_get_aio_context(bs), queue, _err);
-if (local_err != NULL) {
-error_propagate(errp, local_err);
-}
-
-bdrv_subtree_drained_end(bs);
-
-return ret;
-}
-
int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
Error **errp)
{
diff --git a/include/block/block.h b/include/block/block.h
index 382e6643fc..de72c7a093 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -302,7 +302,6 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue
*bs_queue,
BlockDriverState *bs,
QDict *options, int flags);
int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error
**errp);
-int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
Error **errp);
int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
--
2.11.0
[Qemu-block] [PATCH v4 06/15] block: Use bdrv_reopen_set_read_only() in qmp_change_backing_file()
This patch replaces the bdrv_reopen() calls that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
blockdev.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index e5b5eb46e2..20d1d84bab 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4100,7 +4100,6 @@ void qmp_change_backing_file(const char *device,
BlockDriverState *image_bs = NULL;
Error *local_err = NULL;
bool ro;
-int open_flags;
int ret;
bs = qmp_get_root_bs(device, errp);
@@ -4142,13 +4141,10 @@ void qmp_change_backing_file(const char *device,
}
/* if not r/w, reopen to make r/w */
-open_flags = image_bs->open_flags;
ro = bdrv_is_read_only(image_bs);
if (ro) {
-bdrv_reopen(image_bs, open_flags | BDRV_O_RDWR, _err);
-if (local_err) {
-error_propagate(errp, local_err);
+if (bdrv_reopen_set_read_only(image_bs, false, errp) != 0) {
goto out;
}
}
@@ -4164,7 +4160,7 @@ void qmp_change_backing_file(const char *device,
}
if (ro) {
-bdrv_reopen(image_bs, open_flags, _err);
+bdrv_reopen_set_read_only(image_bs, true, _err);
error_propagate(errp, local_err);
}
--
2.11.0
[Qemu-block] [PATCH v4 03/15] block: Use bdrv_reopen_set_read_only() in commit_start/complete()
This patch replaces the bdrv_reopen() calls that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
block/commit.c | 16 ++--
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index a2da5740b0..a53c2d04b0 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -38,7 +38,7 @@ typedef struct CommitBlockJob {
BlockBackend *base;
BlockDriverState *base_bs;
BlockdevOnError on_error;
-int base_flags;
+bool base_read_only;
char *backing_file_str;
} CommitBlockJob;
@@ -124,8 +124,8 @@ static void commit_clean(Job *job)
/* restore base open flags here if appropriate (e.g., change the base back
* to r/o). These reopens do not need to be atomic, since we won't abort
* even on failure here */
-if (s->base_flags != bdrv_get_flags(s->base_bs)) {
-bdrv_reopen(s->base_bs, s->base_flags, NULL);
+if (s->base_read_only) {
+bdrv_reopen_set_read_only(s->base_bs, true, NULL);
}
g_free(s->backing_file_str);
@@ -264,7 +264,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
const char *filter_node_name, Error **errp)
{
CommitBlockJob *s;
-int orig_base_flags;
BlockDriverState *iter;
BlockDriverState *commit_top_bs = NULL;
Error *local_err = NULL;
@@ -283,11 +282,9 @@ void commit_start(const char *job_id, BlockDriverState *bs,
}
/* convert base to r/w, if necessary */
-orig_base_flags = bdrv_get_flags(base);
-if (!(orig_base_flags & BDRV_O_RDWR)) {
-bdrv_reopen(base, orig_base_flags | BDRV_O_RDWR, _err);
-if (local_err != NULL) {
-error_propagate(errp, local_err);
+s->base_read_only = bdrv_is_read_only(base);
+if (s->base_read_only) {
+if (bdrv_reopen_set_read_only(base, false, errp) != 0) {
goto fail;
}
}
@@ -363,7 +360,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
goto fail;
}
-s->base_flags = orig_base_flags;
s->backing_file_str = g_strdup(backing_file_str);
s->on_error = on_error;
--
2.11.0
[Qemu-block] [PATCH v4 14/15] block: Remove assertions from update_flags_from_options()
This function takes three options (cache.direct, cache.no-flush and
read-only) from a QemuOpts object and updates the flags accordingly.
If any of those options is not set (because it was missing from the
original QDict or because it had an invalid value) then the function
aborts with a failed assertion:
$ qemu-io -c 'reopen -o read-only=foo' hd.qcow2
block.c:1126: update_flags_from_options: Assertion `qemu_opt_find(opts,
BDRV_OPT_CACHE_DIRECT)' failed.
Aborted
This assertion is unnecessary, and it forces any caller of
bdrv_reopen() to pass all the aforementioned three options. This may
have made sense in order to remove ambiguity when bdrv_reopen() was
taking both flags and options, but that's not the case anymore.
It's also unnecessary if we want to validate the option values,
because bdrv_reopen_prepare() already takes care of that, as we can
see if we remove the assertions:
$ qemu-io -c 'reopen -o read-only=foo' hd.qcow2
Parameter 'read-only' expects 'on' or 'off'
Signed-off-by: Alberto Garcia
---
block.c| 4
tests/qemu-iotests/133 | 8
tests/qemu-iotests/133.out | 6 ++
3 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/block.c b/block.c
index 8bc808d6f3..68f1e3b45e 100644
--- a/block.c
+++ b/block.c
@@ -1139,24 +1139,20 @@ static void update_flags_from_options(int *flags,
QemuOpts *opts)
{
*flags &= ~BDRV_O_CACHE_MASK;
-assert(qemu_opt_find(opts, BDRV_OPT_CACHE_NO_FLUSH));
if (qemu_opt_get_bool_del(opts, BDRV_OPT_CACHE_NO_FLUSH, false)) {
*flags |= BDRV_O_NO_FLUSH;
}
-assert(qemu_opt_find(opts, BDRV_OPT_CACHE_DIRECT));
if (qemu_opt_get_bool_del(opts, BDRV_OPT_CACHE_DIRECT, false)) {
*flags |= BDRV_O_NOCACHE;
}
*flags &= ~BDRV_O_RDWR;
-assert(qemu_opt_find(opts, BDRV_OPT_READ_ONLY));
if (!qemu_opt_get_bool_del(opts, BDRV_OPT_READ_ONLY, false)) {
*flags |= BDRV_O_RDWR;
}
-assert(qemu_opt_find(opts, BDRV_OPT_AUTO_READ_ONLY));
if (qemu_opt_get_bool_del(opts, BDRV_OPT_AUTO_READ_ONLY, false)) {
*flags |= BDRV_O_AUTO_RDONLY;
}
diff --git a/tests/qemu-iotests/133 b/tests/qemu-iotests/133
index 14e6b3b972..59d5e2ea25 100755
--- a/tests/qemu-iotests/133
+++ b/tests/qemu-iotests/133
@@ -101,6 +101,14 @@ $QEMU_IO -c 'reopen -w -o read-only=on' $TEST_IMG
$QEMU_IO -c 'reopen -c none -o cache.direct=on' $TEST_IMG
$QEMU_IO -c 'reopen -c writeback -o cache.direct=on' $TEST_IMG
$QEMU_IO -c 'reopen -c directsync -o cache.no-flush=on' $TEST_IMG
+
+echo
+echo "=== Check that invalid options are handled correctly ==="
+echo
+
+$QEMU_IO -c 'reopen -o read-only=foo' $TEST_IMG
+$QEMU_IO -c 'reopen -o cache.no-flush=bar' $TEST_IMG
+$QEMU_IO -c 'reopen -o cache.direct=baz' $TEST_IMG
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/133.out b/tests/qemu-iotests/133.out
index 48a9d087f0..551096a9c4 100644
--- a/tests/qemu-iotests/133.out
+++ b/tests/qemu-iotests/133.out
@@ -32,4 +32,10 @@ Cannot set both -r/-w and 'read-only'
Cannot set both -c and the cache options
Cannot set both -c and the cache options
Cannot set both -c and the cache options
+
+=== Check that invalid options are handled correctly ===
+
+Parameter 'read-only' expects 'on' or 'off'
+Parameter 'cache.no-flush' expects 'on' or 'off'
+Parameter 'cache.direct' expects 'on' or 'off'
*** done
--
2.11.0
[Qemu-block] [PATCH v4 04/15] block: Use bdrv_reopen_set_read_only() in bdrv_commit()
This patch replaces the bdrv_reopen() calls that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
block/commit.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index a53c2d04b0..53148e610b 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -391,7 +391,7 @@ int bdrv_commit(BlockDriverState *bs)
BlockDriverState *commit_top_bs = NULL;
BlockDriver *drv = bs->drv;
int64_t offset, length, backing_length;
-int ro, open_flags;
+int ro;
int64_t n;
int ret = 0;
uint8_t *buf = NULL;
@@ -410,10 +410,9 @@ int bdrv_commit(BlockDriverState *bs)
}
ro = bs->backing->bs->read_only;
-open_flags = bs->backing->bs->open_flags;
if (ro) {
-if (bdrv_reopen(bs->backing->bs, open_flags | BDRV_O_RDWR, NULL)) {
+if (bdrv_reopen_set_read_only(bs->backing->bs, false, NULL)) {
return -EACCES;
}
}
@@ -523,7 +522,7 @@ ro_cleanup:
if (ro) {
/* ignoring error return here */
-bdrv_reopen(bs->backing->bs, open_flags & ~BDRV_O_RDWR, NULL);
+bdrv_reopen_set_read_only(bs->backing->bs, true, NULL);
}
return ret;
--
2.11.0
[Qemu-block] [PATCH v4 10/15] qemu-io: Put flag changes in the options QDict in reopen_f()
When reopen_f() puts a block device in the reopen queue, some of the
new options are passed using a QDict, but others ("read-only" and the
cache options) are passed as flags.
This patch puts those flags in the QDict. This way the flags parameter
becomes redundant and we'll be able to get rid of it in a subsequent
patch.
Signed-off-by: Alberto Garcia
---
qemu-io-cmds.c | 27 ++-
tests/qemu-iotests/133 | 9 +
tests/qemu-iotests/133.out | 8
3 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 5363482213..c9ae09d574 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -10,6 +10,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
#include "qemu-io.h"
#include "sysemu/block-backend.h"
#include "block/block.h"
@@ -1978,6 +1979,7 @@ static int reopen_f(BlockBackend *blk, int argc, char
**argv)
int flags = bs->open_flags;
bool writethrough = !blk_enable_write_cache(blk);
bool has_rw_option = false;
+bool has_cache_option = false;
BlockReopenQueue *brq;
Error *local_err = NULL;
@@ -1989,6 +1991,7 @@ static int reopen_f(BlockBackend *blk, int argc, char
**argv)
error_report("Invalid cache option: %s", optarg);
return -EINVAL;
}
+has_cache_option = true;
break;
case 'o':
if (!qemu_opts_parse_noisily(_opts, optarg, 0)) {
@@ -2046,9 +2049,31 @@ static int reopen_f(BlockBackend *blk, int argc, char
**argv)
}
qopts = qemu_opts_find(_opts, NULL);
-opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL;
+opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : qdict_new();
qemu_opts_reset(_opts);
+if (qdict_haskey(opts, BDRV_OPT_READ_ONLY)) {
+if (has_rw_option) {
+error_report("Cannot set both -r/-w and '" BDRV_OPT_READ_ONLY "'");
+qobject_unref(opts);
+return -EINVAL;
+}
+} else {
+qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !(flags & BDRV_O_RDWR));
+}
+
+if (qdict_haskey(opts, BDRV_OPT_CACHE_DIRECT) ||
+qdict_haskey(opts, BDRV_OPT_CACHE_NO_FLUSH)) {
+if (has_cache_option) {
+error_report("Cannot set both -c and the cache options");
+qobject_unref(opts);
+return -EINVAL;
+}
+} else {
+qdict_put_bool(opts, BDRV_OPT_CACHE_DIRECT, flags & BDRV_O_NOCACHE);
+qdict_put_bool(opts, BDRV_OPT_CACHE_NO_FLUSH, flags & BDRV_O_NO_FLUSH);
+}
+
bdrv_subtree_drained_begin(bs);
brq = bdrv_reopen_queue(NULL, bs, opts, flags);
bdrv_reopen_multiple(bdrv_get_aio_context(bs), brq, _err);
diff --git a/tests/qemu-iotests/133 b/tests/qemu-iotests/133
index af6b3e1dd4..14e6b3b972 100755
--- a/tests/qemu-iotests/133
+++ b/tests/qemu-iotests/133
@@ -92,6 +92,15 @@ echo
IMGOPTSSYNTAX=false $QEMU_IO -f null-co -c 'reopen' -c 'info' \
"json:{'driver': 'null-co', 'size': 65536}"
+echo
+echo "=== Check that mixing -c/-r/-w and their corresponding options is
forbidden ==="
+echo
+
+$QEMU_IO -c 'reopen -r -o read-only=on' $TEST_IMG
+$QEMU_IO -c 'reopen -w -o read-only=on' $TEST_IMG
+$QEMU_IO -c 'reopen -c none -o cache.direct=on' $TEST_IMG
+$QEMU_IO -c 'reopen -c writeback -o cache.direct=on' $TEST_IMG
+$QEMU_IO -c 'reopen -c directsync -o cache.no-flush=on' $TEST_IMG
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/133.out b/tests/qemu-iotests/133.out
index f4a85aeb63..48a9d087f0 100644
--- a/tests/qemu-iotests/133.out
+++ b/tests/qemu-iotests/133.out
@@ -24,4 +24,12 @@ Cannot change the option 'driver'
format name: null-co
format name: null-co
+
+=== Check that mixing -c/-r/-w and their corresponding options is forbidden ===
+
+Cannot set both -r/-w and 'read-only'
+Cannot set both -r/-w and 'read-only'
+Cannot set both -c and the cache options
+Cannot set both -c and the cache options
+Cannot set both -c and the cache options
*** done
--
2.11.0
[Qemu-block] [PATCH v4 02/15] block: Use bdrv_reopen_set_read_only() in bdrv_backing_update_filename()
This patch replaces the bdrv_reopen() calls that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
block.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/block.c b/block.c
index 4e82c71ba4..cfa53f7114 100644
--- a/block.c
+++ b/block.c
@@ -1079,11 +1079,11 @@ static int bdrv_backing_update_filename(BdrvChild *c,
BlockDriverState *base,
const char *filename, Error **errp)
{
BlockDriverState *parent = c->opaque;
-int orig_flags = bdrv_get_flags(parent);
+bool read_only = bdrv_is_read_only(parent);
int ret;
-if (!(orig_flags & BDRV_O_RDWR)) {
-ret = bdrv_reopen(parent, orig_flags | BDRV_O_RDWR, errp);
+if (read_only) {
+ret = bdrv_reopen_set_read_only(parent, false, errp);
if (ret < 0) {
return ret;
}
@@ -1095,8 +1095,8 @@ static int bdrv_backing_update_filename(BdrvChild *c,
BlockDriverState *base,
error_setg_errno(errp, -ret, "Could not update backing file link");
}
-if (!(orig_flags & BDRV_O_RDWR)) {
-bdrv_reopen(parent, orig_flags, NULL);
+if (read_only) {
+bdrv_reopen_set_read_only(parent, true, NULL);
}
return ret;
--
2.11.0
[Qemu-block] [PATCH v4 01/15] block: Add bdrv_reopen_set_read_only()
Most callers of bdrv_reopen() only use it to switch a BlockDriverState
between read-only and read-write, so this patch adds a new function
that does just that.
We also want to get rid of the flags parameter in the bdrv_reopen()
API, so this function sets the "read-only" option and passes the
original flags (which will then be updated in bdrv_reopen_prepare()).
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
block.c | 17 +
include/block/block.h | 2 ++
2 files changed, 19 insertions(+)
diff --git a/block.c b/block.c
index fd67e14dfa..4e82c71ba4 100644
--- a/block.c
+++ b/block.c
@@ -3127,6 +3127,23 @@ int bdrv_reopen(BlockDriverState *bs, int bdrv_flags,
Error **errp)
return ret;
}
+int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
+ Error **errp)
+{
+int ret;
+BlockReopenQueue *queue;
+QDict *opts = qdict_new();
+
+qdict_put_bool(opts, BDRV_OPT_READ_ONLY, read_only);
+
+bdrv_subtree_drained_begin(bs);
+queue = bdrv_reopen_queue(NULL, bs, opts, bdrv_get_flags(bs));
+ret = bdrv_reopen_multiple(bdrv_get_aio_context(bs), queue, errp);
+bdrv_subtree_drained_end(bs);
+
+return ret;
+}
+
static BlockReopenQueueEntry *find_parent_in_reopen_queue(BlockReopenQueue *q,
BdrvChild *c)
{
diff --git a/include/block/block.h b/include/block/block.h
index 7f5453b45b..382e6643fc 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -303,6 +303,8 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue
*bs_queue,
QDict *options, int flags);
int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error
**errp);
int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
+int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
+ Error **errp);
int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue, Error **errp);
void bdrv_reopen_commit(BDRVReopenState *reopen_state);
--
2.11.0
[Qemu-block] [PATCH v4 15/15] block: Assert that flags are up-to-date in bdrv_reopen_prepare()
Towards the end of bdrv_reopen_queue_child(), before starting to
process the children, the update_flags_from_options() function is
called in order to have BDRVReopenState.flags in sync with the options
from the QDict.
This is necessary because during the reopen process flags must be
updated for all nodes in the queue so bdrv_is_writable_after_reopen()
and the permission checks work correctly.
Because of that, calling update_flags_from_options() again in
bdrv_reopen_prepare() doesn't really change the flags (they are
already up-to-date). But we need to call it in order to remove those
options from QemuOpts and that way indicate that they have been
processed.
Signed-off-by: Alberto Garcia
---
block.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/block.c b/block.c
index 68f1e3b45e..03277b3d19 100644
--- a/block.c
+++ b/block.c
@@ -3178,6 +3178,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue,
Error **errp)
{
int ret = -1;
+int old_flags;
Error *local_err = NULL;
BlockDriver *drv;
QemuOpts *opts;
@@ -3203,7 +3204,12 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue,
goto error;
}
+/* This was already called in bdrv_reopen_queue_child() so the flags
+ * are up-to-date. This time we simply want to remove the options from
+ * QemuOpts in order to indicate that they have been processed. */
+old_flags = reopen_state->flags;
update_flags_from_options(_state->flags, opts);
+assert(old_flags == reopen_state->flags);
discard = qemu_opt_get_del(opts, BDRV_OPT_DISCARD);
if (discard != NULL) {
--
2.11.0
[Qemu-block] [PATCH v4 07/15] block: Use bdrv_reopen_set_read_only() in external_snapshot_commit()
This patch replaces the bdrv_reopen() call that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
blockdev.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 20d1d84bab..112bc62ae5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1703,8 +1703,7 @@ static void external_snapshot_commit(BlkActionState
*common)
* bdrv_reopen_multiple() across all the entries at once, because we
* don't want to abort all of them if one of them fails the reopen */
if (!atomic_read(>old_bs->copy_on_read)) {
-bdrv_reopen(state->old_bs, state->old_bs->open_flags & ~BDRV_O_RDWR,
-NULL);
+bdrv_reopen_set_read_only(state->old_bs, true, NULL);
}
aio_context_release(aio_context);
--
2.11.0
[Qemu-block] [PATCH v4 11/15] block: Clean up reopen_backing_file() in block/replication.c
This function is used to put the hidden and secondary disks in
read-write mode before launching the backup job, and back in read-only
mode afterwards.
This patch does the following changes:
- Use an options QDict with the "read-only" option instead of
passing the changes as flags only.
- Simplify the code (it was unnecessarily complicated and verbose).
- Fix a bug due to which the secondary disk was not being put back
in read-only mode when writable=false (because in this case
orig_secondary_flags always had the BDRV_O_RDWR flag set).
- Stop clearing the BDRV_O_INACTIVE flag.
The flags parameter to bdrv_reopen_queue() becomes redundant and we'll
be able to get rid of it in a subsequent patch.
Signed-off-by: Alberto Garcia
---
block/replication.c | 45 +
1 file changed, 21 insertions(+), 24 deletions(-)
diff --git a/block/replication.c b/block/replication.c
index 6349d6958e..481a225924 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -20,6 +20,7 @@
#include "block/block_backup.h"
#include "sysemu/block-backend.h"
#include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
#include "replication.h"
typedef enum {
@@ -39,8 +40,8 @@ typedef struct BDRVReplicationState {
char *top_id;
ReplicationState *rs;
Error *blocker;
-int orig_hidden_flags;
-int orig_secondary_flags;
+bool orig_hidden_read_only;
+bool orig_secondary_read_only;
int error;
} BDRVReplicationState;
@@ -371,44 +372,40 @@ static void secondary_do_checkpoint(BDRVReplicationState
*s, Error **errp)
}
}
+/* This function is supposed to be called twice:
+ * first with writable = true, then with writable = false.
+ * The first call puts s->hidden_disk and s->secondary_disk in
+ * r/w mode, and the second puts them back in their original state.
+ */
static void reopen_backing_file(BlockDriverState *bs, bool writable,
Error **errp)
{
BDRVReplicationState *s = bs->opaque;
BlockReopenQueue *reopen_queue = NULL;
-int orig_hidden_flags, orig_secondary_flags;
-int new_hidden_flags, new_secondary_flags;
Error *local_err = NULL;
if (writable) {
-orig_hidden_flags = s->orig_hidden_flags =
-bdrv_get_flags(s->hidden_disk->bs);
-new_hidden_flags = (orig_hidden_flags | BDRV_O_RDWR) &
-~BDRV_O_INACTIVE;
-orig_secondary_flags = s->orig_secondary_flags =
-bdrv_get_flags(s->secondary_disk->bs);
-new_secondary_flags = (orig_secondary_flags | BDRV_O_RDWR) &
- ~BDRV_O_INACTIVE;
-} else {
-orig_hidden_flags = (s->orig_hidden_flags | BDRV_O_RDWR) &
-~BDRV_O_INACTIVE;
-new_hidden_flags = s->orig_hidden_flags;
-orig_secondary_flags = (s->orig_secondary_flags | BDRV_O_RDWR) &
-~BDRV_O_INACTIVE;
-new_secondary_flags = s->orig_secondary_flags;
+s->orig_hidden_read_only = bdrv_is_read_only(s->hidden_disk->bs);
+s->orig_secondary_read_only = bdrv_is_read_only(s->secondary_disk->bs);
}
bdrv_subtree_drained_begin(s->hidden_disk->bs);
bdrv_subtree_drained_begin(s->secondary_disk->bs);
-if (orig_hidden_flags != new_hidden_flags) {
-reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs,
NULL,
- new_hidden_flags);
+if (s->orig_hidden_read_only) {
+int flags = bdrv_get_flags(s->hidden_disk->bs);
+QDict *opts = qdict_new();
+qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
+reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs,
+ opts, flags);
}
-if (!(orig_secondary_flags & BDRV_O_RDWR)) {
+if (s->orig_secondary_read_only) {
+int flags = bdrv_get_flags(s->secondary_disk->bs);
+QDict *opts = qdict_new();
+qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
reopen_queue = bdrv_reopen_queue(reopen_queue, s->secondary_disk->bs,
- NULL, new_secondary_flags);
+ opts, flags);
}
if (reopen_queue) {
--
2.11.0
[Qemu-block] [PATCH v4 05/15] block: Use bdrv_reopen_set_read_only() in stream_start/complete()
This patch replaces the bdrv_reopen() calls that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
block/stream.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index 81a7ec8ece..262d280ccd 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -34,7 +34,7 @@ typedef struct StreamBlockJob {
BlockDriverState *base;
BlockdevOnError on_error;
char *backing_file_str;
-int bs_flags;
+bool bs_read_only;
} StreamBlockJob;
static int coroutine_fn stream_populate(BlockBackend *blk,
@@ -89,10 +89,10 @@ static void stream_clean(Job *job)
BlockDriverState *bs = blk_bs(bjob->blk);
/* Reopen the image back in read-only mode if necessary */
-if (s->bs_flags != bdrv_get_flags(bs)) {
+if (s->bs_read_only) {
/* Give up write permissions before making it read-only */
blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, _abort);
-bdrv_reopen(bs, s->bs_flags, NULL);
+bdrv_reopen_set_read_only(bs, true, NULL);
}
g_free(s->backing_file_str);
@@ -226,12 +226,12 @@ void stream_start(const char *job_id, BlockDriverState
*bs,
{
StreamBlockJob *s;
BlockDriverState *iter;
-int orig_bs_flags;
+int bs_read_only;
/* Make sure that the image is opened in read-write mode */
-orig_bs_flags = bdrv_get_flags(bs);
-if (!(orig_bs_flags & BDRV_O_RDWR)) {
-if (bdrv_reopen(bs, orig_bs_flags | BDRV_O_RDWR, errp) != 0) {
+bs_read_only = bdrv_is_read_only(bs);
+if (bs_read_only) {
+if (bdrv_reopen_set_read_only(bs, false, errp) != 0) {
return;
}
}
@@ -261,7 +261,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
s->base = base;
s->backing_file_str = g_strdup(backing_file_str);
-s->bs_flags = orig_bs_flags;
+s->bs_read_only = bs_read_only;
s->on_error = on_error;
trace_stream_start(bs, base, s);
@@ -269,7 +269,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
return;
fail:
-if (orig_bs_flags != bdrv_get_flags(bs)) {
-bdrv_reopen(bs, orig_bs_flags, NULL);
+if (bs_read_only) {
+bdrv_reopen_set_read_only(bs, true, NULL);
}
}
--
2.11.0
[Qemu-block] [PATCH v4 00/15] Don't pass flags to bdrv_reopen_queue()
Hi all, when reopening a BlockDriverState using bdrv_reopen() and friends the new options can be specified either with a QDict or with flags. Both methods overlap and that makes the semantics and the implementation unnecessarily complicated. This series removes the 'flags' parameter from these functions, so from now on all option changes must be specified using a QDict. Apart from simplifying the API, a few bugs are fixed along the way. See the individual patches for more details. This was tested with the current master (4de6bb0c02ad3f0ec48f0f84ba1). Regards, Berto v4: - Patch 14: Don't assert if BDRV_OPT_AUTO_READ_ONLY is missing, e.g. qemu-io -c 'reopen -o auto-read-only=foo' -f raw null-co:// v3: https://lists.gnu.org/archive/html/qemu-block/2018-10/msg00648.html - Patch 1 [from v2] has been removed and all other patch numbers are shifted. - Patches 10 and 13: Fixed rebase conflicts after the patch removal. - Patch 14: Replacement for the removed patch using a different approach. - Patch 15: Document a non-obvious call to update_flags_from_options() v2: https://lists.gnu.org/archive/html/qemu-block/2018-10/msg00534.html - Patches 4 and 9: Fixed trivial rebase conflict - Patch 11: Update messages [Max] - Patch 12: Add comment and use bdrv_is_read_only() instead of using the flags directly [Max] - Patch 14: Remove inner block and move all variable declarations to the beginning of the function [Max] v1: https://lists.gnu.org/archive/html/qemu-block/2018-09/msg00483.html - Initial version Output of backport-diff against v3: Key: [] : patches are identical [] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively 001/15:[] [--] 'block: Add bdrv_reopen_set_read_only()' 002/15:[] [--] 'block: Use bdrv_reopen_set_read_only() in bdrv_backing_update_filename()' 003/15:[] [--] 'block: Use bdrv_reopen_set_read_only() in commit_start/complete()' 004/15:[] [--] 'block: Use bdrv_reopen_set_read_only() in bdrv_commit()' 005/15:[] [--] 'block: Use bdrv_reopen_set_read_only() in stream_start/complete()' 006/15:[] [--] 'block: Use bdrv_reopen_set_read_only() in qmp_change_backing_file()' 007/15:[] [--] 'block: Use bdrv_reopen_set_read_only() in external_snapshot_commit()' 008/15:[] [--] 'block: Use bdrv_reopen_set_read_only() in the mirror driver' 009/15:[] [--] 'block: Drop bdrv_reopen()' 010/15:[] [--] 'qemu-io: Put flag changes in the options QDict in reopen_f()' 011/15:[] [--] 'block: Clean up reopen_backing_file() in block/replication.c' 012/15:[] [--] 'block: Remove flags parameter from bdrv_reopen_queue()' 013/15:[] [--] 'block: Stop passing flags to bdrv_reopen_queue_child()' 014/15:[0001] [FC] 'block: Remove assertions from update_flags_from_options()' 015/15:[] [-C] 'block: Assert that flags are up-to-date in bdrv_reopen_prepare()' Alberto Garcia (15): block: Add bdrv_reopen_set_read_only() block: Use bdrv_reopen_set_read_only() in bdrv_backing_update_filename() block: Use bdrv_reopen_set_read_only() in commit_start/complete() block: Use bdrv_reopen_set_read_only() in bdrv_commit() block: Use bdrv_reopen_set_read_only() in stream_start/complete() block: Use bdrv_reopen_set_read_only() in qmp_change_backing_file() block: Use bdrv_reopen_set_read_only() in external_snapshot_commit() block: Use bdrv_reopen_set_read_only() in the mirror driver block: Drop bdrv_reopen() qemu-io: Put flag changes in the options QDict in reopen_f() block: Clean up reopen_backing_file() in block/replication.c block: Remove flags parameter from bdrv_reopen_queue() block: Stop passing flags to bdrv_reopen_queue_child() block: Remove assertions from update_flags_from_options() block: Assert that flags are up-to-date in bdrv_reopen_prepare() block.c| 89 -- block/commit.c | 23 +--- block/mirror.c | 19 ++ block/replication.c| 43 ++ block/stream.c | 20 +-- blockdev.c | 11 ++ include/block/block.h | 6 ++-- qemu-io-cmds.c | 29 +-- tests/qemu-iotests/133 | 17 + tests/qemu-iotests/133.out | 14 10 files changed, 153 insertions(+), 118 deletions(-) -- 2.11.0
[Qemu-block] [PATCH v4 08/15] block: Use bdrv_reopen_set_read_only() in the mirror driver
The 'block-commit' QMP command is implemented internally using two
different drivers. If the source image is the active layer then the
mirror driver is used (commit_active_start()), otherwise the commit
driver is used (commit_start()).
In both cases the destination image must be put temporarily in
read-write mode. This is done correctly in the latter case, but what
commit_active_start() does is copy all flags instead.
This patch replaces the bdrv_reopen() calls in that function with
bdrv_reopen_set_read_only() so that only the read-only status is
changed.
A similar change is made in mirror_exit(), which is also used by the
'drive-mirror' and 'blockdev-mirror' commands.
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
block/mirror.c | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 56d9ef7474..41b6cbaad6 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -672,9 +672,10 @@ static int mirror_exit_common(Job *job)
if (s->should_complete && !abort) {
BlockDriverState *to_replace = s->to_replace ?: src;
+bool ro = bdrv_is_read_only(to_replace);
-if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
-bdrv_reopen(target_bs, bdrv_get_flags(to_replace), NULL);
+if (ro != bdrv_is_read_only(target_bs)) {
+bdrv_reopen_set_read_only(target_bs, ro, NULL);
}
/* The mirror job has no requests in flight any more, but we need to
@@ -1692,13 +1693,15 @@ void commit_active_start(const char *job_id,
BlockDriverState *bs,
BlockCompletionFunc *cb, void *opaque,
bool auto_complete, Error **errp)
{
-int orig_base_flags;
+bool base_read_only;
Error *local_err = NULL;
-orig_base_flags = bdrv_get_flags(base);
+base_read_only = bdrv_is_read_only(base);
-if (bdrv_reopen(base, bs->open_flags, errp)) {
-return;
+if (base_read_only) {
+if (bdrv_reopen_set_read_only(base, false, errp) < 0) {
+return;
+}
}
mirror_start_job(job_id, bs, creation_flags, base, NULL, speed, 0, 0,
@@ -1717,6 +1720,8 @@ void commit_active_start(const char *job_id,
BlockDriverState *bs,
error_restore_flags:
/* ignore error and errp for bdrv_reopen, because we want to propagate
* the original error */
-bdrv_reopen(base, orig_base_flags, NULL);
+if (base_read_only) {
+bdrv_reopen_set_read_only(base, true, NULL);
+}
return;
}
--
2.11.0
Re: [Qemu-block] [PATCH] tests: Fix Python 3 detection on older GNU make versions
On Wed, Nov 07, 2018 at 07:05:03AM +0100, Markus Armbruster wrote: > Eduardo Habkost writes: > > > The $(SHELLSTATUS) variable requires GNU make >= 4.2, but Travis > > seems to provide an older version. Change the existing rules to > > use command output instead of exit code, to make it compatible > > with older GNU make versions. > > > > Signed-off-by: Eduardo Habkost > > --- > > I think that's the cause of the Travis failures. I have > > submitted a test job right now, at: > > https://travis-ci.org/ehabkost/qemu-hacks/jobs/451387962 > > Let's see if it fixes the issue. > > --- > > tests/Makefile.include | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/tests/Makefile.include b/tests/Makefile.include > > index d2e577eabb..074eece558 100644 > > --- a/tests/Makefile.include > > +++ b/tests/Makefile.include > > @@ -913,8 +913,8 @@ TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results > > # information please refer to "avocado --help". > > AVOCADO_SHOW=none > > > > -$(shell $(PYTHON) -c 'import sys; assert sys.version_info >= (3,0)' > > >/dev/null 2>&1) > > -ifeq ($(.SHELLSTATUS),0) > > +PYTHON3 = $(shell $(PYTHON) -c 'import sys; print(1 if sys.version_info >= > > (3, 0) else 0)') > > +ifeq ($(PYTHON3), 1) > > $(TESTS_VENV_DIR): $(TESTS_VENV_REQ) > > $(call quiet-command, \ > > $(PYTHON) -m venv --system-site-packages $@, \ > > PEP 394 recommends software distributions install Python 3 into the > default path as python3, and users use that instead of python, except > for programs that are source compatible with both 2 and 3. So, is > finding out whether python is a Python 3 really appropriate? Why can't > we just use python3 and be done with it? Because './configure --with-python=...' exists, and I didn't want to break it. Now, why do we need --with-python, and why do we need to use $(PYTHON) when running tests? If somebody wants to use a different Python binary when running tests, they can already use $PATH for that. (That's the same argument I used for iotests a while ago: https://www.mail-archive.com/qemu-devel@nongnu.org/msg566631.html) > > If we can't: isn't this a configure problem? It is, and I think Cleber mentioned that he planned to do it in ./configure, a while ago. But just using the python3 binary from $PATH would be even better. -- Eduardo
Re: [Qemu-block] [Qemu-devel] [PATCH v6 00/10] hw/m68k: add Apple Machintosh Quadra 800 machine
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20181102152257.20637-1-mark.cave-ayl...@ilande.co.uk
Subject: [Qemu-devel] [PATCH v6 00/10] hw/m68k: add Apple Machintosh Quadra 800
machine
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
f411b4e279 hw/m68k: define Macintosh Quadra 800
45ed15b61e dp8393x: manage big endian bus
720eeb4748 hw/m68k: add a dummy SWIM floppy controller
3e83c22b31 hw/m68k: add Nubus support for macfb video card
28c7e4c0ab hw/m68k: add Nubus support
83574a525d esp: add pseudo-DMA as used by Macintosh
b9ebf41148 hw/m68k: add macfb video card
c374c97795 escc: introduce a selector for the register bit
f169706c12 hw/m68k: implement ADB bus support for via
a09662d340 hw/m68k: add via support
=== OUTPUT BEGIN ===
Checking PATCH 1/10: hw/m68k: add via support...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#26:
new file mode 100644
ERROR: space prohibited after that '&&' (ctx:WxW)
#348: FILE: hw/misc/mac_via.c:318:
+if (!(v1s->last_b & VIA1B_vRTCClk) && (s->b & VIA1B_vRTCClk)) {
^
total: 1 errors, 1 warnings, 778 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 2/10: hw/m68k: implement ADB bus support for via...
Checking PATCH 3/10: escc: introduce a selector for the register bit...
Checking PATCH 4/10: hw/m68k: add macfb video card...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#44:
new file mode 100644
total: 0 errors, 1 warnings, 496 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 5/10: esp: add pseudo-DMA as used by Macintosh...
Checking PATCH 6/10: hw/m68k: add Nubus support...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#25:
new file mode 100644
total: 0 errors, 1 warnings, 509 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 7/10: hw/m68k: add Nubus support for macfb video card...
Checking PATCH 8/10: hw/m68k: add a dummy SWIM floppy controller...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#28:
new file mode 100644
total: 0 errors, 1 warnings, 498 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 9/10: dp8393x: manage big endian bus...
Checking PATCH 10/10: hw/m68k: define Macintosh Quadra 800...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#90:
new file mode 100644
total: 0 errors, 1 warnings, 605 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@redhat.com
[Qemu-block] Change in qemu 2.12 causes qemu-img convert to NBD to write more data
(I'm not going to claim this is a bug, but it causes a large, easily measurable performance regression in virt-v2v). In qemu 2.10, when you do ‘qemu-img convert’ to an NBD target, qemu interleaves write and zero requests. We can observe this as follows: $ virt-builder fedora-28 $ nbdkit --filter=log memory size=6G logfile=/tmp/log \ --run './qemu-img convert ./fedora-28.img -n $nbd' $ grep '\.\.\.$' /tmp/log | sed 's/.*\([A-Z][a-z]*\).*/\1/' | uniq -c 1 Write 2 Zero 1 Write 3 Zero 1 Write 1 Zero 1 Write [etc for over 1000 lines] Looking at the log file in detail we can see it is writing serially from the beginning to the end of the disk. In qemu 2.12 this behaviour changed: $ nbdkit --filter=log memory size=6G logfile=/tmp/log \ --run './qemu-img convert ./fedora-28.img -n $nbd' $ grep '\.\.\.$' /tmp/log | sed 's/.*\([A-Z][a-z]*\).*/\1/' | uniq -c 193 Zero 1246 Write It now zeroes the whole disk up front and then writes data over the top of the zeroed blocks. The reason for the performance regression is that in the first case we write 6G in total. In the second case we write 6G of zeroes up front, followed by the amount of data in the disk image (in this case the test disk image contains 1G of non-sparse data, so we write about 7G in total). In real world cases this makes a great difference: we might have 100s of G of data in the disk. The ultimate backend storage (a Linux block device) doesn't support efficient BLKZEROOUT so zeroing is pretty slow too. I bisected the change to the commit shown at the end of this email. Any suggestions how to fix or work around this problem welcome. Rich. commit 9776f0db6a19a0510e89b7aae38190b4811c95ba Author: Edgar Kaziakhmedov Date: Thu Jan 18 14:51:58 2018 +0300 nbd: implement bdrv_get_info callback Since mirror job supports efficient zero out target mechanism (see in mirror_dirty_init()), implement bdrv_get_info to make it work over NBD. Such improvement will allow using the largest chunk possible and will decrease the number of NBD_CMD_WRITE_ZEROES requests on the wire. Signed-off-by: Edgar Kaziakhmedov Message-Id: <20180118115158.17219-1-edgar.kaziakhme...@virtuozzo.com> Reviewed-by: Paolo Bonzini Signed-off-by: Eric Blake -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
Re: [Qemu-block] [PATCH] tests: Fix Python 3 detection on older GNU make versions
On 7 November 2018 at 06:05, Markus Armbruster wrote: > Eduardo Habkost writes: > >> The $(SHELLSTATUS) variable requires GNU make >= 4.2, but Travis >> seems to provide an older version. Change the existing rules to >> use command output instead of exit code, to make it compatible >> with older GNU make versions. >> >> Signed-off-by: Eduardo Habkost >> --- >> I think that's the cause of the Travis failures. I have >> submitted a test job right now, at: >> https://travis-ci.org/ehabkost/qemu-hacks/jobs/451387962 >> Let's see if it fixes the issue. >> --- >> tests/Makefile.include | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/tests/Makefile.include b/tests/Makefile.include >> index d2e577eabb..074eece558 100644 >> --- a/tests/Makefile.include >> +++ b/tests/Makefile.include >> @@ -913,8 +913,8 @@ TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results >> # information please refer to "avocado --help". >> AVOCADO_SHOW=none >> >> -$(shell $(PYTHON) -c 'import sys; assert sys.version_info >= (3,0)' >> >/dev/null 2>&1) >> -ifeq ($(.SHELLSTATUS),0) >> +PYTHON3 = $(shell $(PYTHON) -c 'import sys; print(1 if sys.version_info >= >> (3, 0) else 0)') >> +ifeq ($(PYTHON3), 1) >> $(TESTS_VENV_DIR): $(TESTS_VENV_REQ) >> $(call quiet-command, \ >> $(PYTHON) -m venv --system-site-packages $@, \ > > PEP 394 recommends software distributions install Python 3 into the > default path as python3, and users use that instead of python, except > for programs that are source compatible with both 2 and 3. So, is > finding out whether python is a Python 3 really appropriate? Why can't > we just use python3 and be done with it? > > If we can't: isn't this a configure problem? You can't just use python3 and be done with it because python3 might not exist, and because (as with python 2) the user might want to tell us the path to it. You could have configure detect whether python3 exists and set a PYTHON3 as well as a PYTHON (plus I guess support for the user to say "my python3 is this binary"), and then have this code in Makefile.include handle "PYTHON3 is not set" to mean "python 3 isn't available". thanks -- PMM
Re: [Qemu-block] ping Re: [PATCH v4 00/11] backup-top filter driver for backup
07.11.2018 01:35, John Snow wrote: > > On 11/06/2018 12:21 PM, Kevin Wolf wrote: >> Am 02.11.2018 um 17:41 hat Vladimir Sementsov-Ogievskiy geschrieben: >>> ping >>> >>> 15.10.2018 19:06, Vladimir Sementsov-Ogievskiy wrote: Hi all! These series introduce backup-top driver. It's a filter-node, which do copy-before-write operation. Mirror uses filter-node for handling guest writes, let's move to filter-node (from write-notifiers) for backup too (patch 16) v4: fixes, rewrite driver to be implicit, drop new interfaces and don't move to BdrvDirtyBitmap for now, as it's not obvious will it be really needed and don't relate to these series more. v3 was "[PATCH v3 00/18] fleecing-hook driver for backup" v2 was "[RFC v2] new, node-graph-based fleecing and backup" These series are based on [PATCH v4 0/8] dirty-bitmap: rewrite bdrv_dirty_iter_next_area and [PATCH 0/2] replication: drop extra sync >> Before we can move forward here, we need to deal with the dependencies. >> I merged the second one now (because there wasn't any feedback from the >> replication maintainers), but the first one looks like it was going >> through John's or Eric's tree? >> > I was expecting it to go through Eric's because it was predicated on NBD > patches that he was staging. > > Is that no longer true? yes, it's merged to master. > --js -- Best regards, Vladimir
Re: [Qemu-block] [PATCH v4 03/11] block: allow serialized reads to intersect
06.11.2018 20:57, Kevin Wolf wrote: > Am 15.10.2018 um 18:06 hat Vladimir Sementsov-Ogievskiy geschrieben: >> Otherwise, if we have serialized read-part in copy_range from backing >> file to its parent if CoW take place, this CoW's sub-reads will >> intersect with firstly created serialized read request. >> >> Anyway, reads should not influence on disk view, let's allow them to >> intersect. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy > What about reads that internally trigger writes, such as copy-on-read? oops :). yes, I've to rethink this thing. > > Kevin -- Best regards, Vladimir
Re: [Qemu-block] [PATCH 0/2] nvme: fix two issues in nvme unhotplug
Am 29.10.2018 um 07:29 hat Li Qiang geschrieben: > The first corrent the refcount and second fix a memory leak. Thanks, applied to the block branch. Kevin
