Re: [Qemu-block] [PATCH for-2.11 v2] file-posix: Clear out first sector in hdev_create

[Kevin Wolf]

Am 11.08.2017 um 10:09 hat Fam Zheng geschrieben:
> People get surprised when, after "qemu-img create -f raw /dev/sdX", they
> still see qcow2 with "qemu-img info", if previously the bdev had a qcow2
> header. While this is natural because raw doesn't need to write any
> magic bytes during creation, hdev_create is free to clear out the first
> sector to make sure the stale qcow2 header doesn't cause such confusion.
> 
> Signed-off-by: Fam Zheng 
> 
> ---
> 
> v2: Use stack allocated buffer. [Eric]
> Fix return value.
> (Keep qemu_write_full instead of switching to qemu_pwritev because
> the former handles short writes.)
> Fix typo "qemu-img". [Changlong]
> ---
>  block/file-posix.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index f4de022ae0..a63bbf2b90 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2703,6 +2703,16 @@ static int hdev_create(const char *filename, QemuOpts 
> *opts,
>  ret = -ENOSPC;
>  }

So the error paths above only set ret, but don't actually return or jump
to the end of the function.

> +if (total_size) {
> +uint8_t buf[BDRV_SECTOR_SIZE] = { 0 };
> +int64_t zero_size = MIN(BDRV_SECTOR_SIZE, total_size);
> +if (lseek(fd, 0, SEEK_SET) == -1) {
> +ret = -errno;
> +} else {
> +ret = qemu_write_full(fd, buf, zero_size);
> +ret = ret == zero_size ? 0 : -errno;

Which means that an error above (like a too small block device or using
a regular file) can be overwritten with a success value if clearing the
first sector works. That's probably not quite right.

> +}
> +}
>  qemu_close(fd);
>  return ret;
>  }

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v6 12/29] libqos: Track QTestState with QPCIBus

[Eric Blake]

On 09/07/2017 12:35 AM, Thomas Huth wrote:
> On 06.09.2017 23:00, Eric Blake wrote:
>> On 09/05/2017 04:36 AM, Thomas Huth wrote:
>>> On 01.09.2017 20:03, Eric Blake wrote:
 When initializing a QPCIBus, track which QTestState the bus is
 associated with (so that a later patch can then explicitly use
 that test state for all communication on the bus, rather than
 blindly relying on global_qtest).  Update the initialization
 functions to take another parameter, and update all callers to
 pass in state (for now, most callers get away with passing the
 current global_qtest as the current state, although this required
 fixing the order of initialization to ensure qtest_start() is
 called before qpci_init*() in rtl8139-test, and provided an
 opportunity to pass in the allocator in e1000e-test).


>>>
>>> Why did you remove the check for qs->alloc?
>>>
 +qs->pcibus = ops->qpci_init(qs->qts, qs->alloc);
>>
>> Because we want to ensure qpci_init() is called to set qs->qts
>> (presumably, whether or not qs->alloc is set).  Furthermore, only two
>> files declare a 'static QOSOps' structure in the first place
>> (libqos-pc.c and libqos-spapr.c); where both files set both the
>> .init_allocator and .qpci_init callbacks; a little bit of auditing shows
>> that the .init_allocator() never returns NULL (although that requires
>> browsing yet more files for malloc-{pc,spapr}.c).
> 
> OK, thanks for the explanation! ... but maybe we should
> g_assert(gs->alloc) somewhere instead? ... just an idea, I'm also fine
> if you leave it away.

Users of QOSOps always supply qs->alloc, but there are tests that
successfully use NULL for qs->alloc (because they did not go through
QOSOps).  At any rate, I can certainly update the commit message.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 4/7] block: remove legacy I/O throttling

[Kevin Wolf]

