[Qemu-devel] [PATCH v2 0/3] block: pause block jobs for bdrv_drain_begin/end

[Qemu-devel] [PATCH v2 0/3] block: pause block jobs for bdrv_drain_begin/end

[John Snow]

Reference: https://bugzilla.redhat.com/show_bug.cgi?id=1367369#c8

It's possible to wedge QEMU if the guest tries to reset a virtio-pci
device as QEMU is also using the drive for a blockjob. This patchset
aims to allow us to safely pause/resume jobs attached to individual
nodes in a manner similar to how bdrv_drain_all_begin/end do.

John Snow (3):
  blockjob: add block_job_start_shim
  block-backend: add drained_begin / drained_end ops
  blockjob: add devops to blockjob backends

 block/block-backend.c          | 24 ++++++++++++++++--
 blockjob.c                     | 55 +++++++++++++++++++++++++++++++++---------
 include/sysemu/block-backend.h |  8 ++++++
 3 files changed, 73 insertions(+), 14 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v2 1/3] blockjob: add block_job_start_shim

[John Snow]

The purpose of this shim is to allow us to pause pre-started jobs.
The purpose of *that* is to allow us to buffer a pause request that
will be able to take effect before the job ever does any work, allowing
us to create jobs during a quiescent state (under which they will be
automatically paused), then resuming the jobs after the critical section
in any order, either:

(1) -block_job_start
    -block_job_resume (via e.g. drained_end)

(2) -block_job_resume (via e.g. drained_end)
    -block_job_start

The problem that requires a startup wrapper is the idea that a job must
start in the busy=true state only its first time-- all subsequent entries
require busy to be false, and the toggling of this state is otherwise
handled during existing pause and yield points.

The wrapper simply allows us to mandate that a job can "start," set busy
to true, then immediately pause only if necessary. We could avoid
requiring a wrapper, but all jobs would need to do it, so it's been
factored out here.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockjob.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index 69126af..69b4ec6 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -250,16 +250,28 @@ static bool block_job_started(BlockJob *job)
     return job->co;
 }
 
+/**
+ * All jobs must allow a pause point before entering their job proper. This
+ * ensures that jobs can be paused prior to being started, then resumed later.
+ */
+static void coroutine_fn block_job_co_entry(void *opaque)
+{
+    BlockJob *job = opaque;
+
+    assert(job && job->driver && job->driver->start);
+    block_job_pause_point(job);
+    job->driver->start(job);
+}
+
 void block_job_start(BlockJob *job)
 {
     assert(job && !block_job_started(job) && job->paused &&
-           !job->busy && job->driver->start);
-    job->co = qemu_coroutine_create(job->driver->start, job);
-    if (--job->pause_count == 0) {
-        job->paused = false;
-        job->busy = true;
-        qemu_coroutine_enter(job->co);
-    }
+           job->driver && job->driver->start);
+    job->co = qemu_coroutine_create(block_job_co_entry, job);
+    job->pause_count--;
+    job->busy = true;
+    job->paused = false;
+    qemu_coroutine_enter(job->co);
 }
 
 void block_job_ref(BlockJob *job)
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 1/3] blockjob: add block_job_start_shim

[Jeff Cody]

On Thu, Mar 16, 2017 at 05:23:49PM -0400, John Snow wrote:
> The purpose of this shim is to allow us to pause pre-started jobs.
> The purpose of *that* is to allow us to buffer a pause request that
> will be able to take effect before the job ever does any work, allowing
> us to create jobs during a quiescent state (under which they will be
> automatically paused), then resuming the jobs after the critical section
> in any order, either:
> 
> (1) -block_job_start
>     -block_job_resume (via e.g. drained_end)
> 
> (2) -block_job_resume (via e.g. drained_end)
>     -block_job_start
> 
> The problem that requires a startup wrapper is the idea that a job must
> start in the busy=true state only its first time-- all subsequent entries
> require busy to be false, and the toggling of this state is otherwise
> handled during existing pause and yield points.
> 
> The wrapper simply allows us to mandate that a job can "start," set busy
> to true, then immediately pause only if necessary. We could avoid
> requiring a wrapper, but all jobs would need to do it, so it's been
> factored out here.

I think this makes sense.  So when this happens:

* block_job_create
* block_job_pause
* block_job_resume  <-- only effects pause_count, rest is noop
* block_job_start

The block_job_resume is mostly a no-op, only affecting the pause_count but
since there is no job coroutine created yet, the block_job_enter does
nothing.

> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  blockjob.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/blockjob.c b/blockjob.c
> index 69126af..69b4ec6 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -250,16 +250,28 @@ static bool block_job_started(BlockJob *job)
>      return job->co;
>  }
>  
> +/**
> + * All jobs must allow a pause point before entering their job proper. This
> + * ensures that jobs can be paused prior to being started, then resumed later.
> + */
> +static void coroutine_fn block_job_co_entry(void *opaque)
> +{
> +    BlockJob *job = opaque;
> +
> +    assert(job && job->driver && job->driver->start);
> +    block_job_pause_point(job);
> +    job->driver->start(job);
> +}
> +
>  void block_job_start(BlockJob *job)
>  {
>      assert(job && !block_job_started(job) && job->paused &&
> -           !job->busy && job->driver->start);
> -    job->co = qemu_coroutine_create(job->driver->start, job);
> -    if (--job->pause_count == 0) {
> -        job->paused = false;
> -        job->busy = true;
> -        qemu_coroutine_enter(job->co);
> -    }
> +           job->driver && job->driver->start);
> +    job->co = qemu_coroutine_create(block_job_co_entry, job);
> +    job->pause_count--;
> +    job->busy = true;
> +    job->paused = false;
> +    qemu_coroutine_enter(job->co);
>  }
>  
>  void block_job_ref(BlockJob *job)
> -- 
> 2.9.3
> 

Re: [Qemu-devel] [PATCH v2 1/3] blockjob: add block_job_start_shim

[Jeff Cody]

On Wed, Mar 22, 2017 at 08:57:51AM -0400, Jeff Cody wrote:
> On Thu, Mar 16, 2017 at 05:23:49PM -0400, John Snow wrote:
> > The purpose of this shim is to allow us to pause pre-started jobs.
> > The purpose of *that* is to allow us to buffer a pause request that
> > will be able to take effect before the job ever does any work, allowing
> > us to create jobs during a quiescent state (under which they will be
> > automatically paused), then resuming the jobs after the critical section
> > in any order, either:
> > 
> > (1) -block_job_start
> >     -block_job_resume (via e.g. drained_end)
> > 
> > (2) -block_job_resume (via e.g. drained_end)
> >     -block_job_start
> > 
> > The problem that requires a startup wrapper is the idea that a job must
> > start in the busy=true state only its first time-- all subsequent entries
> > require busy to be false, and the toggling of this state is otherwise
> > handled during existing pause and yield points.
> > 
> > The wrapper simply allows us to mandate that a job can "start," set busy
> > to true, then immediately pause only if necessary. We could avoid
> > requiring a wrapper, but all jobs would need to do it, so it's been
> > factored out here.
> 
> I think this makes sense.  So when this happens:
> 
> * block_job_create
> * block_job_pause
> * block_job_resume  <-- only effects pause_count, rest is noop
> * block_job_start
> 
> The block_job_resume is mostly a no-op, only affecting the pause_count but
> since there is no job coroutine created yet, the block_job_enter does
> nothing.
>

I should have added:

Reviewed-by: Jeff Cody <jcody@redhat.com>

> > 
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  blockjob.c | 26 +++++++++++++++++++-------
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> > 
> > diff --git a/blockjob.c b/blockjob.c
> > index 69126af..69b4ec6 100644
> > --- a/blockjob.c
> > +++ b/blockjob.c
> > @@ -250,16 +250,28 @@ static bool block_job_started(BlockJob *job)
> >      return job->co;
> >  }
> >  
> > +/**
> > + * All jobs must allow a pause point before entering their job proper. This
> > + * ensures that jobs can be paused prior to being started, then resumed later.
> > + */
> > +static void coroutine_fn block_job_co_entry(void *opaque)
> > +{
> > +    BlockJob *job = opaque;
> > +
> > +    assert(job && job->driver && job->driver->start);
> > +    block_job_pause_point(job);
> > +    job->driver->start(job);
> > +}
> > +
> >  void block_job_start(BlockJob *job)
> >  {
> >      assert(job && !block_job_started(job) && job->paused &&
> > -           !job->busy && job->driver->start);
> > -    job->co = qemu_coroutine_create(job->driver->start, job);
> > -    if (--job->pause_count == 0) {
> > -        job->paused = false;
> > -        job->busy = true;
> > -        qemu_coroutine_enter(job->co);
> > -    }
> > +           job->driver && job->driver->start);
> > +    job->co = qemu_coroutine_create(block_job_co_entry, job);
> > +    job->pause_count--;
> > +    job->busy = true;
> > +    job->paused = false;
> > +    qemu_coroutine_enter(job->co);
> >  }
> >  
> >  void block_job_ref(BlockJob *job)
> > -- 
> > 2.9.3
> > 

[Qemu-devel] [PATCH v2 2/3] block-backend: add drained_begin / drained_end ops

[John Snow]

