qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PULL 00/14] Block layer patches
@ 2017-01-09 13:44 Kevin Wolf
  2017-01-09 15:30 ` Peter Maydell
  0 siblings, 1 reply; 24+ messages in thread
From: Kevin Wolf @ 2017-01-09 13:44 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel

The following changes since commit ffe22bf51065dd33022cf91f77a821d1f11c250d:

  Merge remote-tracking branch 'remotes/gonglei/tags/cryptodev-next-20161224' into staging (2017-01-06 15:18:09 +0000)

are available in the git repository at:


  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to c1bb86cd8ae67c14f79422b6e544d1e2bf40eeb2:

  block: Rename raw-{posix,win32} to file-*.c (2017-01-09 13:30:53 +0100)

----------------------------------------------------------------
Block layer patches

----------------------------------------------------------------
Eric Blake (2):
      block: Rename raw_bsd to raw-format.c
      block: Rename raw-{posix,win32} to file-*.c

Kevin Wolf (11):
      coroutine: Introduce qemu_coroutine_enter_if_inactive()
      quorum: Remove s from quorum_aio_get() arguments
      quorum: Implement .bdrv_co_readv/writev
      quorum: Do cleanup in caller coroutine
      quorum: Inline quorum_aio_cb()
      quorum: Avoid bdrv_aio_writev() for rewrites
      quorum: Implement .bdrv_co_preadv/pwritev()
      quorum: Inline quorum_fifo_aio_cb()
      quorum: Clean up quorum_aio_get()
      blkdebug: Implement bdrv_co_preadv/pwritev/flush
      blkverify: Implement bdrv_co_preadv/pwritev/flush

Paolo Bonzini (1):
      qemu-img: fix in-flight count for qemu-img bench

 MAINTAINERS                         |   6 +-
 block/Makefile.objs                 |   6 +-
 block/blkdebug.c                    |  86 ++++----
 block/blkverify.c                   | 201 +++++++++---------
 block/{raw-posix.c => file-posix.c} |   0
 block/{raw-win32.c => file-win32.c} |   0
 block/gluster.c                     |   4 +-
 block/quorum.c                      | 410 +++++++++++++++++++-----------------
 block/{raw_bsd.c => raw-format.c}   |   2 +-
 block/trace-events                  |   4 +-
 configure                           |   2 +-
 include/block/block_int.h           |   2 +-
 include/qemu/coroutine.h            |   6 +
 qemu-img.c                          |  17 +-
 tests/qemu-iotests/071.out          |   8 +-
 util/qemu-coroutine.c               |   7 +
 16 files changed, 392 insertions(+), 369 deletions(-)
 rename block/{raw-posix.c => file-posix.c} (100%)
 rename block/{raw-win32.c => file-win32.c} (100%)
 rename block/{raw_bsd.c => raw-format.c} (99%)

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Qemu-devel] [PULL 00/14] Block layer patches
  2017-01-09 13:44 Kevin Wolf
@ 2017-01-09 15:30 ` Peter Maydell
  0 siblings, 0 replies; 24+ messages in thread
From: Peter Maydell @ 2017-01-09 15:30 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: Qemu-block, QEMU Developers

On 9 January 2017 at 13:44, Kevin Wolf <kwolf@redhat.com> wrote:
> The following changes since commit ffe22bf51065dd33022cf91f77a821d1f11c250d:
>
>   Merge remote-tracking branch 'remotes/gonglei/tags/cryptodev-next-20161224' into staging (2017-01-06 15:18:09 +0000)
>
> are available in the git repository at:
>
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to c1bb86cd8ae67c14f79422b6e544d1e2bf40eeb2:
>
>   block: Rename raw-{posix,win32} to file-*.c (2017-01-09 13:30:53 +0100)
>
> ----------------------------------------------------------------
> Block layer patches
>
> ----------------------------------------------------------------

Applied, thanks.

-- PMM

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PULL 00/14] Block layer patches
@ 2017-09-06 14:02 Kevin Wolf
  2017-09-07 10:50 ` Peter Maydell
  0 siblings, 1 reply; 24+ messages in thread
From: Kevin Wolf @ 2017-09-06 14:02 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel

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

----------------------------------------------------------------
Daniel P. Berrange (1):
      block: document semantics of bdrv_co_preadv|pwritev

Eric Blake (2):
      qcow: Change signature of get_cluster_offset()
      qcow: Check failure of bdrv_getlength() and bdrv_truncate()

Manos Pitsidianakis (10):
      block: pass bdrv_* methods to bs->file by default in block filters
      block: remove unused bdrv_media_changed
      block: remove bdrv_truncate callback in blkdebug
      block: add default implementations for bdrv_co_get_block_status()
      block: move ThrottleGroup membership to ThrottleGroupMember
      block: add aio_context field in ThrottleGroupMember
      block: tidy ThrottleGroupMember initializations
      block: convert ThrottleGroup to object with QOM
      block: add throttle block filter driver
      qemu-iotests: add 184 for throttle filter driver

Pavel Butsykin (1):
      qcow2: move qcow2_store_persistent_dirty_bitmaps() before cache flushing

 qapi/block-core.json            |  66 +++-
 include/block/block.h           |   1 -
 include/block/block_int.h       |  56 ++-
 include/block/throttle-groups.h |  52 ++-
 include/qemu/throttle-options.h |  60 +++-
 include/qemu/throttle.h         |   3 +
 include/sysemu/block-backend.h  |  20 +-
 block.c                         |  35 +-
 block/blkdebug.c                |  20 +-
 block/block-backend.c           |  62 ++--
 block/commit.c                  |  12 +-
 block/io.c                      |  26 ++
 block/mirror.c                  |  12 +-
 block/qapi.c                    |   8 +-
 block/qcow.c                    | 153 ++++----
 block/qcow2.c                   |  16 +-
 block/raw-format.c              |   6 -
 block/throttle-groups.c         | 750 ++++++++++++++++++++++++++++++----------
 block/throttle.c                | 237 +++++++++++++
 blockdev.c                      |   4 +-
 tests/test-throttle.c           | 111 +++---
 util/throttle.c                 | 151 ++++++++
 block/Makefile.objs             |   1 +
 tests/qemu-iotests/184          | 205 +++++++++++
 tests/qemu-iotests/184.out      | 302 ++++++++++++++++
 tests/qemu-iotests/group        |   1 +
 26 files changed, 1917 insertions(+), 453 deletions(-)
 create mode 100644 block/throttle.c
 create mode 100755 tests/qemu-iotests/184
 create mode 100644 tests/qemu-iotests/184.out

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Qemu-devel] [PULL 00/14] Block layer patches
  2017-09-06 14:02 Kevin Wolf
@ 2017-09-07 10:50 ` Peter Maydell
  0 siblings, 0 replies; 24+ messages in thread
From: Peter Maydell @ 2017-09-07 10:50 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: Qemu-block, QEMU Developers

On 6 September 2017 at 15:02, Kevin Wolf <kwolf@redhat.com> 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PULL 00/14] Block layer patches
@ 2017-12-20 13:19 Kevin Wolf
  2017-12-20 15:30 ` Peter Maydell
  0 siblings, 1 reply; 24+ messages in thread
From: Kevin Wolf @ 2017-12-20 13:19 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel

The following changes since commit af352675efb7e92a1f5f6461a042a12015ab3d12:

  Merge remote-tracking branch 'remotes/aurel/tags/pull-target-sh4-20171218' into staging (2017-12-19 19:11:11 +0000)

are available in the git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to b4d526c87b26aff6d8b353951aa175a1236ad887:

  nvme: Add tracing (2017-12-20 11:05:49 +0100)

----------------------------------------------------------------
Block layer patches

----------------------------------------------------------------
Doug Gale (1):
      nvme: Add tracing

Edgar Kaziakhmedov (1):
      qcow2: get rid of qcow2_backing_read1 routine

Fam Zheng (1):
      qemu-img: Document --force-share / -U

John Snow (1):
      iotests: fix 197 for vpc

Kevin Wolf (9):
      block: Formats don't need CONSISTENT_READ with NO_IO
      block: Make bdrv_drain_invoke() recursive
      block: Call .drain_begin only once in bdrv_drain_all_begin()
      test-bdrv-drain: Test BlockDriver callbacks for drain
      block: bdrv_drain_recurse(): Remove unused begin parameter
      block: Don't wait for requests in bdrv_drain*_end()
      block: Unify order in drain functions
      block: Don't acquire AioContext in hmp_qemu_io()
      block: Document that x-blockdev-change breaks quorum children list

Mao Zhongyi (1):
      hw/block/nvme: Convert to realize

 qapi/block-core.json             |   4 +
 block/qcow2.h                    |   3 -
 block.c                          |   6 +-
 block/io.c                       |  31 ++--
 block/qcow2.c                    |  51 +-----
 hmp.c                            |   6 -
 hw/block/nvme.c                  | 367 ++++++++++++++++++++++++++++++++-------
 tests/test-bdrv-drain.c          | 137 +++++++++++++++
 hw/block/trace-events            |  93 ++++++++++
 qemu-img.texi                    |   9 +
 tests/Makefile.include           |   2 +
 tests/qemu-iotests/197           |   4 +
 tests/qemu-iotests/common.filter |   3 +-
 13 files changed, 591 insertions(+), 125 deletions(-)
 create mode 100644 tests/test-bdrv-drain.c

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Qemu-devel] [PULL 00/14] Block layer patches
  2017-12-20 13:19 Kevin Wolf
@ 2017-12-20 15:30 ` Peter Maydell
  0 siblings, 0 replies; 24+ messages in thread
