Re: [PATCH v4 00/11] RFC: [for 5.0]: HMP monitor handlers refactoring
* Maxim Levitsky (mlevi...@redhat.com) wrote: > On Mon, 2020-02-03 at 19:57 +, Dr. David Alan Gilbert wrote: > > * Maxim Levitsky (mlevi...@redhat.com) wrote: > > > This patch series is bunch of cleanups to the hmp monitor code. > > > It mostly moves the blockdev related hmp handlers to its own file, > > > and does some minor refactoring. > > > > > > No functional changes expected. > > > > You've still got the title marked as RFC - are you actually ready for > > this log? > > I forgot to update this to be honest, I don't consider this an RFC, > especially since I dropped for now the patches that might cause > issues. This is now just a nice refactoring. OK, so if we can get some block people to say they're happy, then I'd be happy to take this through HMP or they can take it through block. Dave > Best regards, > Maxim Levitsky > > > > > Dave > > > > > > > > Changes from V1: > > >* move the handlers to block/monitor/block-hmp-cmds.c > > >* tiny cleanup for the commit messages > > > > > > Changes from V2: > > >* Moved all the function prototypes to new header (blockdev-hmp-cmds.h) > > >* Set the license of blockdev-hmp-cmds.c to GPLv2+ > > >* Moved hmp_snapshot_* functions to blockdev-hmp-cmds.c > > >* Moved hmp_drive_add_node to blockdev-hmp-cmds.c > > > (this change needed some new exports, thus in separate new patch) > > >* Moved hmp_qemu_io and hmp_eject to blockdev-hmp-cmds.c > > >* Added 'error:' prefix to vreport, and updated the iotests > > > This is invasive change, but really feels like the right one > > >* Added minor refactoring patch that drops an unused #include > > > > > > Changes from V3: > > >* Dropped the error prefix patches for now due to fact that it seems > > > that libvirt doesn't need that after all. Oh well... > > > I'll send them in a separate series. > > > > > >* Hopefully correctly merged the copyright info the new files > > > Both files are GPLv2 now (due to code from hmp.h/hmp-cmds.c) > > > > > >* Addressed review feedback > > >* Renamed the added header to block-hmp-cmds.h > > > > > >* Got rid of checkpatch.pl warnings in the moved code > > > (cosmetic code changes only) > > > > > >* I kept the reviewed-by tags, since the changes I did are minor. > > > I hope that this is right thing to do. > > > > > > Best regards, > > > Maxim Levitsky > > > > > > Maxim Levitsky (11): > > > usb/dev-storage: remove unused include > > > monitor/hmp: uninline add_init_drive > > > monitor/hmp: rename device-hotplug.c to block/monitor/block-hmp-cmds.c > > > monitor/hmp: move hmp_drive_del and hmp_commit to block-hmp-cmds.c > > > monitor/hmp: move hmp_drive_mirror and hmp_drive_backup to > > > block-hmp-cmds.c Moved code was added after 2012-01-13, thus under > > > GPLv2+ > > > monitor/hmp: move hmp_block_job* to block-hmp-cmds.c > > > monitor/hmp: move hmp_snapshot_* to block-hmp-cmds.c > > > hmp_snapshot_blkdev is from GPLv2 version of the hmp-cmds.c thus > > > have to change the licence to GPLv2 > > > monitor/hmp: move hmp_nbd_server* to block-hmp-cmds.c > > > monitor/hmp: move remaining hmp_block* functions to block-hmp-cmds.c > > > monitor/hmp: move hmp_info_block* to block-hmp-cmds.c > > > monitor/hmp: Move hmp_drive_add_node to block-hmp-cmds.c > > > > > > MAINTAINERS|1 + > > > Makefile.objs |2 +- > > > block/Makefile.objs|1 + > > > block/monitor/Makefile.objs|1 + > > > block/monitor/block-hmp-cmds.c | 1002 > > > blockdev.c | 137 + > > > device-hotplug.c | 91 --- > > > hw/usb/dev-storage.c |1 - > > > include/block/block-hmp-cmds.h | 54 ++ > > > include/block/block_int.h |5 +- > > > include/monitor/hmp.h | 24 - > > > include/sysemu/blockdev.h |4 - > > > include/sysemu/sysemu.h|3 - > > > monitor/hmp-cmds.c | 769 > > > monitor/misc.c |1 + > > > 15 files changed, 1072 insertions(+), 1024 deletions(-) > > > create mode 100644 block/monitor/Makefile.objs > > > create mode 100644 block/monitor/block-hmp-cmds.c > > > delete mode 100644 device-hotplug.c > > > create mode 100644 include/block/block-hmp-cmds.h > > > > > > -- > > > 2.17.2 > > > > > > > -- > > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK > > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH v2 for-5.0 0/7] block-copy improvements: part I
On 20.01.20 10:09, Vladimir Sementsov-Ogievskiy wrote: > ping Sorry, I only got to patch 5 so far (without major complaints). I’ll have to pack things up for the weekend now and I’ll be on PTO next week, so I won’t get to review the final two patches before Feb 17, I’m afraid... Max signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 5/7] block/block-copy: rename start to offset in interfaces
On 27.11.19 19:08, Vladimir Sementsov-Ogievskiy wrote: > offset/bytes pair is more usual naming in block layer, let's use it. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > include/block/block-copy.h | 2 +- > block/block-copy.c | 80 +++--- > 2 files changed, 41 insertions(+), 41 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 4/7] block/block-copy: refactor interfaces to use bytes instead of end
On 27.11.19 19:08, Vladimir Sementsov-Ogievskiy wrote:
> We have a lot of "chunk_end - start" invocations, let's switch to
> bytes/cur_bytes scheme instead.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> include/block/block-copy.h | 4 +--
> block/block-copy.c | 68 --
> 2 files changed, 37 insertions(+), 35 deletions(-)
[...]
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 94e7e855ef..cc273b6cb8 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
[...]
> @@ -150,24 +150,26 @@ void block_copy_set_callbacks(
[...]
> static int coroutine_fn block_copy_do_copy(BlockCopyState *s,
> - int64_t start, int64_t end,
> + int64_t start, int64_t bytes,
I wonder whether it would make more sense to make some of these @bytes
parameters plain ints, because...
> bool zeroes, bool *error_is_read)
> {
> int ret;
> -int nbytes = MIN(end, s->len) - start;
> +int nbytes = MIN(start + bytes, s->len) - start;
...things like this look a bit dangerous now. So if the interface
already clearly shows that we’re always expecting something less than
INT_MAX, it might all be a bit clearer.
I’ll leave it up to you:
Reviewed-by: Max Reitz
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2 3/7] block/block-copy: factor out block_copy_find_inflight_req
On 27.11.19 19:08, Vladimir Sementsov-Ogievskiy wrote:
> Split block_copy_find_inflight_req to be used in seprate.
*separate, but separate what?
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/block-copy.c | 31 +++
> 1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 74295d93d5..94e7e855ef 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -24,23 +24,30 @@
> #define BLOCK_COPY_MAX_BUFFER (1 * MiB)
> #define BLOCK_COPY_MAX_MEM (128 * MiB)
>
> +static BlockCopyInFlightReq *block_copy_find_inflight_req(BlockCopyState *s,
Hm. Long already, but the name begs the question what in-flight
requests this is about. Maybe drop the block_copy prefix (unless you
plan to make this function global) and call it “find_inflight_conflict”?
(Or “find_conflicting_inflight_req” to be more verbose)
> + int64_t start,
> + int64_t end)
> +{
> +BlockCopyInFlightReq *req;
> +
> +QLIST_FOREACH(req, &s->inflight_reqs, list) {
> +if (end > req->start_byte && start < req->end_byte) {
> +return req;
> +}
> +}
> +
> +return NULL;
> +}
> +
> static void coroutine_fn block_copy_wait_inflight_reqs(BlockCopyState *s,
> int64_t start,
> int64_t end)
> {
> BlockCopyInFlightReq *req;
> -bool waited;
> -
> -do {
> -waited = false;
> -QLIST_FOREACH(req, &s->inflight_reqs, list) {
> -if (end > req->start_byte && start < req->end_byte) {
> -qemu_co_queue_wait(&req->wait_queue, NULL);
> -waited = true;
> -break;
> -}
> -}
> -} while (waited);
> +
> +while ((req = block_copy_find_inflight_req(s, start, end))) {
> +qemu_co_queue_wait(&req->wait_queue, NULL);
> +}
> }
The change itself looks rather nice to me. Even without other users of
this new function, it turns what’s happening into a more obvious pattern.
Max
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2 2/7] block/block-copy: use block_status
On 27.11.19 19:08, Vladimir Sementsov-Ogievskiy wrote:
> Use bdrv_block_status_above to chose effective chunk size and to handle
> zeroes effectively.
>
> This substitutes checking for just being allocated or not, and drops
> old code path for it. Assistance by backup job is dropped too, as
> caching block-status information is more difficult than just caching
> is-allocated information in our dirty bitmap, and backup job is not
> good place for this caching anyway.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/block-copy.c | 67 +-
> block/trace-events | 1 +
> 2 files changed, 55 insertions(+), 13 deletions(-)
>
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 8602e2cae7..74295d93d5 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
[...]
> @@ -336,23 +375,25 @@ int coroutine_fn block_copy(BlockCopyState *s,
> chunk_end = next_zero;
> }
>
> -if (s->skip_unallocated) {
> -ret = block_copy_reset_unallocated(s, start, &status_bytes);
> -if (ret == 0) {
> -trace_block_copy_skip_range(s, start, status_bytes);
> -start += status_bytes;
> -continue;
> -}
> -/* Clamp to known allocated region */
> -chunk_end = MIN(chunk_end, start + status_bytes);
> +ret = block_copy_block_status(s, start, chunk_end - start,
> + &status_bytes);
> +if (s->skip_unallocated && !(ret & BDRV_BLOCK_ALLOCATED)) {
> +bdrv_reset_dirty_bitmap(s->copy_bitmap, start, status_bytes);
> +s->progress_reset_callback(s->progress_opaque);
> +trace_block_copy_skip_range(s, start, status_bytes);
> +start += status_bytes;
> +continue;
> }
>
> +chunk_end = MIN(chunk_end, start + status_bytes);
I’m not sure how much the old “Clamp to known allocated region” actually
helps, but I wouldn’t drop it anyway.
Apart from this nit, the patch looks good to me.
Max
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2 1/7] block/block-copy: specialcase first copy_range request
On 27.11.19 19:08, Vladimir Sementsov-Ogievskiy wrote:
> In block_copy_do_copy we fallback to read+write if copy_range failed.
> In this case copy_size is larger than defined for buffered IO, and
> there is corresponding commit. Still, backup copies data cluster by
> cluster, and most of requests are limited to one cluster anyway, so the
> only source of this one bad-limited request is copy-before-write
> operation.
>
> Further patch will move backup to use block_copy directly, than for
> cases where copy_range is not supported, first request will be
> oversized in each backup. It's not good, let's change it now.
>
> Fix is simple: just limit first copy_range request like buffer-based
> request. If it succeed, set larger copy_range limit.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/block-copy.c | 41 ++---
> 1 file changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 79798a1567..8602e2cae7 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
[...]
> @@ -168,7 +170,21 @@ static int coroutine_fn
> block_copy_do_copy(BlockCopyState *s,
> s->use_copy_range = false;
> s->copy_size = MAX(s->cluster_size, BLOCK_COPY_MAX_BUFFER);
> /* Fallback to read+write with allocated buffer */
> -} else {
> +} else if (s->use_copy_range) {
> +/*
> + * Successful copy-range. Now increase copy_size.
> + * copy_range does not respect max_transfer (it's a TODO), so we
> + * factor that in here.
> + *
> + * Note: we double-check s->use_copy_range for the case when
> + * parallel block-copy request unset it during previous
> + * bdrv_co_copy_range call.
But we should still goto out anyway, shouldn’t we?
> + */
> +s->copy_size =
> +MIN(MAX(s->cluster_size, BLOCK_COPY_MAX_COPY_RANGE),
> +QEMU_ALIGN_DOWN(block_copy_max_transfer(s->source,
> +s->target),
> +s->cluster_size));
> goto out;
> }
> }
> @@ -176,7 +192,10 @@ static int coroutine_fn
> block_copy_do_copy(BlockCopyState *s,
> /*
> * In case of failed copy_range request above, we may proceed with
> buffered
> * request larger than BLOCK_COPY_MAX_BUFFER. Still, further requests
> will
> - * be properly limited, so don't care too much.
> + * be properly limited, so don't care too much. Moreover the most
> possible
s/possible/likely/
Max
> + * case (copy_range is unsupported for the configuration, so the very
> first
> + * copy_range request fails) is handled by setting large copy_size only
> + * after first successful copy_range.
> */
>
> bounce_buffer = qemu_blockalign(s->source->bs, nbytes);
>
signature.asc
Description: OpenPGP digital signature
Re: [PATCH] block: fix crash on zero-length unaligned write and read
07.02.2020 19:47, Stefan Hajnoczi wrote: On Thu, Feb 06, 2020 at 07:42:45PM +0300, Vladimir Sementsov-Ogievskiy wrote: Commit 7a3f542fbd "block/io: refactor padding" occasionally dropped aligning for zero-length request: bdrv_init_padding() blindly return false if bytes == 0, like there is nothing to align. This leads the following command to crash: ./qemu-io --image-opts -c 'write 1 0' \ driver=blkdebug,align=512,image.driver=null-co,image.size=512 qemu-io: block/io.c:1955: bdrv_aligned_pwritev: Assertion `(offset & (align - 1)) == 0' failed. Aborted (core dumped) Prior to 7a3f542fbd we does aligning of such zero requests. Instead of recovering this behavior let's just do nothing on such requests as it is useless. Note that driver may have special meaning of zero-length reqeusts, like qcow2_co_pwritev_compressed_part, so we can't skip any zero-length operation. But for unaligned ones, we can't pass it to driver anyway. This commit also fixes crash in iotest 80 running with -nocache: ./check -nocache -qcow2 80 which crashes on same assertion due to trying to read empty extra data in qcow2_do_read_snapshots(). Cc: qemu-sta...@nongnu.org # v4.2 Fixes: 7a3f542fbd Signed-off-by: Vladimir Sementsov-Ogievskiy --- block/io.c | 28 +++- 1 file changed, 27 insertions(+), 1 deletion(-) Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Thanks! -- Best regards, Vladimir
Re: [PULL v2 00/46] Python queue 2020-02-07
On Fri, Feb 07, 2020 at 04:11:12PM +0100, Philippe Mathieu-Daudé wrote: > Hi Peter, > > I prepared this series on behalf of Eduardo and > Cleber. > > Eduardo already ack'ed yesterday version (2020-02-06) cover: > https://www.mail-archive.com/qemu-devel@nongnu.org/msg677636.html Acked-by: Eduardo Habkost > > Since 2020-02-06 (v1): > - rebased to cover new iotests #283 (merged yesterday). > > Regards, > > Phil. > > The following changes since commit 863d2ed5823f90c42dcd481687cc99cbc9c4a17c: > > Merge remote-tracking branch 'remotes/maxreitz/tags/pull-block-2020-02-06' > into staging (2020-02-06 16:22:05 +) > > are available in the Git repository at: > > https://gitlab.com/philmd/qemu.git tags/python-next-20200207 > > for you to fetch changes up to 66e7dde18cc4085ca47124be4ca08fa8e6bcdd3a: > > .readthedocs.yml: specify some minimum python requirements (2020-02-07 > 15:15:16 +0100) > -- Eduardo signature.asc Description: PGP signature
Re: [PATCH] block: fix crash on zero-length unaligned write and read
On Thu, Feb 06, 2020 at 07:42:45PM +0300, Vladimir Sementsov-Ogievskiy wrote: > Commit 7a3f542fbd "block/io: refactor padding" occasionally dropped > aligning for zero-length request: bdrv_init_padding() blindly return > false if bytes == 0, like there is nothing to align. > > This leads the following command to crash: > > ./qemu-io --image-opts -c 'write 1 0' \ > driver=blkdebug,align=512,image.driver=null-co,image.size=512 > > >> qemu-io: block/io.c:1955: bdrv_aligned_pwritev: Assertion > `(offset & (align - 1)) == 0' failed. > >> Aborted (core dumped) > > Prior to 7a3f542fbd we does aligning of such zero requests. Instead of > recovering this behavior let's just do nothing on such requests as it > is useless. > > Note that driver may have special meaning of zero-length reqeusts, like > qcow2_co_pwritev_compressed_part, so we can't skip any zero-length > operation. But for unaligned ones, we can't pass it to driver anyway. > > This commit also fixes crash in iotest 80 running with -nocache: > > ./check -nocache -qcow2 80 > > which crashes on same assertion due to trying to read empty extra data > in qcow2_do_read_snapshots(). > > Cc: qemu-sta...@nongnu.org # v4.2 > Fixes: 7a3f542fbd > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/io.c | 28 +++- > 1 file changed, 27 insertions(+), 1 deletion(-) Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan signature.asc Description: PGP signature
Re: [PATCH v3] block/backup-top: fix flags handling
07.02.2020 19:30, Max Reitz wrote: On 07.02.20 17:12, Vladimir Sementsov-Ogievskiy wrote: backup-top "supports" write-unchanged, by skipping CBW operation in backup_top_co_pwritev. But it forgets to do the same in backup_top_co_pwrite_zeroes, as well as declare support for BDRV_REQ_WRITE_UNCHANGED. Fix this, and, while being here, declare also support for flags supported by source child. Signed-off-by: Vladimir Sementsov-Ogievskiy --- v3: rebase on master, keep state initialization after check top != NULL. v2: restrict flags propagation like it is done in other filters [Eric] move state variable initialization to the top block/backup-top.c | 31 --- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/block/backup-top.c b/block/backup-top.c index fa78f3256d..1bfb360bd3 100644 --- a/block/backup-top.c +++ b/block/backup-top.c [...] @@ -196,8 +200,13 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, return NULL; } -top->total_sectors = source->total_sectors; state = top->opaque; +top->total_sectors = source->total_sectors; This looks a bit accidental, but, well, whatever. I failed to restrict myself in a wish to keep all "top->.. =" initializers went together :) Hmm, I could "state =" intializer to go after them. But it's good to keep it at top too. Thanks, applied to my block branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block Thanks! -- Best regards, Vladimir
Re: [PATCH v3] block/backup-top: fix flags handling
On 07.02.20 17:12, Vladimir Sementsov-Ogievskiy wrote: > backup-top "supports" write-unchanged, by skipping CBW operation in > backup_top_co_pwritev. But it forgets to do the same in > backup_top_co_pwrite_zeroes, as well as declare support for > BDRV_REQ_WRITE_UNCHANGED. > > Fix this, and, while being here, declare also support for flags > supported by source child. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > > v3: rebase on master, keep state initialization after check top != NULL. > > v2: restrict flags propagation like it is done in other filters [Eric] > move state variable initialization to the top > block/backup-top.c | 31 --- > 1 file changed, 20 insertions(+), 11 deletions(-) > > diff --git a/block/backup-top.c b/block/backup-top.c > index fa78f3256d..1bfb360bd3 100644 > --- a/block/backup-top.c > +++ b/block/backup-top.c [...] > @@ -196,8 +200,13 @@ BlockDriverState > *bdrv_backup_top_append(BlockDriverState *source, > return NULL; > } > > -top->total_sectors = source->total_sectors; > state = top->opaque; > +top->total_sectors = source->total_sectors; This looks a bit accidental, but, well, whatever. Thanks, applied to my block branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block Max signature.asc Description: OpenPGP digital signature
Re: [PATCH 2/3] MAINTAINERS: Cover qapi/block{-core}.json in 'Block layer core' section
On 2/7/20 5:07 PM, Markus Armbruster wrote: Kevin Wolf writes: Am 07.02.2020 um 15:01 hat Markus Armbruster geschrieben: Philippe Mathieu-Daudé writes: List this file in the proper section, so maintainers get notified when it is modified. Signed-off-by: Philippe Mathieu-Daudé --- Cc: Kevin Wolf Cc: Max Reitz Cc: qemu-block@nongnu.org --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 903831e0a4..e269e9092c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1842,6 +1842,8 @@ S: Supported Block layer core M: Kevin Wolf M: Max Reitz L: qemu-block@nongnu.org S: Supported F: block* F: block/ F: hw/block/ +F: qapi/block.json +F: qapi/block-core.json F: include/block/ F: qemu-img* F: docs/interop/qemu-img.rst This is in addition to Block QAPI, monitor, command line M: Markus Armbruster S: Supported F: blockdev.c F: block/qapi.c F: qapi/block*.json F: qapi/transaction.json T: git https://repo.or.cz/qemu/armbru.git block-next I'm not sure this section makes much sense anymore. This is probably for you to decide. Though the block-next branch from the T: line doesn't even exist any more... I have the questionable habit to delete my -next branches when they're empty. Having dangling -next branches pointing to something very previous is also questionable...
Re: [PATCH v1 2/4] virtio: increase virtuqueue size for virtio-scsi and virtio-blk
On Fri, Feb 07, 2020 at 11:48:05AM +0300, Denis Plotnikov wrote:
>
>
> On 05.02.2020 14:19, Stefan Hajnoczi wrote:
> > On Tue, Feb 04, 2020 at 12:59:04PM +0300, Denis Plotnikov wrote:
> > >
> > > On 30.01.2020 17:58, Stefan Hajnoczi wrote:
> > > > On Wed, Jan 29, 2020 at 05:07:00PM +0300, Denis Plotnikov wrote:
> > > > > The goal is to reduce the amount of requests issued by a guest on
> > > > > 1M reads/writes. This rises the performance up to 4% on that kind of
> > > > > disk access pattern.
> > > > >
> > > > > The maximum chunk size to be used for the guest disk accessing is
> > > > > limited with seg_max parameter, which represents the max amount of
> > > > > pices in the scatter-geather list in one guest disk request.
> > > > >
> > > > > Since seg_max is virqueue_size dependent, increasing the virtqueue
> > > > > size increases seg_max, which, in turn, increases the maximum size
> > > > > of data to be read/write from guest disk.
> > > > >
> > > > > More details in the original problem statment:
> > > > > https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg03721.html
> > > > >
> > > > > Suggested-by: Denis V. Lunev
> > > > > Signed-off-by: Denis Plotnikov
> > > > > ---
> > > > >hw/core/machine.c | 3 +++
> > > > >include/hw/virtio/virtio.h | 2 +-
> > > > >2 files changed, 4 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > index 3e288bfceb..8bc401d8b7 100644
> > > > > --- a/hw/core/machine.c
> > > > > +++ b/hw/core/machine.c
> > > > > @@ -28,6 +28,9 @@
> > > > >#include "hw/mem/nvdimm.h"
> > > > >GlobalProperty hw_compat_4_2[] = {
> > > > > +{ "virtio-blk-device", "queue-size", "128"},
> > > > > +{ "virtio-scsi-device", "virtqueue_size", "128"},
> > > > > +{ "vhost-blk-device", "virtqueue_size", "128"},
> > > > vhost-blk-device?! Who has this? It's not in qemu.git so please omit
> > > > this line. ;-)
> > > So in this case the line:
> > >
> > > { "vhost-blk-device", "seg_max_adjust", "off"},
> > >
> > > introduced by my patch:
> > >
> > > commit 1bf8a989a566b2ba41c197004ec2a02562a766a4
> > > Author: Denis Plotnikov
> > > Date: Fri Dec 20 17:09:04 2019 +0300
> > >
> > > virtio: make seg_max virtqueue size dependent
> > >
> > > is also wrong. It should be:
> > >
> > > { "vhost-scsi-device", "seg_max_adjust", "off"},
> > >
> > > Am I right?
> > It's just called "vhost-scsi":
> >
> > include/hw/virtio/vhost-scsi.h:#define TYPE_VHOST_SCSI "vhost-scsi"
> >
> > > > On the other hand, do you want to do this for the vhost-user-blk,
> > > > vhost-user-scsi, and vhost-scsi devices that exist in qemu.git? Those
> > > > devices would benefit from better performance too.
> After thinking about that for a while, I think we shouldn't extend queue
> sizes for vhost-user-blk, vhost-user-scsi and vhost-scsi.
> This is because increasing the queue sizes seems to be just useless for
> them: the all thing is about increasing the queue sizes for increasing
> seg_max (it limits the max block query size from the guest). For
> virtio-blk-device and virtio-scsi-device it makes sense, since they have
> seg-max-adjust property which, if true, sets seg_max to virtqueue_size-2.
> vhost-scsi also have this property but it seems the property just doesn't
> affect anything (remove it?).
> Also vhost-user-blk, vhost-user-scsi and vhost-scsi don't do any seg_max
> settings. If I understand correctly, their backends are ment to be
> responsible for doing that.
> So, what about changing the queue sizes just for virtio-blk-device and
> virtio-scsi-device?
That's fine. If it's beneficial to extend it to the vhost devices then
it can be done later. I didn't look into it (and the way that
responsibility for these parameters is shared between QEMU and the
vhost-user device backend is a little complicated).
Stefan
signature.asc
Description: PGP signature
[PATCH v3] block/backup-top: fix flags handling
backup-top "supports" write-unchanged, by skipping CBW operation in
backup_top_co_pwritev. But it forgets to do the same in
backup_top_co_pwrite_zeroes, as well as declare support for
BDRV_REQ_WRITE_UNCHANGED.
Fix this, and, while being here, declare also support for flags
supported by source child.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
v3: rebase on master, keep state initialization after check top != NULL.
v2: restrict flags propagation like it is done in other filters [Eric]
move state variable initialization to the top
block/backup-top.c | 31 ---
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/block/backup-top.c b/block/backup-top.c
index fa78f3256d..1bfb360bd3 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -48,11 +48,17 @@ static coroutine_fn int backup_top_co_preadv(
}
static coroutine_fn int backup_top_cbw(BlockDriverState *bs, uint64_t offset,
- uint64_t bytes)
+ uint64_t bytes, BdrvRequestFlags flags)
{
BDRVBackupTopState *s = bs->opaque;
-uint64_t end = QEMU_ALIGN_UP(offset + bytes, s->bcs->cluster_size);
-uint64_t off = QEMU_ALIGN_DOWN(offset, s->bcs->cluster_size);
+uint64_t off, end;
+
+if (flags & BDRV_REQ_WRITE_UNCHANGED) {
+return 0;
+}
+
+off = QEMU_ALIGN_DOWN(offset, s->bcs->cluster_size);
+end = QEMU_ALIGN_UP(offset + bytes, s->bcs->cluster_size);
return block_copy(s->bcs, off, end - off, NULL);
}
@@ -60,7 +66,7 @@ static coroutine_fn int backup_top_cbw(BlockDriverState *bs,
uint64_t offset,
static int coroutine_fn backup_top_co_pdiscard(BlockDriverState *bs,
int64_t offset, int bytes)
{
-int ret = backup_top_cbw(bs, offset, bytes);
+int ret = backup_top_cbw(bs, offset, bytes, 0);
if (ret < 0) {
return ret;
}
@@ -71,7 +77,7 @@ static int coroutine_fn
backup_top_co_pdiscard(BlockDriverState *bs,
static int coroutine_fn backup_top_co_pwrite_zeroes(BlockDriverState *bs,
int64_t offset, int bytes, BdrvRequestFlags flags)
{
-int ret = backup_top_cbw(bs, offset, bytes);
+int ret = backup_top_cbw(bs, offset, bytes, flags);
if (ret < 0) {
return ret;
}
@@ -84,11 +90,9 @@ static coroutine_fn int
backup_top_co_pwritev(BlockDriverState *bs,
uint64_t bytes,
QEMUIOVector *qiov, int flags)
{
-if (!(flags & BDRV_REQ_WRITE_UNCHANGED)) {
-int ret = backup_top_cbw(bs, offset, bytes);
-if (ret < 0) {
-return ret;
-}
+int ret = backup_top_cbw(bs, offset, bytes, flags);
+if (ret < 0) {
+return ret;
}
return bdrv_co_pwritev(bs->backing, offset, bytes, qiov, flags);
@@ -196,8 +200,13 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState
*source,
return NULL;
}
-top->total_sectors = source->total_sectors;
state = top->opaque;
+top->total_sectors = source->total_sectors;
+top->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
+(BDRV_REQ_FUA & source->supported_write_flags);
+top->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
+((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
+ source->supported_zero_flags);
bdrv_ref(target);
state->target = bdrv_attach_child(top, target, "target", &child_file,
errp);
--
2.21.0
Re: [PATCH 2/3] MAINTAINERS: Cover qapi/block{-core}.json in 'Block layer core' section
Kevin Wolf writes: > Am 07.02.2020 um 15:01 hat Markus Armbruster geschrieben: >> Philippe Mathieu-Daudé writes: >> >> > List this file in the proper section, so maintainers get >> > notified when it is modified. >> > >> > Signed-off-by: Philippe Mathieu-Daudé >> > --- >> > Cc: Kevin Wolf >> > Cc: Max Reitz >> > Cc: qemu-block@nongnu.org >> > --- >> > MAINTAINERS | 2 ++ >> > 1 file changed, 2 insertions(+) >> > >> > diff --git a/MAINTAINERS b/MAINTAINERS >> > index 903831e0a4..e269e9092c 100644 >> > --- a/MAINTAINERS >> > +++ b/MAINTAINERS >> > @@ -1842,6 +1842,8 @@ S: Supported >>Block layer core >>M: Kevin Wolf >>M: Max Reitz >>L: qemu-block@nongnu.org >>S: Supported >> > F: block* >> > F: block/ >> > F: hw/block/ >> > +F: qapi/block.json >> > +F: qapi/block-core.json >> > F: include/block/ >> > F: qemu-img* >> > F: docs/interop/qemu-img.rst >> >> This is in addition to >> >> Block QAPI, monitor, command line >> M: Markus Armbruster >> S: Supported >> F: blockdev.c >> F: block/qapi.c >> F: qapi/block*.json >> F: qapi/transaction.json >> T: git https://repo.or.cz/qemu/armbru.git block-next >> >> I'm not sure this section makes much sense anymore. > > This is probably for you to decide. > > Though the block-next branch from the T: line doesn't even exist any > more... I have the questionable habit to delete my -next branches when they're empty. >> Should qapi/transaction.json also be added to "Block layer core"? Or >> should it go into John's section "Block Jobs"? > > I think at the moment it only supports actions that are more related to > block jobs, so moving it there would make sense to me. Alright, what about this: diff --git a/MAINTAINERS b/MAINTAINERS index e72b5e5f69..43e821c901 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1842,6 +1842,8 @@ F: block* F: block/ F: hw/block/ F: include/block/ +F: qapi/block.json +F: qapi/block-core.json F: qemu-img* F: docs/interop/qemu-img.rst F: qemu-io* @@ -1887,16 +1889,8 @@ F: block/commit.c F: block/stream.c F: block/mirror.c F: qapi/job.json -T: git https://github.com/jnsnow/qemu.git jobs - -Block QAPI, monitor, command line -M: Markus Armbruster -S: Supported -F: blockdev.c -F: block/qapi.c -F: qapi/block*.json F: qapi/transaction.json -T: git https://repo.or.cz/qemu/armbru.git block-next +T: git https://github.com/jnsnow/qemu.git jobs Dirty Bitmaps M: John Snow
Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert
07.02.2020 18:12, Max Reitz wrote: On 07.02.20 15:57, Eric Blake wrote: On 2/7/20 8:41 AM, Max Reitz wrote: I could imagine a user creating a qcow2 image on some block device with preallocation where we cannot verify that the result will be zero. But they want qemu not to zero the device, so they would specify --target-is-zero. If user create image, setting --target-is-zero is always valid. But if we in same operation create the image automatically, having --target-is-zero, when we know that what we are creating is not zero is misleading and should fail.. bdrv_has_zero_init() doesn’t return false only for images that we know are not zero. It returns true for images where we know they are. But if we don’t know, then it returns false also. Huh? bdrv_has_zero_init() currently returns 1 if a driver knows that creating an image results in that image reading as 0. That means it can return 1 even for non-zero images that were not just created. Thus, that interface has both false positives (returning 1 for a non-zero image if the driver hard-codes it to 1) and false negatives (returning 0 for an image that happens to read as zero). The false negatives are less important (if we don't know if the image is already zero, then zeroing it again is a waste of time but not semantically wrong) than the false positives (but as long as you don't rely on bdrv_has_zero_init() unless you also know the image was just created, you are safely avoiding the false positives). And that's the whole point of my series to add a qcow2 persistent bit to track whether an image has known-zero contents: qemu-img should not be calling bdrv_has_zero_init(), since it is so finicky on what it means. Sorry, I was unclear. I meant “that we know are not zero immediately after creation”. My point that it may return false even for (newly created) images that are zero stands. One could also say it returns only “yes” (is zero) or “maybe”, and not “yes” or “no”. If we want to add a behavior to skip zeros unconditionally, we should call new option --skip-zeroes, to clearly specify what we want. It was my impression that this was exactly what --target-is-zero means and implies. --target-is-zero turns into the behavior of 'skip a pre-zeroing pass'. If the destination is already zero, then copying zeroes from the source is a waste of time. If the destination is not already zero, then zeroes from the source are not copied, and the destination will not be identical to the source. We then have a choice of whether --target-is-zero is merely a way to tell qemu something that it couldn't learn otherwise but still be as safe as possible (if we can quickly prove the target has non-zero data, the user lied about it being already zero, so fail the command, so add yet another option to bypass the safety check), or whether it really is synonymous with 'only copy the non-zero portions of the source, and if the destination was not all zero the copy is not faithful but I meant for it to be that way'. If you claim that it isn’t supposed to be an unsafe override, and if we plan to take your series in some form or another, it follows that we will have to drop this patch here. Because after your series, qemu can have some insight into existing images (either in the driver’s implementation of make_zero, or in qemu-img itself by virtue of some is_zero function). Therefore, we could do the same “safety check” and see whether our insight agrees on what the user told us. This would make the flag completely superfluous, though, because when qemu knows the image to be zero, it does the right thing anyway. Therefore, I see this flag to be for cases where qemu doesn’t know. And that makes it an unsafe override. IMHO, the flag just sounds wrong for images created by Qemu, and sounds OK for images provided by user (still, it can never be safe, as user may mistake). Still, as I understand, in most of real cases, Qemu actually can determine is-zero by itself, we just didn't have this coded (Eric's series does). And may be we really don't need this arguable flag.. So, really, may be rename it to --skip-zeroes, to make obvious that it is unsafe and user is responsible for the result? Or even to --x-skip-zeroes, and drop it when all needed cases are covered by auto detection? -- Best regards, Vladimir
Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert
On 07.02.20 16:16, Vladimir Sementsov-Ogievskiy wrote:
> 07.02.2020 18:03, Max Reitz wrote:
>> On 07.02.20 15:53, Vladimir Sementsov-Ogievskiy wrote:
>>> 07.02.2020 17:41, Max Reitz wrote:
On 07.02.20 13:07, Vladimir Sementsov-Ogievskiy wrote:
> 07.02.2020 13:33, Max Reitz wrote:
>> On 04.02.20 15:23, Eric Blake wrote:
>>> On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>
> I understand that it is safer to have restrictions now and lift
> them
> later, than to allow use of the option at any time and leave room
> for
> the user to shoot themselves in the foot with no way to add safety
> later. The argument against no backing file is somewhat
> understandable (technically, as long as the backing file also
> reads
> as all zeroes, then the overall image reads as all zeroes - but
> why
> have a backing file that has no content?); the argument
> requiring -n
> is a bit weaker (if I'm creating an image, I _know_ it reads as
> all
> zeroes, so the --target-is-zero argument is redundant, but it
> shouldn't hurt to allow it).
I know that it reads as all zeroes, only if this format provides
zero
initialization..
>
>> +++ b/qemu-img.c
>
>> @@ -2247,6 +2256,11 @@ static int img_convert(int argc, char
>> **argv)
>> warn_report("This will become an error in future
>> QEMU
>> versions.");
>> }
>> + if (s.has_zero_init && !skip_create) {
>> + error_report("--target-is-zero requires use of -n
>> flag");
>> + goto fail_getopt;
>> + }
>
> So I think we could drop this hunk with no change in behavior.
I think, no we can't. If we allow target-is-zero, with -n, we'd
better
to check that what we are creating is zero-initialized (format has
zero-init), and if not we should report error.
>>>
>>> Good call. Yes, if we allow --target-is-zero without -n, we MUST
>>> insist
>>> that bdrv_has_zero_init() returns 1 (or, after my followup series,
>>> bdrv_known_zeroes() includes BDRV_ZERO_CREATE).
>>
>> Why?
>>
>> I could imagine a user creating a qcow2 image on some block device
>> with
>> preallocation where we cannot verify that the result will be
>> zero. But
>> they want qemu not to zero the device, so they would specify
>> --target-is-zero.
>
> If user create image, setting --target-is-zero is always valid. But if
> we in
> same operation create the image automatically, having
> --target-is-zero,
> when
> we know that what we are creating is not zero is misleading and should
> fail..
bdrv_has_zero_init() doesn’t return false only for images that we know
are not zero. It returns true for images where we know they are. But
if we don’t know, then it returns false also.
>>>
>>> yes, but we don't have better check.
>>
>> Correct, but maybe the user knows more, hence why it may make sense for
>> them to provide us with some information we don’t have.
>>
> If we want to add a behavior to skip zeros unconditionally, we should
> call new
> option --skip-zeroes, to clearly specify what we want.
It was my impression that this was exactly what --target-is-zero means
and implies.
>>>
>>> For me it sounds strange that user has better knowledge about what Qemu
>>> creates than Qemu itself. And if it so - it should be fixed in Qemu,
>>> rather than creating user interface to hint Qemu what it does.
>>
>> I brought an example where qemu cannot know whether the image is zero
>> (preallocation on a block device), but the user / management layer might
>> know.
>>
>
> It sounds unsafe for me. User can't know how exactly Qemu do preallocation,
> which syscalls it calls, etc. How can user be sure, that Qemu produces
> all-zero image, if even Qemu doesn't sure in it?
For example, qcow2 images are always zero if the underlying storage is zero.
Then again, it isn’t like we need --target-is-zero without -n anyway.
If users want that, they can always use qemu-img create +
qemu-img convert -n --target-is-zero. So seeing that you’re
uncomfortable with the idea of --target-is-zero with -n, I can’t say
there is any actual reason why we’d have to allow it.
Max
> Otherwise, we should document, how exactly (up to syscalls, their
> parameters, flags, the whole logic and algorithm) preallocation is done,
> so user can analyze it and be sure that produced image would be all-zero
> (when Qemu can't determine it because some specific block device, for which
> Qemu doesn't know that its preallocation algorithm produces all-zero, but
> user is sure in it)..
signature.asc
Description: OpenPGP digital sig
Re: [PATCH v2] block: always fill entire LUKS header space with zeros
On 07.02.20 14:55, Daniel P. Berrangé wrote:
> When initializing the LUKS header the size with default encryption
> parameters will currently be 2068480 bytes. This is rounded up to
> a multiple of the cluster size, 2081792, with 64k sectors. If the
> end of the header is not the same as the end of the cluster we fill
> the extra space with zeros. This was forgetting that not even the
> space allocated for the header will be fully initialized, as we
> only write key material for the first key slot. The space left
> for the other 7 slots is never written to.
>
> An optimization to the ref count checking code:
>
> commit a5fff8d4b4d928311a5005efa12d0991fe3b66f9 (refs/bisect/bad)
> Author: Vladimir Sementsov-Ogievskiy
> Date: Wed Feb 27 16:14:30 2019 +0300
>
> qcow2-refcount: avoid eating RAM
>
> made the assumption that every cluster which was allocated would
> have at least some data written to it. This was violated by way
> the LUKS header is only partially written, with much space simply
> reserved for future use.
>
> Depending on the cluster size this problem was masked by the
> logic which wrote zeros between the end of the LUKS header and
> the end of the cluster.
>
> $ qemu-img create --object secret,id=cluster_encrypt0,data=123456 \
>-f qcow2 -o cluster_size=2k,encrypt.iter-time=1,\
>encrypt.format=luks,encrypt.key-secret=cluster_encrypt0 \
>cluster_size_check.qcow2 100M
> Formatting 'cluster_size_check.qcow2', fmt=qcow2 size=104857600
> encrypt.format=luks encrypt.key-secret=cluster_encrypt0
> encrypt.iter-time=1 cluster_size=2048 lazy_refcounts=off refcount_bits=16
>
> $ qemu-img check --object secret,id=cluster_encrypt0,data=redhat \
> 'json:{"driver": "qcow2", "encrypt.format": "luks", \
>"encrypt.key-secret": "cluster_encrypt0", \
>"file.driver": "file", "file.filename":
> "cluster_size_check.qcow2"}'
> ERROR: counting reference for region exceeding the end of the file by one
> cluster or more: offset 0x2000 size 0x1f9000
> Leaked cluster 4 refcount=1 reference=0
> ...snip...
> Leaked cluster 130 refcount=1 reference=0
>
> 1 errors were found on the image.
> Data may be corrupted, or further writes to the image may corrupt it.
>
> 127 leaked clusters were found on the image.
> This means waste of disk space, but no harm to data.
> Image end offset: 268288
>
> The problem only exists when the disk image is entirely empty. Writing
> data to the disk image payload will solve the problem by causing the
> end of the file to be extended further.
>
> The change fixes it by ensuring that the entire allocated LUKS header
> region is fully initialized with zeros. The qemu-img check will still
> fail for any pre-existing disk images created prior to this change,
> unless at least 1 byte of the payload is written to.
>
> Fully writing zeros to the entire LUKS header is a good idea regardless
> as it ensures that space has been allocated on the host filesystem (or
> whatever block storage backend is used).
>
> Signed-off-by: Daniel P. Berrangé
> ---
>
> In v2:
>
> - Cut down test scenarios to speed up
> - Use _check_test_img helper
> - Fix outdated comments
> - Use space not tabs
Using eight spaces for indentation is... Interesting, but at least
consistent. :-)
> block/qcow2.c | 11 +++--
> tests/qemu-iotests/284 | 97 ++
> tests/qemu-iotests/284.out | 62
> tests/qemu-iotests/group | 1 +
> 4 files changed, 167 insertions(+), 4 deletions(-)
> create mode 100755 tests/qemu-iotests/284
> create mode 100644 tests/qemu-iotests/284.out
Thanks, applied to my block branch:
https://git.xanclic.moe/XanClic/qemu/commits/branch/block
Max
signature.asc
Description: OpenPGP digital signature
[PULL v2 00/46] Python queue 2020-02-07
Hi Peter, I prepared this series on behalf of Eduardo and Cleber. Eduardo already ack'ed yesterday version (2020-02-06) cover: https://www.mail-archive.com/qemu-devel@nongnu.org/msg677636.html Since 2020-02-06 (v1): - rebased to cover new iotests #283 (merged yesterday). Regards, Phil. The following changes since commit 863d2ed5823f90c42dcd481687cc99cbc9c4a17c: Merge remote-tracking branch 'remotes/maxreitz/tags/pull-block-2020-02-06' into staging (2020-02-06 16:22:05 +) are available in the Git repository at: https://gitlab.com/philmd/qemu.git tags/python-next-20200207 for you to fetch changes up to 66e7dde18cc4085ca47124be4ca08fa8e6bcdd3a: .readthedocs.yml: specify some minimum python requirements (2020-02-07 15:15:16 +0100) - Python 3 cleanups: . Remove text about Python 2 in qemu-deprecated (Thomas) . Remove shebang header (Paolo, Philippe) . scripts/checkpatch.pl now allows Python 3 interpreter (Philippe) . Explicit usage of Python 3 interpreter in scripts (Philippe) . Fix Python scripts permissions (Paolo, Philippe) . Drop 'from __future__ import print_function' (Paolo) . Specify minimum python requirements in ReadTheDocs configuration (Alex) - Test UNIX/EXEC transports with migration (Oksana) - Added extract_from_rpm helper, improved extract_from_deb (Liam) - Allow to use other serial consoles than default one (Philippe) - Various improvements in QEMUMonitorProtocol (Wainer) - Fix kvm_available() on ppc64le (Wainer) Alex Bennée (1): .readthedocs.yml: specify some minimum python requirements Denis Plotnikov (1): tests: rename virtio_seg_max_adjust to virtio_check_params Liam Merwick (4): travis.yml: install rpm2cpio for acceptance tests tests/boot_linux_console: add extract_from_rpm method tests/boot_linux_console: use os.path for filesystem paths tests/boot_linux_console: fix extract_from_deb() comment Lukáš Doktor (1): python: Treat None-return of greeting cmd Oksana Vohchana (4): tests/acceptance/migration: Factor out assert_migration() tests/acceptance/migration: Factor out do_migrate() tests/acceptance/migration: Test UNIX transport when migrating tests/acceptance/migration: Test EXEC transport when migrating Paolo Bonzini (3): scripts/signrom: remove Python 2 support, add shebang make all Python scripts executable drop "from __future__ import print_function" Philippe Mathieu-Daudé (24): python/qemu/machine: Allow to use other serial consoles than default Acceptance tests: Extract _console_interaction() Acceptance tests: Add interrupt_interactive_console_until_pattern() tests/boot_linux_console: Tag Emcraft Smartfusion2 as running 'u-boot' tests/acceptance/virtio_check_params: Improve exception logging tests/acceptance/virtio_check_params: List machine being tested tests/acceptance/virtio_check_params: Default to -nodefaults tests/acceptance/virtio_check_params: Disable the test tests/acceptance/boot_linux_console: Do not use VGA on Clipper machine tests/acceptance/version: Default to -nodefaults tests/acceptance/migration: Add the 'migration' tag tests/acceptance/migration: Default to -nodefaults scripts/checkpatch.pl: Only allow Python 3 interpreter tests/qemu-iotests/check: Allow use of python3 interpreter tests/qemu-iotests: Explicit usage of Python 3 (scripts with __main__) tests: Explicit usage of Python 3 scripts: Explicit usage of Python 3 (scripts with __main__) scripts/minikconf: Explicit usage of Python 3 scripts/tracetool: Remove shebang header tests/acceptance: Remove shebang header tests/vm: Remove shebang header tests/qemu-iotests: Explicit usage of Python3 (scripts without __main__) scripts: Explicit usage of Python 3 (scripts without __main__) tests/qemu-iotests/check: Only check for Python 3 interpreter Thomas Huth (2): qemu-deprecated: Remove text about Python 2 tests/acceptance: Add boot tests for some of the QEMU advent calendar images Wainer dos Santos Moschetta (6): python/qemu: qmp: Replace socket.error with OSError python/qemu: Delint the qmp module python/qemu: qmp: Make accept()'s timeout configurable python/qemu: qmp: Make QEMUMonitorProtocol a context manager python/qemu: qmp: Remove unnused attributes python/qemu: accel: Fix kvm_available() on ppc64le qemu-deprecated.texi | 8 -- .readthedocs.yml | 20 +++ .travis.yml | 3 +- python/qemu/accel.py | 3 +- python/qemu/machine.py| 10 +- python/qemu/qmp.py| 99 ++ scripts/analyse-9p-simpletrace.py | 3 +- scripts/analyse-locks-simpletrace.py | 3 +- scripts/checkpat
Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert
On 07.02.20 15:57, Eric Blake wrote: > On 2/7/20 8:41 AM, Max Reitz wrote: > I could imagine a user creating a qcow2 image on some block device with preallocation where we cannot verify that the result will be zero. But they want qemu not to zero the device, so they would specify --target-is-zero. >>> >>> If user create image, setting --target-is-zero is always valid. But if >>> we in >>> same operation create the image automatically, having --target-is-zero, >>> when >>> we know that what we are creating is not zero is misleading and should >>> fail.. >> >> bdrv_has_zero_init() doesn’t return false only for images that we know >> are not zero. It returns true for images where we know they are. But >> if we don’t know, then it returns false also. > > Huh? > > bdrv_has_zero_init() currently returns 1 if a driver knows that creating > an image results in that image reading as 0. That means it can return 1 > even for non-zero images that were not just created. Thus, that > interface has both false positives (returning 1 for a non-zero image if > the driver hard-codes it to 1) and false negatives (returning 0 for an > image that happens to read as zero). The false negatives are less > important (if we don't know if the image is already zero, then zeroing > it again is a waste of time but not semantically wrong) than the false > positives (but as long as you don't rely on bdrv_has_zero_init() unless > you also know the image was just created, you are safely avoiding the > false positives). > > And that's the whole point of my series to add a qcow2 persistent bit to > track whether an image has known-zero contents: qemu-img should not be > calling bdrv_has_zero_init(), since it is so finicky on what it means. Sorry, I was unclear. I meant “that we know are not zero immediately after creation”. My point that it may return false even for (newly created) images that are zero stands. One could also say it returns only “yes” (is zero) or “maybe”, and not “yes” or “no”. >>> If we want to add a behavior to skip zeros unconditionally, we should >>> call new >>> option --skip-zeroes, to clearly specify what we want. >> >> It was my impression that this was exactly what --target-is-zero means >> and implies. > > --target-is-zero turns into the behavior of 'skip a pre-zeroing pass'. > If the destination is already zero, then copying zeroes from the source > is a waste of time. If the destination is not already zero, then zeroes > from the source are not copied, and the destination will not be > identical to the source. We then have a choice of whether > --target-is-zero is merely a way to tell qemu something that it couldn't > learn otherwise but still be as safe as possible (if we can quickly > prove the target has non-zero data, the user lied about it being already > zero, so fail the command, so add yet another option to bypass the > safety check), or whether it really is synonymous with 'only copy the > non-zero portions of the source, and if the destination was not all zero > the copy is not faithful but I meant for it to be that way'. If you claim that it isn’t supposed to be an unsafe override, and if we plan to take your series in some form or another, it follows that we will have to drop this patch here. Because after your series, qemu can have some insight into existing images (either in the driver’s implementation of make_zero, or in qemu-img itself by virtue of some is_zero function). Therefore, we could do the same “safety check” and see whether our insight agrees on what the user told us. This would make the flag completely superfluous, though, because when qemu knows the image to be zero, it does the right thing anyway. Therefore, I see this flag to be for cases where qemu doesn’t know. And that makes it an unsafe override. Max signature.asc Description: OpenPGP digital signature
Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert
07.02.2020 18:03, Max Reitz wrote:
On 07.02.20 15:53, Vladimir Sementsov-Ogievskiy wrote:
07.02.2020 17:41, Max Reitz wrote:
On 07.02.20 13:07, Vladimir Sementsov-Ogievskiy wrote:
07.02.2020 13:33, Max Reitz wrote:
On 04.02.20 15:23, Eric Blake wrote:
On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:
I understand that it is safer to have restrictions now and lift them
later, than to allow use of the option at any time and leave room
for
the user to shoot themselves in the foot with no way to add safety
later. The argument against no backing file is somewhat
understandable (technically, as long as the backing file also reads
as all zeroes, then the overall image reads as all zeroes - but why
have a backing file that has no content?); the argument requiring -n
is a bit weaker (if I'm creating an image, I _know_ it reads as all
zeroes, so the --target-is-zero argument is redundant, but it
shouldn't hurt to allow it).
I know that it reads as all zeroes, only if this format provides zero
initialization..
+++ b/qemu-img.c
@@ -2247,6 +2256,11 @@ static int img_convert(int argc, char
**argv)
warn_report("This will become an error in future QEMU
versions.");
}
+ if (s.has_zero_init && !skip_create) {
+ error_report("--target-is-zero requires use of -n flag");
+ goto fail_getopt;
+ }
So I think we could drop this hunk with no change in behavior.
I think, no we can't. If we allow target-is-zero, with -n, we'd
better
to check that what we are creating is zero-initialized (format has
zero-init), and if not we should report error.
Good call. Yes, if we allow --target-is-zero without -n, we MUST
insist
that bdrv_has_zero_init() returns 1 (or, after my followup series,
bdrv_known_zeroes() includes BDRV_ZERO_CREATE).
Why?
I could imagine a user creating a qcow2 image on some block device with
preallocation where we cannot verify that the result will be zero. But
they want qemu not to zero the device, so they would specify
--target-is-zero.
If user create image, setting --target-is-zero is always valid. But if
we in
same operation create the image automatically, having --target-is-zero,
when
we know that what we are creating is not zero is misleading and should
fail..
bdrv_has_zero_init() doesn’t return false only for images that we know
are not zero. It returns true for images where we know they are. But
if we don’t know, then it returns false also.
yes, but we don't have better check.
Correct, but maybe the user knows more, hence why it may make sense for
them to provide us with some information we don’t have.
If we want to add a behavior to skip zeros unconditionally, we should
call new
option --skip-zeroes, to clearly specify what we want.
It was my impression that this was exactly what --target-is-zero means
and implies.
For me it sounds strange that user has better knowledge about what Qemu
creates than Qemu itself. And if it so - it should be fixed in Qemu,
rather than creating user interface to hint Qemu what it does.
I brought an example where qemu cannot know whether the image is zero
(preallocation on a block device), but the user / management layer might
know.
It sounds unsafe for me. User can't know how exactly Qemu do preallocation,
which syscalls it calls, etc. How can user be sure, that Qemu produces
all-zero image, if even Qemu doesn't sure in it?
Otherwise, we should document, how exactly (up to syscalls, their
parameters, flags, the whole logic and algorithm) preallocation is done,
so user can analyze it and be sure that produced image would be all-zero
(when Qemu can't determine it because some specific block device, for which
Qemu doesn't know that its preallocation algorithm produces all-zero, but
user is sure in it)...
--
Best regards,
Vladimir
Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert
On 07.02.20 15:53, Vladimir Sementsov-Ogievskiy wrote:
> 07.02.2020 17:41, Max Reitz wrote:
>> On 07.02.20 13:07, Vladimir Sementsov-Ogievskiy wrote:
>>> 07.02.2020 13:33, Max Reitz wrote:
On 04.02.20 15:23, Eric Blake wrote:
> On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:
>
>>> I understand that it is safer to have restrictions now and lift them
>>> later, than to allow use of the option at any time and leave room
>>> for
>>> the user to shoot themselves in the foot with no way to add safety
>>> later. The argument against no backing file is somewhat
>>> understandable (technically, as long as the backing file also reads
>>> as all zeroes, then the overall image reads as all zeroes - but why
>>> have a backing file that has no content?); the argument requiring -n
>>> is a bit weaker (if I'm creating an image, I _know_ it reads as all
>>> zeroes, so the --target-is-zero argument is redundant, but it
>>> shouldn't hurt to allow it).
>>
>> I know that it reads as all zeroes, only if this format provides zero
>> initialization..
>>
>>>
+++ b/qemu-img.c
>>>
@@ -2247,6 +2256,11 @@ static int img_convert(int argc, char
**argv)
warn_report("This will become an error in future QEMU
versions.");
}
+ if (s.has_zero_init && !skip_create) {
+ error_report("--target-is-zero requires use of -n flag");
+ goto fail_getopt;
+ }
>>>
>>> So I think we could drop this hunk with no change in behavior.
>>
>> I think, no we can't. If we allow target-is-zero, with -n, we'd
>> better
>> to check that what we are creating is zero-initialized (format has
>> zero-init), and if not we should report error.
>>
>
> Good call. Yes, if we allow --target-is-zero without -n, we MUST
> insist
> that bdrv_has_zero_init() returns 1 (or, after my followup series,
> bdrv_known_zeroes() includes BDRV_ZERO_CREATE).
Why?
I could imagine a user creating a qcow2 image on some block device with
preallocation where we cannot verify that the result will be zero. But
they want qemu not to zero the device, so they would specify
--target-is-zero.
>>>
>>> If user create image, setting --target-is-zero is always valid. But if
>>> we in
>>> same operation create the image automatically, having --target-is-zero,
>>> when
>>> we know that what we are creating is not zero is misleading and should
>>> fail..
>>
>> bdrv_has_zero_init() doesn’t return false only for images that we know
>> are not zero. It returns true for images where we know they are. But
>> if we don’t know, then it returns false also.
>
> yes, but we don't have better check.
Correct, but maybe the user knows more, hence why it may make sense for
them to provide us with some information we don’t have.
>>> If we want to add a behavior to skip zeros unconditionally, we should
>>> call new
>>> option --skip-zeroes, to clearly specify what we want.
>>
>> It was my impression that this was exactly what --target-is-zero means
>> and implies.
>>
>
> For me it sounds strange that user has better knowledge about what Qemu
> creates than Qemu itself. And if it so - it should be fixed in Qemu,
> rather than creating user interface to hint Qemu what it does.
I brought an example where qemu cannot know whether the image is zero
(preallocation on a block device), but the user / management layer might
know.
Max
signature.asc
Description: OpenPGP digital signature
[PULL v2 40/46] tests/qemu-iotests: Explicit usage of Python3 (scripts without __main__)
Use the program search path to find the Python 3 interpreter. Patch created mechanically by running: $ sed -i "s,^#\!/usr/bin/\(env\ \)\?python$,#\!/usr/bin/env python3," \ $(git grep -lF '#!/usr/bin/env python' \ | xargs grep -L 'if __name__.*__main__') Reported-by: Vladimir Sementsov-Ogievskiy Suggested-by: Daniel P. Berrangé Suggested-by: Stefan Hajnoczi Acked-by: Stefan Hajnoczi Acked-by: Paolo Bonzini Message-Id: <20200130163232.10446-11-phi...@redhat.com> Signed-off-by: Philippe Mathieu-Daudé --- v2: Rebased to include tests/qemu-iotests/283 tests/qemu-iotests/149 | 2 +- tests/qemu-iotests/194 | 2 +- tests/qemu-iotests/202 | 2 +- tests/qemu-iotests/203 | 2 +- tests/qemu-iotests/206 | 2 +- tests/qemu-iotests/207 | 2 +- tests/qemu-iotests/208 | 2 +- tests/qemu-iotests/209 | 2 +- tests/qemu-iotests/210 | 2 +- tests/qemu-iotests/211 | 2 +- tests/qemu-iotests/212 | 2 +- tests/qemu-iotests/213 | 2 +- tests/qemu-iotests/216 | 2 +- tests/qemu-iotests/218 | 2 +- tests/qemu-iotests/219 | 2 +- tests/qemu-iotests/222 | 2 +- tests/qemu-iotests/224 | 2 +- tests/qemu-iotests/228 | 2 +- tests/qemu-iotests/234 | 2 +- tests/qemu-iotests/235 | 2 +- tests/qemu-iotests/236 | 2 +- tests/qemu-iotests/237 | 2 +- tests/qemu-iotests/238 | 2 +- tests/qemu-iotests/242 | 2 +- tests/qemu-iotests/246 | 2 +- tests/qemu-iotests/248 | 2 +- tests/qemu-iotests/254 | 2 +- tests/qemu-iotests/255 | 2 +- tests/qemu-iotests/256 | 2 +- tests/qemu-iotests/260 | 2 +- tests/qemu-iotests/262 | 2 +- tests/qemu-iotests/264 | 2 +- tests/qemu-iotests/266 | 2 +- tests/qemu-iotests/277 | 2 +- tests/qemu-iotests/280 | 2 +- tests/qemu-iotests/283 | 2 +- 36 files changed, 36 insertions(+), 36 deletions(-) diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149 index 8ab42e94c6..0a7b765d07 100755 --- a/tests/qemu-iotests/149 +++ b/tests/qemu-iotests/149 @@ -1,4 +1,4 @@ -#!/usr/bin/env python +#!/usr/bin/env python3 # # Copyright (C) 2016 Red Hat, Inc. # diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194 index 72e47e8833..9dc1bd3510 100755 --- a/tests/qemu-iotests/194 +++ b/tests/qemu-iotests/194 @@ -1,4 +1,4 @@ -#!/usr/bin/env python +#!/usr/bin/env python3 # # Copyright (C) 2017 Red Hat, Inc. # diff --git a/tests/qemu-iotests/202 b/tests/qemu-iotests/202 index 581ca34d79..920a8683ef 100755 --- a/tests/qemu-iotests/202 +++ b/tests/qemu-iotests/202 @@ -1,4 +1,4 @@ -#!/usr/bin/env python +#!/usr/bin/env python3 # # Copyright (C) 2017 Red Hat, Inc. # diff --git a/tests/qemu-iotests/203 b/tests/qemu-iotests/203 index 4874a1a0d8..49eff5d405 100755 --- a/tests/qemu-iotests/203 +++ b/tests/qemu-iotests/203 @@ -1,4 +1,4 @@ -#!/usr/bin/env python +#!/usr/bin/env python3 # # Copyright (C) 2017 Red Hat, Inc. # diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206 index 9f16a7df8d..e2b50ae24d 100755 --- a/tests/qemu-iotests/206 +++ b/tests/qemu-iotests/206 @@ -1,4 +1,4 @@ -#!/usr/bin/env python +#!/usr/bin/env python3 # # Test qcow2 and file image creation # diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207 index 812ab34e47..3d9c1208ca 100755 --- a/tests/qemu-iotests/207 +++ b/tests/qemu-iotests/207 @@ -1,4 +1,4 @@ -#!/usr/bin/env python +#!/usr/bin/env python3 # # Test ssh image creation # diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208 index 546eb1de3e..1c3fc8c7fd 100755 --- a/tests/qemu-iotests/208 +++ b/tests/qemu-iotests/208 @@ -1,4 +1,4 @@ -#!/usr/bin/env python +#!/usr/bin/env python3 # # Copyright (C) 2018 Red Hat, Inc. # diff --git a/tests/qemu-iotests/209 b/tests/qemu-iotests/209 index e0f464bcbe..65c1a1e70a 100755 --- a/tests/qemu-iotests/209 +++ b/tests/qemu-iotests/209 @@ -1,4 +1,4 @@ -#!/usr/bin/env python +#!/usr/bin/env python3 # # Tests for NBD BLOCK_STATUS extension # diff --git a/tests/qemu-iotests/210 b/tests/qemu-iotests/210 index 4ca0fe26ef..e49896e23d 100755 --- a/tests/qemu-iotests/210 +++ b/tests/qemu-iotests/210 @@ -1,4 +1,4 @@ -#!/usr/bin/env python +#!/usr/bin/env python3 # # Test luks and file image creation # diff --git a/tests/qemu-iotests/211 b/tests/qemu-iotests/211 index 8834ebfe85..163994d559 100755 --- a/tests/qemu-iotests/211 +++ b/tests/qemu-iotests/211 @@ -1,4 +1,4 @@ -#!/usr/bin/env python +#!/usr/bin/env python3 # # Test VDI and file image creation # diff --git a/tests/qemu-iotests/212 b/tests/qemu-iotests/212 index 8f3ccc7b15..800f92dd84 100755 --- a/tests/qemu-iotests/212 +++ b/tests/qemu-iotests/212 @@ -1,4 +1,4 @@ -#!/usr/bin/env python +#!/usr/bin/env python3 # # Test parallels and file image creation # diff --git a/tests/qemu-iotests/213 b/tests/qemu-iotests/213 index 3fc8dc6eaa..1eee45276a 100755 --- a/tests/qemu-iotests/213 +++ b/tests/qemu-iotests/213 @@ -1,4 +1,4 @@ -#!/usr/bin/env python +#!/usr/bin/env python3 # # Test vhdx and file image creation # diff --git a/tests/qemu-iotests/216 b/tests/qemu-iotests/216 index 3c0ae54b44..372f042d3e 100755 --- a/tests/qemu-iotests/216
Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert
On 2/7/20 8:41 AM, Max Reitz wrote: I could imagine a user creating a qcow2 image on some block device with preallocation where we cannot verify that the result will be zero. But they want qemu not to zero the device, so they would specify --target-is-zero. If user create image, setting --target-is-zero is always valid. But if we in same operation create the image automatically, having --target-is-zero, when we know that what we are creating is not zero is misleading and should fail.. bdrv_has_zero_init() doesn’t return false only for images that we know are not zero. It returns true for images where we know they are. But if we don’t know, then it returns false also. Huh? bdrv_has_zero_init() currently returns 1 if a driver knows that creating an image results in that image reading as 0. That means it can return 1 even for non-zero images that were not just created. Thus, that interface has both false positives (returning 1 for a non-zero image if the driver hard-codes it to 1) and false negatives (returning 0 for an image that happens to read as zero). The false negatives are less important (if we don't know if the image is already zero, then zeroing it again is a waste of time but not semantically wrong) than the false positives (but as long as you don't rely on bdrv_has_zero_init() unless you also know the image was just created, you are safely avoiding the false positives). And that's the whole point of my series to add a qcow2 persistent bit to track whether an image has known-zero contents: qemu-img should not be calling bdrv_has_zero_init(), since it is so finicky on what it means. If we want to add a behavior to skip zeros unconditionally, we should call new option --skip-zeroes, to clearly specify what we want. It was my impression that this was exactly what --target-is-zero means and implies. --target-is-zero turns into the behavior of 'skip a pre-zeroing pass'. If the destination is already zero, then copying zeroes from the source is a waste of time. If the destination is not already zero, then zeroes from the source are not copied, and the destination will not be identical to the source. We then have a choice of whether --target-is-zero is merely a way to tell qemu something that it couldn't learn otherwise but still be as safe as possible (if we can quickly prove the target has non-zero data, the user lied about it being already zero, so fail the command, so add yet another option to bypass the safety check), or whether it really is synonymous with 'only copy the non-zero portions of the source, and if the destination was not all zero the copy is not faithful but I meant for it to be that way'. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert
On 07.02.20 13:07, Vladimir Sementsov-Ogievskiy wrote:
> 07.02.2020 13:33, Max Reitz wrote:
>> On 04.02.20 15:23, Eric Blake wrote:
>>> On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>
> I understand that it is safer to have restrictions now and lift them
> later, than to allow use of the option at any time and leave room for
> the user to shoot themselves in the foot with no way to add safety
> later. The argument against no backing file is somewhat
> understandable (technically, as long as the backing file also reads
> as all zeroes, then the overall image reads as all zeroes - but why
> have a backing file that has no content?); the argument requiring -n
> is a bit weaker (if I'm creating an image, I _know_ it reads as all
> zeroes, so the --target-is-zero argument is redundant, but it
> shouldn't hurt to allow it).
I know that it reads as all zeroes, only if this format provides zero
initialization..
>
>> +++ b/qemu-img.c
>
>> @@ -2247,6 +2256,11 @@ static int img_convert(int argc, char **argv)
>> warn_report("This will become an error in future QEMU
>> versions.");
>> }
>> + if (s.has_zero_init && !skip_create) {
>> + error_report("--target-is-zero requires use of -n flag");
>> + goto fail_getopt;
>> + }
>
> So I think we could drop this hunk with no change in behavior.
I think, no we can't. If we allow target-is-zero, with -n, we'd better
to check that what we are creating is zero-initialized (format has
zero-init), and if not we should report error.
>>>
>>> Good call. Yes, if we allow --target-is-zero without -n, we MUST insist
>>> that bdrv_has_zero_init() returns 1 (or, after my followup series,
>>> bdrv_known_zeroes() includes BDRV_ZERO_CREATE).
>>
>> Why?
>>
>> I could imagine a user creating a qcow2 image on some block device with
>> preallocation where we cannot verify that the result will be zero. But
>> they want qemu not to zero the device, so they would specify
>> --target-is-zero.
>
> If user create image, setting --target-is-zero is always valid. But if
> we in
> same operation create the image automatically, having --target-is-zero,
> when
> we know that what we are creating is not zero is misleading and should
> fail..
bdrv_has_zero_init() doesn’t return false only for images that we know
are not zero. It returns true for images where we know they are. But
if we don’t know, then it returns false also.
> If we want to add a behavior to skip zeros unconditionally, we should
> call new
> option --skip-zeroes, to clearly specify what we want.
It was my impression that this was exactly what --target-is-zero means
and implies.
Max
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert
07.02.2020 17:41, Max Reitz wrote:
On 07.02.20 13:07, Vladimir Sementsov-Ogievskiy wrote:
07.02.2020 13:33, Max Reitz wrote:
On 04.02.20 15:23, Eric Blake wrote:
On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:
I understand that it is safer to have restrictions now and lift them
later, than to allow use of the option at any time and leave room for
the user to shoot themselves in the foot with no way to add safety
later. The argument against no backing file is somewhat
understandable (technically, as long as the backing file also reads
as all zeroes, then the overall image reads as all zeroes - but why
have a backing file that has no content?); the argument requiring -n
is a bit weaker (if I'm creating an image, I _know_ it reads as all
zeroes, so the --target-is-zero argument is redundant, but it
shouldn't hurt to allow it).
I know that it reads as all zeroes, only if this format provides zero
initialization..
+++ b/qemu-img.c
@@ -2247,6 +2256,11 @@ static int img_convert(int argc, char **argv)
warn_report("This will become an error in future QEMU
versions.");
}
+ if (s.has_zero_init && !skip_create) {
+ error_report("--target-is-zero requires use of -n flag");
+ goto fail_getopt;
+ }
So I think we could drop this hunk with no change in behavior.
I think, no we can't. If we allow target-is-zero, with -n, we'd better
to check that what we are creating is zero-initialized (format has
zero-init), and if not we should report error.
Good call. Yes, if we allow --target-is-zero without -n, we MUST insist
that bdrv_has_zero_init() returns 1 (or, after my followup series,
bdrv_known_zeroes() includes BDRV_ZERO_CREATE).
Why?
I could imagine a user creating a qcow2 image on some block device with
preallocation where we cannot verify that the result will be zero. But
they want qemu not to zero the device, so they would specify
--target-is-zero.
If user create image, setting --target-is-zero is always valid. But if
we in
same operation create the image automatically, having --target-is-zero,
when
we know that what we are creating is not zero is misleading and should
fail..
bdrv_has_zero_init() doesn’t return false only for images that we know
are not zero. It returns true for images where we know they are. But
if we don’t know, then it returns false also.
yes, but we don't have better check.
If we want to add a behavior to skip zeros unconditionally, we should
call new
option --skip-zeroes, to clearly specify what we want.
It was my impression that this was exactly what --target-is-zero means
and implies.
For me it sounds strange that user has better knowledge about what Qemu
creates than Qemu itself. And if it so - it should be fixed in Qemu,
rather than creating user interface to hint Qemu what it does.
--
Best regards,
Vladimir
Re: [PATCH v2] block: always fill entire LUKS header space with zeros
On 2/7/20 7:55 AM, Daniel P. Berrangé wrote:
When initializing the LUKS header the size with default encryption
parameters will currently be 2068480 bytes. This is rounded up to
a multiple of the cluster size, 2081792, with 64k sectors. If the
end of the header is not the same as the end of the cluster we fill
the extra space with zeros. This was forgetting that not even the
space allocated for the header will be fully initialized, as we
only write key material for the first key slot. The space left
for the other 7 slots is never written to.
The problem only exists when the disk image is entirely empty. Writing
data to the disk image payload will solve the problem by causing the
end of the file to be extended further.
The change fixes it by ensuring that the entire allocated LUKS header
region is fully initialized with zeros. The qemu-img check will still
fail for any pre-existing disk images created prior to this change,
unless at least 1 byte of the payload is written to.
Fully writing zeros to the entire LUKS header is a good idea regardless
as it ensures that space has been allocated on the host filesystem (or
whatever block storage backend is used).
What's more, we avoid a possible bug where creating a LUKS image backed
by a block device protocol where the block device happens to already
contain stale data from an earlier use of that block device in a
different LUKS image, which could make it appear as though we have
populated key slots. It's unlikely that those other slots would decode
the current image correctly (as the stale keyslot would decode to a
different master key), but being able to supply the passphrase to that
stale keyslot to decode garbage out of the new image does not seem
desirable.
Signed-off-by: Daniel P. Berrangé
---
+++ b/block/qcow2.c
@@ -135,13 +135,16 @@ static ssize_t qcow2_crypto_hdr_init_func(QCryptoBlock
*block, size_t headerlen,
s->crypto_header.length = headerlen;
s->crypto_header.offset = ret;
-/* Zero fill remaining space in cluster so it has predictable
- * content in case of future spec changes */
+/*
+ * Zero fill all space in cluster so it has predictable
+ * content, as we may not initialize some regions of the
+ * header (eg only 1 out of 8 key slots will be initialized)
+ */
clusterlen = size_to_clusters(s, headerlen) * s->cluster_size;
assert(qcow2_pre_write_overlap_check(bs, 0, ret, clusterlen, false) == 0);
ret = bdrv_pwrite_zeroes(bs->file,
- ret + headerlen,
- clusterlen - headerlen, 0);
+ ret,
+ clusterlen, 0);
if (ret < 0) {
error_setg_errno(errp, -ret, "Could not zero fill encryption header");
return -1;
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 2/3] MAINTAINERS: Cover qapi/block{-core}.json in 'Block layer core' section
Am 07.02.2020 um 15:01 hat Markus Armbruster geschrieben: > Philippe Mathieu-Daudé writes: > > > List this file in the proper section, so maintainers get > > notified when it is modified. > > > > Signed-off-by: Philippe Mathieu-Daudé > > --- > > Cc: Kevin Wolf > > Cc: Max Reitz > > Cc: qemu-block@nongnu.org > > --- > > MAINTAINERS | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 903831e0a4..e269e9092c 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -1842,6 +1842,8 @@ S: Supported >Block layer core >M: Kevin Wolf >M: Max Reitz >L: qemu-block@nongnu.org >S: Supported > > F: block* > > F: block/ > > F: hw/block/ > > +F: qapi/block.json > > +F: qapi/block-core.json > > F: include/block/ > > F: qemu-img* > > F: docs/interop/qemu-img.rst > > This is in addition to > > Block QAPI, monitor, command line > M: Markus Armbruster > S: Supported > F: blockdev.c > F: block/qapi.c > F: qapi/block*.json > F: qapi/transaction.json > T: git https://repo.or.cz/qemu/armbru.git block-next > > I'm not sure this section makes much sense anymore. This is probably for you to decide. Though the block-next branch from the T: line doesn't even exist any more... > Should qapi/transaction.json also be added to "Block layer core"? Or > should it go into John's section "Block Jobs"? I think at the moment it only supports actions that are more related to block jobs, so moving it there would make sense to me. Kevin
Re: [PATCH 2/3] MAINTAINERS: Cover qapi/block{-core}.json in 'Block layer core' section
Philippe Mathieu-Daudé writes: > List this file in the proper section, so maintainers get > notified when it is modified. > > Signed-off-by: Philippe Mathieu-Daudé > --- > Cc: Kevin Wolf > Cc: Max Reitz > Cc: qemu-block@nongnu.org > --- > MAINTAINERS | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 903831e0a4..e269e9092c 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1842,6 +1842,8 @@ S: Supported Block layer core M: Kevin Wolf M: Max Reitz L: qemu-block@nongnu.org S: Supported > F: block* > F: block/ > F: hw/block/ > +F: qapi/block.json > +F: qapi/block-core.json > F: include/block/ > F: qemu-img* > F: docs/interop/qemu-img.rst This is in addition to Block QAPI, monitor, command line M: Markus Armbruster S: Supported F: blockdev.c F: block/qapi.c F: qapi/block*.json F: qapi/transaction.json T: git https://repo.or.cz/qemu/armbru.git block-next I'm not sure this section makes much sense anymore. Should qapi/transaction.json also be added to "Block layer core"? Or should it go into John's section "Block Jobs"?
Re: [PULL 00/46] Python queue 2020-02-06
On 2/7/20 1:39 PM, Philippe Mathieu-Daudé wrote:
On 2/7/20 12:51 PM, Peter Maydell wrote:
On Thu, 6 Feb 2020 at 21:21, Philippe Mathieu-Daudé
wrote:
Hi Peter,
I prepared this series on behalf of Eduardo and
Cleber (one of them will ack this cover).
Regards,
Phil.
The following changes since commit
418fa86dd465b4fd8394373cf83db8fa65d7611c:
Merge remote-tracking branch
'remotes/stsquad/tags/pull-testing-040220-1' into staging (2020-02-04
18:55:06 +)
are available in the Git repository at:
https://gitlab.com/philmd/qemu.git tags/python-next-20200206
for you to fetch changes up to 3e3481a5df933a26b47f08e5913821672d28d308:
.readthedocs.yml: specify some minimum python requirements
(2020-02-06 21:48:24 +0100)
Hi; this fails 'make check' (all hosts):
TEST iotest-qcow2: 252
TEST iotest-qcow2: 256
TEST iotest-qcow2: 265
TEST iotest-qcow2: 267
TEST iotest-qcow2: 268
TEST iotest-qcow2: 283 [fail]
QEMU --
"/home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64"
-nodefaults -display none -accel qtest
QEMU_IMG --
"/home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/../../qemu-img"
QEMU_IO --
"/home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/../../qemu-io"
--cache writeback --aio threads -f qcow2
QEMU_NBD --
"/home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/../../qemu-nbd"
IMGFMT -- qcow2 (compat=1.1)
IMGPROTO -- file
PLATFORM -- Linux/x86_64 e104462 4.15.0-74-generic
TEST_DIR --
/home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/scratch
SOCK_DIR -- /tmp/tmp.oppAzNNHIY
SOCKET_SCM_HELPER --
/home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/socket_scm_helper
--- /home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/283.out
2020-02-06 18:59:06.291529139 +
+++
/home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/283.out.bad
2020-02-07 11:25:38.477373907 +
@@ -1,8 +1 @@
-{"execute": "blockdev-add", "arguments": {"driver": "null-co",
"node-name": "target"}}
-{"return": {}}
-{"execute": "blockdev-add", "arguments": {"driver": "blkdebug",
"image": {"driver": "null-co", "node-name": "base", "size": 1048576},
"node-name": "source"}}
-{"return": {}}
-{"execute": "blockdev-add", "arguments": {"driver": "blkdebug",
"image": "base", "node-name": "other", "take-child-perms": ["write"]}}
-{"return": {}}
-{"execute": "blockdev-backup", "arguments": {"device": "source",
"sync": "full", "target": "target"}}
-{"error": {"class": "GenericError", "desc": "Cannot set permissions
for backup-top filter: Conflicts with use by other as 'image', which
uses 'write' on base"}}
+./check: line 866: ./283: Permission denied
Not run: 220
Failures: 283
Interesting.
I apologize this test is not in my suite.
Actually test 283 was merged yesterday few hours before I send this pull
request (which is why it passed the new checkpatch test), and it doesn't
use the Python 3 interpreter after shebang.
Once updated to Python 3, with this hunk, the test pass:
-- >8 --
--- a/tests/qemu-iotests/283
+++ b/tests/qemu-iotests/283
@@ -1,4 +1,4 @@
-#!/usr/bin/env python
+#!/usr/bin/env python3
#
# Test for backup-top filter permission activation failure
#
---
...
TESTiotest-qcow2: 244
TESTiotest-qcow2: 249
TESTiotest-qcow2: 251
TESTiotest-qcow2: 252
TESTiotest-qcow2: 256
TESTiotest-qcow2: 265
TESTiotest-qcow2: 267
TESTiotest-qcow2: 268
TESTiotest-qcow2: 283
Not run: 220
Passed all 115 iotests
I'll rebase and respin.
Failed 1 of 115 iotests
/home/petmay01/linaro/qemu-for-merges/tests/Makefile.include:842:
recipe for target 'check-tests/check-block.sh' failed
[PATCH v2] block: always fill entire LUKS header space with zeros
When initializing the LUKS header the size with default encryption
parameters will currently be 2068480 bytes. This is rounded up to
a multiple of the cluster size, 2081792, with 64k sectors. If the
end of the header is not the same as the end of the cluster we fill
the extra space with zeros. This was forgetting that not even the
space allocated for the header will be fully initialized, as we
only write key material for the first key slot. The space left
for the other 7 slots is never written to.
An optimization to the ref count checking code:
commit a5fff8d4b4d928311a5005efa12d0991fe3b66f9 (refs/bisect/bad)
Author: Vladimir Sementsov-Ogievskiy
Date: Wed Feb 27 16:14:30 2019 +0300
qcow2-refcount: avoid eating RAM
made the assumption that every cluster which was allocated would
have at least some data written to it. This was violated by way
the LUKS header is only partially written, with much space simply
reserved for future use.
Depending on the cluster size this problem was masked by the
logic which wrote zeros between the end of the LUKS header and
the end of the cluster.
$ qemu-img create --object secret,id=cluster_encrypt0,data=123456 \
-f qcow2 -o cluster_size=2k,encrypt.iter-time=1,\
encrypt.format=luks,encrypt.key-secret=cluster_encrypt0 \
cluster_size_check.qcow2 100M
Formatting 'cluster_size_check.qcow2', fmt=qcow2 size=104857600
encrypt.format=luks encrypt.key-secret=cluster_encrypt0
encrypt.iter-time=1 cluster_size=2048 lazy_refcounts=off refcount_bits=16
$ qemu-img check --object secret,id=cluster_encrypt0,data=redhat \
'json:{"driver": "qcow2", "encrypt.format": "luks", \
"encrypt.key-secret": "cluster_encrypt0", \
"file.driver": "file", "file.filename": "cluster_size_check.qcow2"}'
ERROR: counting reference for region exceeding the end of the file by one
cluster or more: offset 0x2000 size 0x1f9000
Leaked cluster 4 refcount=1 reference=0
...snip...
Leaked cluster 130 refcount=1 reference=0
1 errors were found on the image.
Data may be corrupted, or further writes to the image may corrupt it.
127 leaked clusters were found on the image.
This means waste of disk space, but no harm to data.
Image end offset: 268288
The problem only exists when the disk image is entirely empty. Writing
data to the disk image payload will solve the problem by causing the
end of the file to be extended further.
The change fixes it by ensuring that the entire allocated LUKS header
region is fully initialized with zeros. The qemu-img check will still
fail for any pre-existing disk images created prior to this change,
unless at least 1 byte of the payload is written to.
Fully writing zeros to the entire LUKS header is a good idea regardless
as it ensures that space has been allocated on the host filesystem (or
whatever block storage backend is used).
Signed-off-by: Daniel P. Berrangé
---
In v2:
- Cut down test scenarios to speed up
- Use _check_test_img helper
- Fix outdated comments
- Use space not tabs
block/qcow2.c | 11 +++--
tests/qemu-iotests/284 | 97 ++
tests/qemu-iotests/284.out | 62
tests/qemu-iotests/group | 1 +
4 files changed, 167 insertions(+), 4 deletions(-)
create mode 100755 tests/qemu-iotests/284
create mode 100644 tests/qemu-iotests/284.out
diff --git a/block/qcow2.c b/block/qcow2.c
index ef96606f8d..b2ab25cce5 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -135,13 +135,16 @@ static ssize_t qcow2_crypto_hdr_init_func(QCryptoBlock
*block, size_t headerlen,
s->crypto_header.length = headerlen;
s->crypto_header.offset = ret;
-/* Zero fill remaining space in cluster so it has predictable
- * content in case of future spec changes */
+/*
+ * Zero fill all space in cluster so it has predictable
+ * content, as we may not initialize some regions of the
+ * header (eg only 1 out of 8 key slots will be initialized)
+ */
clusterlen = size_to_clusters(s, headerlen) * s->cluster_size;
assert(qcow2_pre_write_overlap_check(bs, 0, ret, clusterlen, false) == 0);
ret = bdrv_pwrite_zeroes(bs->file,
- ret + headerlen,
- clusterlen - headerlen, 0);
+ ret,
+ clusterlen, 0);
if (ret < 0) {
error_setg_errno(errp, -ret, "Could not zero fill encryption header");
return -1;
diff --git a/tests/qemu-iotests/284 b/tests/qemu-iotests/284
new file mode 100755
index 00..071e89b33e
--- /dev/null
+++ b/tests/qemu-iotests/284
@@ -0,0 +1,97 @@
+#!/usr/bin/env bash
+#
+# Test ref count checks on encrypted images
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of th
Re: [PULL 00/46] Python queue 2020-02-06
On Fri, Feb 7, 2020 at 2:30 PM Philippe Mathieu-Daudé wrote:
>
> Cc'ing qemu-block@
>
> On 2/7/20 12:51 PM, Peter Maydell wrote:
> > On Thu, 6 Feb 2020 at 21:21, Philippe Mathieu-Daudé
> > wrote:
> >>
> >> Hi Peter,
> >>
> >> I prepared this series on behalf of Eduardo and
> >> Cleber (one of them will ack this cover).
> >>
> >> Regards,
> >>
> >> Phil.
> >>
> >> The following changes since commit
> >> 418fa86dd465b4fd8394373cf83db8fa65d7611c:
> >>
> >>Merge remote-tracking branch
> >> 'remotes/stsquad/tags/pull-testing-040220-1' into staging (2020-02-04
> >> 18:55:06 +)
> >>
> >> are available in the Git repository at:
> >>
> >>https://gitlab.com/philmd/qemu.git tags/python-next-20200206
> >>
> >> for you to fetch changes up to 3e3481a5df933a26b47f08e5913821672d28d308:
> >>
> >>.readthedocs.yml: specify some minimum python requirements (2020-02-06
> >> 21:48:24 +0100)
> >
> > Hi; this fails 'make check' (all hosts):
> >
> >TESTiotest-qcow2: 252
> >TESTiotest-qcow2: 256
> >TESTiotest-qcow2: 265
> >TESTiotest-qcow2: 267
> >TESTiotest-qcow2: 268
> >TESTiotest-qcow2: 283 [fail]
> > QEMU --
> > "/home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64"
> > -nodefaults -display none -accel qtest
> > QEMU_IMG --
> > "/home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/../../qemu-img"
> > QEMU_IO --
> > "/home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/../../qemu-io"
> > --cache writeback --aio threads -f qcow2
> > QEMU_NBD --
> > "/home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/../../qemu-nbd"
> > IMGFMT-- qcow2 (compat=1.1)
> > IMGPROTO -- file
> > PLATFORM -- Linux/x86_64 e104462 4.15.0-74-generic
> > TEST_DIR --
> > /home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/scratch
> > SOCK_DIR -- /tmp/tmp.oppAzNNHIY
> > SOCKET_SCM_HELPER --
> > /home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/socket_scm_helper
> >
> > --- /home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/283.out
> > 2020-02-06 18:59:06.291529139 +
> > +++
> > /home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/283.out.bad
> > 2020-02-07 11:25:38.477373907 +
> > @@ -1,8 +1 @@
> > -{"execute": "blockdev-add", "arguments": {"driver": "null-co",
> > "node-name": "target"}}
> > -{"return": {}}
> > -{"execute": "blockdev-add", "arguments": {"driver": "blkdebug",
> > "image": {"driver": "null-co", "node-name": "base", "size": 1048576},
> > "node-name": "source"}}
> > -{"return": {}}
> > -{"execute": "blockdev-add", "arguments": {"driver": "blkdebug",
> > "image": "base", "node-name": "other", "take-child-perms": ["write"]}}
> > -{"return": {}}
> > -{"execute": "blockdev-backup", "arguments": {"device": "source",
> > "sync": "full", "target": "target"}}
> > -{"error": {"class": "GenericError", "desc": "Cannot set permissions
> > for backup-top filter: Conflicts with use by other as 'image', which
> > uses 'write' on base"}}
> > +./check: line 866: ./283: Permission denied
> > Not run: 220
> > Failures: 283
> > Failed 1 of 115 iotests
> > /home/petmay01/linaro/qemu-for-merges/tests/Makefile.include:842:
> > recipe for target 'check-tests/check-block.sh' failed
>
> I only run out-of-tree builds.
>
> I noticed the block tests were not run until I add this change:
>
> -- >8 --
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -836,7 +836,7 @@ clean-tcg: $(CLEAN_TCG_TARGET_RULES)
> QEMU_IOTESTS_HELPERS-$(call land,$(CONFIG_SOFTMMU),$(CONFIG_LINUX)) =
> tests/qemu-iotests/socket_scm_helper$(EXESUF)
>
> .PHONY: check-tests/check-block.sh
> -check-tests/check-block.sh: tests/check-block.sh qemu-img$(EXESUF) \
> +check-tests/check-block.sh: $(SRC_PATH)/tests/check-block.sh
> qemu-img$(EXESUF) \
> qemu-io$(EXESUF) qemu-nbd$(EXESUF)
> $(QEMU_IOTESTS_HELPERS-y) \
> $(patsubst %,%/all,$(filter %-softmmu,$(TARGET_DIRS)))
> @$<
> ---
>
> Peter, are you running only in-tree builds?
Oops nevermind, I was in a '--disable-tools' build directory when I
restarted testing.
Re: [PULL 00/46] Python queue 2020-02-06
Cc'ing qemu-block@
On 2/7/20 12:51 PM, Peter Maydell wrote:
On Thu, 6 Feb 2020 at 21:21, Philippe Mathieu-Daudé wrote:
Hi Peter,
I prepared this series on behalf of Eduardo and
Cleber (one of them will ack this cover).
Regards,
Phil.
The following changes since commit 418fa86dd465b4fd8394373cf83db8fa65d7611c:
Merge remote-tracking branch 'remotes/stsquad/tags/pull-testing-040220-1'
into staging (2020-02-04 18:55:06 +)
are available in the Git repository at:
https://gitlab.com/philmd/qemu.git tags/python-next-20200206
for you to fetch changes up to 3e3481a5df933a26b47f08e5913821672d28d308:
.readthedocs.yml: specify some minimum python requirements (2020-02-06
21:48:24 +0100)
Hi; this fails 'make check' (all hosts):
TESTiotest-qcow2: 252
TESTiotest-qcow2: 256
TESTiotest-qcow2: 265
TESTiotest-qcow2: 267
TESTiotest-qcow2: 268
TESTiotest-qcow2: 283 [fail]
QEMU --
"/home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64"
-nodefaults -display none -accel qtest
QEMU_IMG --
"/home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/../../qemu-img"
QEMU_IO --
"/home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/../../qemu-io"
--cache writeback --aio threads -f qcow2
QEMU_NBD --
"/home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/../../qemu-nbd"
IMGFMT-- qcow2 (compat=1.1)
IMGPROTO -- file
PLATFORM -- Linux/x86_64 e104462 4.15.0-74-generic
TEST_DIR --
/home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/scratch
SOCK_DIR -- /tmp/tmp.oppAzNNHIY
SOCKET_SCM_HELPER --
/home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/socket_scm_helper
--- /home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/283.out
2020-02-06 18:59:06.291529139 +
+++
/home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/283.out.bad
2020-02-07 11:25:38.477373907 +
@@ -1,8 +1 @@
-{"execute": "blockdev-add", "arguments": {"driver": "null-co",
"node-name": "target"}}
-{"return": {}}
-{"execute": "blockdev-add", "arguments": {"driver": "blkdebug",
"image": {"driver": "null-co", "node-name": "base", "size": 1048576},
"node-name": "source"}}
-{"return": {}}
-{"execute": "blockdev-add", "arguments": {"driver": "blkdebug",
"image": "base", "node-name": "other", "take-child-perms": ["write"]}}
-{"return": {}}
-{"execute": "blockdev-backup", "arguments": {"device": "source",
"sync": "full", "target": "target"}}
-{"error": {"class": "GenericError", "desc": "Cannot set permissions
for backup-top filter: Conflicts with use by other as 'image', which
uses 'write' on base"}}
+./check: line 866: ./283: Permission denied
Not run: 220
Failures: 283
Failed 1 of 115 iotests
/home/petmay01/linaro/qemu-for-merges/tests/Makefile.include:842:
recipe for target 'check-tests/check-block.sh' failed
I only run out-of-tree builds.
I noticed the block tests were not run until I add this change:
-- >8 --
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -836,7 +836,7 @@ clean-tcg: $(CLEAN_TCG_TARGET_RULES)
QEMU_IOTESTS_HELPERS-$(call land,$(CONFIG_SOFTMMU),$(CONFIG_LINUX)) =
tests/qemu-iotests/socket_scm_helper$(EXESUF)
.PHONY: check-tests/check-block.sh
-check-tests/check-block.sh: tests/check-block.sh qemu-img$(EXESUF) \
+check-tests/check-block.sh: $(SRC_PATH)/tests/check-block.sh
qemu-img$(EXESUF) \
qemu-io$(EXESUF) qemu-nbd$(EXESUF)
$(QEMU_IOTESTS_HELPERS-y) \
$(patsubst %,%/all,$(filter %-softmmu,$(TARGET_DIRS)))
@$<
---
Peter, are you running only in-tree builds?
Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert
07.02.2020 13:33, Max Reitz wrote:
On 04.02.20 15:23, Eric Blake wrote:
On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:
I understand that it is safer to have restrictions now and lift them
later, than to allow use of the option at any time and leave room for
the user to shoot themselves in the foot with no way to add safety
later. The argument against no backing file is somewhat
understandable (technically, as long as the backing file also reads
as all zeroes, then the overall image reads as all zeroes - but why
have a backing file that has no content?); the argument requiring -n
is a bit weaker (if I'm creating an image, I _know_ it reads as all
zeroes, so the --target-is-zero argument is redundant, but it
shouldn't hurt to allow it).
I know that it reads as all zeroes, only if this format provides zero
initialization..
+++ b/qemu-img.c
@@ -2247,6 +2256,11 @@ static int img_convert(int argc, char **argv)
warn_report("This will become an error in future QEMU
versions.");
}
+ if (s.has_zero_init && !skip_create) {
+ error_report("--target-is-zero requires use of -n flag");
+ goto fail_getopt;
+ }
So I think we could drop this hunk with no change in behavior.
I think, no we can't. If we allow target-is-zero, with -n, we'd better
to check that what we are creating is zero-initialized (format has
zero-init), and if not we should report error.
Good call. Yes, if we allow --target-is-zero without -n, we MUST insist
that bdrv_has_zero_init() returns 1 (or, after my followup series,
bdrv_known_zeroes() includes BDRV_ZERO_CREATE).
Why?
I could imagine a user creating a qcow2 image on some block device with
preallocation where we cannot verify that the result will be zero. But
they want qemu not to zero the device, so they would specify
--target-is-zero.
If user create image, setting --target-is-zero is always valid. But if we in
same operation create the image automatically, having --target-is-zero, when
we know that what we are creating is not zero is misleading and should
fail..
If we want to add a behavior to skip zeros unconditionally, we should call new
option --skip-zeroes, to clearly specify what we want.
OTOH, we can always choose to allow that behavior at a later point.
Max
--
Best regards,
Vladimir
Re: [PATCH v4 1/1] qemu-img: Add --target-is-zero to convert
On 05.02.20 12:02, David Edmondson wrote: > In many cases the target of a convert operation is a newly provisioned > target that the user knows is blank (reads as zero). In this situation > there is no requirement for qemu-img to wastefully zero out the entire > device. > > Add a new option, --target-is-zero, allowing the user to indicate that > an existing target device will return zeros for all reads. > > Signed-off-by: David Edmondson > --- > docs/interop/qemu-img.rst | 9 - > qemu-img-cmds.hx | 4 ++-- > qemu-img.c| 26 +++--- > 3 files changed, 33 insertions(+), 6 deletions(-) Thanks, I’ve applied the patch to my block branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block Max signature.asc Description: OpenPGP digital signature
Re: [PATCH 2/3] MAINTAINERS: Cover qapi/block{-core}.json in 'Block layer core' section
On 07.02.20 11:30, Philippe Mathieu-Daudé wrote: > List this file in the proper section, so maintainers get > notified when it is modified. > > Signed-off-by: Philippe Mathieu-Daudé > --- > Cc: Kevin Wolf > Cc: Max Reitz > Cc: qemu-block@nongnu.org > --- > MAINTAINERS | 2 ++ > 1 file changed, 2 insertions(+) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert
On 04.02.20 15:23, Eric Blake wrote:
> On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:
>
>>> I understand that it is safer to have restrictions now and lift them
>>> later, than to allow use of the option at any time and leave room for
>>> the user to shoot themselves in the foot with no way to add safety
>>> later. The argument against no backing file is somewhat
>>> understandable (technically, as long as the backing file also reads
>>> as all zeroes, then the overall image reads as all zeroes - but why
>>> have a backing file that has no content?); the argument requiring -n
>>> is a bit weaker (if I'm creating an image, I _know_ it reads as all
>>> zeroes, so the --target-is-zero argument is redundant, but it
>>> shouldn't hurt to allow it).
>>
>> I know that it reads as all zeroes, only if this format provides zero
>> initialization..
>>
>>>
+++ b/qemu-img.c
>>>
@@ -2247,6 +2256,11 @@ static int img_convert(int argc, char **argv)
warn_report("This will become an error in future QEMU
versions.");
}
+ if (s.has_zero_init && !skip_create) {
+ error_report("--target-is-zero requires use of -n flag");
+ goto fail_getopt;
+ }
>>>
>>> So I think we could drop this hunk with no change in behavior.
>>
>> I think, no we can't. If we allow target-is-zero, with -n, we'd better
>> to check that what we are creating is zero-initialized (format has
>> zero-init), and if not we should report error.
>>
>
> Good call. Yes, if we allow --target-is-zero without -n, we MUST insist
> that bdrv_has_zero_init() returns 1 (or, after my followup series,
> bdrv_known_zeroes() includes BDRV_ZERO_CREATE).
Why?
I could imagine a user creating a qcow2 image on some block device with
preallocation where we cannot verify that the result will be zero. But
they want qemu not to zero the device, so they would specify
--target-is-zero.
OTOH, we can always choose to allow that behavior at a later point.
Max
signature.asc
Description: OpenPGP digital signature
[PATCH 2/3] MAINTAINERS: Cover qapi/block{-core}.json in 'Block layer core' section
List this file in the proper section, so maintainers get notified when it is modified. Signed-off-by: Philippe Mathieu-Daudé --- Cc: Kevin Wolf Cc: Max Reitz Cc: qemu-block@nongnu.org --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 903831e0a4..e269e9092c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1842,6 +1842,8 @@ S: Supported F: block* F: block/ F: hw/block/ +F: qapi/block.json +F: qapi/block-core.json F: include/block/ F: qemu-img* F: docs/interop/qemu-img.rst -- 2.21.1
Re: [PATCH v2] qapi: Allow getting flat output from 'query-named-block-nodes'
On 20.01.20 09:50, Peter Krempa wrote:
> When a management application manages node names there's no reason to
> recurse into backing images in the output of query-named-block-nodes.
>
> Add a parameter to the command which will return just the top level
> structs.
>
> Signed-off-by: Peter Krempa
> ---
>
> Diff to v1:
> - rewrote setting of 'return_flat' in qmp_query_named_block_nodes
> - tried to clarify the QMP schema docs for the new field
>
> This patch does not aim to fix the rather suboptimal original
> documentation of the command as that is going to end up in a bunch of
> bikeshedding.
>
> While I know that there are plans for a new command that should fix
> this, the plans were already there for quite some time without much
> happening. This is a quick fix to a real problem, because if you have
> (maybe unpractically) deep backing chains, the returned JSON is getting
> huge. (140 nesting levels exceeds 10MiB of JSON)
The main reason nothing is happening is because nobody is pressing for
it, I think. We talk about it from time to time but then our result is
“As long as nobody seriously complains and tells us what we need, we’re
going to assume what we have is good enough.”
For example:
https://lists.nongnu.org/archive/html/qemu-block/2020-01/msg00049.html
(Under “Query function situation”)
> block.c | 5 +++--
> block/qapi.c | 10 --
> blockdev.c| 8 ++--
> include/block/block.h | 2 +-
> include/block/qapi.h | 4 +++-
> monitor/hmp-cmds.c| 2 +-
> qapi/block-core.json | 7 ++-
> 7 files changed, 28 insertions(+), 10 deletions(-)
[...]
> diff --git a/block/qapi.c b/block/qapi.c
> index 9a5d0c9b27..84048e1a57 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -42,7 +42,9 @@
> #include "qemu/cutils.h"
>
> BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
> -BlockDriverState *bs, Error **errp)
> +BlockDriverState *bs,
> +bool flat,
> +Error **errp)
> {
> ImageInfo **p_image_info;
> BlockDriverState *bs0;
> @@ -156,6 +158,10 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend
> *blk,
> return NULL;
> }
>
> +/* stop gathering data for flat output */
> +if (flat)
> +break;
This should be enclosed in curly brackets (qemu coding style).
Shall I fix that up?
Max
> +
> if (bs0->drv && bs0->backing) {
> info->backing_file_depth++;
> bs0 = bs0->backing->bs;
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v1 2/4] virtio: increase virtuqueue size for virtio-scsi and virtio-blk
On 05.02.2020 14:19, Stefan Hajnoczi wrote:
On Tue, Feb 04, 2020 at 12:59:04PM +0300, Denis Plotnikov wrote:
On 30.01.2020 17:58, Stefan Hajnoczi wrote:
On Wed, Jan 29, 2020 at 05:07:00PM +0300, Denis Plotnikov wrote:
The goal is to reduce the amount of requests issued by a guest on
1M reads/writes. This rises the performance up to 4% on that kind of
disk access pattern.
The maximum chunk size to be used for the guest disk accessing is
limited with seg_max parameter, which represents the max amount of
pices in the scatter-geather list in one guest disk request.
Since seg_max is virqueue_size dependent, increasing the virtqueue
size increases seg_max, which, in turn, increases the maximum size
of data to be read/write from guest disk.
More details in the original problem statment:
https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg03721.html
Suggested-by: Denis V. Lunev
Signed-off-by: Denis Plotnikov
---
hw/core/machine.c | 3 +++
include/hw/virtio/virtio.h | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 3e288bfceb..8bc401d8b7 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -28,6 +28,9 @@
#include "hw/mem/nvdimm.h"
GlobalProperty hw_compat_4_2[] = {
+{ "virtio-blk-device", "queue-size", "128"},
+{ "virtio-scsi-device", "virtqueue_size", "128"},
+{ "vhost-blk-device", "virtqueue_size", "128"},
vhost-blk-device?! Who has this? It's not in qemu.git so please omit
this line. ;-)
So in this case the line:
{ "vhost-blk-device", "seg_max_adjust", "off"},
introduced by my patch:
commit 1bf8a989a566b2ba41c197004ec2a02562a766a4
Author: Denis Plotnikov
Date: Fri Dec 20 17:09:04 2019 +0300
virtio: make seg_max virtqueue size dependent
is also wrong. It should be:
{ "vhost-scsi-device", "seg_max_adjust", "off"},
Am I right?
It's just called "vhost-scsi":
include/hw/virtio/vhost-scsi.h:#define TYPE_VHOST_SCSI "vhost-scsi"
On the other hand, do you want to do this for the vhost-user-blk,
vhost-user-scsi, and vhost-scsi devices that exist in qemu.git? Those
devices would benefit from better performance too.
After thinking about that for a while, I think we shouldn't extend queue
sizes for vhost-user-blk, vhost-user-scsi and vhost-scsi.
This is because increasing the queue sizes seems to be just useless for
them: the all thing is about increasing the queue sizes for increasing
seg_max (it limits the max block query size from the guest). For
virtio-blk-device and virtio-scsi-device it makes sense, since they have
seg-max-adjust property which, if true, sets seg_max to
virtqueue_size-2. vhost-scsi also have this property but it seems the
property just doesn't affect anything (remove it?).
Also vhost-user-blk, vhost-user-scsi and vhost-scsi don't do any seg_max
settings. If I understand correctly, their backends are ment to be
responsible for doing that.
So, what about changing the queue sizes just for virtio-blk-device and
virtio-scsi-device?
Denis
It seems to be so. We also have the test checking those settings:
tests/acceptance/virtio_seg_max_adjust.py
For now it checks virtio-scsi-pci and virtio-blk-pci.
I'm going to extend it for the virtqueue size checking.
If I change vhost-user-blk, vhost-user-scsi and vhost-scsi it's worth
to check those devices too. But I don't know how to form a command line
for that 3 devices since they should involve some third party components as
backends (kernel modules, DPDK, etc.) and they seems to be not available in
the
qemu git.
Is there any way to do it with some qit.qemu available stubs or something
else?
If so, could you please point out the proper way to do it?
qemu.git has contrib/vhost-user-blk/ and contrib/vhost-user-scsi/ if
you need to test those vhost-user devices without external dependencies.
Stefan
Re: [PATCH] tests/qemu_iotests: Minimize usage of used ports
On 06.02.20 19:33, Lukáš Doktor wrote: > Dne 06. 02. 20 v 17:59 Max Reitz napsal(a): >> On 06.02.20 17:48, Eric Blake wrote: >>> On 2/6/20 10:37 AM, Max Reitz wrote: [...] OTOH, would it work if we just did a %s/localhost/127.0.0.1/ in the test? We have specific cases for IPv6, so I think it makes sense to force IPv4 in all other cases. >>> >>> Except then it will fail on machines configured for IPv6-only. >> >> So we’ll just have to test whether IPv4 works, just like we already do >> for IPv6, no? >> > > Sure, using ::1 for IPv6 and 127.0.0.1 for IPv4 cases would work. The > question is whether the behavior is really expected. In migration it was > considered dangerous, because you can have 2 VMs starting the migration at > the same time. They both might attempt to get the same port, one would > receive IPv4 the other IPv6. Then depending on the order of start migrate you > might end-up attempting to migrate the other VMs instead of the intended ones. > > The same can happen here, you might start 2 nbd exports at the same time, get > the same port without any failures and then depending on luck get the right > or wrong export when attempting to connect. So I think bailing out in case we > fail to get all interfaces would be the most appropriate (note the same > situation is for 0.0.0.0 where you might end-up serving multiple different > disks on different interfaces). Anyway I leave it to you. If you decide you > don't want to fail than using ::1/127.0.0.1 sounds like a good idea. > Otherwise it'd make sense to create a test that especially uses ::1 and then > localhost to make sure it bails-out. OK. I’ll defer to Eric on that one. O:-) Max