Allow block backends to forward drain requests to their devices/users.
The initial intended purpose for this patch is to allow BBs to forward
requests along to BlockJobs, which will want to pause if their associated
BB has entered a drained region.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/block-backend.c          | 24 ++++++++++++++++++++++--
 include/sysemu/block-backend.h |  8 ++++++++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 5742c09..dec4a1c 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -65,6 +65,8 @@ struct BlockBackend {
     bool allow_write_beyond_eof;
 
     NotifierList remove_bs_notifiers, insert_bs_notifiers;
+
+    int quiesce_counter;
 };
 
 typedef struct BlockBackendAIOCB {
@@ -699,12 +701,17 @@ void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops,
                      void *opaque)
 {
     /* All drivers that use blk_set_dev_ops() are qdevified and we want to keep
-     * it that way, so we can assume blk->dev is a DeviceState if blk->dev_ops
-     * is set. */
+     * it that way, so we can assume blk->dev, if present, is a DeviceState if
+     * blk->dev_ops is set. Non-device users may use dev_ops without device. */
     assert(!blk->legacy_dev);
 
     blk->dev_ops = ops;
     blk->dev_opaque = opaque;
+
+    /* Are we currently quiesced? Should we enforce this right now? */
+    if (blk->quiesce_counter && ops->drained_begin) {
+        ops->drained_begin(opaque);
+    }
 }
 
 /*
@@ -1870,6 +1877,12 @@ static void blk_root_drained_begin(BdrvChild *child)
 {
     BlockBackend *blk = child->opaque;
 
+    if (++blk->quiesce_counter == 1) {
+        if (blk->dev_ops && blk->dev_ops->drained_begin) {
+            blk->dev_ops->drained_begin(blk->dev_opaque);
+        }
+    }
+
     /* Note that blk->root may not be accessible here yet if we are just
      * attaching to a BlockDriverState that is drained. Use child instead. */
 