From: Peter Maydell @ 2017-12-20 15:30 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: Qemu-block, QEMU Developers

On 20 December 2017 at 13:19, Kevin Wolf <kwolf@redhat.com> wrote:
> The following changes since commit af352675efb7e92a1f5f6461a042a12015ab3d12:
>
>   Merge remote-tracking branch 'remotes/aurel/tags/pull-target-sh4-20171218' into staging (2017-12-19 19:11:11 +0000)
>
> are available in the git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to b4d526c87b26aff6d8b353951aa175a1236ad887:
>
>   nvme: Add tracing (2017-12-20 11:05:49 +0100)
>
> ----------------------------------------------------------------
> Block layer patches

Hi; this has a merge conflict in hw/block/nvme.c where both
sets of changes are changing the way that the function
handles errors, and I'm not sure which is correct. Can you
resolve and resend, please?

thanks
-- PMM

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PULL 00/14] Block layer patches
@ 2018-11-12 17:05 Kevin Wolf
  2018-11-13 10:14 ` Peter Maydell
  0 siblings, 1 reply; 24+ messages in thread
From: Kevin Wolf @ 2018-11-12 17:05 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel

The following changes since commit 5704c36d25ee84e7129722cb0db53df9faefe943:

  Merge remote-tracking branch 'remotes/kraxel/tags/fixes-31-20181112-pull-request' into staging (2018-11-12 15:55:40 +0000)

are available in the Git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 1a42e5d8298d1b0f90d2254e7d559391dd3a45ca:

  Merge remote-tracking branch 'mreitz/tags/pull-block-2018-11-12' into queue-block (2018-11-12 17:57:32 +0100)

----------------------------------------------------------------
Block layer patches:

- file-posix: Don't waste a file descriptor for locking, don't lock the
  same bit multiple times
- nvme: Fix double free and memory leak
- Misc error handling fixes
- Added NULL checks found by static analysis
- Allow more block drivers to not be included in the qemu build

----------------------------------------------------------------
Fam Zheng (4):
      file-posix: Use error API properly
      file-posix: Skip effectiveless OFD lock operations
      file-posix: Drop s->lock_fd
      tests: Add unit tests for image locking

Jeff Cody (1):
      block: Make more block drivers compile-time configurable

Kevin Wolf (1):
      Merge remote-tracking branch 'mreitz/tags/pull-block-2018-11-12' into queue-block

Li Qiang (2):
      nvme: don't unref ctrl_mem when device unrealized
      nvme: free cmbuf in nvme_exit

Liam Merwick (5):
      job: Fix off-by-one assert checks for JobSTT and JobVerbTable
      block: Null pointer dereference in blk_root_get_parent_desc()
      qemu-img: assert block_job_get() does not return NULL in img_commit()
      block: Fix potential Null pointer dereferences in vvfat.c
      qcow2: Read outside array bounds in qcow2_pre_write_overlap_check()

Peter Maydell (1):
      blockdev: Consistently use snapshot_node_name in external_snapshot_prepare()

zhenwei pi (1):
      blockdev: handle error on block latency histogram set error

 configure                  |  91 ++++++++++++++++++++++++++
 block/block-backend.c      |   3 +-
 block/file-posix.c         | 122 ++++++++++++++++++++---------------
 block/qcow2-refcount.c     |  18 +++---
 block/vvfat.c              |  46 ++++++++-----
 blockdev.c                 |  21 ++++--
 hw/block/nvme.c            |   6 +-
 job.c                      |   4 +-
 qemu-img.c                 |   1 +
 tests/test-image-locking.c | 157 +++++++++++++++++++++++++++++++++++++++++++++
 block/Makefile.objs        |  22 +++++--
 tests/Makefile.include     |   2 +
 12 files changed, 400 insertions(+), 93 deletions(-)
 create mode 100644 tests/test-image-locking.c

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Qemu-devel] [PULL 00/14] Block layer patches
  2018-11-12 17:05 Kevin Wolf
@ 2018-11-13 10:14 ` Peter Maydell
  0 siblings, 0 replies; 24+ messages in thread
From: Peter Maydell @ 2018-11-13 10:14 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: Qemu-block, QEMU Developers

On 12 November 2018 at 17:05, Kevin Wolf <kwolf@redhat.com> wrote:
> The following changes since commit 5704c36d25ee84e7129722cb0db53df9faefe943:
>
>   Merge remote-tracking branch 'remotes/kraxel/tags/fixes-31-20181112-pull-request' into staging (2018-11-12 15:55:40 +0000)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 1a42e5d8298d1b0f90d2254e7d559391dd3a45ca:
>
>   Merge remote-tracking branch 'mreitz/tags/pull-block-2018-11-12' into queue-block (2018-11-12 17:57:32 +0100)
>
> ----------------------------------------------------------------
> Block layer patches:
>
> - file-posix: Don't waste a file descriptor for locking, don't lock the
>   same bit multiple times
> - nvme: Fix double free and memory leak
> - Misc error handling fixes
> - Added NULL checks found by static analysis
> - Allow more block drivers to not be included in the qemu build
>

Applied, thanks.

-- PMM

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PULL 00/14] Block layer patches
@ 2019-06-18 15:23 Kevin Wolf
  2019-06-18 15:23 ` [Qemu-devel] [PULL 01/14] iotests: Hide timestamps for skipped tests Kevin Wolf
                   ` (14 more replies)
  0 siblings, 15 replies; 24+ messages in thread
From: Kevin Wolf @ 2019-06-18 15:23 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel

The following changes since commit cdfaa2720f4a09e5254868bd1f6e33f3e9eae76f:

  Merge remote-tracking branch 'remotes/armbru/tags/pull-monitor-2019-06-17-v2' into staging (2019-06-18 10:47:00 +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 128b05f7e00765d883164631b974a27af5b4b613:

  block/null: Expose read-zeroes option in QAPI schema (2019-06-18 16:41:10 +0200)

----------------------------------------------------------------
Block layer patches:

- block: Remove bs->job
- block: Ignore loosening perm restrictions failures
- block/null: Expose read-zeroes option in QAPI schema
- iotests: Hide timestamps for skipped tests

----------------------------------------------------------------
Kevin Wolf (1):
      block/null: Expose read-zeroes option in QAPI schema

Max Reitz (9):
      iotests: Hide timestamps for skipped tests
      file-posix: Update open_flags in raw_set_perm()
      block: Add bdrv_child_refresh_perms()
      block/mirror: Fix child permissions
      block/commit: Drop bdrv_child_try_set_perm()
      block: Fix order in bdrv_replace_child()
      block: Add *tighten_restrictions to *check*_perm()
      block: Ignore loosening perm restrictions failures
      iotests: Test failure to loosen restrictions

Vladimir Sementsov-Ogievskiy (4):
      block/replication: drop usage of bs->job
      block/block-backend: blk_iostatus_reset: drop usage of bs->job
      blockdev: blockdev_mark_auto_del: drop usage of bs->job
      block: drop bs->job

 qapi/block-core.json       |   4 +-
 include/block/block_int.h  |  30 ++++++---
 include/block/blockjob.h   |   9 +++
 block.c                    | 155 +++++++++++++++++++++++++++++++++++++--------
 block/block-backend.c      |   4 --
 block/commit.c             |   2 -
 block/file-posix.c         |   4 ++
 block/mirror.c             |  70 ++++++++++++--------
 block/replication.c        |  21 +++---
 blockdev.c                 |  19 +++---
 blockjob.c                 |  22 ++++---
 monitor/qmp-cmds.c         |   5 ++
 tests/test-blockjob.c      |   5 +-
 block/trace-events         |   2 +-
 tests/qemu-iotests/182     |  21 ++++++
 tests/qemu-iotests/182.out |   6 ++
 tests/qemu-iotests/check   |   1 +
 17 files changed, 283 insertions(+), 97 deletions(-)


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PULL 01/14] iotests: Hide timestamps for skipped tests
  2019-06-18 15:23 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
@ 2019-06-18 15:23 ` Kevin Wolf
  2019-06-18 15:23 ` [Qemu-devel] [PULL 02/14] block/replication: drop usage of bs->job Kevin Wolf
                   ` (13 subsequent siblings)
  14 siblings, 0 replies; 24+ messages in thread
From: Kevin Wolf @ 2019-06-18 15:23 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel

From: Max Reitz <mreitz@redhat.com>

Currently, the "thistime" variable is not reinitialized on every loop
iteration.  This leads to tests that do not yield a run time (because
they failed or were skipped) printing the run time of the previous test
that did.  Fix that by reinitializing "thistime" for every test.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/check | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 44ebf24080..f925606cc5 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -773,6 +773,7 @@ do
     printdiff=false # show diff to reference output?
     status=""       # test result summary
     results=""      # test result details
+    thistime=""     # time the test took
 
     if [ -n "$TESTS_REMAINING_LOG" ] ; then
         sed -e "s/$seq//" -e 's/  / /' -e 's/^ *//' $TESTS_REMAINING_LOG > $TESTS_REMAINING_LOG.tmp
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PULL 02/14] block/replication: drop usage of bs->job
  2019-06-18 15:23 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
  2019-06-18 15:23 ` [Qemu-devel] [PULL 01/14] iotests: Hide timestamps for skipped tests Kevin Wolf
