[Qemu-devel] [PATCH v2 0/4] Fix "stop" draining block jobs

[Qemu-devel] [PATCH v2 0/4] Fix "stop" draining block jobs

[Fam Zheng]

v2: Use "pause" approach as suggested by Paolo.

Stopping the vm will drive the block job all the way to the end, because the
sleep duration is too short, which means the block_job_sleep_ns in the block
jobs are unhelpful. That is because the timer will fire too soon, even before
the aio_poll in bdrv_drain_all returns.

Fix this by pausing all the block jobs during bdrv_drain_all.

Please review!

Fam


Fam Zheng (4):
  blockjob: Allow nested pause
  block: Pause block jobs in bdrv_drain_all
  qemu-iotests: Test that "stop" doesn't drain block jobs
  blockjob: Update function name in comments

 block.c                    | 20 +++++++++++
 block/backup.c             |  2 +-
 block/mirror.c             |  4 +--
 blockdev.c                 |  8 +++--
 blockjob.c                 | 23 +++++++++----
 include/block/blockjob.h   | 20 ++++++++---
 tests/qemu-iotests/129     | 86 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/129.out |  5 +++
 tests/qemu-iotests/group   |  1 +
 9 files changed, 153 insertions(+), 16 deletions(-)
 create mode 100644 tests/qemu-iotests/129
 create mode 100644 tests/qemu-iotests/129.out

-- 
2.1.0

[Qemu-devel] [PATCH v2 1/4] blockjob: Allow nested pause

[Fam Zheng]

This patch changes block_job_pause to increase the pause counter and
block_job_resume to decrease it.

The counter will allow calling block_job_pause/block_job_resume
unconditionally on a job when we need to suspend the IO temporarily.

>From now on, each block_job_resume must be paired with a block_job_pause
to keep the counter balanced.

The user pause from QMP or HMP will only trigger block_job_pause once
until it's resumed, this is achieved by adding a user_paused flag in
BlockJob.

One occurrence of block_job_resume in mirror_complete is replaced with
block_job_enter which does what is necessary.

In block_job_cancel, the cancel flag is good enough to instruct
coroutines to quit loop, so use block_job_enter to replace the unpaired
block_job_resume.

Upon block job IO error, user is notified about the entering to the
pause state, so this pause belongs to user pause, set the flag
accordingly and expect a matching QMP resume.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/mirror.c           |  2 +-
 blockdev.c               |  8 +++++---
 blockjob.c               | 23 +++++++++++++++++------
 include/block/blockjob.h | 20 ++++++++++++++++----
 4 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 4056164..65b1718 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -634,7 +634,7 @@ static void mirror_complete(BlockJob *job, Error **errp)
     }
 
     s->should_complete = true;
