Re: [Qemu-devel] [PATCH v3] job: drop job_drain

2019-09-02 Thread Kevin Wolf
Am 29.08.2019 um 11:09 hat Vladimir Sementsov-Ogievskiy geschrieben:
> In job_finish_sync job_enter should be enough for a job to make some
> progress and draining is a wrong tool for it. So use job_enter directly
> here and drop job_drain with all related staff not used more.
> 
> Suggested-by: Kevin Wolf 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Tested-by: John Snow 
> Reviewed-by: John Snow 

Thanks, applied to the block branch.

Kevin



[Qemu-devel] [PATCH v3] job: drop job_drain

2019-08-29 Thread Vladimir Sementsov-Ogievskiy
In job_finish_sync job_enter should be enough for a job to make some
progress and draining is a wrong tool for it. So use job_enter directly
here and drop job_drain with all related staff not used more.

Suggested-by: Kevin Wolf 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
Tested-by: John Snow 
Reviewed-by: John Snow 
---

v3: rebase on master
drop drain from test_simple_job_driver too
add John's r-b and t-b

 include/block/blockjob_int.h | 19 ---
 include/qemu/job.h   | 13 -
 block/backup.c   | 19 +--
 block/commit.c   |  1 -
 block/mirror.c   | 28 +++-
 block/stream.c   |  1 -
 blockjob.c   | 13 -
 job.c| 12 +---
 tests/test-bdrv-drain.c  |  3 ---
 tests/test-block-iothread.c  |  1 -
 tests/test-blockjob-txn.c|  1 -
 tests/test-blockjob.c|  2 --
 12 files changed, 5 insertions(+), 108 deletions(-)

diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index e4a318dd15..e2824a36a8 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -52,17 +52,6 @@ struct BlockJobDriver {
  * besides job->blk to the new AioContext.
  */
 void (*attached_aio_context)(BlockJob *job, AioContext *new_context);
-
-/*
- * If the callback is not NULL, it will be invoked when the job has to be
- * synchronously cancelled or completed; it should drain BlockDriverStates
- * as required to ensure progress.
- *
- * Block jobs must use the default implementation for job_driver.drain,
- * which will in turn call this callback after doing generic block job
- * stuff.
- */
-void (*drain)(BlockJob *job);
 };
 
 /**
@@ -107,14 +96,6 @@ void block_job_free(Job *job);
  */
 void block_job_user_resume(Job *job);
 
-/**
- * block_job_drain:
- * Callback to be used for JobDriver.drain in all block jobs. Drains the main
- * block node associated with the block jobs and calls BlockJobDriver.drain for
- * job-specific actions.
- */
-void block_job_drain(Job *job);
-
 /**
  * block_job_ratelimit_get_delay:
  *
diff --git a/include/qemu/job.h b/include/qemu/job.h
index 73c67d3175..bd59cd8944 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -220,13 +220,6 @@ struct JobDriver {
  */
 void (*complete)(Job *job, Error **errp);
 
-/*
- * If the callback is not NULL, it will be invoked when the job has to be
- * synchronously cancelled or completed; it should drain any activities
- * as required to ensure progress.
- */
-void (*drain)(Job *job);
-
 /**
  * If the callback is not NULL, prepare will be invoked when all the jobs
  * belonging to the same transaction complete; or upon this job's 
completion
@@ -470,12 +463,6 @@ bool job_user_paused(Job *job);
  */
 void job_user_resume(Job *job, Error **errp);
 
-/*
- * Drain any activities as required to ensure progress. This can be called in a
- * loop to synchronously complete a job.
- */
-void job_drain(Job *job);
-
 /**
  * Get the next element from the list of block jobs after @job, or the
  * first one if @job is %NULL.
diff --git a/block/backup.c b/block/backup.c
index 2baf7bed65..2a81ed3d74 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -425,21 +425,6 @@ void backup_do_checkpoint(BlockJob *job, Error **errp)
 bdrv_set_dirty_bitmap(backup_job->copy_bitmap, 0, backup_job->len);
 }
 
-static void backup_drain(BlockJob *job)
-{
-BackupBlockJob *s = container_of(job, BackupBlockJob, common);
-
-/* Need to keep a reference in case blk_drain triggers execution
- * of backup_complete...
- */
-if (s->target) {
-BlockBackend *target = s->target;
-blk_ref(target);
-blk_drain(target);
-blk_unref(target);
-}
-}
-
 static BlockErrorAction backup_error_action(BackupBlockJob *job,
 bool read, int error)
 {
@@ -588,13 +573,11 @@ static const BlockJobDriver backup_job_driver = {
 .job_type   = JOB_TYPE_BACKUP,
 .free   = block_job_free,
 .user_resume= block_job_user_resume,
-.drain  = block_job_drain,
 .run= backup_run,
 .commit = backup_commit,
 .abort  = backup_abort,
 .clean  = backup_clean,
-},
-.drain  = backup_drain,
+}
 };
 
 static int64_t backup_calculate_cluster_size(BlockDriverState *target,
diff --git a/block/commit.c b/block/commit.c
index 408ae15389..bc8454463d 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -216,7 +216,6 @@ static const BlockJobDriver commit_job_driver = {
 .job_type  = JOB_TYPE_COMMIT,
 .free  = block_job_free,
 .user_resume   = block_job_user_resume,
-.drain