@ 2019-06-18 15:23 ` Kevin Wolf
  2019-06-18 15:23 ` [Qemu-devel] [PULL 03/14] block/block-backend: blk_iostatus_reset: " Kevin Wolf
                   ` (12 subsequent siblings)
  14 siblings, 0 replies; 24+ messages in thread
From: Kevin Wolf @ 2019-06-18 15:23 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

We are going to remove bs->job pointer. Drop it's usage in replication
code. Additionally we have to return job pointer from some mirror APIs.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block_int.h | 12 ++++++------
 block/mirror.c            | 38 ++++++++++++++++++++++----------------
 block/replication.c       | 21 ++++++++++++---------
 3 files changed, 40 insertions(+), 31 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 06df2bda1b..8bb1cfb80a 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1082,12 +1082,12 @@ void commit_start(const char *job_id, BlockDriverState *bs,
  * @errp: Error object.
  *
  */
-void commit_active_start(const char *job_id, BlockDriverState *bs,
-                         BlockDriverState *base, int creation_flags,
-                         int64_t speed, BlockdevOnError on_error,
-                         const char *filter_node_name,
-                         BlockCompletionFunc *cb, void *opaque,
-                         bool auto_complete, Error **errp);
+BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
+                              BlockDriverState *base, int creation_flags,
+                              int64_t speed, BlockdevOnError on_error,
+                              const char *filter_node_name,
+                              BlockCompletionFunc *cb, void *opaque,
+                              bool auto_complete, Error **errp);
 /*
  * mirror_start:
  * @job_id: The id of the newly-created job, or %NULL to use the
diff --git a/block/mirror.c b/block/mirror.c
index f8bdb5b21b..b5878ba574 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1482,7 +1482,8 @@ static BlockDriver bdrv_mirror_top = {
     .bdrv_child_perm            = bdrv_mirror_top_child_perm,
 };
 
-static void mirror_start_job(const char *job_id, BlockDriverState *bs,
+static BlockJob *mirror_start_job(
+                             const char *job_id, BlockDriverState *bs,
                              int creation_flags, BlockDriverState *target,
                              const char *replaces, int64_t speed,
                              uint32_t granularity, int64_t buf_size,
@@ -1514,7 +1515,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
 
     if (buf_size < 0) {
         error_setg(errp, "Invalid parameter 'buf-size'");
-        return;
+        return NULL;
     }
 
     if (buf_size == 0) {
@@ -1523,7 +1524,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
 
     if (bs == target) {
         error_setg(errp, "Can't mirror node into itself");
-        return;
+        return NULL;
     }
 
     /* In the case of active commit, add dummy driver to provide consistent
@@ -1532,7 +1533,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
     mirror_top_bs = bdrv_new_open_driver(&bdrv_mirror_top, filter_node_name,
                                          BDRV_O_RDWR, errp);
     if (mirror_top_bs == NULL) {
-        return;
+        return NULL;
     }
     if (!filter_node_name) {
         mirror_top_bs->implicit = true;
@@ -1554,7 +1555,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
     if (local_err) {
         bdrv_unref(mirror_top_bs);
         error_propagate(errp, local_err);
-        return;
+        return NULL;
     }
 
     /* Make sure that the source is not resized while the job is running */
@@ -1662,7 +1663,8 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
 
     trace_mirror_start(bs, s, opaque);
     job_start(&s->common.job);
-    return;
+
+    return &s->common;
 
 fail:
     if (s) {
@@ -1684,6 +1686,8 @@ fail:
     bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
 
     bdrv_unref(mirror_top_bs);
+
+    return NULL;
 }
 
 void mirror_start(const char *job_id, BlockDriverState *bs,
@@ -1712,25 +1716,27 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
                      filter_node_name, true, copy_mode, errp);
 }
 
-void commit_active_start(const char *job_id, BlockDriverState *bs,
-                         BlockDriverState *base, int creation_flags,
-                         int64_t speed, BlockdevOnError on_error,
-                         const char *filter_node_name,
-                         BlockCompletionFunc *cb, void *opaque,
-                         bool auto_complete, Error **errp)
+BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
+                              BlockDriverState *base, int creation_flags,
+                              int64_t speed, BlockdevOnError on_error,
+                              const char *filter_node_name,
+                              BlockCompletionFunc *cb, void *opaque,
+                              bool auto_complete, Error **errp)
 {
     bool base_read_only;
     Error *local_err = NULL;
+    BlockJob *ret;
 
     base_read_only = bdrv_is_read_only(base);
 
     if (base_read_only) {
         if (bdrv_reopen_set_read_only(base, false, errp) < 0) {
-            return;
+            return NULL;
         }
     }
 
-    mirror_start_job(job_id, bs, creation_flags, base, NULL, speed, 0, 0,
+    ret = mirror_start_job(
+                     job_id, bs, creation_flags, base, NULL, speed, 0, 0,
                      MIRROR_LEAVE_BACKING_CHAIN,
                      on_error, on_error, true, cb, opaque,
                      &commit_active_job_driver, false, base, auto_complete,
@@ -1741,7 +1747,7 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
         goto error_restore_flags;
     }
 
-    return;
+    return ret;
 
 error_restore_flags:
     /* ignore error and errp for bdrv_reopen, because we want to propagate
@@ -1749,5 +1755,5 @@ error_restore_flags:
     if (base_read_only) {
         bdrv_reopen_set_read_only(base, true, NULL);
     }
-    return;
+    return NULL;
 }
diff --git a/block/replication.c b/block/replication.c
index 5cffb790b3..b41bc507c0 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -36,8 +36,10 @@ typedef struct BDRVReplicationState {
     ReplicationMode mode;
     ReplicationStage stage;
     BdrvChild *active_disk;
+    BlockJob *commit_job;
     BdrvChild *hidden_disk;
     BdrvChild *secondary_disk;
+    BlockJob *backup_job;
     char *top_id;
     ReplicationState *rs;
     Error *blocker;
@@ -147,7 +149,7 @@ static void replication_close(BlockDriverState *bs)
         replication_stop(s->rs, false, NULL);
     }
     if (s->stage == BLOCK_REPLICATION_FAILOVER) {
-        job_cancel_sync(&s->active_disk->bs->job->job);
+        job_cancel_sync(&s->commit_job->job);
     }
 
     if (s->mode == REPLICATION_MODE_SECONDARY) {
@@ -315,12 +317,12 @@ static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
     Error *local_err = NULL;
     int ret;
 
-    if (!s->secondary_disk->bs->job) {
+    if (!s->backup_job) {
         error_setg(errp, "Backup job was cancelled unexpectedly");
         return;
     }
 
-    backup_do_checkpoint(s->secondary_disk->bs->job, &local_err);
+    backup_do_checkpoint(s->backup_job, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -449,7 +451,6 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
     int64_t active_length, hidden_length, disk_length;
     AioContext *aio_context;
     Error *local_err = NULL;
-    BlockJob *job;
 
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
@@ -540,7 +541,8 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
         bdrv_op_block_all(top_bs, s->blocker);
         bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
 
-        job = backup_job_create(NULL, s->secondary_disk->bs, s->hidden_disk->bs,
+        s->backup_job = backup_job_create(
+                                NULL, s->secondary_disk->bs, s->hidden_disk->bs,
                                 0, MIRROR_SYNC_MODE_NONE, NULL, false,
                                 BLOCKDEV_ON_ERROR_REPORT,
                                 BLOCKDEV_ON_ERROR_REPORT, JOB_INTERNAL,
@@ -551,7 +553,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
             aio_context_release(aio_context);
             return;
         }
-        job_start(&job->job);
+        job_start(&s->backup_job->job);
         break;
     default:
         aio_context_release(aio_context);
@@ -653,8 +655,8 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
          * before the BDS is closed, because we will access hidden
          * disk, secondary disk in backup_job_completed().
          */
-        if (s->secondary_disk->bs->job) {
-            job_cancel_sync(&s->secondary_disk->bs->job->job);
+        if (s->backup_job) {
+            job_cancel_sync(&s->backup_job->job);
         }
 
         if (!failover) {
@@ -665,7 +667,8 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
         }
 
         s->stage = BLOCK_REPLICATION_FAILOVER;
-        commit_active_start(NULL, s->active_disk->bs, s->secondary_disk->bs,
+        s->commit_job = commit_active_start(
+                            NULL, s->active_disk->bs, s->secondary_disk->bs,
                             JOB_INTERNAL, 0, BLOCKDEV_ON_ERROR_REPORT,
                             NULL, replication_done, bs, true, errp);
         break;
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PULL 03/14] block/block-backend: blk_iostatus_reset: drop usage of bs->job
  2019-06-18 15:23 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
  2019-06-18 15:23 ` [Qemu-devel] [PULL 01/14] iotests: Hide timestamps for skipped tests Kevin Wolf
  2019-06-18 15:23 ` [Qemu-devel] [PULL 02/14] block/replication: drop usage of bs->job Kevin Wolf
@ 2019-06-18 15:23 ` Kevin Wolf
  2019-06-18 15:23 ` [Qemu-devel] [PULL 04/14] blockdev: blockdev_mark_auto_del: " Kevin Wolf
                   ` (11 subsequent siblings)
  14 siblings, 0 replies; 24+ messages in thread
