[Qemu-commits] [qemu/qemu] a68624: iotests: Set read-zeroes on in null block driver f...

[Peter Maydell]

  Branch: refs/heads/master
  Home:   https://github.com/qemu/qemu
  Commit: a6862418fec40727b392c86dc13d9ec980efcb15
      
https://github.com/qemu/qemu/commit/a6862418fec40727b392c86dc13d9ec980efcb15
  Author: Andrey Shinkevich <address@hidden>
  Date:   2019-07-19 (Fri, 19 Jul 2019)

  Changed paths:
    M tests/qemu-iotests/051
    M tests/qemu-iotests/051.pc.out
    M tests/qemu-iotests/093
    M tests/qemu-iotests/136
    M tests/qemu-iotests/186
    M tests/qemu-iotests/186.out
    M tests/qemu-iotests/227
    M tests/qemu-iotests/227.out
    M tests/qemu-iotests/238
    M tests/qemu-iotests/240

  Log Message:
  -----------
  iotests: Set read-zeroes on in null block driver for Valgrind

The Valgrind tool reports about the uninitialised buffer 'buf'
instantiated on the stack of the function guess_disk_lchs().
Pass 'read-zeroes=on' to the null block driver to make it deterministic.
The output of the tests 051, 186 and 227 now includes the parameter
'read-zeroes'. So, the benchmark output files are being changed too.

Suggested-by: Kevin Wolf <address@hidden>
Signed-off-by: Andrey Shinkevich <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: 804db8ea00626d7be9902bc98ce3fa690bc95032
      
https://github.com/qemu/qemu/commit/804db8ea00626d7be9902bc98ce3fa690bc95032
  Author: Max Reitz <address@hidden>
  Date:   2019-07-19 (Fri, 19 Jul 2019)

  Changed paths:
    M block.c
    M block/io.c
    M include/block/block.h
    M include/block/block_int.h

  Log Message:
  -----------
  block: Introduce BdrvChild.parent_quiesce_counter

Commit 5cb2737e925042e6c7cd3fb0b01313950b03cddf laid out why
bdrv_do_drained_end() must decrement the quiesce_counter after
bdrv_drain_invoke().  It did not give a very good reason why it has to
happen after bdrv_parent_drained_end(), instead only claiming symmetry
to bdrv_do_drained_begin().

It turns out that delaying it for so long is wrong.

Situation: We have an active commit job (i.e. a mirror job) from top to
base for the following graph:

                  filter
                    |
                  [file]
                    |
                    v
top --[backing]--> base

Now the VM is closed, which results in the job being cancelled and a
bdrv_drain_all() happening pretty much simultaneously.

Beginning the drain means the job is paused once whenever one of its
nodes is quiesced.  This is reversed when the drain ends.

With how the code currently is, after base's drain ends (which means
that it will have unpaused the job once), its quiesce_counter remains at
1 while it goes to undrain its parents (bdrv_parent_drained_end()).  For
some reason or another, undraining filter causes the job to be kicked
and enter mirror_exit_common(), where it proceeds to invoke
block_job_remove_all_bdrv().

Now base will be detached from the job.  Because its quiesce_counter is
still 1, it will unpause the job once more.  So in total, undraining
base will unpause the job twice.  Eventually, this will lead to the
job's pause_count going negative -- well, it would, were there not an
assertion against this, which crashes qemu.

The general problem is that if in bdrv_parent_drained_end() we undrain
parent A, and then undrain parent B, which then leads to A detaching the
child, bdrv_replace_child_noperm() will undrain A as if we had not done
so yet; that is, one time too many.

It follows that we cannot decrement the quiesce_counter after invoking
bdrv_parent_drained_end().

Unfortunately, decrementing it before bdrv_parent_drained_end() would be
wrong, too.  Imagine the above situation in reverse: Undraining A leads
to B detaching the child.  If we had already decremented the
quiesce_counter by that point, bdrv_replace_child_noperm() would undrain
B one time too little; because it expects bdrv_parent_drained_end() to
issue this undrain.  But bdrv_parent_drained_end() won't do that,
because B is no longer a parent.

