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>

--

RFC topics:

- BlkDevOps is traditionally only for Qdev devices, and a BlockJob is not
  currently a 'device'... Do we want to loosen this restriction, find another
  way to deliver callbacks to BlockJobs attached to BlkBackends, or do something
  crazy like make a BlockJob device object?

  struct JobDevice {
    DeviceState parent_obj;
    BlockJob *job;
  } ...??

- Not so sure about leaving the initial job refcount at 1, since we are giving
  away a handle to this job as dev_opaque. By incrementing it to 2, however,
  it's not clear whose responsibility it is to decrement it again.

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

diff --git a/blockjob.c b/blockjob.c
index 6d08ce5..0542677 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,20 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
     job->paused        = true;
     job->pause_count   = 1;
     job->refcnt        = 1;
+    /* RFC: job is now stored both in bs->job and blk->dev_opaque,
+     * but we cannot expect the backend to know how to put down the ref
+     * so it'd be up to this code here to increment it anyway, making it
+     * a bit of a useless exercise. It would be up to us to ensure that
+     * we destruct the blkbackend before putting down our last reference. */
+
+    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

On 16/03/2017 01:46, John Snow wrote:
> - BlkDevOps is traditionally only for Qdev devices, and a BlockJob is not
>   currently a 'device'... Do we want to loosen this restriction, find another
>   way to deliver callbacks to BlockJobs attached to BlkBackends, or do something
>   crazy like make a BlockJob device object?
> 
>   struct JobDevice {
>     DeviceState parent_obj;
>     BlockJob *job;
>   } ...??

I think we want to loosen it.

> - Not so sure about leaving the initial job refcount at 1, since we are giving
>   away a handle to this job as dev_opaque. By incrementing it to 2, however,
>   it's not clear whose responsibility it is to decrement it again.

It would be another callback, sent at the time the dev_ops are removed.
But devices have been treating dev_opaque as a weak reference, it makes
sense to do the same for jobs.

Paolo

Am 16.03.2017 um 01:46 hat John Snow geschrieben:
> 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>
> 
> --
> 
> RFC topics:
> 
> - BlkDevOps is traditionally only for Qdev devices, and a BlockJob is not
>   currently a 'device'... Do we want to loosen this restriction, find another
>   way to deliver callbacks to BlockJobs attached to BlkBackends, or do something
>   crazy like make a BlockJob device object?
> 
>   struct JobDevice {
>     DeviceState parent_obj;
>     BlockJob *job;
>   } ...??

We should probably rename BlkDevOps to something that works not only for
devices, but for any user of a BlockBackend. I don't think the
implementation has to be changed, it's just a struct of callbacks and an
opaque pointer that is actually properly treated as opaque.

BlockBackend also has a dev field, which is indeed supposed to be a
DeviceState and is sometimes casted to one (if we can assert
!blk->legacy_dev), but it's a concept completely separate from
BlkDevOps. So we just need to be sure not to call blk_attach_dev() or
blk_attach_dev_legacy().

Kevin