From: Kevin Wolf @ 2019-06-18 15:23 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

We are going to remove bs->job pointer. Drop it's usage in
blk_iostatus_reset.

blk_iostatus_reset() has only two callers:

1. blk_attach_dev(). This doesn't have anything to do with jobs and
    attaching a new guest device won't solve any problem the job
    encountered, so no reason to reset the iostatus for the job.

2. qmp_cont(). This resets the iostatus for everything. We can just
    call block_job_iostatus_reset() for all block jobs instead of going
    through BlockBackend.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c | 4 ----
 monitor/qmp-cmds.c    | 5 +++++
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index f5d9407d20..a8d160fd5d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1073,11 +1073,7 @@ void blk_iostatus_disable(BlockBackend *blk)
 void blk_iostatus_reset(BlockBackend *blk)
 {
     if (blk_iostatus_is_enabled(blk)) {
-        BlockDriverState *bs = blk_bs(blk);
         blk->iostatus = BLOCK_DEVICE_IO_STATUS_OK;
-        if (bs && bs->job) {
-            block_job_iostatus_reset(bs->job);
-        }
     }
 }
 
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index f1b1e4f08b..01ce77e129 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -143,6 +143,7 @@ void qmp_x_exit_preconfig(Error **errp)
 void qmp_cont(Error **errp)
 {
     BlockBackend *blk;
+    BlockJob *job;
     Error *local_err = NULL;
 
     /* if there is a dump in background, we should wait until the dump
@@ -166,6 +167,10 @@ void qmp_cont(Error **errp)
         blk_iostatus_reset(blk);
     }
 
+    for (job = block_job_next(NULL); job; job = block_job_next(job)) {
+        block_job_iostatus_reset(job);
+    }
+
     /* Continuing after completed migration. Images have been inactivated to
      * allow the destination to take control. Need to get control back now.
      *
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PULL 04/14] blockdev: blockdev_mark_auto_del: drop usage of bs->job
  2019-06-18 15:23 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
                   ` (2 preceding siblings ...)
  2019-06-18 15:23 ` [Qemu-devel] [PULL 03/14] block/block-backend: blk_iostatus_reset: " Kevin Wolf
@ 2019-06-18 15:23 ` Kevin Wolf
  2019-06-18 15:23 ` [Qemu-devel] [PULL 05/14] block: drop bs->job Kevin Wolf
                   ` (10 subsequent siblings)
  14 siblings, 0 replies; 24+ messages in thread
From: Kevin Wolf @ 2019-06-18 15:23 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

We are going to remove bs->job pointer. Drop it's usage in
blockdev_mark_auto_del: instead of looking at bs->job let's check all
jobs for references to bs.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/blockjob.h |  9 +++++++++
 blockdev.c               | 17 ++++++++---------
 blockjob.c               | 14 ++++++++++++++
 3 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index ede0bd8dcb..35faa3aa26 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -121,6 +121,15 @@ int block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,
  */
 void block_job_remove_all_bdrv(BlockJob *job);
 
+/**
+ * block_job_has_bdrv:
+ * @job: The block job
+ *
+ * Searches for @bs in the list of nodes that are involved in the
+ * job.
+ */
+bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs);
+
 /**
  * block_job_set_speed:
  * @job: The job to set the speed for.
diff --git a/blockdev.c b/blockdev.c
index b5c0fd3c49..a9dd73eafc 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -140,22 +140,21 @@ void override_max_devs(BlockInterfaceType type, int max_devs)
 void blockdev_mark_auto_del(BlockBackend *blk)
 {
     DriveInfo *dinfo = blk_legacy_dinfo(blk);
-    BlockDriverState *bs = blk_bs(blk);
-    AioContext *aio_context;
+    BlockJob *job;
 
     if (!dinfo) {
         return;
     }
 
-    if (bs) {
-        aio_context = bdrv_get_aio_context(bs);
-        aio_context_acquire(aio_context);
+    for (job = block_job_next(NULL); job; job = block_job_next(job)) {
+        if (block_job_has_bdrv(job, blk_bs(blk))) {
+            AioContext *aio_context = job->job.aio_context;
+            aio_context_acquire(aio_context);
 
-        if (bs->job) {
-            job_cancel(&bs->job->job, false);
-        }
+            job_cancel(&job->job, false);
 
-        aio_context_release(aio_context);
+            aio_context_release(aio_context);
+        }
     }
 
     dinfo->auto_del = 1;
diff --git a/blockjob.c b/blockjob.c
index 1fac6bb8a7..c3620ec544 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -198,6 +198,20 @@ void block_job_remove_all_bdrv(BlockJob *job)
     job->nodes = NULL;
 }
 
+bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs)
+{
+    GSList *el;
+
+    for (el = job->nodes; el; el = el->next) {
+        BdrvChild *c = el->data;
+        if (c->bs == bs) {
+            return true;
+        }
+    }
+
+    return false;
+}
+
 int block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,
                        uint64_t perm, uint64_t shared_perm, Error **errp)
 {
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PULL 05/14] block: drop bs->job
  2019-06-18 15:23 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
                   ` (3 preceding siblings ...)
  2019-06-18 15:23 ` [Qemu-devel] [PULL 04/14] blockdev: blockdev_mark_auto_del: " Kevin Wolf
@ 2019-06-18 15:23 ` Kevin Wolf
  2019-06-18 15:23 ` [Qemu-devel] [PULL 06/14] file-posix: Update open_flags in raw_set_perm() Kevin Wolf
                   ` (9 subsequent siblings)
  14 siblings, 0 replies; 24+ messages in thread
From: Kevin Wolf @ 2019-06-18 15:23 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Drop remaining users of bs->job:
1. assertions actually duplicated by assert(!bs->refcnt)
2. trace-point seems not enough reason to change stream_start to return
   BlockJob pointer
3. Restricting creation of two jobs based on same bs is bad idea, as
   3.1 Some jobs creates filters to be their main node, so, this check
   don't actually prevent creating second job on same real node (which
   will create another filter node) (but I hope it is restricted by
   other mechanisms)
   3.2 Even without bs->job we have two systems of permissions:
   op-blockers and BLK_PERM
   3.3 We may want to run several jobs on one node one day

And finally, drop bs->job pointer itself. Hurrah!

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block_int.h | 3 ---
 block.c                   | 2 --
 blockdev.c                | 2 +-
 blockjob.c                | 8 --------
 tests/test-blockjob.c     | 5 +++--
 block/trace-events        | 2 +-
 6 files changed, 5 insertions(+), 17 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8bb1cfb80a..a498c2670b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -812,9 +812,6 @@ struct BlockDriverState {
     /* operation blockers */
     QLIST_HEAD(, BdrvOpBlocker) op_blockers[BLOCK_OP_TYPE_MAX];
 
-    /* long-running background operation */
-    BlockJob *job;
-
     /* The node that this node inherited default options from (and a reopen on
      * which can affect this node by changing these defaults). This is always a
      * parent node of this node. */
diff --git a/block.c b/block.c
index e3e77feee0..ceb2ea23c5 100644
--- a/block.c
+++ b/block.c
@@ -3905,7 +3905,6 @@ static void bdrv_close(BlockDriverState *bs)
     BdrvAioNotifier *ban, *ban_next;
     BdrvChild *child, *next;
 
-    assert(!bs->job);
     assert(!bs->refcnt);
 
     bdrv_drained_begin(bs); /* complete I/O */
@@ -4146,7 +4145,6 @@ out:
 
 static void bdrv_delete(BlockDriverState *bs)
 {
-    assert(!bs->job);
     assert(bdrv_op_blocker_is_empty(bs));
     assert(!bs->refcnt);
 
diff --git a/blockdev.c b/blockdev.c
index a9dd73eafc..5d6a13dea9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3260,7 +3260,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
         goto out;
     }
 
-    trace_qmp_block_stream(bs, bs->job);
+    trace_qmp_block_stream(bs);
 
 out:
     aio_context_release(aio_context);