Therefore, we have to do something else.  This patch opts for
introducing a second quiesce_counter that counts how many times a
child's parent has been quiesced (though c->role->drained_*).  With
that, bdrv_replace_child_noperm() just has to undrain the parent exactly
that many times when removing a child, and it will always be right.

Signed-off-by: Max Reitz <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: 8e4428106aecb05a55c8cc97d927adaab7454b71
      
https://github.com/qemu/qemu/commit/8e4428106aecb05a55c8cc97d927adaab7454b71
  Author: Max Reitz <address@hidden>
  Date:   2019-07-19 (Fri, 19 Jul 2019)

  Changed paths:
    M tests/test-bdrv-drain.c

  Log Message:
  -----------
  tests: Add job commit by drained_end test

Signed-off-by: Max Reitz <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: 8e1da77e6e4866876236d0f0c7b02dea87efd2a4
      
https://github.com/qemu/qemu/commit/8e1da77e6e4866876236d0f0c7b02dea87efd2a4
  Author: Max Reitz <address@hidden>
  Date:   2019-07-19 (Fri, 19 Jul 2019)

  Changed paths:
    M block/io.c

  Log Message:
  -----------
  block: Add @drained_end_counter

Callers can now pass a pointer to an integer that bdrv_drain_invoke()
(and its recursive callees) will increment for every
bdrv_drain_invoke_entry() operation they schedule.
bdrv_drain_invoke_entry() in turn will decrement it once it has invoked
BlockDriver.bdrv_co_drain_end().

We use atomic operations to access the pointee, because the
bdrv_do_drained_end() caller may wish to end drained sections for
multiple nodes in different AioContexts (bdrv_drain_all_end() does, for
example).

This is the first step to moving the polling for BdrvCoDrainData.done to
become true out of bdrv_drain_invoke() and into the root drained_end
function.

Signed-off-by: Max Reitz <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: f4c8a43be080fc919bc1ba18e70d83eb0e5be7ec
      
https://github.com/qemu/qemu/commit/f4c8a43be080fc919bc1ba18e70d83eb0e5be7ec
  Author: Max Reitz <address@hidden>
  Date:   2019-07-19 (Fri, 19 Jul 2019)

  Changed paths:
    M block/io.c
    M include/block/block.h

  Log Message:
  -----------
  block: Make bdrv_parent_drained_[^_]*() static

These functions are not used outside of block/io.c, there is no reason
why they should be globally available.

Signed-off-by: Max Reitz <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: 1b285657687c7f08761759092fa05fa33578fc00
      
https://github.com/qemu/qemu/commit/1b285657687c7f08761759092fa05fa33578fc00
  Author: Max Reitz <address@hidden>
  Date:   2019-07-19 (Fri, 19 Jul 2019)

  Changed paths:
    M tests/test-block-iothread.c

  Log Message:
  -----------
  tests: Lock AioContexts in test-block-iothread

When changing a node's AioContext, the caller must acquire the old
AioContext (unless it currently runs in that old context).  Therefore,
unless the node currently is in the main context, we always have to
acquire the old context around calls that may change a node's
AioContext.

Signed-off-by: Max Reitz <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: e037c09c78520cbdb6da7cfc6ad0256d5870b814
      
https://github.com/qemu/qemu/commit/e037c09c78520cbdb6da7cfc6ad0256d5870b814
  Author: Max Reitz <address@hidden>
  Date:   2019-07-19 (Fri, 19 Jul 2019)

  Changed paths:
    M block.c
    M block/block-backend.c
    M block/io.c
    M blockjob.c
    M include/block/block.h
    M include/block/block_int.h

  Log Message:
  -----------
  block: Do not poll in bdrv_do_drained_end()

We should never poll anywhere in bdrv_do_drained_end() (including its
recursive callees like bdrv_drain_invoke()), because it does not cope
well with graph changes.  In fact, it has been written based on the
postulation that no graph changes will happen in it.

Instead, the callers that want to poll must poll, i.e. all currently
globally available wrappers: bdrv_drained_end(),
bdrv_subtree_drained_end(), bdrv_unapply_subtree_drain(), and
bdrv_drain_all_end().  Graph changes there do not matter.

