Re: [Qemu-devel] [PATCH 04/10] blockjob: introduce block_job_pause/resume_all

2017-04-10 Thread Stefan Hajnoczi 
On Thu, Mar 23, 2017 at 06:39:22PM +0100, Paolo Bonzini wrote:
> Remove use of block_job_pause/resume from outside blockjob.c, thus
> making them static.  Again, one reason to do this is that
> block_job_pause/resume will have different locking rules compared
> to everything else that block.c and block/io.c use.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  block/io.c   |  18 +---
>  blockjob.c   | 114 
> ---
>  include/block/blockjob.h |  14 +++---
>  3 files changed, 77 insertions(+), 69 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 04/10] blockjob: introduce block_job_pause/resume_all

2017-04-07 Thread John Snow 


On 03/23/2017 01:39 PM, Paolo Bonzini wrote:
> Remove use of block_job_pause/resume from outside blockjob.c, thus
> making them static.  Again, one reason to do this is that
> block_job_pause/resume will have different locking rules compared
> to everything else that block.c and block/io.c use.
> 

Ominous

> Signed-off-by: Paolo Bonzini 
> ---
>  block/io.c   |  18 +---
>  blockjob.c   | 114 
> ---
>  include/block/blockjob.h |  14 +++---
>  3 files changed, 77 insertions(+), 69 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 2709a70..1cc9318 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -284,16 +284,9 @@ void bdrv_drain_all_begin(void)
>  bool waited = true;
>  BlockDriverState *bs;
>  BdrvNextIterator it;
> -BlockJob *job = NULL;
>  GSList *aio_ctxs = NULL, *ctx;
>  
> -while ((job = block_job_next(job))) {
> -AioContext *aio_context = blk_get_aio_context(job->blk);
> -
> -aio_context_acquire(aio_context);
> -block_job_pause(job);
> -aio_context_release(aio_context);
> -}
> +block_job_pause_all();
>  

Cool. Well, maybe cool. I think it would be cool if we didn't have to
explicitly pause jobs here at all. This is better at least, though.