diff --git a/blockjob.c b/blockjob.c
index c3620ec544..458ae76f51 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -83,9 +83,7 @@ BlockJob *block_job_get(const char *id)
 void block_job_free(Job *job)
 {
     BlockJob *bjob = container_of(job, BlockJob, job);
-    BlockDriverState *bs = blk_bs(bjob->blk);
 
-    bs->job = NULL;
     block_job_remove_all_bdrv(bjob);
     blk_unref(bjob->blk);
     error_free(bjob->blocker);
@@ -402,11 +400,6 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
     BlockJob *job;
     int ret;
 
-    if (bs->job) {
-        error_setg(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
-        return NULL;
-    }
-
     if (job_id == NULL && !(flags & JOB_INTERNAL)) {
         job_id = bdrv_get_device_name(bs);
     }
@@ -449,7 +442,6 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
     error_setg(&job->blocker, "block device is in use by block job: %s",
                job_type_str(&job->job));
     block_job_add_bdrv(job, "main node", bs, 0, BLK_PERM_ALL, &error_abort);
-    bs->job = job;
 
     bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker);
 
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index 8c91980c70..b33f899873 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -122,8 +122,9 @@ static void test_job_ids(void)
     /* This one is valid */
     job[0] = do_test_id(blk[0], "id0", true);
 
-    /* We cannot have two jobs in the same BDS */
-    do_test_id(blk[0], "id1", false);
+    /* We can have two jobs in the same BDS */
+    job[1] = do_test_id(blk[0], "id1", true);
+    job_early_fail(&job[1]->job);
 
     /* Duplicate job IDs are not allowed */
     job[1] = do_test_id(blk[1], "id0", false);
diff --git a/block/trace-events b/block/trace-events
index f6e43ee023..9ccea755da 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -53,7 +53,7 @@ qmp_block_job_resume(void *job) "job %p"
 qmp_block_job_complete(void *job) "job %p"
 qmp_block_job_finalize(void *job) "job %p"
 qmp_block_job_dismiss(void *job) "job %p"
-qmp_block_stream(void *bs, void *job) "bs %p job %p"
+qmp_block_stream(void *bs) "bs %p"
 
 # file-posix.c
 # file-win32.c
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PULL 06/14] file-posix: Update open_flags in raw_set_perm()
  2019-06-18 15:23 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
                   ` (4 preceding siblings ...)
  2019-06-18 15:23 ` [Qemu-devel] [PULL 05/14] block: drop bs->job Kevin Wolf
@ 2019-06-18 15:23 ` Kevin Wolf
  2019-06-18 15:23 ` [Qemu-devel] [PULL 07/14] block: Add bdrv_child_refresh_perms() Kevin Wolf
                   ` (8 subsequent siblings)
  14 siblings, 0 replies; 24+ messages in thread
From: Kevin Wolf @ 2019-06-18 15:23 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel

From: Max Reitz <mreitz@redhat.com>

raw_check_perm() + raw_set_perm() can change the flags associated with
the current FD.  If so, we have to update BDRVRawState.open_flags
accordingly.  Otherwise, we may keep reopening the FD even though the
current one already has the correct flags.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index 83ab1b78ef..ab05b51a66 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -146,6 +146,7 @@ typedef struct BDRVRawState {
     uint64_t locked_shared_perm;
 
     int perm_change_fd;
+    int perm_change_flags;
     BDRVReopenState *reopen_state;
 
 #ifdef CONFIG_XFS
@@ -2788,6 +2789,7 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared,
         assert(s->reopen_state->shared_perm == shared);
         rs = s->reopen_state->opaque;
         s->perm_change_fd = rs->fd;
+        s->perm_change_flags = rs->open_flags;
     } else {
         /* We may need a new fd if auto-read-only switches the mode */
         ret = raw_reconfigure_getfd(bs, bs->open_flags, &open_flags, perm,
@@ -2796,6 +2798,7 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared,
             return ret;
         } else if (ret != s->fd) {
             s->perm_change_fd = ret;
+            s->perm_change_flags = open_flags;
         }
     }
 
@@ -2834,6 +2837,7 @@ static void raw_set_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared)
     if (s->perm_change_fd && s->fd != s->perm_change_fd) {
         qemu_close(s->fd);
         s->fd = s->perm_change_fd;
+        s->open_flags = s->perm_change_flags;
     }
     s->perm_change_fd = 0;
 
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PULL 07/14] block: Add bdrv_child_refresh_perms()
  2019-06-18 15:23 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
                   ` (5 preceding siblings ...)
  2019-06-18 15:23 ` [Qemu-devel] [PULL 06/14] file-posix: Update open_flags in raw_set_perm() Kevin Wolf
@ 2019-06-18 15:23 ` Kevin Wolf
  2019-06-18 15:23 ` [Qemu-devel] [PULL 08/14] block/mirror: Fix child permissions Kevin Wolf
                   ` (7 subsequent siblings)
  14 siblings, 0 replies; 24+ messages in thread
From: Kevin Wolf @ 2019-06-18 15:23 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel

From: Max Reitz <mreitz@redhat.com>

If a block node uses bdrv_child_try_set_perm() to change the permission
it takes on its child, the result may be very short-lived.  If anything
makes the block layer recalculate the permissions internally, it will
invoke the node driver's .bdrv_child_perm() implementation.  The
permission/shared permissions masks that returns will then override the
values previously passed to bdrv_child_try_set_perm().

If drivers want a child edge to have specific values for the
permissions/shared permissions mask, it must return them in
.bdrv_child_perm().  Consequentially, there is no need for them to pass
the same values to bdrv_child_try_set_perm() then: It is better to have
a function that invokes .bdrv_child_perm() and calls
bdrv_child_try_set_perm() with the result.  This patch adds such a
function under the name of bdrv_child_refresh_perms().

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block_int.h | 15 +++++++++++++++
 block.c                   | 12 ++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index a498c2670b..d6415b53c1 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1165,9 +1165,24 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
                                   void *opaque, Error **errp);
 void bdrv_root_unref_child(BdrvChild *child);
 
+/**
+ * Sets a BdrvChild's permissions.  Avoid if the parent is a BDS; use
+ * bdrv_child_refresh_perms() instead and make the parent's
+ * .bdrv_child_perm() implementation return the correct values.
+ */
 int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
                             Error **errp);
 
+/**
+ * Calls bs->drv->bdrv_child_perm() and updates the child's permission
+ * masks with the result.
+ * Drivers should invoke this function whenever an event occurs that
+ * makes their .bdrv_child_perm() implementation return different
+ * values than before, but which will not result in the block layer
+ * automatically refreshing the permissions.
+ */
+int bdrv_child_refresh_perms(BlockDriverState *bs, BdrvChild *c, Error **errp);
+
 /* Default implementation for BlockDriver.bdrv_child_perm() that can be used by
  * block filters: Forward CONSISTENT_READ, WRITE, WRITE_UNCHANGED and RESIZE to
  * all children */
diff --git a/block.c b/block.c
index ceb2ea23c5..013369851b 100644
--- a/block.c
+++ b/block.c
@@ -2083,6 +2083,18 @@ int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
     return 0;
 }
 
+int bdrv_child_refresh_perms(BlockDriverState *bs, BdrvChild *c, Error **errp)
+{
+    uint64_t parent_perms, parent_shared;
+    uint64_t perms, shared;
+
+    bdrv_get_cumulative_perm(bs, &parent_perms, &parent_shared);
+    bdrv_child_perm(bs, c->bs, c, c->role, NULL, parent_perms, parent_shared,
+                    &perms, &shared);
+
+    return bdrv_child_try_set_perm(c, perms, shared, errp);
+}
+
 void bdrv_filter_default_perms(BlockDriverState *bs, BdrvChild *c,
                                const BdrvChildRole *role,
                                BlockReopenQueue *reopen_queue,
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PULL 08/14] block/mirror: Fix child permissions
  2019-06-18 15:23 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
                   ` (6 preceding siblings ...)
  2019-06-18 15:23 ` [Qemu-devel] [PULL 07/14] block: Add bdrv_child_refresh_perms() Kevin Wolf
@ 2019-06-18 15:23 ` Kevin Wolf
  2019-06-18 15:23 ` [Qemu-devel] [PULL 09/14] block/commit: Drop bdrv_child_try_set_perm() Kevin Wolf
                   ` (6 subsequent siblings)
  14 siblings, 0 replies; 24+ messages in thread
From: Kevin Wolf @ 2019-06-18 15:23 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel

From: Max Reitz <mreitz@redhat.com>

We cannot use bdrv_child_try_set_perm() to give up all restrictions on
the child edge, and still have bdrv_mirror_top_child_perm() request
BLK_PERM_WRITE.  Fix this by making bdrv_mirror_top_child_perm() return
0/BLK_PERM_ALL when we want to give up all permissions, and replacing
bdrv_child_try_set_perm() by bdrv_child_refresh_perms().

The bdrv_child_try_set_perm() before removing the node with
bdrv_replace_node() is then unnecessary.  No permissions have changed
since the previous invocation of bdrv_child_try_set_perm().

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/mirror.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index b5878ba574..d17be4cdbc 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -85,6 +85,7 @@ typedef struct MirrorBlockJob {
 
 typedef struct MirrorBDSOpaque {
     MirrorBlockJob *job;
+    bool stop;
 } MirrorBDSOpaque;
 
 struct MirrorOp {
@@ -656,8 +657,9 @@ static int mirror_exit_common(Job *job)
 
     /* We don't access the source any more. Dropping any WRITE/RESIZE is
      * required before it could become a backing file of target_bs. */
-    bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
-                            &error_abort);
+    bs_opaque->stop = true;
+    bdrv_child_refresh_perms(mirror_top_bs, mirror_top_bs->backing,
+                             &error_abort);
     if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
         BlockDriverState *backing = s->is_none_mode ? src : s->base;
         if (backing_bs(target_bs) != backing) {
@@ -704,13 +706,12 @@ static int mirror_exit_common(Job *job)
     g_free(s->replaces);
     bdrv_unref(target_bs);
 
-    /* Remove the mirror filter driver from the graph. Before this, get rid of
+    /*
+     * Remove the mirror filter driver from the graph. Before this, get rid of
      * the blockers on the intermediate nodes so that the resulting state is
-     * valid. Also give up permissions on mirror_top_bs->backing, which might
-     * block the removal. */
+     * valid.
+     */
     block_job_remove_all_bdrv(bjob);
-    bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
-                            &error_abort);
     bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
 
     /* We just changed the BDS the job BB refers to (with either or both of the
@@ -1459,6 +1460,18 @@ static void bdrv_mirror_top_child_perm(BlockDriverState *bs, BdrvChild *c,
                                        uint64_t perm, uint64_t shared,
                                        uint64_t *nperm, uint64_t *nshared)
 {
+    MirrorBDSOpaque *s = bs->opaque;
+
+    if (s->stop) {
+        /*
+         * If the job is to be stopped, we do not need to forward
+         * anything to the real image.
+         */
+        *nperm = 0;
+        *nshared = BLK_PERM_ALL;
+        return;
+    }
+
     /* Must be able to forward guest writes to the real image */
     *nperm = 0;
     if (perm & BLK_PERM_WRITE) {
@@ -1681,8 +1694,9 @@ fail:
         job_early_fail(&s->common.job);
     }
 
-    bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
-                            &error_abort);
+    bs_opaque->stop = true;
+    bdrv_child_refresh_perms(mirror_top_bs, mirror_top_bs->backing,
+                             &error_abort);
     bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
 
     bdrv_unref(mirror_top_bs);
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PULL 09/14] block/commit: Drop bdrv_child_try_set_perm()
  2019-06-18 15:23 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
                   ` (7 preceding siblings ...)
  2019-06-18 15:23 ` [Qemu-devel] [PULL 08/14] block/mirror: Fix child permissions Kevin Wolf
@ 2019-06-18 15:23 ` Kevin Wolf
  2019-06-18 15:23 ` [Qemu-devel] [PULL 10/14] block: Fix order in bdrv_replace_child() Kevin Wolf
                   ` (5 subsequent siblings)
  14 siblings, 0 replies; 24+ messages in thread