Am 25.08.2017 um 15:23 hat Manos Pitsidianakis geschrieben:
> This commit removes all I/O throttling from block/block-backend.c. In
> order to support the existing interface, it is changed to use the
> block/throttle.c filter driver.
> 
> The throttle filter node that is created by the legacy interface is
> stored in a 'throttle_node' field in the BlockBackendPublic of the
> device. The legacy throttle node is managed by the legacy interface
> completely. More advanced configurations with the filter drive are
> possible using the QMP API, but these will be ignored by the legacy
> interface.
> 
> Signed-off-by: Manos Pitsidianakis 

This patch doesn't apply cleanly any more and needs a rebase.

>  /* should be called before blk_set_io_limits if a limit is set */
> -void blk_io_limits_enable(BlockBackend *blk, const char *group)
> +void blk_io_limits_enable(BlockBackend *blk, const char *group,  Error 
> **errp)
>  {
> -assert(!blk->public.throttle_group_member.throttle_state);
> -throttle_group_register_tgm(>public.throttle_group_member,
> -group, blk_get_aio_context(blk));
> +BlockDriverState *bs = blk_bs(blk), *throttle_node;
> +QDict *options = qdict_new();
> +Error *local_err = NULL;
> +ThrottleState *ts;
> +
> +bdrv_drained_begin(bs);

bs can be NULL:

$ x86_64-softmmu/qemu-system-x86_64 -drive media=cdrom,bps=1024
Segmentation fault (core dumped)

>  static void blk_root_drained_begin(BdrvChild *child)
>  {
> +ThrottleGroupMember *tgm;
>  BlockBackend *blk = child->opaque;
>  
>  if (++blk->quiesce_counter == 1) {
> @@ -1997,19 +2025,25 @@ static void blk_root_drained_begin(BdrvChild *child)
>  
>  /* Note that blk->root may not be accessible here yet if we are just
>   * attaching to a BlockDriverState that is drained. Use child instead. */
> -
> -if 
> (atomic_fetch_inc(>public.throttle_group_member.io_limits_disabled) == 
> 0) {
> -throttle_group_restart_tgm(>public.throttle_group_member);
> +if (blk->public.throttle_node) {
> +tgm = throttle_get_tgm(blk->public.throttle_node);
> +if (atomic_fetch_inc(>io_limits_disabled) == 0) {
> +throttle_group_restart_tgm(tgm);
> +}
>  }
>  }
>  
>  static void blk_root_drained_end(BdrvChild *child)
>  {
> +ThrottleGroupMember *tgm;
>  BlockBackend *blk = child->opaque;
>  assert(blk->quiesce_counter);
>  
> -assert(blk->public.throttle_group_member.io_limits_disabled);
> -atomic_dec(>public.throttle_group_member.io_limits_disabled);
> +if (blk->public.throttle_node) {
> +tgm = throttle_get_tgm(blk->public.throttle_node);
> +assert(tgm->io_limits_disabled);
> +atomic_dec(>io_limits_disabled);
> +}

We shouldn't really need any throttling code in
blk_root_drained_begin/end any more now because the throttle node will
be drained. If this code is necessary, a bdrv_drain() on an explicit
throttle node will work differently from one on an implicit one.

Unfortunately, this seems to be true about the throttle node. Implicit
throttle nodes will keep ignoring the throttle limit in order to
complete the drain request quickly, where as explicit throttle nodes
will process their requests at the configured speed before the drain
request can be completed.

This doesn't feel right to me, both should behave the same.

Kevin

Re: [Qemu-block] [PATCH v3 3/7] block: require job-id when device is a node name

[Kevin Wolf]

Am 25.08.2017 um 15:23 hat Manos Pitsidianakis geschrieben:
> With implicit filter nodes on the top of the graph it is not possible to
> generate job-ids with the name of the device in block_job_create()
> anymore, since the job's bs will not be a child_root.
> 
> Instead we can require that job-id is not NULL in block_job_create(),
> and check that a job-id has been set in the callers of
> block_job_create() in blockdev.c. It is more consistent to require an
> explicit job-id when the device parameter in the job creation command,
> eg
> 
>  { "execute": "drive-backup",
>"arguments": { "device": "drive0",
>   "sync": "full",
>   "target": "backup.img" } }
> 
> is not a BlockBackend name, instead of automatically getting it from the
> root BS if device is a node name. That information is lost after calling
> block_job_create(), so we can do it in its caller instead.
> 
> Signed-off-by: Manos Pitsidianakis 

Reviewed-by: Kevin Wolf 

Re: [Qemu-block] [PATCH v3 2/7] block: add options parameter to bdrv_new_open_driver()

[Kevin Wolf]

Am 25.08.2017 um 15:23 hat Manos Pitsidianakis geschrieben:
> Allow passing a QDict *options parameter to bdrv_new_open_driver() so
> that it can be used if a driver needs it upon creation. The previous
> behaviour (empty bs->options and bs->explicit_options) remains when
> options is NULL.
> 
> Reviewed-by: Alberto Garcia 
> Signed-off-by: Manos Pitsidianakis 

Reviewed-by: Kevin Wolf 

Re: [Qemu-block] [Qemu-devel] [PULL 00/14] Block layer patches

[Peter Maydell]

On 6 September 2017 at 15:02, Kevin Wolf  wrote:
> The following changes since commit 98bfaac788be0ca63d7d010c8d4ba100ff1d8278:
>
>   Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2017-09-01-v3' 
> into staging (2017-09-04 13:28:09 +0100)
>
> are available in the git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 83a8c775a8bf134eb18a719322939b74a818d750:
>
>   qcow2: move qcow2_store_persistent_dirty_bitmaps() before cache flushing 
> (2017-09-06 14:40:18 +0200)
>
> 
> Block layer patches
>

Applied, thanks.

-- PMM

Re: [Qemu-block] [PATCH v3 1/7] block: skip implicit nodes in snapshots, blockjobs

[Kevin Wolf]

Am 25.08.2017 um 15:23 hat Manos Pitsidianakis geschrieben:
> Implicit filter nodes added at the top of nodes can interfere with block
> jobs. This is not a problem when they are added by other jobs since
> adding another job will issue a QERR_DEVICE_IN_USE, but it can happen in
> the next commit which introduces an implicitly created throttle filter
> node below BlockBackend, which we want to be skipped during automatic
> operations on the graph since the user does not necessarily know about
> their existence.
> 
> All implicit filters will have either bs->file or bs->backing set. This
> is a needed assumption so we can know which direction we will skip down
> the graph.
> 
> Signed-off-by: Manos Pitsidianakis 
> ---
>  include/block/block_int.h | 17 +
>  block.c   | 10 ++
>  block/qapi.c  | 14 +-
>  blockdev.c| 34 ++
>  4 files changed, 66 insertions(+), 9 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 7571c0aaaf..9e0f70e055 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -699,6 +699,21 @@ static inline BlockDriverState 
> *backing_bs(BlockDriverState *bs)
>  return bs->backing ? bs->backing->bs : NULL;
>  }
>  
> +static inline BlockDriverState *file_bs(BlockDriverState *bs)
> +{
> +return bs->file ? bs->file->bs : NULL;
> +}
> +
> +/* This is a convenience function to get either bs->file->bs or
> + * bs->backing->bs * */
> +static inline BlockDriverState *child_bs(BlockDriverState *bs)
> +{
> +BlockDriverState *backing = backing_bs(bs);
> +BlockDriverState *file = file_bs(bs);
> +assert(!(file && backing));
> +return backing ?: file;
> +}

I still think you should just get the only child and not restrict it to
bs->file or bs->backing.

child = QLIST_FIRST(>children);
assert(!QLIST_NEXT(child, next));
return child->bs;

file_bs() isn't needed at all then. And Berto is probably
right that this is simple enough that it can just be inlined in
bdrv_get_first_explicit().

> diff --git a/blockdev.c b/blockdev.c
> index 23475abb72..fc7b65c3f0 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1300,6 +1300,10 @@ SnapshotInfo 
> *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
>  if (!bs) {
>  return NULL;
>  }
> +
> +/* Skip implicit filter nodes */
> +bs = bdrv_get_first_explicit(bs);
> +
> [...]

Most, if not all, of the commands that you change accept both node names
and BlockBackend names, which function as aliases for their root node.
If a node name is given, the user claims that they know the graph
structure and it is supposed to be exact. We should not skip the
implicit filter node then. (For example, for creating an external
snapshot, creating the snapshot above the filter is meaningful. It
requires that the user knows about the filter node, but by using a node
name they tell us that they do.)

Please make sure that your qemu-iotests case covers all of the cases for
which you add a bdrv_get_first_explicit() call in this patch. So far, we
seem to be completely missing:

* Create external snapshot
* Create internal snapshot
* Delete internal snapshot
* blockdev-backup
* blockdev-mirror

All of them should cover the case where a BlockBackend name is given as
well as the cases with a node name.

Kevin

Re: [Qemu-block] [PATCH] qemu-iotests: Add missing -machine accel=qtest

[Cornelia Huck]

On Thu,  7 Sep 2017 10:53:10 +0200
Kevin Wolf  wrote:

> A basic set of qemu options is initialised in ./common:
> 
> export QEMU_OPTIONS="-nodefaults -machine accel=qtest"
> 
> However, two test cases (172 and 186) overwrite QEMU_OPTIONS and neglect
> to manually set '-machine accel=qtest'. Add the missing option for 172.
> 186 probably only copied the code from 172, it doesn't actually need to
> overwrite QEMU_OPTIONS, so remove that in 186.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/172 | 2 +-
>  tests/qemu-iotests/186 | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)

I tested this on top of my change switching the default machine to
"tcg:kvm:xen:hax", and this now passes on x86_64 builds both with and
without tcg.

Tested-by: Cornelia Huck 
Reviewed-by: Cornelia Huck 

[Qemu-block] [PATCH] qemu-iotests: Add missing -machine accel=qtest

[Kevin Wolf]

A basic set of qemu options is initialised in ./common:

export QEMU_OPTIONS="-nodefaults -machine accel=qtest"

However, two test cases (172 and 186) overwrite QEMU_OPTIONS and neglect
to manually set '-machine accel=qtest'. Add the missing option for 172.
186 probably only copied the code from 172, it doesn't actually need to
overwrite QEMU_OPTIONS, so remove that in 186.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/172 | 2 +-
 tests/qemu-iotests/186 | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/172 b/tests/qemu-iotests/172
index 826d6fecd3..02c5f79bab 100755
--- a/tests/qemu-iotests/172
+++ b/tests/qemu-iotests/172
@@ -56,7 +56,7 @@ function do_run_qemu()
 done
 fi
 echo quit
-) | $QEMU -nographic -monitor stdio -serial none "$@"
+) | $QEMU -machine accel=qtest -nographic -monitor stdio -serial none "$@"
 echo
 }
 
diff --git a/tests/qemu-iotests/186 b/tests/qemu-iotests/186
index 2b9f618f90..44cc01ed87 100755
--- a/tests/qemu-iotests/186
+++ b/tests/qemu-iotests/186
@@ -56,15 +56,15 @@ function do_run_qemu()
 done
 fi
 echo quit
-) | $QEMU -S -nodefaults -display none -device virtio-scsi-pci -monitor 
stdio "$@" 2>&1
+) | $QEMU -S -display none -device virtio-scsi-pci -monitor stdio "$@" 2>&1
 echo
 }
 
 function check_info_block()
 {
 echo "info block" |
-QEMU_OPTIONS="" do_run_qemu "$@" | _filter_win32 | _filter_hmp |
-_filter_qemu | _filter_generated_node_ids
+do_run_qemu "$@" | _filter_win32 | _filter_hmp | _filter_qemu |
+_filter_generated_node_ids
 }
 
 
-- 
2.13.5