>  for (bs = bdrv_first(); bs; bs = bdrv_next()) {
>  AioContext *aio_context = bdrv_get_aio_context(bs);
> @@ -337,7 +330,6 @@ void bdrv_drain_all_end(void)
>  {
>  BlockDriverState *bs;
>  BdrvNextIterator it;
> -BlockJob *job = NULL;
>  
>  for (bs = bdrv_first(); bs; bs = bdrv_next()) {
>  AioContext *aio_context = bdrv_get_aio_context(bs);
> @@ -348,13 +340,7 @@ void bdrv_drain_all_end(void)
>  aio_context_release(aio_context);
>  }
>  
> -while ((job = block_job_next(job))) {
> -AioContext *aio_context = blk_get_aio_context(job->blk);
> -
> -aio_context_acquire(aio_context);
> -block_job_resume(job);
> -aio_context_release(aio_context);
> -}
> +block_job_resume_all();
>  }
>  
>  void bdrv_drain_all(void)
> diff --git a/blockjob.c b/blockjob.c
> index 1c63d15..caca718 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -55,36 +55,6 @@ struct BlockJobTxn {
>  
>  static QLIST_HEAD(, BlockJob) block_jobs = 
> QLIST_HEAD_INITIALIZER(block_jobs);
>  
> -static char *child_job_get_parent_desc(BdrvChild *c)
> -{
> -BlockJob *job = c->opaque;
> -return g_strdup_printf("%s job '%s'",
> -   BlockJobType_lookup[job->driver->job_type],
> -   job->id);
> -}
> -
> -static const BdrvChildRole child_job = {
> -.get_parent_desc= child_job_get_parent_desc,
> -.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) {
> @@ -106,6 +76,21 @@ BlockJob *block_job_get(const char *id)
>  return NULL;
>  }
>  
> +static void block_job_pause(BlockJob *job)
> +{
> +job->pause_count++;
> +}
> +
> +static void block_job_resume(BlockJob *job)
> +{
> +assert(job->pause_count > 0);
> +job->pause_count--;
> +if (job->pause_count) {
> +return;
> +}
> +block_job_enter(job);
> +}
> +
>  static void block_job_ref(BlockJob *job)
>  {
>  ++job->refcnt;
> @@ -171,6 +156,36 @@ static void block_job_detach_aio_context(void *opaque)
>  block_job_unref(job);
>  }
>  
> +static char *child_job_get_parent_desc(BdrvChild *c)
> +{
> +BlockJob *job = c->opaque;
> +return g_strdup_printf("%s job '%s'",
> +   BlockJobType_lookup[job->driver->job_type],
> +   job->id);
> +}
> +
> +static const BdrvChildRole child_job = {
> +.get_parent_desc= child_job_get_parent_desc,
> +.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,
> +};
> +

Movement should probably be its own patch, but I'm not a cop or nothin'

>  void block_job_remove_all_bdrv(BlockJob *job)
>  {
>  GSList *l;
> @@ -471,11 +486,6 @@ void block_job_complete(BlockJob *job, Error **errp)
>  job->driver->complete(job, errp);
>  }
>  
> -void block_job_pause(BlockJob *job)
> -{
> -

[Qemu-devel] [PATCH 04/10] blockjob: introduce block_job_pause/resume_all

2017-03-23 Thread Paolo Bonzini 
Remove use of block_job_pause/resume from outside blockjob.c, thus
making them static.  Again, one reason to do this is that
block_job_pause/resume will have different locking rules compared
to everything else that block.c and block/io.c use.

Signed-off-by: Paolo Bonzini 
---
 block/io.c   |  18 +---
 blockjob.c   | 114 ---
 include/block/blockjob.h |  14 +++---
 3 files changed, 77 insertions(+), 69 deletions(-)

diff --git a/block/io.c b/block/io.c
index 2709a70..1cc9318 100644
--- a/block/io.c
+++ b/block/io.c
@@ -284,16 +284,9 @@ void bdrv_drain_all_begin(void)
 bool waited = true;
 BlockDriverState *bs;
 BdrvNextIterator it;
-BlockJob *job = NULL;
 GSList *aio_ctxs = NULL, *ctx;
 
-while ((job = block_job_next(job))) {
-AioContext *aio_context = blk_get_aio_context(job->blk);
-
-aio_context_acquire(aio_context);
-block_job_pause(job);
-aio_context_release(aio_context);
-}
+block_job_pause_all();
 
 for (bs = bdrv_first(); bs; bs = bdrv_next()) {
 AioContext *aio_context = bdrv_get_aio_context(bs);
@@ -337,7 +330,6 @@ void bdrv_drain_all_end(void)
 {
 BlockDriverState *bs;
 BdrvNextIterator it;
-BlockJob *job = NULL;
 
 for (bs = bdrv_first(); bs; bs = bdrv_next()) {
 AioContext *aio_context = bdrv_get_aio_context(bs);
@@ -348,13 +340,7 @@ void bdrv_drain_all_end(void)
 aio_context_release(aio_context);
 }
 
-while ((job = block_job_next(job))) {
-AioContext *aio_context = blk_get_aio_context(job->blk);
-
-aio_context_acquire(aio_context);
-block_job_resume(job);
-aio_context_release(aio_context);
-}
+block_job_resume_all();
 }
 
 void bdrv_drain_all(void)
diff --git a/blockjob.c b/blockjob.c
index 1c63d15..caca718 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -55,36 +55,6 @@ struct BlockJobTxn {
 
 static QLIST_HEAD(, BlockJob) block_jobs = QLIST_HEAD_INITIALIZER(block_jobs);
 
-static char *child_job_get_parent_desc(BdrvChild *c)
-{
-BlockJob *job = c->opaque;
-return g_strdup_printf("%s job '%s'",
-   BlockJobType_lookup[job->driver->job_type],
-   job->id);
-}
-
-static const BdrvChildRole child_job = {
-.get_parent_desc= child_job_get_parent_desc,
-.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) {
@@ -106,6 +76,21 @@ BlockJob *block_job_get(const char *id)
 return NULL;
 }
 
+static void block_job_pause(BlockJob *job)
+{
+job->pause_count++;
+}
+
+static void block_job_resume(BlockJob *job)
+{
+assert(job->pause_count > 0);
+job->pause_count--;
+if (job->pause_count) {
+return;
+}
+block_job_enter(job);
+}
+
 static void block_job_ref(BlockJob *job)
 {
 ++job->refcnt;
@@ -171,6 +156,36 @@ static void block_job_detach_aio_context(void *opaque)
 block_job_unref(job);
 }
 
+static char *child_job_get_parent_desc(BdrvChild *c)
+{
+BlockJob *job = c->opaque;
+return g_strdup_printf("%s job '%s'",
+   BlockJobType_lookup[job->driver->job_type],
+   job->id);
+}
+
+static const BdrvChildRole child_job = {
+.get_parent_desc= child_job_get_parent_desc,
+.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,
+};
+
 void block_job_remove_all_bdrv(BlockJob *job)
 {
 GSList *l;
@@ -471,11 +486,6 @@ void block_job_complete(BlockJob *job, Error **errp)
 job->driver->complete(job, errp);
 }
 
-void block_job_pause(BlockJob *job)
-{
-job->pause_count++;
-}
-
 void block_job_user_pause(BlockJob *job)
 {
 job->user_paused = true;
@@ -520,16 +530,6 @@ void coroutine_fn block_job_pause_point(BlockJob *job)
 }
 }
 
-void block_job_resume(BlockJob *job)
-{
-assert(job->pause_count > 0);
-job->pause_count--;
-if (job->pause_count) {
-return;
-}
-block_job_enter(job);
-}
-
 void block_job_user_resume(BlockJob *job)
 {
 if (job && job->user_paused && job->pause_count > 0) {
@@ -723,6 +723,30 @@ static void block_job_event_completed(BlockJob *job, const 
char *msg)