They can poll simply by passing a pointer to a drained_end_counter and
wait until it reaches 0.

This patch also adds a non-polling global wrapper for
bdrv_do_drained_end() that takes a drained_end_counter pointer.  We need
such a variant because now no function called anywhere from
bdrv_do_drained_end() must poll.  This includes
BdrvChildRole.drained_end(), which already must not poll according to
its interface documentation, but bdrv_child_cb_drained_end() just
violates that by invoking bdrv_drained_end() (which does poll).
Therefore, BdrvChildRole.drained_end() must take a *drained_end_counter
parameter, which bdrv_child_cb_drained_end() can pass on to the new
bdrv_drained_end_no_poll() function.

Note that we now have a pattern of all drained_end-related functions
either polling or receiving a *drained_end_counter to let the caller
poll based on that.

A problem with a single poll loop is that when the drained section in
bdrv_set_aio_context_ignore() ends, some nodes in the subgraph may be in
the old contexts, while others are in the new context already.  To let
the collective poll in bdrv_drained_end() work correctly, we must not
hold a lock to the old context, so that the old context can make
progress in case it is different from the current context.

(In the process, remove the comment saying that the current context is
always the old context, because it is wrong.)

In all other places, all nodes in a subtree must be in the same context,
so we can just poll that.  The exception of course is
bdrv_drain_all_end(), but that always runs in the main context, so we
can just poll NULL (like bdrv_drain_all_begin() does).

Signed-off-by: Max Reitz <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: 2afdc790ec0e5cb61d2f738adafa93f990ca553d
      
https://github.com/qemu/qemu/commit/2afdc790ec0e5cb61d2f738adafa93f990ca553d
  Author: Max Reitz <address@hidden>
  Date:   2019-07-19 (Fri, 19 Jul 2019)

  Changed paths:
    M tests/test-bdrv-drain.c

  Log Message:
  -----------
  tests: Extend commit by drained_end test

Signed-off-by: Max Reitz <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: 61ad631cee21f878540681274fe0f53e7ee9f59e
      
https://github.com/qemu/qemu/commit/61ad631cee21f878540681274fe0f53e7ee9f59e
  Author: Max Reitz <address@hidden>
  Date:   2019-07-19 (Fri, 19 Jul 2019)

  Changed paths:
    M block/io.c

  Log Message:
  -----------
  block: Loop unsafely in bdrv*drained_end()

The graph must not change in these loops (or a QLIST_FOREACH_SAFE would
not even be enough).  We now ensure this by only polling once in the
root bdrv_drained_end() call, so we can drop the _SAFE suffix.  Doing so
makes it clear that the graph must not change.

Signed-off-by: Max Reitz <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: 4687133b812323b743e490a21510a1e1ac0fb6df
      
https://github.com/qemu/qemu/commit/4687133b812323b743e490a21510a1e1ac0fb6df
  Author: Max Reitz <address@hidden>
  Date:   2019-07-19 (Fri, 19 Jul 2019)

  Changed paths:
    M python/qemu/machine.py
    M tests/qemu-iotests/255

  Log Message:
  -----------
  iotests: Add @has_quit to vm.shutdown()

If a test has issued a quit command already (which may be useful to do
explicitly because the test wants to show its effects),
QEMUMachine.shutdown() should not do so again.  Otherwise, the VM may
well return an ECONNRESET which will lead QEMUMachine.shutdown() to
killing it, which then turns into a "qemu received signal 9" line.

Signed-off-by: Max Reitz <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: 86472071f484f0df26b62b6f8cb25059df62d889
      
https://github.com/qemu/qemu/commit/86472071f484f0df26b62b6f8cb25059df62d889
  Author: Max Reitz <address@hidden>
  Date:   2019-07-19 (Fri, 19 Jul 2019)

  Changed paths:
    M tests/qemu-iotests/040
    M tests/qemu-iotests/040.out

  Log Message:
  -----------
  iotests: Test commit with a filter on the chain

Before the previous patches, the first case resulted in a failed
assertion (which is noted as qemu receiving a SIGABRT in the test
output), and the second usually triggered a segmentation fault.