-    block_job_resume(job);
+    block_job_enter(&s->common);
 }
 
 static const BlockJobDriver mirror_job_driver = {
diff --git a/blockdev.c b/blockdev.c
index fbb3a79..9132d69 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2699,7 +2699,7 @@ void qmp_block_job_cancel(const char *device,
         force = false;
     }
 
-    if (job->paused && !force) {
+    if (job->user_paused && !force) {
         error_setg(errp, "The block job for device '%s' is currently paused",
                    device);
         goto out;
@@ -2716,10 +2716,11 @@ void qmp_block_job_pause(const char *device, Error **errp)
     AioContext *aio_context;
     BlockJob *job = find_block_job(device, &aio_context, errp);
 
-    if (!job) {
+    if (!job || job->user_paused) {
         return;
     }
 
+    job->user_paused = true;
     trace_qmp_block_job_pause(job);
     block_job_pause(job);
     aio_context_release(aio_context);
@@ -2730,10 +2731,11 @@ void qmp_block_job_resume(const char *device, Error **errp)
     AioContext *aio_context;
     BlockJob *job = find_block_job(device, &aio_context, errp);
 
-    if (!job) {
+    if (!job || !job->user_paused) {
         return;
     }
 
+    job->user_paused = false;
     trace_qmp_block_job_resume(job);
     block_job_resume(job);
     aio_context_release(aio_context);
diff --git a/blockjob.c b/blockjob.c
index ba2255d..2755465 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -107,7 +107,7 @@ void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
 
 void block_job_complete(BlockJob *job, Error **errp)
 {
-    if (job->paused || job->cancelled || !job->driver->complete) {
+    if (job->pause_count || job->cancelled || !job->driver->complete) {
         error_set(errp, QERR_BLOCK_JOB_NOT_READY,
                   bdrv_get_device_name(job->bs));
         return;
@@ -118,17 +118,26 @@ void block_job_complete(BlockJob *job, Error **errp)
 
 void block_job_pause(BlockJob *job)
 {
-    job->paused = true;
+    job->pause_count++;
 }
 
 bool block_job_is_paused(BlockJob *job)
 {
-    return job->paused;
+    return job->pause_count > 0;
 }
 
 void block_job_resume(BlockJob *job)
 {
-    job->paused = false;
+    assert(job->pause_count > 0);
+    job->pause_count--;
+    if (job->pause_count) {
+        return;
+    }
+    block_job_enter(job);
+}
+
+void block_job_enter(BlockJob *job)
+{
     block_job_iostatus_reset(job);
     if (job->co && !job->busy) {
         qemu_coroutine_enter(job->co, NULL);
@@ -138,7 +147,7 @@ void block_job_resume(BlockJob *job)
 void block_job_cancel(BlockJob *job)
 {
     job->cancelled = true;
-    block_job_resume(job);
+    block_job_enter(job);
 }
 
 bool block_job_is_cancelled(BlockJob *job)
@@ -258,7 +267,7 @@ BlockJobInfo *block_job_query(BlockJob *job)
     info->device    = g_strdup(bdrv_get_device_name(job->bs));
     info->len       = job->len;
     info->busy      = job->busy;
-    info->paused    = job->paused;
+    info->paused    = job->pause_count > 0;
     info->offset    = job->offset;
     info->speed     = job->speed;
     info->io_status = job->iostatus;
@@ -335,6 +344,8 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs,
                                     IO_OPERATION_TYPE_WRITE,
                                     action, &error_abort);
     if (action == BLOCK_ERROR_ACTION_STOP) {
+        /* make the pause user visible, which will be resumed from QMP. */
+        job->user_paused = true;
         block_job_pause(job);
         block_job_iostatus_set_err(job, error);
         if (bs != job->bs) {
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index b6d4ebb..9f4b20d 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -79,10 +79,14 @@ struct BlockJob {
     bool cancelled;
 
     /**
-     * Set to true if the job is either paused, or will pause itself
-     * as soon as possible (if busy == true).
+     * Counter for pause request. If non-zero, the block job will pause.
      */
-    bool paused;
+    int pause_count;
+
+    /**
+     * Set to true if the job is paused by user.
+     */
+    bool user_paused;
 
     /**
      * Set to false by the job while it is in a quiescent state, where
@@ -225,11 +229,19 @@ void block_job_pause(BlockJob *job);
  * block_job_resume:
  * @job: The job to be resumed.
  *
- * Resume the specified job.
+ * Resume the specified job.  Must be paired with a preceding block_job_pause.
  */
 void block_job_resume(BlockJob *job);
 
 /**
+ * block_job_enter:
+ * @job: The job to enter.
+ *
+ * Continue the specified job by entering the coroutine.
+ */
+void block_job_enter(BlockJob *job);
+
+/**
  * block_job_event_cancelled:
  * @job: The job whose information is requested.
  *
-- 
2.1.0

[Qemu-devel] [PATCH v2 2/4] block: Pause block jobs in bdrv_drain_all

[Fam Zheng]

This is necessary to suppress more IO requests from being generated from
block job coroutines.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/block.c b/block.c
index f2f8ae7..00cd91e 100644
--- a/block.c
+++ b/block.c
@@ -2033,6 +2033,16 @@ void bdrv_drain_all(void)
     bool busy = true;
     BlockDriverState *bs;
 
+    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
+        AioContext *aio_context = bdrv_get_aio_context(bs);
+
+        aio_context_acquire(aio_context);
+        if (bs->job) {
+            block_job_pause(bs->job);
+        }
+        aio_context_release(aio_context);
+    }
+
     while (busy) {
         busy = false;
 
@@ -2044,6 +2054,16 @@ void bdrv_drain_all(void)
             aio_context_release(aio_context);
         }
     }
+
+    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
+        AioContext *aio_context = bdrv_get_aio_context(bs);
+
+        aio_context_acquire(aio_context);
+        if (bs->job) {
+            block_job_resume(bs->job);
+        }
+        aio_context_release(aio_context);
+    }
 }
 
 /* make a BlockDriverState anonymous by removing from bdrv_state and
-- 
2.1.0

[Qemu-devel] [PATCH v2 3/4] qemu-iotests: Test that "stop" doesn't drain block jobs

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/129     | 86 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/129.out |  5 +++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 92 insertions(+)
 create mode 100644 tests/qemu-iotests/129
 create mode 100644 tests/qemu-iotests/129.out

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
new file mode 100644
index 0000000..9e87e1c
--- /dev/null
+++ b/tests/qemu-iotests/129
@@ -0,0 +1,86 @@
+#!/usr/bin/env python
+#
+# Tests that "bdrv_drain_all" doesn't drain block jobs
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import iotests
+import time
+
+class TestStopWithBlockJob(iotests.QMPTestCase):
+    test_img = os.path.join(iotests.test_dir, 'test.img')
+    target_img = os.path.join(iotests.test_dir, 'target.img')
+    base_img = os.path.join(iotests.test_dir, 'base.img')
+
+    def setUp(self):
+        iotests.qemu_img('create', '-f', iotests.imgfmt, self.base_img, "1G")
+        iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img, "-b", self.base_img)
+        iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', self.test_img)
+        self.vm = iotests.VM().add_drive(self.test_img)
+        self.vm.launch()
+
+    def tearDown(self):
+        params = {"device": "drive0",
+                  "bps": 0,
+                  "bps_rd": 0,
+                  "bps_wr": 0,
+                  "iops": 0,
+                  "iops_rd": 0,
+                  "iops_wr": 0,
+                 }
+        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
+                             **params)
+        self.vm.shutdown()
+
+    def do_test_stop(self, cmd, **args):
+        """Test 'stop' while block job is running on a throttled drive.
+        The 'stop' command shouldn't drain the job"""
+        params = {"device": "drive0",
+                  "bps": 1024,
+                  "bps_rd": 0,
+                  "bps_wr": 0,
+                  "iops": 0,
+                  "iops_rd": 0,
+                  "iops_wr": 0,
+                 }
+        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
+                             **params)
+        self.assert_qmp(result, 'return', {})
+        result = self.vm.qmp(cmd, **args)
+        self.assert_qmp(result, 'return', {})
+        result = self.vm.qmp("stop")
+        self.assert_qmp(result, 'return', {})
+        result = self.vm.qmp("query-block-jobs")
+        self.assert_qmp(result, 'return[0]/busy', True)
+        self.assert_qmp(result, 'return[0]/ready', False)
+
+    def test_drive_mirror(self):
+        self.do_test_stop("drive-mirror", device="drive0",
+                          target=self.target_img,
+                          sync="full")
+
+    def test_drive_backup(self):
+        self.do_test_stop("drive-backup", device="drive0",
+                          target=self.target_img,
+                          sync="full")
+
+    def test_block_commit(self):
+        self.do_test_stop("block-commit", device="drive0")
+
+if __name__ == '__main__':
+    iotests.main(supported_fmts=["qcow2"])
diff --git a/tests/qemu-iotests/129.out b/tests/qemu-iotests/129.out
new file mode 100644
index 0000000..8d7e996
--- /dev/null
+++ b/tests/qemu-iotests/129.out
@@ -0,0 +1,5 @@
+...
+----------------------------------------------------------------------
+Ran 3 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 6262127..69071ac 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -124,3 +124,4 @@
 121 rw auto
 123 rw auto quick
 128 rw auto quick
+129 rw auto quick
-- 
2.1.0

[Qemu-devel] [PATCH v2 4/4] blockjob: Update function name in comments

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/backup.c | 2 +-
 block/mirror.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 1c535b1..3312476 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -287,7 +287,7 @@ static void coroutine_fn backup_run(void *opaque)
                 break;
             }
 
-            /* we need to yield so that qemu_aio_flush() returns.
+            /* we need to yield so that bdrv_drain_all() returns.
              * (without, VM does not reboot)
              */
             if (job->common.speed) {
diff --git a/block/mirror.c b/block/mirror.c
index 65b1718..d421fce 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -475,7 +475,7 @@ static void coroutine_fn mirror_run(void *opaque)
                         (cnt + s->sectors_in_flight) * BDRV_SECTOR_SIZE;
 
         /* Note that even when no rate limit is applied we need to yield
-         * periodically with no pending I/O so that qemu_aio_flush() returns.
+         * periodically with no pending I/O so that bdrv_drain_all() returns.
          * We do so every SLICE_TIME nanoseconds, or when there is an error,
          * or when the source is clean, whichever comes first.
          */
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 1/4] blockjob: Allow nested pause

[Paolo Bonzini]

On 03/04/2015 16:05, Fam Zheng wrote:
> This patch changes block_job_pause to increase the pause counter and
> block_job_resume to decrease it.
> 
> The counter will allow calling block_job_pause/block_job_resume
> unconditionally on a job when we need to suspend the IO temporarily.
> 
> From now on, each block_job_resume must be paired with a block_job_pause
> to keep the counter balanced.
> 
> The user pause from QMP or HMP will only trigger block_job_pause once
> until it's resumed, this is achieved by adding a user_paused flag in
> BlockJob.
> 
> One occurrence of block_job_resume in mirror_complete is replaced with
> block_job_enter which does what is necessary.
> 
> In block_job_cancel, the cancel flag is good enough to instruct
> coroutines to quit loop, so use block_job_enter to replace the unpaired
> block_job_resume.
> 
> Upon block job IO error, user is notified about the entering to the
> pause state, so this pause belongs to user pause, set the flag
> accordingly and expect a matching QMP resume.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/mirror.c           |  2 +-
>  blockdev.c               |  8 +++++---
>  blockjob.c               | 23 +++++++++++++++++------
>  include/block/blockjob.h | 20 ++++++++++++++++----
>  4 files changed, 39 insertions(+), 14 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 4056164..65b1718 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -634,7 +634,7 @@ static void mirror_complete(BlockJob *job, Error **errp)
>      }
>  
>      s->should_complete = true;
> -    block_job_resume(job);
> +    block_job_enter(&s->common);
>  }
>  
>  static const BlockJobDriver mirror_job_driver = {
> diff --git a/blockdev.c b/blockdev.c
> index fbb3a79..9132d69 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2699,7 +2699,7 @@ void qmp_block_job_cancel(const char *device,
>          force = false;
>      }
>  
> -    if (job->paused && !force) {
> +    if (job->user_paused && !force) {
>          error_setg(errp, "The block job for device '%s' is currently paused",
>                     device);
>          goto out;
> @@ -2716,10 +2716,11 @@ void qmp_block_job_pause(const char *device, Error **errp)
>      AioContext *aio_context;
>      BlockJob *job = find_block_job(device, &aio_context, errp);
>  
> -    if (!job) {
> +    if (!job || job->user_paused) {
>          return;
>      }
>  
> +    job->user_paused = true;
>      trace_qmp_block_job_pause(job);
>      block_job_pause(job);
>      aio_context_release(aio_context);
> @@ -2730,10 +2731,11 @@ void qmp_block_job_resume(const char *device, Error **errp)
>      AioContext *aio_context;
>      BlockJob *job = find_block_job(device, &aio_context, errp);
>  
> -    if (!job) {
> +    if (!job || !job->user_paused) {
>          return;
>      }
>  
> +    job->user_paused = false;
>      trace_qmp_block_job_resume(job);
>      block_job_resume(job);
>      aio_context_release(aio_context);
> diff --git a/blockjob.c b/blockjob.c
> index ba2255d..2755465 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -107,7 +107,7 @@ void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
>  
>  void block_job_complete(BlockJob *job, Error **errp)
>  {
> -    if (job->paused || job->cancelled || !job->driver->complete) {
> +    if (job->pause_count || job->cancelled || !job->driver->complete) {
>          error_set(errp, QERR_BLOCK_JOB_NOT_READY,
>                    bdrv_get_device_name(job->bs));
>          return;
> @@ -118,17 +118,26 @@ void block_job_complete(BlockJob *job, Error **errp)
>  
>  void block_job_pause(BlockJob *job)
>  {
> -    job->paused = true;
> +    job->pause_count++;
>  }
>  
>  bool block_job_is_paused(BlockJob *job)
>  {
> -    return job->paused;
> +    return job->pause_count > 0;
>  }
>  
>  void block_job_resume(BlockJob *job)
>  {
> -    job->paused = false;
> +    assert(job->pause_count > 0);
> +    job->pause_count--;
> +    if (job->pause_count) {
> +        return;
> +    }
> +    block_job_enter(job);
> +}
> +
> +void block_job_enter(BlockJob *job)
> +{
>      block_job_iostatus_reset(job);
>      if (job->co && !job->busy) {
>          qemu_coroutine_enter(job->co, NULL);
> @@ -138,7 +147,7 @@ void block_job_resume(BlockJob *job)
>  void block_job_cancel(BlockJob *job)
>  {
>      job->cancelled = true;
> -    block_job_resume(job);
> +    block_job_enter(job);
>  }
>  
>  bool block_job_is_cancelled(BlockJob *job)
> @@ -258,7 +267,7 @@ BlockJobInfo *block_job_query(BlockJob *job)
>      info->device    = g_strdup(bdrv_get_device_name(job->bs));
>      info->len       = job->len;
>      info->busy      = job->busy;
> -    info->paused    = job->paused;
> +    info->paused    = job->pause_count > 0;
>      info->offset    = job->offset;
>      info->speed     = job->speed;
>      info->io_status = job->iostatus;
> @@ -335,6 +344,8 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs,
>                                      IO_OPERATION_TYPE_WRITE,
>                                      action, &error_abort);
>      if (action == BLOCK_ERROR_ACTION_STOP) {
> +        /* make the pause user visible, which will be resumed from QMP. */
> +        job->user_paused = true;
>          block_job_pause(job);
>          block_job_iostatus_set_err(job, error);
>          if (bs != job->bs) {
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index b6d4ebb..9f4b20d 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -79,10 +79,14 @@ struct BlockJob {
>      bool cancelled;
>  
>      /**
> -     * Set to true if the job is either paused, or will pause itself
> -     * as soon as possible (if busy == true).
> +     * Counter for pause request. If non-zero, the block job will pause.

Why not keep the more complete comment ("If non-zero, the block job is
either paused, or if busy == true will pause itself as soon as possible")?

>       */
> -    bool paused;
> +    int pause_count;
> +
> +    /**
> +     * Set to true if the job is paused by user.

... and can be unpaused with the block-job-resume QMP command.

> +     */
> +    bool user_paused;
>  
>      /**
>       * Set to false by the job while it is in a quiescent state, where
> @@ -225,11 +229,19 @@ void block_job_pause(BlockJob *job);
>   * block_job_resume:
>   * @job: The job to be resumed.
>   *
> - * Resume the specified job.
> + * Resume the specified job.  Must be paired with a preceding block_job_pause.
>   */
>  void block_job_resume(BlockJob *job);
>  
>  /**
> + * block_job_enter:
> + * @job: The job to enter.
> + *
> + * Continue the specified job by entering the coroutine.
> + */
> +void block_job_enter(BlockJob *job);
> +
> +/**
>   * block_job_event_cancelled:
>   * @job: The job whose information is requested.
>   *
> 

Apart from this,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v2 2/4] block: Pause block jobs in bdrv_drain_all

[Alberto Garcia]

On Fri, Apr 03, 2015 at 10:05:19PM +0800, Fam Zheng wrote:

> +    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> +        AioContext *aio_context = bdrv_get_aio_context(bs);
> +
> +        aio_context_acquire(aio_context);
> +        if (bs->job) {
> +            block_job_pause(bs->job);
> +        }
> +        aio_context_release(aio_context);
> +    }

Not directly a problem in your code, but since I'm playing with the
idea of allowing block jobs to reside in any arbitrary node we'll have
to figure out a simple and global way to iterate over all block jobs.
The one that I wrote is probably not the best one. Maybe we can have
block_job_create/completed() maintain a list of jobs instead.

   https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg04800.html

But anyway I don't think it's something for this patchset, therefore:

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH v2 3/4] qemu-iotests: Test that "stop" doesn't drain block jobs

[Alberto Garcia]

On Fri, Apr 03, 2015 at 10:05:20PM +0800, Fam Zheng wrote:

> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/129     | 86 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/129.out |  5 +++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 92 insertions(+)
>  create mode 100644 tests/qemu-iotests/129
>  create mode 100644 tests/qemu-iotests/129.out

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH v2 4/4] blockjob: Update function name in comments

[Alberto Garcia]

On Fri, Apr 03, 2015 at 10:05:21PM +0800, Fam Zheng wrote:

> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/backup.c | 2 +-
>  block/mirror.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH v2 1/4] blockjob: Allow nested pause

[Alberto Garcia]

On Fri, Apr 03, 2015 at 10:05:18PM +0800, Fam Zheng wrote:

> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/mirror.c           |  2 +-
>  blockdev.c               |  8 +++++---
>  blockjob.c               | 23 +++++++++++++++++------
>  include/block/blockjob.h | 20 ++++++++++++++++----
>  4 files changed, 39 insertions(+), 14 deletions(-)

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH v2 1/4] blockjob: Allow nested pause

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1429 bytes --]

On Fri, Apr 03, 2015 at 10:05:18PM +0800, Fam Zheng wrote:
> This patch changes block_job_pause to increase the pause counter and
> block_job_resume to decrease it.
> 
> The counter will allow calling block_job_pause/block_job_resume
> unconditionally on a job when we need to suspend the IO temporarily.
> 
> From now on, each block_job_resume must be paired with a block_job_pause
> to keep the counter balanced.
> 
> The user pause from QMP or HMP will only trigger block_job_pause once
> until it's resumed, this is achieved by adding a user_paused flag in
> BlockJob.
> 
> One occurrence of block_job_resume in mirror_complete is replaced with
> block_job_enter which does what is necessary.
> 
> In block_job_cancel, the cancel flag is good enough to instruct
> coroutines to quit loop, so use block_job_enter to replace the unpaired
> block_job_resume.
> 
> Upon block job IO error, user is notified about the entering to the
> pause state, so this pause belongs to user pause, set the flag
> accordingly and expect a matching QMP resume.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/mirror.c           |  2 +-
>  blockdev.c               |  8 +++++---
>  blockjob.c               | 23 +++++++++++++++++------
>  include/block/blockjob.h | 20 ++++++++++++++++----
>  4 files changed, 39 insertions(+), 14 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 2/4] block: Pause block jobs in bdrv_drain_all

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1931 bytes --]

On Fri, Apr 03, 2015 at 10:05:19PM +0800, Fam Zheng wrote:
> This is necessary to suppress more IO requests from being generated from
> block job coroutines.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/block.c b/block.c
> index f2f8ae7..00cd91e 100644
> --- a/block.c
> +++ b/block.c
> @@ -2033,6 +2033,16 @@ void bdrv_drain_all(void)
>      bool busy = true;
>      BlockDriverState *bs;
>  
> +    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> +        AioContext *aio_context = bdrv_get_aio_context(bs);
> +
> +        aio_context_acquire(aio_context);
> +        if (bs->job) {
> +            block_job_pause(bs->job);
> +        }
> +        aio_context_release(aio_context);
> +    }
> +
>      while (busy) {
>          busy = false;
>  
> @@ -2044,6 +2054,16 @@ void bdrv_drain_all(void)
>              aio_context_release(aio_context);
>          }
>      }
> +
> +    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> +        AioContext *aio_context = bdrv_get_aio_context(bs);
> +
> +        aio_context_acquire(aio_context);
> +        if (bs->job) {
> +            block_job_resume(bs->job);
> +        }
> +        aio_context_release(aio_context);
> +    }
>  }

There is a tiny chance that we pause a job (which actually just sets
job->paused = true but there's no guarantee the job's coroutine reacts
to this) right before it terminates.  Then aio_poll() enters the
coroutine one last time and the job terminates.

We then reach the resume portion of bdrv_drain_all() and the job no
longer exists.  Hopefully nothing started a new job in the meantime.
bs->job should either be the original block job or NULL.

This code seems under current assumptions, but I just wanted to raise
these issues in case someone sees problems that I've missed.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 3/4] qemu-iotests: Test that "stop" doesn't drain block jobs

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 451 bytes --]

On Fri, Apr 03, 2015 at 10:05:20PM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/129     | 86 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/129.out |  5 +++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 92 insertions(+)
>  create mode 100644 tests/qemu-iotests/129
>  create mode 100644 tests/qemu-iotests/129.out

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 0/4] Fix "stop" draining block jobs

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1379 bytes --]

On Fri, Apr 03, 2015 at 10:05:17PM +0800, Fam Zheng wrote:
> v2: Use "pause" approach as suggested by Paolo.
> 
> Stopping the vm will drive the block job all the way to the end, because the
> sleep duration is too short, which means the block_job_sleep_ns in the block
> jobs are unhelpful. That is because the timer will fire too soon, even before
> the aio_poll in bdrv_drain_all returns.
> 
> Fix this by pausing all the block jobs during bdrv_drain_all.
> 
> Please review!
> 
> Fam
> 
> 
> Fam Zheng (4):
>   blockjob: Allow nested pause
>   block: Pause block jobs in bdrv_drain_all
>   qemu-iotests: Test that "stop" doesn't drain block jobs
>   blockjob: Update function name in comments
> 
>  block.c                    | 20 +++++++++++
>  block/backup.c             |  2 +-
>  block/mirror.c             |  4 +--
>  blockdev.c                 |  8 +++--
>  blockjob.c                 | 23 +++++++++----
>  include/block/blockjob.h   | 20 ++++++++---
>  tests/qemu-iotests/129     | 86 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/129.out |  5 +++
>  tests/qemu-iotests/group   |  1 +
>  9 files changed, 153 insertions(+), 16 deletions(-)
>  create mode 100644 tests/qemu-iotests/129
>  create mode 100644 tests/qemu-iotests/129.out
> 
> -- 
> 2.1.0
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 2/4] block: Pause block jobs in bdrv_drain_all

[Alberto Garcia]

On Wed, Apr 08, 2015 at 11:37:52AM +0100, Stefan Hajnoczi wrote:

> > +    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> > +        AioContext *aio_context = bdrv_get_aio_context(bs);
> > +
> > +        aio_context_acquire(aio_context);
> > +        if (bs->job) {
> > +            block_job_pause(bs->job);
> > +        }
> > +        aio_context_release(aio_context);
> > +    }
> > +
> >      while (busy) {
> >          busy = false;
> >  
> > @@ -2044,6 +2054,16 @@ void bdrv_drain_all(void)
> >              aio_context_release(aio_context);
> >          }
> >      }
> > +
> > +    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> > +        AioContext *aio_context = bdrv_get_aio_context(bs);
> > +
> > +        aio_context_acquire(aio_context);
> > +        if (bs->job) {
> > +            block_job_resume(bs->job);
> > +        }
> > +        aio_context_release(aio_context);
> > +    }
> >  }
> 
> There is a tiny chance that we pause a job (which actually just sets
> job->paused = true but there's no guarantee the job's coroutine
> reacts to this) right before it terminates.  Then aio_poll() enters
> the coroutine one last time and the job terminates.
> 
> We then reach the resume portion of bdrv_drain_all() and the job no
> longer exists.  Hopefully nothing started a new job in the meantime.
> bs->job should either be the original block job or NULL.
> 
> This code seems under current assumptions, but I just wanted to
> raise these issues in case someone sees problems that I've missed.

Is it possible that a new job is started in the meantime? If that's
the case this will hit the assertion in block_job_resume().

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/4] block: Pause block jobs in bdrv_drain_all

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2285 bytes --]

On Wed, Apr 08, 2015 at 04:56:14PM +0200, Alberto Garcia wrote:
> On Wed, Apr 08, 2015 at 11:37:52AM +0100, Stefan Hajnoczi wrote:
> 
> > > +    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> > > +        AioContext *aio_context = bdrv_get_aio_context(bs);
> > > +
> > > +        aio_context_acquire(aio_context);
> > > +        if (bs->job) {
> > > +            block_job_pause(bs->job);
> > > +        }
> > > +        aio_context_release(aio_context);
> > > +    }
> > > +
> > >      while (busy) {
> > >          busy = false;
> > >  
> > > @@ -2044,6 +2054,16 @@ void bdrv_drain_all(void)
> > >              aio_context_release(aio_context);
> > >          }
> > >      }
> > > +
> > > +    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> > > +        AioContext *aio_context = bdrv_get_aio_context(bs);
> > > +
> > > +        aio_context_acquire(aio_context);
> > > +        if (bs->job) {
> > > +            block_job_resume(bs->job);
> > > +        }
> > > +        aio_context_release(aio_context);
> > > +    }
> > >  }
> > 
> > There is a tiny chance that we pause a job (which actually just sets
> > job->paused = true but there's no guarantee the job's coroutine
> > reacts to this) right before it terminates.  Then aio_poll() enters
> > the coroutine one last time and the job terminates.
> > 
> > We then reach the resume portion of bdrv_drain_all() and the job no
> > longer exists.  Hopefully nothing started a new job in the meantime.
> > bs->job should either be the original block job or NULL.
> > 
> > This code seems under current assumptions, but I just wanted to
> > raise these issues in case someone sees problems that I've missed.
> 
> Is it possible that a new job is started in the meantime? If that's
> the case this will hit the assertion in block_job_resume().

That is currently not possible since the QEMU monitor does not run while
we're waiting in aio_poll().

Therefore no block job monitor commands could spawn a new job.

If code is added that spawns a job based on an AioContext timer or due
to some other event, then this assumption no longer holds and there is a
problem because block_job_resume() is called on a job that never paused.

But for now there is no problem.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/4] Fix "stop" draining block jobs

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]

On Fri, Apr 03, 2015 at 10:05:17PM +0800, Fam Zheng wrote:
> v2: Use "pause" approach as suggested by Paolo.
> 
> Stopping the vm will drive the block job all the way to the end, because the
> sleep duration is too short, which means the block_job_sleep_ns in the block
> jobs are unhelpful. That is because the timer will fire too soon, even before
> the aio_poll in bdrv_drain_all returns.
> 
> Fix this by pausing all the block jobs during bdrv_drain_all.
> 
> Please review!
> 
> Fam
> 
> 
> Fam Zheng (4):
>   blockjob: Allow nested pause
>   block: Pause block jobs in bdrv_drain_all
>   qemu-iotests: Test that "stop" doesn't drain block jobs
>   blockjob: Update function name in comments
> 
>  block.c                    | 20 +++++++++++
>  block/backup.c             |  2 +-
>  block/mirror.c             |  4 +--
>  blockdev.c                 |  8 +++--
>  blockjob.c                 | 23 +++++++++----
>  include/block/blockjob.h   | 20 ++++++++---
>  tests/qemu-iotests/129     | 86 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/129.out |  5 +++
>  tests/qemu-iotests/group   |  1 +
>  9 files changed, 153 insertions(+), 16 deletions(-)
>  create mode 100644 tests/qemu-iotests/129
>  create mode 100644 tests/qemu-iotests/129.out

Thanks, applied to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/4] blockjob: Allow nested pause

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 6788 bytes --]

On Fri, Apr 03, 2015 at 04:13:55PM +0200, Paolo Bonzini wrote:
> 
> 
> On 03/04/2015 16:05, Fam Zheng wrote:
> > This patch changes block_job_pause to increase the pause counter and
> > block_job_resume to decrease it.
> > 
> > The counter will allow calling block_job_pause/block_job_resume
> > unconditionally on a job when we need to suspend the IO temporarily.
> > 
> > From now on, each block_job_resume must be paired with a block_job_pause
> > to keep the counter balanced.
> > 
> > The user pause from QMP or HMP will only trigger block_job_pause once
> > until it's resumed, this is achieved by adding a user_paused flag in
> > BlockJob.
> > 
> > One occurrence of block_job_resume in mirror_complete is replaced with
> > block_job_enter which does what is necessary.
> > 
> > In block_job_cancel, the cancel flag is good enough to instruct
> > coroutines to quit loop, so use block_job_enter to replace the unpaired
> > block_job_resume.
> > 
> > Upon block job IO error, user is notified about the entering to the
> > pause state, so this pause belongs to user pause, set the flag
> > accordingly and expect a matching QMP resume.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/mirror.c           |  2 +-
> >  blockdev.c               |  8 +++++---
> >  blockjob.c               | 23 +++++++++++++++++------
> >  include/block/blockjob.h | 20 ++++++++++++++++----
> >  4 files changed, 39 insertions(+), 14 deletions(-)
> > 
> > diff --git a/block/mirror.c b/block/mirror.c
> > index 4056164..65b1718 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -634,7 +634,7 @@ static void mirror_complete(BlockJob *job, Error **errp)
> >      }
> >  
> >      s->should_complete = true;
> > -    block_job_resume(job);
> > +    block_job_enter(&s->common);
> >  }
> >  
> >  static const BlockJobDriver mirror_job_driver = {
> > diff --git a/blockdev.c b/blockdev.c
> > index fbb3a79..9132d69 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -2699,7 +2699,7 @@ void qmp_block_job_cancel(const char *device,
> >          force = false;
> >      }
> >  
> > -    if (job->paused && !force) {
> > +    if (job->user_paused && !force) {
> >          error_setg(errp, "The block job for device '%s' is currently paused",
> >                     device);
> >          goto out;
> > @@ -2716,10 +2716,11 @@ void qmp_block_job_pause(const char *device, Error **errp)
> >      AioContext *aio_context;
> >      BlockJob *job = find_block_job(device, &aio_context, errp);
> >  
> > -    if (!job) {
> > +    if (!job || job->user_paused) {
> >          return;
> >      }
> >  
> > +    job->user_paused = true;
> >      trace_qmp_block_job_pause(job);
> >      block_job_pause(job);
> >      aio_context_release(aio_context);
> > @@ -2730,10 +2731,11 @@ void qmp_block_job_resume(const char *device, Error **errp)
> >      AioContext *aio_context;
> >      BlockJob *job = find_block_job(device, &aio_context, errp);
> >  
> > -    if (!job) {
> > +    if (!job || !job->user_paused) {
> >          return;
> >      }
> >  
> > +    job->user_paused = false;
> >      trace_qmp_block_job_resume(job);
> >      block_job_resume(job);
> >      aio_context_release(aio_context);
> > diff --git a/blockjob.c b/blockjob.c
> > index ba2255d..2755465 100644
> > --- a/blockjob.c
> > +++ b/blockjob.c
> > @@ -107,7 +107,7 @@ void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
> >  
> >  void block_job_complete(BlockJob *job, Error **errp)
> >  {
> > -    if (job->paused || job->cancelled || !job->driver->complete) {
> > +    if (job->pause_count || job->cancelled || !job->driver->complete) {
> >          error_set(errp, QERR_BLOCK_JOB_NOT_READY,
> >                    bdrv_get_device_name(job->bs));
> >          return;
> > @@ -118,17 +118,26 @@ void block_job_complete(BlockJob *job, Error **errp)
> >  
> >  void block_job_pause(BlockJob *job)
> >  {
> > -    job->paused = true;
> > +    job->pause_count++;
> >  }
> >  
> >  bool block_job_is_paused(BlockJob *job)
> >  {
> > -    return job->paused;
> > +    return job->pause_count > 0;
> >  }
> >  
> >  void block_job_resume(BlockJob *job)
> >  {
> > -    job->paused = false;
> > +    assert(job->pause_count > 0);
> > +    job->pause_count--;
> > +    if (job->pause_count) {
> > +        return;
> > +    }
> > +    block_job_enter(job);
> > +}
> > +
> > +void block_job_enter(BlockJob *job)
> > +{
> >      block_job_iostatus_reset(job);
> >      if (job->co && !job->busy) {
> >          qemu_coroutine_enter(job->co, NULL);
> > @@ -138,7 +147,7 @@ void block_job_resume(BlockJob *job)
> >  void block_job_cancel(BlockJob *job)
> >  {
> >      job->cancelled = true;
> > -    block_job_resume(job);
> > +    block_job_enter(job);
> >  }
> >  
> >  bool block_job_is_cancelled(BlockJob *job)
> > @@ -258,7 +267,7 @@ BlockJobInfo *block_job_query(BlockJob *job)
> >      info->device    = g_strdup(bdrv_get_device_name(job->bs));
> >      info->len       = job->len;
> >      info->busy      = job->busy;
> > -    info->paused    = job->paused;
> > +    info->paused    = job->pause_count > 0;
> >      info->offset    = job->offset;
> >      info->speed     = job->speed;
> >      info->io_status = job->iostatus;
> > @@ -335,6 +344,8 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs,
> >                                      IO_OPERATION_TYPE_WRITE,
> >                                      action, &error_abort);
> >      if (action == BLOCK_ERROR_ACTION_STOP) {
> > +        /* make the pause user visible, which will be resumed from QMP. */
> > +        job->user_paused = true;
> >          block_job_pause(job);
> >          block_job_iostatus_set_err(job, error);
> >          if (bs != job->bs) {
> > diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> > index b6d4ebb..9f4b20d 100644
> > --- a/include/block/blockjob.h
> > +++ b/include/block/blockjob.h
> > @@ -79,10 +79,14 @@ struct BlockJob {
> >      bool cancelled;
> >  
> >      /**
> > -     * Set to true if the job is either paused, or will pause itself
> > -     * as soon as possible (if busy == true).
> > +     * Counter for pause request. If non-zero, the block job will pause.
> 
> Why not keep the more complete comment ("If non-zero, the block job is
> either paused, or if busy == true will pause itself as soon as possible")?
> 
> >       */
> > -    bool paused;
> > +    int pause_count;
> > +
> > +    /**
> > +     * Set to true if the job is paused by user.
> 
> ... and can be unpaused with the block-job-resume QMP command.

I squashed in your suggestions.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v2 3/4] qemu-iotests: Test that "stop" doesn't drain block jobs

[Max Reitz]

On 03.04.2015 16:05, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   tests/qemu-iotests/129     | 86 ++++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/129.out |  5 +++
>   tests/qemu-iotests/group   |  1 +
>   3 files changed, 92 insertions(+)
>   create mode 100644 tests/qemu-iotests/129
>   create mode 100644 tests/qemu-iotests/129.out
>
> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
> new file mode 100644
> index 0000000..9e87e1c
> --- /dev/null
> +++ b/tests/qemu-iotests/129
> @@ -0,0 +1,86 @@
> +#!/usr/bin/env python
> +#
> +# Tests that "bdrv_drain_all" doesn't drain block jobs
> +#
> +# Copyright (C) 2015 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import os
> +import iotests
> +import time
> +
> +class TestStopWithBlockJob(iotests.QMPTestCase):
> +    test_img = os.path.join(iotests.test_dir, 'test.img')
> +    target_img = os.path.join(iotests.test_dir, 'target.img')
> +    base_img = os.path.join(iotests.test_dir, 'base.img')
> +
> +    def setUp(self):
> +        iotests.qemu_img('create', '-f', iotests.imgfmt, self.base_img, "1G")
> +        iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img, "-b", self.base_img)
> +        iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', self.test_img)
> +        self.vm = iotests.VM().add_drive(self.test_img)
> +        self.vm.launch()
> +
> +    def tearDown(self):
> +        params = {"device": "drive0",
> +                  "bps": 0,
> +                  "bps_rd": 0,
> +                  "bps_wr": 0,
> +                  "iops": 0,
> +                  "iops_rd": 0,
> +                  "iops_wr": 0,
> +                 }
> +        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
> +                             **params)
> +        self.vm.shutdown()
> +
> +    def do_test_stop(self, cmd, **args):
> +        """Test 'stop' while block job is running on a throttled drive.
> +        The 'stop' command shouldn't drain the job"""
> +        params = {"device": "drive0",
> +                  "bps": 1024,
> +                  "bps_rd": 0,
> +                  "bps_wr": 0,
> +                  "iops": 0,
> +                  "iops_rd": 0,
> +                  "iops_wr": 0,
> +                 }
> +        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
> +                             **params)
> +        self.assert_qmp(result, 'return', {})
> +        result = self.vm.qmp(cmd, **args)
> +        self.assert_qmp(result, 'return', {})
> +        result = self.vm.qmp("stop")
> +        self.assert_qmp(result, 'return', {})
> +        result = self.vm.qmp("query-block-jobs")
> +        self.assert_qmp(result, 'return[0]/busy', True)

I don't know why, but this assertion fails in tmpfs (for me, at least).

Max

> +        self.assert_qmp(result, 'return[0]/ready', False)
> +
> +    def test_drive_mirror(self):
> +        self.do_test_stop("drive-mirror", device="drive0",
> +                          target=self.target_img,
> +                          sync="full")
> +
> +    def test_drive_backup(self):
> +        self.do_test_stop("drive-backup", device="drive0",
> +                          target=self.target_img,
> +                          sync="full")
> +
> +    def test_block_commit(self):
> +        self.do_test_stop("block-commit", device="drive0")
> +
> +if __name__ == '__main__':
> +    iotests.main(supported_fmts=["qcow2"])
> diff --git a/tests/qemu-iotests/129.out b/tests/qemu-iotests/129.out
> new file mode 100644
> index 0000000..8d7e996
> --- /dev/null
> +++ b/tests/qemu-iotests/129.out
> @@ -0,0 +1,5 @@
> +...
> +----------------------------------------------------------------------
> +Ran 3 tests
> +
> +OK
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 6262127..69071ac 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -124,3 +124,4 @@
>   121 rw auto
>   123 rw auto quick
>   128 rw auto quick
> +129 rw auto quick

Re: [Qemu-devel] [Qemu-block] [PATCH v2 3/4] qemu-iotests: Test that "stop" doesn't drain block jobs

[Fam Zheng]

On Fri, 04/24 17:43, Max Reitz wrote:
> On 03.04.2015 16:05, Fam Zheng wrote:
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >---
> >  tests/qemu-iotests/129     | 86 ++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/129.out |  5 +++
> >  tests/qemu-iotests/group   |  1 +
> >  3 files changed, 92 insertions(+)
> >  create mode 100644 tests/qemu-iotests/129
> >  create mode 100644 tests/qemu-iotests/129.out
> >
> >diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
> >new file mode 100644
> >index 0000000..9e87e1c
> >--- /dev/null
> >+++ b/tests/qemu-iotests/129
> >@@ -0,0 +1,86 @@
> >+#!/usr/bin/env python
> >+#
> >+# Tests that "bdrv_drain_all" doesn't drain block jobs
> >+#
> >+# Copyright (C) 2015 Red Hat, Inc.
> >+#
> >+# This program is free software; you can redistribute it and/or modify
> >+# it under the terms of the GNU General Public License as published by
> >+# the Free Software Foundation; either version 2 of the License, or
> >+# (at your option) any later version.
> >+#
> >+# This program is distributed in the hope that it will be useful,
> >+# but WITHOUT ANY WARRANTY; without even the implied warranty of
> >+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >+# GNU General Public License for more details.
> >+#
> >+# You should have received a copy of the GNU General Public License
> >+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >+#
> >+
> >+import os
> >+import iotests
> >+import time
> >+
> >+class TestStopWithBlockJob(iotests.QMPTestCase):
> >+    test_img = os.path.join(iotests.test_dir, 'test.img')
> >+    target_img = os.path.join(iotests.test_dir, 'target.img')
> >+    base_img = os.path.join(iotests.test_dir, 'base.img')
> >+
> >+    def setUp(self):
> >+        iotests.qemu_img('create', '-f', iotests.imgfmt, self.base_img, "1G")
> >+        iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img, "-b", self.base_img)
> >+        iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', self.test_img)
> >+        self.vm = iotests.VM().add_drive(self.test_img)
> >+        self.vm.launch()
> >+
> >+    def tearDown(self):
> >+        params = {"device": "drive0",
> >+                  "bps": 0,
> >+                  "bps_rd": 0,
> >+                  "bps_wr": 0,
> >+                  "iops": 0,
> >+                  "iops_rd": 0,
> >+                  "iops_wr": 0,
> >+                 }
> >+        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
> >+                             **params)
> >+        self.vm.shutdown()
> >+
> >+    def do_test_stop(self, cmd, **args):
> >+        """Test 'stop' while block job is running on a throttled drive.
> >+        The 'stop' command shouldn't drain the job"""
> >+        params = {"device": "drive0",
> >+                  "bps": 1024,
> >+                  "bps_rd": 0,
> >+                  "bps_wr": 0,
> >+                  "iops": 0,
> >+                  "iops_rd": 0,
> >+                  "iops_wr": 0,
> >+                 }
> >+        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
> >+                             **params)
> >+        self.assert_qmp(result, 'return', {})
> >+        result = self.vm.qmp(cmd, **args)
> >+        self.assert_qmp(result, 'return', {})
> >+        result = self.vm.qmp("stop")
> >+        self.assert_qmp(result, 'return', {})
> >+        result = self.vm.qmp("query-block-jobs")
> >+        self.assert_qmp(result, 'return[0]/busy', True)
> 
> I don't know why, but this assertion fails in tmpfs (for me, at least).

I also run tests in tmpfs, but I don't see the failure.  What is in "result"?
Is the block job completed?

(I assume you've applied and compiled patch 1-2. Worth mentioning because
that's exactly what this assertion is testing against.)

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH v2 3/4] qemu-iotests: Test that "stop" doesn't drain block jobs

[Max Reitz]

On 27.04.2015 07:14, Fam Zheng wrote:
> On Fri, 04/24 17:43, Max Reitz wrote:
>> On 03.04.2015 16:05, Fam Zheng wrote:
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> ---
>>>   tests/qemu-iotests/129     | 86 ++++++++++++++++++++++++++++++++++++++++++++++
>>>   tests/qemu-iotests/129.out |  5 +++
>>>   tests/qemu-iotests/group   |  1 +
>>>   3 files changed, 92 insertions(+)
>>>   create mode 100644 tests/qemu-iotests/129
>>>   create mode 100644 tests/qemu-iotests/129.out
>>>
>>> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
>>> new file mode 100644
>>> index 0000000..9e87e1c
>>> --- /dev/null
>>> +++ b/tests/qemu-iotests/129
>>> @@ -0,0 +1,86 @@
>>> +#!/usr/bin/env python
>>> +#
>>> +# Tests that "bdrv_drain_all" doesn't drain block jobs
>>> +#
>>> +# Copyright (C) 2015 Red Hat, Inc.
>>> +#
>>> +# This program is free software; you can redistribute it and/or modify
>>> +# it under the terms of the GNU General Public License as published by
>>> +# the Free Software Foundation; either version 2 of the License, or
>>> +# (at your option) any later version.
>>> +#
>>> +# This program is distributed in the hope that it will be useful,
>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +# GNU General Public License for more details.
>>> +#
>>> +# You should have received a copy of the GNU General Public License
>>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>> +#
>>> +
>>> +import os
>>> +import iotests
>>> +import time
>>> +
>>> +class TestStopWithBlockJob(iotests.QMPTestCase):
>>> +    test_img = os.path.join(iotests.test_dir, 'test.img')
>>> +    target_img = os.path.join(iotests.test_dir, 'target.img')
>>> +    base_img = os.path.join(iotests.test_dir, 'base.img')
>>> +
>>> +    def setUp(self):
>>> +        iotests.qemu_img('create', '-f', iotests.imgfmt, self.base_img, "1G")
>>> +        iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img, "-b", self.base_img)
>>> +        iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', self.test_img)
>>> +        self.vm = iotests.VM().add_drive(self.test_img)
>>> +        self.vm.launch()
>>> +
>>> +    def tearDown(self):
>>> +        params = {"device": "drive0",
>>> +                  "bps": 0,
>>> +                  "bps_rd": 0,
>>> +                  "bps_wr": 0,
>>> +                  "iops": 0,
>>> +                  "iops_rd": 0,
>>> +                  "iops_wr": 0,
>>> +                 }
>>> +        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
>>> +                             **params)
>>> +        self.vm.shutdown()
>>> +
>>> +    def do_test_stop(self, cmd, **args):
>>> +        """Test 'stop' while block job is running on a throttled drive.
>>> +        The 'stop' command shouldn't drain the job"""
>>> +        params = {"device": "drive0",
>>> +                  "bps": 1024,
>>> +                  "bps_rd": 0,
>>> +                  "bps_wr": 0,
>>> +                  "iops": 0,
>>> +                  "iops_rd": 0,
>>> +                  "iops_wr": 0,
>>> +                 }
>>> +        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
>>> +                             **params)
>>> +        self.assert_qmp(result, 'return', {})
>>> +        result = self.vm.qmp(cmd, **args)
>>> +        self.assert_qmp(result, 'return', {})
>>> +        result = self.vm.qmp("stop")
>>> +        self.assert_qmp(result, 'return', {})
>>> +        result = self.vm.qmp("query-block-jobs")
>>> +        self.assert_qmp(result, 'return[0]/busy', True)
>> I don't know why, but this assertion fails in tmpfs (for me, at least).
> I also run tests in tmpfs, but I don't see the failure.  What is in "result"?
> Is the block job completed?
>
> (I assume you've applied and compiled patch 1-2. Worth mentioning because
> that's exactly what this assertion is testing against.)

On Stefan's block-next branch:

--- tests/qemu-iotests/129.out 2015-04-24 17:39:47.089438232 +0200
+++ 129.out.bad 2015-04-27 11:57:04.537258961 +0200
@@ -1,5 +1,17 @@
-...
+F..
+======================================================================
+FAIL: test_block_commit (__main__.TestStopWithBlockJob)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "129", line 83, in test_block_commit
+    self.do_test_stop("block-commit", device="drive0")
+  File "129", line 69, in do_test_stop
+    self.assert_qmp(result, 'return[0]/busy', True)
+  File "tests/qemu-iotests/iotests.py", line 287, in assert_qmp
+    self.assertEqual(result, value, 'values not equal "%s" and "%s"' % 
(str(result), str(value)))
+AssertionError: values not equal "False" and "True"
+
  ----------------------------------------------------------------------
  Ran 3 tests

-OK
+FAILED (failures=1)

On my SSD it works just fine.

Adding a "print cmd"; "print self.vm.get_qmp_events(wait=False)" and 
"print result" gives:

block-commit
[{u'timestamp': {u'seconds': 1430130787, u'microseconds': 779056}, 
u'event': u'STOP'}, {u'timestamp': {u'seconds': 1430130787, 
u'microseconds': 868729}, u'data': {u'device': u'drive0', u'type': 
u'commit', u'speed': 0, u'len': 134217728, 'offset': 134217728}, 
u'event': u'BLOCK_JOB_READY'}]
{u'return': [{u'busy': False, u'type': u'commit', u'len': 134217728, 
u'paused': False, u'ready': True, u'io-status': u'ok', u'offset': 
134217728, u'device': u'drive0', u'speed': 0}]}
drive-backup
[{u'timestamp': {u'seconds': 1430130788, u'microseconds': 11275}, 
u'event': u'STOP'}]
{u'return': [{u'busy': True, u'type': u'backup', u'len': 1073741824, 
u'paused': False, u'ready': False, u'io-status': u'ok', u'offset': 
65536, u'device': u'drive0', u'speed': 0}]}
drive-mirror
[{u'timestamp': {u'seconds': 1430130788, u'microseconds': 672984}, 
u'event': u'STOP'}]
{u'return': [{u'busy': True, u'type': u'mirror', u'len': 134217728, 
u'paused': False, u'ready': False, u'io-status': u'ok', u'offset': 0, 
u'device': u'drive0', u'speed': 0}]}

So apparently the block-commit job did complete. I guess this might be 
because while the device itself is throttled, the backing file is not, 
so the commit job is not throttled...?

Okay, half an hour of investigation later: It is because block/mirror 
takes a clever shortcut when delay_ns is 0 and simply skips sleeping, 
which makes pausing the mirror (or active commit) block job in 
bdrv_drain_all() moot. I sent a patch for that, because I might as well...

Max