@@ -1881,7 +1894,14 @@ static void blk_root_drained_begin(BdrvChild *child)
 static void blk_root_drained_end(BdrvChild *child)
 {
     BlockBackend *blk = child->opaque;
+    assert(blk->quiesce_counter);
 
     assert(blk->public.io_limits_disabled);
     --blk->public.io_limits_disabled;
+
+    if (--blk->quiesce_counter == 0) {
+        if (blk->dev_ops && blk->dev_ops->drained_end) {
+              blk->dev_ops->drained_end(blk->dev_opaque);
+        }
+    }
 }
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 096c17f..7462228 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -58,6 +58,14 @@ typedef struct BlockDevOps {
      * Runs when the size changed (e.g. monitor command block_resize)
      */
     void (*resize_cb)(void *opaque);
+    /*
+     * Runs when the backend receives a drain request.
+     */
+    void (*drained_begin)(void *opaque);
+    /*
+     * Runs when the backend's last drain request ends.
+     */
+    void (*drained_end)(void *opaque);
 } BlockDevOps;
 
 /* This struct is embedded in (the private) BlockBackend struct and contains
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 2/3] block-backend: add drained_begin / drained_end ops

[Jeff Cody]

On Thu, Mar 16, 2017 at 05:23:50PM -0400, John Snow wrote:
> Allow block backends to forward drain requests to their devices/users.
> The initial intended purpose for this patch is to allow BBs to forward
> requests along to BlockJobs, which will want to pause if their associated
> BB has entered a drained region.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/block-backend.c          | 24 ++++++++++++++++++++++--
>  include/sysemu/block-backend.h |  8 ++++++++
>  2 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 5742c09..dec4a1c 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -65,6 +65,8 @@ struct BlockBackend {
>      bool allow_write_beyond_eof;
>  
>      NotifierList remove_bs_notifiers, insert_bs_notifiers;
> +
> +    int quiesce_counter;
>  };
>  
>  typedef struct BlockBackendAIOCB {
> @@ -699,12 +701,17 @@ void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops,
>                       void *opaque)
>  {
>      /* All drivers that use blk_set_dev_ops() are qdevified and we want to keep
> -     * it that way, so we can assume blk->dev is a DeviceState if blk->dev_ops
> -     * is set. */
> +     * it that way, so we can assume blk->dev, if present, is a DeviceState if
> +     * blk->dev_ops is set. Non-device users may use dev_ops without device. */
>      assert(!blk->legacy_dev);
>  
>      blk->dev_ops = ops;
>      blk->dev_opaque = opaque;
> +
> +    /* Are we currently quiesced? Should we enforce this right now? */
> +    if (blk->quiesce_counter && ops->drained_begin) {
> +        ops->drained_begin(opaque);
> +    }
>  }
>  
>  /*
> @@ -1870,6 +1877,12 @@ static void blk_root_drained_begin(BdrvChild *child)
>  {
>      BlockBackend *blk = child->opaque;
>  
> +    if (++blk->quiesce_counter == 1) {
> +        if (blk->dev_ops && blk->dev_ops->drained_begin) {
> +            blk->dev_ops->drained_begin(blk->dev_opaque);
> +        }
> +    }
> +
>      /* Note that blk->root may not be accessible here yet if we are just
>       * attaching to a BlockDriverState that is drained. Use child instead. */
>  
> @@ -1881,7 +1894,14 @@ static void blk_root_drained_begin(BdrvChild *child)
>  static void blk_root_drained_end(BdrvChild *child)
>  {
>      BlockBackend *blk = child->opaque;
> +    assert(blk->quiesce_counter);
>  
>      assert(blk->public.io_limits_disabled);
>      --blk->public.io_limits_disabled;
> +
> +    if (--blk->quiesce_counter == 0) {
> +        if (blk->dev_ops && blk->dev_ops->drained_end) {
> +              blk->dev_ops->drained_end(blk->dev_opaque);
> +        }
> +    }
>  }
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index 096c17f..7462228 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -58,6 +58,14 @@ typedef struct BlockDevOps {
>       * Runs when the size changed (e.g. monitor command block_resize)
>       */
>      void (*resize_cb)(void *opaque);
> +    /*
> +     * Runs when the backend receives a drain request.
> +     */
> +    void (*drained_begin)(void *opaque);
> +    /*
> +     * Runs when the backend's last drain request ends.
> +     */
> +    void (*drained_end)(void *opaque);
>  } BlockDevOps;
>  
>  /* This struct is embedded in (the private) BlockBackend struct and contains
> -- 
> 2.9.3
>

Reviewed-by: Jeff Cody <jcody@redhat.com>

[Qemu-devel] [PATCH v2 3/3] blockjob: add devops to blockjob backends

[John Snow]

This lets us hook into drained_begin and drained_end requests from the
backend level, which is particularly useful for making sure that all
jobs associated with a particular node (whether the source or the target)
receive a drain request.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockjob.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index 69b4ec6..a11d5ce 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -68,6 +68,23 @@ static const BdrvChildRole child_job = {
     .stay_at_node       = true,
 };
 
+static void block_job_drained_begin(void *opaque)
+{
+    BlockJob *job = opaque;
+    block_job_pause(job);
+}
+
+static void block_job_drained_end(void *opaque)
+{
+    BlockJob *job = opaque;
+    block_job_resume(job);
+}
+
+static const BlockDevOps block_job_dev_ops = {
+    .drained_begin = block_job_drained_begin,
+    .drained_end = block_job_drained_end,
+};
+
 BlockJob *block_job_next(BlockJob *job)
 {
     if (!job) {
@@ -205,11 +222,6 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
     }
 
     job = g_malloc0(driver->instance_size);
-    error_setg(&job->blocker, "block device is in use by block job: %s",
-               BlockJobType_lookup[driver->job_type]);
-    block_job_add_bdrv(job, "main node", bs, 0, BLK_PERM_ALL, &error_abort);
-    bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker);
-
     job->driver        = driver;
     job->id            = g_strdup(job_id);
     job->blk           = blk;
@@ -219,8 +231,15 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
     job->paused        = true;
     job->pause_count   = 1;
     job->refcnt        = 1;
+
+    error_setg(&job->blocker, "block device is in use by block job: %s",
+               BlockJobType_lookup[driver->job_type]);
+    block_job_add_bdrv(job, "main node", bs, 0, BLK_PERM_ALL, &error_abort);
     bs->job = job;
 
+    blk_set_dev_ops(blk, &block_job_dev_ops, job);
+    bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker);
+
     QLIST_INSERT_HEAD(&block_jobs, job, job_list);
 
     blk_add_aio_context_notifier(blk, block_job_attached_aio_context,
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 3/3] blockjob: add devops to blockjob backends

[Jeff Cody]

On Thu, Mar 16, 2017 at 05:23:51PM -0400, John Snow wrote:
> This lets us hook into drained_begin and drained_end requests from the
> backend level, which is particularly useful for making sure that all
> jobs associated with a particular node (whether the source or the target)
> receive a drain request.
> 
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  blockjob.c | 29 ++++++++++++++++++++++++-----
>  1 file changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/blockjob.c b/blockjob.c
> index 69b4ec6..a11d5ce 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -68,6 +68,23 @@ static const BdrvChildRole child_job = {
>      .stay_at_node       = true,
>  };
>  
> +static void block_job_drained_begin(void *opaque)
> +{
> +    BlockJob *job = opaque;
> +    block_job_pause(job);
> +}
> +
> +static void block_job_drained_end(void *opaque)
> +{
> +    BlockJob *job = opaque;
> +    block_job_resume(job);
> +}
> +
> +static const BlockDevOps block_job_dev_ops = {
> +    .drained_begin = block_job_drained_begin,
> +    .drained_end = block_job_drained_end,
> +};
> +
>  BlockJob *block_job_next(BlockJob *job)
>  {
>      if (!job) {
> @@ -205,11 +222,6 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
>      }
>  
>      job = g_malloc0(driver->instance_size);
> -    error_setg(&job->blocker, "block device is in use by block job: %s",
> -               BlockJobType_lookup[driver->job_type]);
> -    block_job_add_bdrv(job, "main node", bs, 0, BLK_PERM_ALL, &error_abort);
> -    bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker);
> -
>      job->driver        = driver;
>      job->id            = g_strdup(job_id);
>      job->blk           = blk;
> @@ -219,8 +231,15 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
>      job->paused        = true;
>      job->pause_count   = 1;
>      job->refcnt        = 1;
> +
> +    error_setg(&job->blocker, "block device is in use by block job: %s",
> +               BlockJobType_lookup[driver->job_type]);
> +    block_job_add_bdrv(job, "main node", bs, 0, BLK_PERM_ALL, &error_abort);
>      bs->job = job;
>  
> +    blk_set_dev_ops(blk, &block_job_dev_ops, job);
> +    bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker);
> +
>      QLIST_INSERT_HEAD(&block_jobs, job, job_list);
>  
>      blk_add_aio_context_notifier(blk, block_job_attached_aio_context,
> -- 
> 2.9.3
>

Reviewed-by: Jeff Cody <jcody@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/3] block: pause block jobs for bdrv_drain_begin/end

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH v2 0/3] block: pause block jobs for bdrv_drain_begin/end
Message-id: 20170316212351.13797-1-jsnow@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20170316212351.13797-1-jsnow@redhat.com -> patchew/20170316212351.13797-1-jsnow@redhat.com
Switched to a new branch 'test'
1cca6f3 blockjob: add devops to blockjob backends
864d906 block-backend: add drained_begin / drained_end ops
5e4f22d blockjob: add block_job_start_shim

=== OUTPUT BEGIN ===
Checking PATCH 1/3: blockjob: add block_job_start_shim...
Checking PATCH 2/3: block-backend: add drained_begin / drained_end ops...
ERROR: suspect code indent for conditional statements (8, 14)
#70: FILE: block/block-backend.c:1903:
+        if (blk->dev_ops && blk->dev_ops->drained_end) {
+              blk->dev_ops->drained_end(blk->dev_opaque);

total: 1 errors, 0 warnings, 67 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/3: blockjob: add devops to blockjob backends...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH v2 0/3] block: pause block jobs for bdrv_drain_begin/end

[John Snow]

ping, is this the only issue? Any feedback? If this can hit 2.9 that
would be good.

--js

On 03/16/2017 05:28 PM, no-reply@patchew.org wrote:
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Type: series
> Subject: [Qemu-devel] [PATCH v2 0/3] block: pause block jobs for bdrv_drain_begin/end
> Message-id: 20170316212351.13797-1-jsnow@redhat.com
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> # Useful git options
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
>     echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>         failed=1
>         echo
>     fi
>     n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
>  * [new tag]         patchew/20170316212351.13797-1-jsnow@redhat.com -> patchew/20170316212351.13797-1-jsnow@redhat.com
> Switched to a new branch 'test'
> 1cca6f3 blockjob: add devops to blockjob backends
> 864d906 block-backend: add drained_begin / drained_end ops
> 5e4f22d blockjob: add block_job_start_shim
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/3: blockjob: add block_job_start_shim...
> Checking PATCH 2/3: block-backend: add drained_begin / drained_end ops...
> ERROR: suspect code indent for conditional statements (8, 14)
> #70: FILE: block/block-backend.c:1903:
> +        if (blk->dev_ops && blk->dev_ops->drained_end) {
> +              blk->dev_ops->drained_end(blk->dev_opaque);
> 
> total: 1 errors, 0 warnings, 67 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> Checking PATCH 3/3: blockjob: add devops to blockjob backends...
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-devel@freelists.org
> 

Re: [Qemu-devel] [PATCH v2 0/3] block: pause block jobs for bdrv_drain_begin/end

[Jeff Cody]

On Wed, Mar 22, 2017 at 11:37:15AM -0400, John Snow wrote:
> ping, is this the only issue? Any feedback? If this can hit 2.9 that
> would be good.
>

The series looks fine to me, and I can patch up the nit from patchew when
applying.  But do you happen to have a qemu-iotest for this case, or is it
not very feasible to create one?

> 
> On 03/16/2017 05:28 PM, no-reply@patchew.org wrote:
> > Hi,
> > 
> > This series seems to have some coding style problems. See output below for
> > more information:
> > 
> > Type: series
> > Subject: [Qemu-devel] [PATCH v2 0/3] block: pause block jobs for bdrv_drain_begin/end
> > Message-id: 20170316212351.13797-1-jsnow@redhat.com
> > 
> > === TEST SCRIPT BEGIN ===
> > #!/bin/bash
> > 
> > BASE=base
> > n=1
> > total=$(git log --oneline $BASE.. | wc -l)
> > failed=0
> > 
> > # Useful git options
> > git config --local diff.renamelimit 0
> > git config --local diff.renames True
> > 
> > commits="$(git log --format=%H --reverse $BASE..)"
> > for c in $commits; do
> >     echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
> >     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
> >         failed=1
> >         echo
> >     fi
> >     n=$((n+1))
> > done
> > 
> > exit $failed
> > === TEST SCRIPT END ===
> > 
> > Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> > From https://github.com/patchew-project/qemu
> >  * [new tag]         patchew/20170316212351.13797-1-jsnow@redhat.com -> patchew/20170316212351.13797-1-jsnow@redhat.com
> > Switched to a new branch 'test'
> > 1cca6f3 blockjob: add devops to blockjob backends
> > 864d906 block-backend: add drained_begin / drained_end ops
> > 5e4f22d blockjob: add block_job_start_shim
> > 
> > === OUTPUT BEGIN ===
> > Checking PATCH 1/3: blockjob: add block_job_start_shim...
> > Checking PATCH 2/3: block-backend: add drained_begin / drained_end ops...
> > ERROR: suspect code indent for conditional statements (8, 14)
> > #70: FILE: block/block-backend.c:1903:
> > +        if (blk->dev_ops && blk->dev_ops->drained_end) {
> > +              blk->dev_ops->drained_end(blk->dev_opaque);
> > 
> > total: 1 errors, 0 warnings, 67 lines checked
> > 
> > Your patch has style problems, please review.  If any of these errors
> > are false positives report them to the maintainer, see
> > CHECKPATCH in MAINTAINERS.
> > 
> > Checking PATCH 3/3: blockjob: add devops to blockjob backends...
> > === OUTPUT END ===
> > 
> > Test command exited with code: 1
> > 
> > 
> > ---
> > Email generated automatically by Patchew [http://patchew.org/].
> > Please send your feedback to patchew-devel@freelists.org
> > 

Re: [Qemu-devel] [PATCH v2 0/3] block: pause block jobs for bdrv_drain_begin/end

[John Snow]

On 03/22/2017 12:01 PM, Jeff Cody wrote:
> On Wed, Mar 22, 2017 at 11:37:15AM -0400, John Snow wrote:
>> ping, is this the only issue? Any feedback? If this can hit 2.9 that
>> would be good.
>>
> 
> The series looks fine to me, and I can patch up the nit from patchew when
> applying.  But do you happen to have a qemu-iotest for this case, or is it
> not very feasible to create one?
> 

I might need a hint from Paolo on how; my reproducer ATM is to literally
boot a Fedora VM and issue reboots/QMP commands and manually observe a
hang. (Which is pretty subjective ...)

An iotest version would probably involve using the qtest socket to issue
a PCI reset of some sort inbetween QMP commands as necessary, but
testing for a hang in iotests seems race-prone. I have no idea how long
this hang would last on other machines, for instance.

There might be a more fool-proof automated testing method, but at the
second I'm drawing a blank.

>>
>> On 03/16/2017 05:28 PM, no-reply@patchew.org wrote:
>>> Hi,
>>>
>>> This series seems to have some coding style problems. See output below for
>>> more information:
>>>
>>> Type: series
>>> Subject: [Qemu-devel] [PATCH v2 0/3] block: pause block jobs for bdrv_drain_begin/end
>>> Message-id: 20170316212351.13797-1-jsnow@redhat.com
>>>
>>> === TEST SCRIPT BEGIN ===
>>> #!/bin/bash
>>>
>>> BASE=base
>>> n=1
>>> total=$(git log --oneline $BASE.. | wc -l)
>>> failed=0
>>>
>>> # Useful git options
>>> git config --local diff.renamelimit 0
>>> git config --local diff.renames True
>>>
>>> commits="$(git log --format=%H --reverse $BASE..)"
>>> for c in $commits; do
>>>     echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>>>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>>>         failed=1
>>>         echo
>>>     fi
>>>     n=$((n+1))
>>> done
>>>
>>> exit $failed
>>> === TEST SCRIPT END ===
>>>
>>> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
>>> From https://github.com/patchew-project/qemu
>>>  * [new tag]         patchew/20170316212351.13797-1-jsnow@redhat.com -> patchew/20170316212351.13797-1-jsnow@redhat.com
>>> Switched to a new branch 'test'
>>> 1cca6f3 blockjob: add devops to blockjob backends
>>> 864d906 block-backend: add drained_begin / drained_end ops
>>> 5e4f22d blockjob: add block_job_start_shim
>>>
>>> === OUTPUT BEGIN ===
>>> Checking PATCH 1/3: blockjob: add block_job_start_shim...
>>> Checking PATCH 2/3: block-backend: add drained_begin / drained_end ops...
>>> ERROR: suspect code indent for conditional statements (8, 14)
>>> #70: FILE: block/block-backend.c:1903:
>>> +        if (blk->dev_ops && blk->dev_ops->drained_end) {
>>> +              blk->dev_ops->drained_end(blk->dev_opaque);
>>>
>>> total: 1 errors, 0 warnings, 67 lines checked
>>>
>>> Your patch has style problems, please review.  If any of these errors
>>> are false positives report them to the maintainer, see
>>> CHECKPATCH in MAINTAINERS.
>>>
>>> Checking PATCH 3/3: blockjob: add devops to blockjob backends...
>>> === OUTPUT END ===
>>>
>>> Test command exited with code: 1
>>>
>>>
>>> ---
>>> Email generated automatically by Patchew [http://patchew.org/].
>>> Please send your feedback to patchew-devel@freelists.org
>>>

Re: [Qemu-devel] [PATCH v2 0/3] block: pause block jobs for bdrv_drain_begin/end

[Jeff Cody]

On Wed, Mar 22, 2017 at 12:05:26PM -0400, John Snow wrote:
> 
> 
> On 03/22/2017 12:01 PM, Jeff Cody wrote:
> > On Wed, Mar 22, 2017 at 11:37:15AM -0400, John Snow wrote:
> >> ping, is this the only issue? Any feedback? If this can hit 2.9 that
> >> would be good.
> >>
> > 
> > The series looks fine to me, and I can patch up the nit from patchew when
> > applying.  But do you happen to have a qemu-iotest for this case, or is it
> > not very feasible to create one?
> > 
> 
> I might need a hint from Paolo on how; my reproducer ATM is to literally
> boot a Fedora VM and issue reboots/QMP commands and manually observe a
> hang. (Which is pretty subjective ...)
> 
> An iotest version would probably involve using the qtest socket to issue
> a PCI reset of some sort inbetween QMP commands as necessary, but
> testing for a hang in iotests seems race-prone. I have no idea how long
> this hang would last on other machines, for instance.
> 
> There might be a more fool-proof automated testing method, but at the
> second I'm drawing a blank.
>

OK, I figured as much.  We'll chalk that up as "not very feasible", at least
for the moment :)


> >>
> >> On 03/16/2017 05:28 PM, no-reply@patchew.org wrote:
> >>> Hi,
> >>>
> >>> This series seems to have some coding style problems. See output below for
> >>> more information:
> >>>
> >>> Type: series
> >>> Subject: [Qemu-devel] [PATCH v2 0/3] block: pause block jobs for bdrv_drain_begin/end
> >>> Message-id: 20170316212351.13797-1-jsnow@redhat.com
> >>>
> >>> === TEST SCRIPT BEGIN ===
> >>> #!/bin/bash
> >>>
> >>> BASE=base
> >>> n=1
> >>> total=$(git log --oneline $BASE.. | wc -l)
> >>> failed=0
> >>>
> >>> # Useful git options
> >>> git config --local diff.renamelimit 0
> >>> git config --local diff.renames True
> >>>
> >>> commits="$(git log --format=%H --reverse $BASE..)"
> >>> for c in $commits; do
> >>>     echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
> >>>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
> >>>         failed=1
> >>>         echo
> >>>     fi
> >>>     n=$((n+1))
> >>> done
> >>>
> >>> exit $failed
> >>> === TEST SCRIPT END ===
> >>>
> >>> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> >>> From https://github.com/patchew-project/qemu
> >>>  * [new tag]         patchew/20170316212351.13797-1-jsnow@redhat.com -> patchew/20170316212351.13797-1-jsnow@redhat.com
> >>> Switched to a new branch 'test'
> >>> 1cca6f3 blockjob: add devops to blockjob backends
> >>> 864d906 block-backend: add drained_begin / drained_end ops
> >>> 5e4f22d blockjob: add block_job_start_shim
> >>>
> >>> === OUTPUT BEGIN ===
> >>> Checking PATCH 1/3: blockjob: add block_job_start_shim...
> >>> Checking PATCH 2/3: block-backend: add drained_begin / drained_end ops...
> >>> ERROR: suspect code indent for conditional statements (8, 14)
> >>> #70: FILE: block/block-backend.c:1903:
> >>> +        if (blk->dev_ops && blk->dev_ops->drained_end) {
> >>> +              blk->dev_ops->drained_end(blk->dev_opaque);
> >>>
> >>> total: 1 errors, 0 warnings, 67 lines checked
> >>>
> >>> Your patch has style problems, please review.  If any of these errors
> >>> are false positives report them to the maintainer, see
> >>> CHECKPATCH in MAINTAINERS.
> >>>
> >>> Checking PATCH 3/3: blockjob: add devops to blockjob backends...
> >>> === OUTPUT END ===
> >>>
> >>> Test command exited with code: 1
> >>>
> >>>
> >>> ---
> >>> Email generated automatically by Patchew [http://patchew.org/].
> >>> Please send your feedback to patchew-devel@freelists.org
> >>>

Re: [Qemu-devel] [PATCH v2 0/3] block: pause block jobs for bdrv_drain_begin/end

[Jeff Cody]

On Thu, Mar 16, 2017 at 02:28:47PM -0700, no-reply@patchew.org wrote:
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Type: series
> Subject: [Qemu-devel] [PATCH v2 0/3] block: pause block jobs for bdrv_drain_begin/end
> Message-id: 20170316212351.13797-1-jsnow@redhat.com
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> # Useful git options
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
>     echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>         failed=1
>         echo
>     fi
>     n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
>  * [new tag]         patchew/20170316212351.13797-1-jsnow@redhat.com -> patchew/20170316212351.13797-1-jsnow@redhat.com
> Switched to a new branch 'test'
> 1cca6f3 blockjob: add devops to blockjob backends
> 864d906 block-backend: add drained_begin / drained_end ops
> 5e4f22d blockjob: add block_job_start_shim
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/3: blockjob: add block_job_start_shim...
> Checking PATCH 2/3: block-backend: add drained_begin / drained_end ops...
> ERROR: suspect code indent for conditional statements (8, 14)
> #70: FILE: block/block-backend.c:1903:
> +        if (blk->dev_ops && blk->dev_ops->drained_end) {
> +              blk->dev_ops->drained_end(blk->dev_opaque);
> 
> total: 1 errors, 0 warnings, 67 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> Checking PATCH 3/3: blockjob: add devops to blockjob backends...
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-devel@freelists.org


Fixed the patchew nit, and:

Thanks,

Applied to my block branch:

git://github.com/codyprime/qemu-kvm-jtc.git block

-Jeff

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: pause block jobs for bdrv_drain_begin/end

[Stefan Hajnoczi]

On Thu, Mar 16, 2017 at 9:23 PM, John Snow <jsnow@redhat.com> wrote:
> Reference: https://bugzilla.redhat.com/show_bug.cgi?id=1367369#c8
>
> It's possible to wedge QEMU if the guest tries to reset a virtio-pci
> device as QEMU is also using the drive for a blockjob. This patchset
> aims to allow us to safely pause/resume jobs attached to individual
> nodes in a manner similar to how bdrv_drain_all_begin/end do.

Weird, I thought the 0 nanosecond sleep that block jobs do in their
main loop allows aio_poll() loops to finish.

Maybe this bug is a regression?

I've CCed Fam because I think he has also tackled this issue in the past.

Stefan

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: pause block jobs for bdrv_drain_begin/end

[Paolo Bonzini]

On 23/03/2017 18:44, Stefan Hajnoczi wrote:
>> It's possible to wedge QEMU if the guest tries to reset a virtio-pci
>> device as QEMU is also using the drive for a blockjob. This patchset
>> aims to allow us to safely pause/resume jobs attached to individual
>> nodes in a manner similar to how bdrv_drain_all_begin/end do.
> 
> Weird, I thought the 0 nanosecond sleep that block jobs do in their
> main loop allows aio_poll() loops to finish.

The 0 nanosecond sleep is now done in the BDS AioContext rather than in 
the "non-aio_poll-aware" main loop:

    commit 0b9caf9b3166c8deb3c4f3a774c2384b069dc29c
    Author: Fam Zheng <famz@redhat.com>
    Date:   Tue Aug 26 15:15:43 2014 +0800

    coroutine: Drop co_sleep_ns
    
    block_job_sleep_ns is the only user. Since we are moving towards
    AioContext aware code, it's better to use the explicit version and drop
    the old one.
    
    Signed-off-by: Fam Zheng <famz@redhat.com>
    Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
    Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

> I've CCed Fam because I think he has also tackled this issue in the past.

Yes, he fixed the same issue for bdrv_drain_all.

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: pause block jobs for bdrv_drain_begin/end

[Stefan Hajnoczi]

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

On Thu, Mar 23, 2017 at 06:57:14PM +0100, Paolo Bonzini wrote:
> 
> 
> On 23/03/2017 18:44, Stefan Hajnoczi wrote:
> >> It's possible to wedge QEMU if the guest tries to reset a virtio-pci
> >> device as QEMU is also using the drive for a blockjob. This patchset
> >> aims to allow us to safely pause/resume jobs attached to individual
> >> nodes in a manner similar to how bdrv_drain_all_begin/end do.
> > 
> > Weird, I thought the 0 nanosecond sleep that block jobs do in their
> > main loop allows aio_poll() loops to finish.
> 
> The 0 nanosecond sleep is now done in the BDS AioContext rather than in 
> the "non-aio_poll-aware" main loop:
> 
>     commit 0b9caf9b3166c8deb3c4f3a774c2384b069dc29c
>     Author: Fam Zheng <famz@redhat.com>
>     Date:   Tue Aug 26 15:15:43 2014 +0800
> 
>     coroutine: Drop co_sleep_ns
>     
>     block_job_sleep_ns is the only user. Since we are moving towards
>     AioContext aware code, it's better to use the explicit version and drop
>     the old one.
>     
>     Signed-off-by: Fam Zheng <famz@redhat.com>
>     Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
>     Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

But we hold the AioContext lock and are calling aio_poll(), so I would
expect our loop to terminate.  The blockjob coroutine should still be
leaving this little gap in activity during which the aio_poll() loop
finishes.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: pause block jobs for bdrv_drain_begin/end

[Fam Zheng]

On Fri, 03/24 15:27, Stefan Hajnoczi wrote:
> On Thu, Mar 23, 2017 at 06:57:14PM +0100, Paolo Bonzini wrote:
> > 
> > 
> > On 23/03/2017 18:44, Stefan Hajnoczi wrote:
> > >> It's possible to wedge QEMU if the guest tries to reset a virtio-pci
> > >> device as QEMU is also using the drive for a blockjob. This patchset
> > >> aims to allow us to safely pause/resume jobs attached to individual
> > >> nodes in a manner similar to how bdrv_drain_all_begin/end do.
> > > 
> > > Weird, I thought the 0 nanosecond sleep that block jobs do in their
> > > main loop allows aio_poll() loops to finish.
> > 
> > The 0 nanosecond sleep is now done in the BDS AioContext rather than in 
> > the "non-aio_poll-aware" main loop:
> > 
> >     commit 0b9caf9b3166c8deb3c4f3a774c2384b069dc29c
> >     Author: Fam Zheng <famz@redhat.com>
> >     Date:   Tue Aug 26 15:15:43 2014 +0800
> > 
> >     coroutine: Drop co_sleep_ns
> >     
> >     block_job_sleep_ns is the only user. Since we are moving towards
> >     AioContext aware code, it's better to use the explicit version and drop
> >     the old one.
> >     
> >     Signed-off-by: Fam Zheng <famz@redhat.com>
> >     Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
> >     Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> But we hold the AioContext lock and are calling aio_poll(), so I would
> expect our loop to terminate.  The blockjob coroutine should still be
> leaving this little gap in activity during which the aio_poll() loop
> finishes.

I am not sure, but at each gap, aio_poll() is free to fire the 0 nanosecond
sleep timer cb already, which will generate more I/O. The correct thing is, like
in this series, set the pause flag.

Fam