Signed-off-by: Max Reitz <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: e6f0ac4d52ae99cf88d66bca8746beb0d25ef0cd
      
https://github.com/qemu/qemu/commit/e6f0ac4d52ae99cf88d66bca8746beb0d25ef0cd
  Author: Max Reitz <address@hidden>
  Date:   2019-07-19 (Fri, 19 Jul 2019)

  Changed paths:
    M vl.c

  Log Message:
  -----------
  vl: Drain before (block) job cancel when quitting

If the main loop cancels all block jobs while the block layer is not
drained, this cancelling may not happen instantaneously.  We can start a
drained section before vm_shutdown(), which entails another
bdrv_drain_all(); this nested bdrv_drain_all() will thus be a no-op,
basically.

We do not have to end the drained section, because we actually do not
want any requests to happen from this point on.

Signed-off-by: Max Reitz <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: 49278ec065da3fbf90f7effcde3b39ac606b2e9e
      
https://github.com/qemu/qemu/commit/49278ec065da3fbf90f7effcde3b39ac606b2e9e
  Author: Max Reitz <address@hidden>
  Date:   2019-07-19 (Fri, 19 Jul 2019)

  Changed paths:
    M tests/qemu-iotests/218
    M tests/qemu-iotests/218.out

  Log Message:
  -----------
  iotests: Test quitting with job on throttled node

When qemu quits, all throttling should be ignored.  That means, if there
is a mirror job running from a throttled node, it should be cancelled
immediately and qemu close without blocking.

Signed-off-by: Max Reitz <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: 4a10982c320740d2d0565180d901a69b043dc282
      
https://github.com/qemu/qemu/commit/4a10982c320740d2d0565180d901a69b043dc282
  Author: Peter Maydell <address@hidden>
  Date:   2019-07-19 (Fri, 19 Jul 2019)

  Changed paths:
    M block.c
    M block/block-backend.c
    M block/io.c
    M blockjob.c
    M include/block/block.h
    M include/block/block_int.h
    M python/qemu/machine.py
    M tests/qemu-iotests/040
    M tests/qemu-iotests/040.out
    M tests/qemu-iotests/051
    M tests/qemu-iotests/051.pc.out
    M tests/qemu-iotests/093
    M tests/qemu-iotests/136
    M tests/qemu-iotests/186
    M tests/qemu-iotests/186.out
    M tests/qemu-iotests/218
    M tests/qemu-iotests/218.out
    M tests/qemu-iotests/227
    M tests/qemu-iotests/227.out
    M tests/qemu-iotests/238
    M tests/qemu-iotests/240
    M tests/qemu-iotests/255
    M tests/test-bdrv-drain.c
    M tests/test-block-iothread.c
    M vl.c

  Log Message:
  -----------
  Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging

Block layer patches:

- block: Fix forbidden use of polling in drained_end
- block: Don't wait for I/O throttling while exiting QEMU
- iotests: Use read-zeroes for the null driver to be Valgrind-friendly

# gpg: Signature made Fri 19 Jul 2019 14:30:14 BST
# gpg:                using RSA key 7F09B272C88F2FD6
# gpg: Good signature from "Kevin Wolf <address@hidden>" [full]
# Primary key fingerprint: DC3D EB15 9A9A F95D 3D74  56FE 7F09 B272 C88F 2FD6

* remotes/kevin/tags/for-upstream:
  iotests: Test quitting with job on throttled node
  vl: Drain before (block) job cancel when quitting
  iotests: Test commit with a filter on the chain
  iotests: Add @has_quit to vm.shutdown()
  block: Loop unsafely in bdrv*drained_end()
  tests: Extend commit by drained_end test
  block: Do not poll in bdrv_do_drained_end()
  tests: Lock AioContexts in test-block-iothread
  block: Make bdrv_parent_drained_[^_]*() static
  block: Add @drained_end_counter
  tests: Add job commit by drained_end test
  block: Introduce BdrvChild.parent_quiesce_counter
  iotests: Set read-zeroes on in null block driver for Valgrind

Signed-off-by: Peter Maydell <address@hidden>


Compare: https://github.com/qemu/qemu/compare/e2b47666fe15...4a10982c3207