From: Kevin Wolf @ 2019-06-18 15:23 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel

From: Max Reitz <mreitz@redhat.com>

commit_top_bs never requests or unshares any permissions.  There is no
reason to make this so explicit here.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/commit.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index c815def89a..212c6f639e 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -110,8 +110,6 @@ static void commit_abort(Job *job)
      * XXX Can (or should) we somehow keep 'consistent read' blocked even
      * after the failed/cancelled commit job is gone? If we already wrote
      * something to base, the intermediate images aren't valid any more. */
-    bdrv_child_try_set_perm(s->commit_top_bs->backing, 0, BLK_PERM_ALL,
-                            &error_abort);
     bdrv_replace_node(s->commit_top_bs, backing_bs(s->commit_top_bs),
                       &error_abort);
 
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PULL 10/14] block: Fix order in bdrv_replace_child()
  2019-06-18 15:23 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
                   ` (8 preceding siblings ...)
  2019-06-18 15:23 ` [Qemu-devel] [PULL 09/14] block/commit: Drop bdrv_child_try_set_perm() Kevin Wolf
@ 2019-06-18 15:23 ` Kevin Wolf
  2019-06-18 15:23 ` [Qemu-devel] [PULL 11/14] block: Add *tighten_restrictions to *check*_perm() Kevin Wolf
                   ` (4 subsequent siblings)
  14 siblings, 0 replies; 24+ messages in thread
From: Kevin Wolf @ 2019-06-18 15:23 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel

From: Max Reitz <mreitz@redhat.com>

We have to start by applying the permission restrictions to new_bs
before we can loosen them on old_bs.  See the comment for the
explanation.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 013369851b..b7d4149c2f 100644
--- a/block.c
+++ b/block.c
@@ -2240,6 +2240,19 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
 
     bdrv_replace_child_noperm(child, new_bs);
 
+    /*
+     * Start with the new node's permissions.  If @new_bs is a (direct
+     * or indirect) child of @old_bs, we must complete the permission
+     * update on @new_bs before we loosen the restrictions on @old_bs.
+     * Otherwise, bdrv_check_perm() on @old_bs would re-initiate
+     * updating the permissions of @new_bs, and thus not purely loosen
+     * restrictions.
+     */
+    if (new_bs) {
+        bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm);
+        bdrv_set_perm(new_bs, perm, shared_perm);
+    }
+
     if (old_bs) {
         /* Update permissions for old node. This is guaranteed to succeed
          * because we're just taking a parent away, so we're loosening
@@ -2252,11 +2265,6 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
          * node moves back to the main AioContext */
         bdrv_try_set_aio_context(old_bs, qemu_get_aio_context(), NULL);
     }
-
-    if (new_bs) {
-        bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm);
-        bdrv_set_perm(new_bs, perm, shared_perm);
-    }
 }
 
 /*
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PULL 11/14] block: Add *tighten_restrictions to *check*_perm()
  2019-06-18 15:23 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
                   ` (9 preceding siblings ...)
  2019-06-18 15:23 ` [Qemu-devel] [PULL 10/14] block: Fix order in bdrv_replace_child() Kevin Wolf
@ 2019-06-18 15:23 ` Kevin Wolf
  2019-06-18 15:23 ` [Qemu-devel] [PULL 12/14] block: Ignore loosening perm restrictions failures Kevin Wolf
                   ` (3 subsequent siblings)
  14 siblings, 0 replies; 24+ messages in thread
From: Kevin Wolf @ 2019-06-18 15:23 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel

From: Max Reitz <mreitz@redhat.com>

This patch makes three functions report whether the necessary permission
change tightens restrictions or not.  These functions are:
- bdrv_check_perm()
- bdrv_check_update_perm()
- bdrv_child_check_perm()

Callers can use this result to decide whether a failure is fatal or not
(see the next patch).

Signed-off-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 70 insertions(+), 17 deletions(-)

diff --git a/block.c b/block.c
index b7d4149c2f..1b10a5ce35 100644
--- a/block.c
+++ b/block.c
@@ -1706,7 +1706,8 @@ static int bdrv_fill_options(QDict **options, const char *filename,
 
 static int bdrv_child_check_perm(BdrvChild *c, BlockReopenQueue *q,
                                  uint64_t perm, uint64_t shared,
-                                 GSList *ignore_children, Error **errp);
+                                 GSList *ignore_children,
+                                 bool *tighten_restrictions, Error **errp);
 static void bdrv_child_abort_perm_update(BdrvChild *c);
 static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
 static void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,
@@ -1781,18 +1782,43 @@ static void bdrv_child_perm(BlockDriverState *bs, BlockDriverState *child_bs,
  * permissions of all its parents. This involves checking whether all necessary
  * permission changes to child nodes can be performed.
  *
+ * Will set *tighten_restrictions to true if and only if new permissions have to
+ * be taken or currently shared permissions are to be unshared.  Otherwise,
+ * errors are not fatal as long as the caller accepts that the restrictions
+ * remain tighter than they need to be.  The caller still has to abort the
+ * transaction.
+ * @tighten_restrictions cannot be used together with @q: When reopening, we may
+ * encounter fatal errors even though no restrictions are to be tightened.  For
+ * example, changing a node from RW to RO will fail if the WRITE permission is
+ * to be kept.
+ *
  * A call to this function must always be followed by a call to bdrv_set_perm()
  * or bdrv_abort_perm_update().
  */
 static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
                            uint64_t cumulative_perms,
                            uint64_t cumulative_shared_perms,
-                           GSList *ignore_children, Error **errp)
+                           GSList *ignore_children,
+                           bool *tighten_restrictions, Error **errp)
 {
     BlockDriver *drv = bs->drv;
     BdrvChild *c;
     int ret;
 
+    assert(!q || !tighten_restrictions);
+
+    if (tighten_restrictions) {
+        uint64_t current_perms, current_shared;
+        uint64_t added_perms, removed_shared_perms;
+
+        bdrv_get_cumulative_perm(bs, &current_perms, &current_shared);
+
+        added_perms = cumulative_perms & ~current_perms;
+        removed_shared_perms = current_shared & ~cumulative_shared_perms;
+
+        *tighten_restrictions = added_perms || removed_shared_perms;
+    }
+
     /* Write permissions never work with read-only images */
     if ((cumulative_perms & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED)) &&
         !bdrv_is_writable_after_reopen(bs, q))
@@ -1833,11 +1859,18 @@ static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
     /* Check all children */
     QLIST_FOREACH(c, &bs->children, next) {
         uint64_t cur_perm, cur_shared;
+        bool child_tighten_restr;
+
         bdrv_child_perm(bs, c->bs, c, c->role, q,
                         cumulative_perms, cumulative_shared_perms,
                         &cur_perm, &cur_shared);
-        ret = bdrv_child_check_perm(c, q, cur_perm, cur_shared,
-                                    ignore_children, errp);
+        ret = bdrv_child_check_perm(c, q, cur_perm, cur_shared, ignore_children,
+                                    tighten_restrictions ? &child_tighten_restr
+                                                         : NULL,
+                                    errp);
+        if (tighten_restrictions) {
+            *tighten_restrictions |= child_tighten_restr;
+        }
         if (ret < 0) {
             return ret;
         }
@@ -1961,17 +1994,23 @@ char *bdrv_perm_names(uint64_t perm)
  * set, the BdrvChild objects in this list are ignored in the calculations;
  * this allows checking permission updates for an existing reference.
  *
+ * See bdrv_check_perm() for the semantics of @tighten_restrictions.
+ *
  * Needs to be followed by a call to either bdrv_set_perm() or
  * bdrv_abort_perm_update(). */
 static int bdrv_check_update_perm(BlockDriverState *bs, BlockReopenQueue *q,
                                   uint64_t new_used_perm,
                                   uint64_t new_shared_perm,
-                                  GSList *ignore_children, Error **errp)
+                                  GSList *ignore_children,
+                                  bool *tighten_restrictions,
+                                  Error **errp)
 {
     BdrvChild *c;
     uint64_t cumulative_perms = new_used_perm;
     uint64_t cumulative_shared_perms = new_shared_perm;
 
+    assert(!q || !tighten_restrictions);
+
     /* There is no reason why anyone couldn't tolerate write_unchanged */
     assert(new_shared_perm & BLK_PERM_WRITE_UNCHANGED);
 
@@ -1983,6 +2022,11 @@ static int bdrv_check_update_perm(BlockDriverState *bs, BlockReopenQueue *q,
         if ((new_used_perm & c->shared_perm) != new_used_perm) {
             char *user = bdrv_child_user_desc(c);
             char *perm_names = bdrv_perm_names(new_used_perm & ~c->shared_perm);
+
+            if (tighten_restrictions) {
+                *tighten_restrictions = true;
+            }
+
             error_setg(errp, "Conflicts with use by %s as '%s', which does not "
                              "allow '%s' on %s",
                        user, c->name, perm_names, bdrv_get_node_name(c->bs));
@@ -1994,6 +2038,11 @@ static int bdrv_check_update_perm(BlockDriverState *bs, BlockReopenQueue *q,
         if ((c->perm & new_shared_perm) != c->perm) {
             char *user = bdrv_child_user_desc(c);
             char *perm_names = bdrv_perm_names(c->perm & ~new_shared_perm);
+
+            if (tighten_restrictions) {
+                *tighten_restrictions = true;
+            }
+
             error_setg(errp, "Conflicts with use by %s as '%s', which uses "
                              "'%s' on %s",
                        user, c->name, perm_names, bdrv_get_node_name(c->bs));
@@ -2007,19 +2056,21 @@ static int bdrv_check_update_perm(BlockDriverState *bs, BlockReopenQueue *q,
     }
 
     return bdrv_check_perm(bs, q, cumulative_perms, cumulative_shared_perms,
-                           ignore_children, errp);
+                           ignore_children, tighten_restrictions, errp);
 }
 
 /* Needs to be followed by a call to either bdrv_child_set_perm() or
  * bdrv_child_abort_perm_update(). */
 static int bdrv_child_check_perm(BdrvChild *c, BlockReopenQueue *q,
                                  uint64_t perm, uint64_t shared,
-                                 GSList *ignore_children, Error **errp)
+                                 GSList *ignore_children,
+                                 bool *tighten_restrictions, Error **errp)
 {
     int ret;
 
     ignore_children = g_slist_prepend(g_slist_copy(ignore_children), c);
-    ret = bdrv_check_update_perm(c->bs, q, perm, shared, ignore_children, errp);
+    ret = bdrv_check_update_perm(c->bs, q, perm, shared, ignore_children,
+                                 tighten_restrictions, errp);
     g_slist_free(ignore_children);
 
     if (ret < 0) {
@@ -2072,7 +2123,7 @@ int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
 {
     int ret;
 
-    ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL, errp);
+    ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL, NULL, errp);
     if (ret < 0) {
         bdrv_child_abort_perm_update(c);
         return ret;
@@ -2258,7 +2309,8 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
          * because we're just taking a parent away, so we're loosening
          * restrictions. */
         bdrv_get_cumulative_perm(old_bs, &perm, &shared_perm);
-        bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL, &error_abort);
+        bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL,
+                        NULL, &error_abort);
         bdrv_set_perm(old_bs, perm, shared_perm);
 
         /* When the parent requiring a non-default AioContext is removed, the
@@ -2288,7 +2340,8 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
     Error *local_err = NULL;
     int ret;
 
-    ret = bdrv_check_update_perm(child_bs, NULL, perm, shared_perm, NULL, errp);
+    ret = bdrv_check_update_perm(child_bs, NULL, perm, shared_perm, NULL, NULL,
+                                 errp);
     if (ret < 0) {
         bdrv_abort_perm_update(child_bs);
         bdrv_unref(child_bs);
@@ -3369,7 +3422,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
     QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
         BDRVReopenState *state = &bs_entry->state;
         ret = bdrv_check_perm(state->bs, bs_queue, state->perm,
-                              state->shared_perm, NULL, errp);
+                              state->shared_perm, NULL, NULL, errp);
         if (ret < 0) {
             goto cleanup_perm;
         }
@@ -3381,7 +3434,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
                             state->perm, state->shared_perm,
                             &nperm, &nshared);
             ret = bdrv_check_update_perm(state->new_backing_bs, NULL,
-                                         nperm, nshared, NULL, errp);
+                                         nperm, nshared, NULL, NULL, errp);
             if (ret < 0) {
                 goto cleanup_perm;
             }
@@ -4097,7 +4150,7 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
 
     /* Check whether the required permissions can be granted on @to, ignoring
      * all BdrvChild in @list so that they can't block themselves. */
-    ret = bdrv_check_update_perm(to, NULL, perm, shared, list, errp);
+    ret = bdrv_check_update_perm(to, NULL, perm, shared, list, NULL, errp);
     if (ret < 0) {
         bdrv_abort_perm_update(to);
         goto out;
@@ -4444,7 +4497,7 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
         /* Check whether we are allowed to switch c from top to base */
         GSList *ignore_children = g_slist_prepend(NULL, c);
         ret = bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm,
-                                     ignore_children, &local_err);
+                                     ignore_children, NULL, &local_err);
         g_slist_free(ignore_children);
         if (ret < 0) {
             error_report_err(local_err);
@@ -5219,7 +5272,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
      */
     bs->open_flags &= ~BDRV_O_INACTIVE;
     bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
-    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, &local_err);
+    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, &local_err);
     if (ret < 0) {
         bs->open_flags |= BDRV_O_INACTIVE;
         error_propagate(errp, local_err);
@@ -5369,7 +5422,7 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs)
 
     /* Update permissions, they may differ for inactive nodes */
     bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
-    bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, &error_abort);
+    bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, &error_abort);
     bdrv_set_perm(bs, perm, shared_perm);
 
 
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PULL 12/14] block: Ignore loosening perm restrictions failures
  2019-06-18 15:23 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
                   ` (10 preceding siblings ...)
  2019-06-18 15:23 ` [Qemu-devel] [PULL 11/14] block: Add *tighten_restrictions to *check*_perm() Kevin Wolf
@ 2019-06-18 15:23 ` Kevin Wolf
  2019-06-18 15:23 ` [Qemu-devel] [PULL 13/14] iotests: Test failure to loosen restrictions Kevin Wolf
                   ` (2 subsequent siblings)
  14 siblings, 0 replies; 24+ messages in thread
From: Kevin Wolf @ 2019-06-18 15:23 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel

From: Max Reitz <mreitz@redhat.com>

We generally assume that loosening permission restrictions can never
fail.  We have seen in the past that this assumption is wrong.  This has
led to crashes because we generally pass &error_abort when loosening
permissions.

However, a failure in such a case should actually be handled in quite
the opposite way: It is very much not fatal, so qemu may report it, but
still consider the operation successful.  The only realistic problem is
that qemu may then retain permissions and thus locks on images it
actually does not require.  But again, that is not fatal.

To implement this behavior, we make all functions that change
permissions and that pass &error_abort to the initiating function
(bdrv_check_perm() or bdrv_child_check_perm()) evaluate the
@loosen_restrictions value introduced in the previous patch.  If it is
true and an error did occur, we abort the permission update, discard the
error, and instead report success to the caller.

bdrv_child_try_set_perm() itself does not pass &error_abort, but it is
the only public function to change permissions.  As such, callers may
pass &error_abort to it, expecting dropping permission restrictions to
never fail.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 44 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 1b10a5ce35..c139540f2b 100644
--- a/block.c
+++ b/block.c
@@ -2121,11 +2121,26 @@ static void bdrv_child_abort_perm_update(BdrvChild *c)
 int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
                             Error **errp)
 {
+    Error *local_err = NULL;
     int ret;
+    bool tighten_restrictions;
 
-    ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL, NULL, errp);
+    ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL,
+                                &tighten_restrictions, &local_err);
     if (ret < 0) {
         bdrv_child_abort_perm_update(c);
+        if (tighten_restrictions) {
+            error_propagate(errp, local_err);
+        } else {
+            /*
+             * Our caller may intend to only loosen restrictions and
+             * does not expect this function to fail.  Errors are not
+             * fatal in such a case, so we can just hide them from our
+             * caller.
+             */
+            error_free(local_err);
+            ret = 0;
+        }
         return ret;
     }
 
@@ -2308,10 +2323,19 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
         /* Update permissions for old node. This is guaranteed to succeed
          * because we're just taking a parent away, so we're loosening
          * restrictions. */
+        bool tighten_restrictions;
+        int ret;
+
         bdrv_get_cumulative_perm(old_bs, &perm, &shared_perm);
-        bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL,
-                        NULL, &error_abort);
-        bdrv_set_perm(old_bs, perm, shared_perm);
+        ret = bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL,
+                              &tighten_restrictions, NULL);
+        assert(tighten_restrictions == false);
+        if (ret < 0) {
+            /* We only tried to loosen restrictions, so errors are not fatal */
+            bdrv_abort_perm_update(old_bs);
+        } else {
+            bdrv_set_perm(old_bs, perm, shared_perm);
+        }
 
         /* When the parent requiring a non-default AioContext is removed, the
          * node moves back to the main AioContext */
@@ -5386,6 +5410,7 @@ static bool bdrv_has_bds_parent(BlockDriverState *bs, bool only_active)
 static int bdrv_inactivate_recurse(BlockDriverState *bs)
 {
     BdrvChild *child, *parent;
+    bool tighten_restrictions;
     uint64_t perm, shared_perm;
     int ret;
 
@@ -5422,8 +5447,15 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs)
 
     /* Update permissions, they may differ for inactive nodes */
     bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
-    bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, &error_abort);
-    bdrv_set_perm(bs, perm, shared_perm);
+    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL,
+                          &tighten_restrictions, NULL);
+    assert(tighten_restrictions == false);
+    if (ret < 0) {
+        /* We only tried to loosen restrictions, so errors are not fatal */
+        bdrv_abort_perm_update(bs);
+    } else {
+        bdrv_set_perm(bs, perm, shared_perm);
+    }
 
 
     /* Recursively inactivate children */
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PULL 13/14] iotests: Test failure to loosen restrictions
  2019-06-18 15:23 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
                   ` (11 preceding siblings ...)
  2019-06-18 15:23 ` [Qemu-devel] [PULL 12/14] block: Ignore loosening perm restrictions failures Kevin Wolf
@ 2019-06-18 15:23 ` Kevin Wolf
  2019-06-18 15:23 ` [Qemu-devel] [PULL 14/14] block/null: Expose read-zeroes option in QAPI schema Kevin Wolf
  2019-06-18 16:32 ` [Qemu-devel] [PULL 00/14] Block layer patches Peter Maydell
  14 siblings, 0 replies; 24+ messages in thread
From: Kevin Wolf @ 2019-06-18 15:23 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel

From: Max Reitz <mreitz@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/182     | 21 +++++++++++++++++++++
 tests/qemu-iotests/182.out |  6 ++++++
 2 files changed, 27 insertions(+)

diff --git a/tests/qemu-iotests/182 b/tests/qemu-iotests/182
index 3a90ebfbfd..7f494eb9bb 100755
--- a/tests/qemu-iotests/182
+++ b/tests/qemu-iotests/182
@@ -152,6 +152,27 @@ success_or_failure=y _send_qemu_cmd $QEMU_HANDLE \
 
 _cleanup_qemu
 
+echo
+echo '=== Testing failure to loosen restrictions ==='
+echo
+
+_launch_qemu -drive file=$TEST_IMG,if=none,file.locking=on
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{'execute': 'qmp_capabilities'}" \
+    'return'
+
+_cleanup_test_img
+
+# When quitting qemu, it will try to drop its locks on the test image.
+# Because that file no longer exists, it will be unable to do so.
+# However, that is not fatal, so it should just move on.
+_send_qemu_cmd $QEMU_HANDLE \
+    "{'execute': 'quit'}" \
+    'return'
+
+wait=1 _cleanup_qemu
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/182.out b/tests/qemu-iotests/182.out
index 33d41eea91..ffef23e32b 100644
--- a/tests/qemu-iotests/182.out
+++ b/tests/qemu-iotests/182.out
@@ -15,4 +15,10 @@ Formatting 'TEST_DIR/t.qcow2.overlay', fmt=qcow2 size=197120 backing_file=TEST_D
 {"return": {}}
 {"return": {}}
 {"return": {}}
+
+=== Testing failure to loosen restrictions ===
+
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
 *** done
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PULL 14/14] block/null: Expose read-zeroes option in QAPI schema
  2019-06-18 15:23 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
                   ` (12 preceding siblings ...)
  2019-06-18 15:23 ` [Qemu-devel] [PULL 13/14] iotests: Test failure to loosen restrictions Kevin Wolf
@ 2019-06-18 15:23 ` Kevin Wolf
  2019-06-18 16:32 ` [Qemu-devel] [PULL 00/14] Block layer patches Peter Maydell
  14 siblings, 0 replies; 24+ messages in thread
From: Kevin Wolf @ 2019-06-18 15:23 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel

Commit cd219eb1e55 added the read-zeroes option for the null-co and
null-aio block driver, but forgot to add them to the QAPI schema.
Therefore, this option wasn't available in -blockdev and blockdev-add
until now.

Add the missing option in the schema to make it available there, too.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 qapi/block-core.json | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 61124431d8..0d43d4f37c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2890,11 +2890,13 @@
 # @latency-ns: emulated latency (in nanoseconds) in processing
 #              requests. Default to zero which completes requests immediately.
 #              (Since 2.4)
+# @read-zeroes: if true, reads from the device produce zeroes; if false, the
+#               buffer is left unchanged. (default: false; since: 4.1)
 #
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsNull',
-  'data': { '*size': 'int', '*latency-ns': 'uint64' } }
+  'data': { '*size': 'int', '*latency-ns': 'uint64', '*read-zeroes': 'bool' } }
 
 ##
 # @BlockdevOptionsNVMe:
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [Qemu-devel] [PULL 00/14] Block layer patches
  2019-06-18 15:23 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
                   ` (13 preceding siblings ...)
  2019-06-18 15:23 ` [Qemu-devel] [PULL 14/14] block/null: Expose read-zeroes option in QAPI schema Kevin Wolf
@ 2019-06-18 16:32 ` Peter Maydell
  14 siblings, 0 replies; 24+ messages in thread
From: Peter Maydell @ 2019-06-18 16:32 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: QEMU Developers, Qemu-block

On Tue, 18 Jun 2019 at 16:59, Kevin Wolf <kwolf@redhat.com> wrote:
>
> The following changes since commit cdfaa2720f4a09e5254868bd1f6e33f3e9eae76f:
>
>   Merge remote-tracking branch 'remotes/armbru/tags/pull-monitor-2019-06-17-v2' into staging (2019-06-18 10:47:00 +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 128b05f7e00765d883164631b974a27af5b4b613:
>
>   block/null: Expose read-zeroes option in QAPI schema (2019-06-18 16:41:10 +0200)
>
> ----------------------------------------------------------------
> Block layer patches:
>
> - block: Remove bs->job
> - block: Ignore loosening perm restrictions failures
> - block/null: Expose read-zeroes option in QAPI schema
> - iotests: Hide timestamps for skipped tests
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.1
for any user-visible changes.

-- PMM


^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2019-06-18 17:16 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-06-18 15:23 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
2019-06-18 15:23 ` [Qemu-devel] [PULL 01/14] iotests: Hide timestamps for skipped tests Kevin Wolf
2019-06-18 15:23 ` [Qemu-devel] [PULL 02/14] block/replication: drop usage of bs->job Kevin Wolf
2019-06-18 15:23 ` [Qemu-devel] [PULL 03/14] block/block-backend: blk_iostatus_reset: " Kevin Wolf
2019-06-18 15:23 ` [Qemu-devel] [PULL 04/14] blockdev: blockdev_mark_auto_del: " Kevin Wolf
2019-06-18 15:23 ` [Qemu-devel] [PULL 05/14] block: drop bs->job Kevin Wolf
2019-06-18 15:23 ` [Qemu-devel] [PULL 06/14] file-posix: Update open_flags in raw_set_perm() Kevin Wolf
2019-06-18 15:23 ` [Qemu-devel] [PULL 07/14] block: Add bdrv_child_refresh_perms() Kevin Wolf
2019-06-18 15:23 ` [Qemu-devel] [PULL 08/14] block/mirror: Fix child permissions Kevin Wolf
2019-06-18 15:23 ` [Qemu-devel] [PULL 09/14] block/commit: Drop bdrv_child_try_set_perm() Kevin Wolf
2019-06-18 15:23 ` [Qemu-devel] [PULL 10/14] block: Fix order in bdrv_replace_child() Kevin Wolf
2019-06-18 15:23 ` [Qemu-devel] [PULL 11/14] block: Add *tighten_restrictions to *check*_perm() Kevin Wolf
2019-06-18 15:23 ` [Qemu-devel] [PULL 12/14] block: Ignore loosening perm restrictions failures Kevin Wolf
2019-06-18 15:23 ` [Qemu-devel] [PULL 13/14] iotests: Test failure to loosen restrictions Kevin Wolf
2019-06-18 15:23 ` [Qemu-devel] [PULL 14/14] block/null: Expose read-zeroes option in QAPI schema Kevin Wolf
2019-06-18 16:32 ` [Qemu-devel] [PULL 00/14] Block layer patches Peter Maydell
  -- strict thread matches above, loose matches on Subject: below --
2018-11-12 17:05 Kevin Wolf
2018-11-13 10:14 ` Peter Maydell
2017-12-20 13:19 Kevin Wolf
2017-12-20 15:30 ` Peter Maydell
2017-09-06 14:02 Kevin Wolf
2017-09-07 10:50 ` Peter Maydell
2017-01-09 13:44 Kevin Wolf
2017-01-09 15:30 ` Peter Maydell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).