[PATCH RFC 0/3] mirror: rework soft-cancelling READY mirror

[PATCH RFC 0/3] mirror: rework soft-cancelling READY mirror

[Vladimir Sementsov-Ogievskiy]

Hi all!

That's an alternative to (part of) Max's
"[PATCH for-6.1? v2 0/7] mirror: Handle errors after READY cancel"
and shows' my idea of handling soft-cancelling READY mirror case
directly in qmp_block_job_cancel. And cleanup all other job cancelling
functions.

That's untested draft, don't take it to heart :)

Vladimir Sementsov-Ogievskiy (3):
  job: add job_complete_ex with do_graph_change argument
  job: use complete(do_graph_change=false) to handle soft cancel
  job: drop force argument of *job*cancel

 include/qemu/job.h               | 19 ++++++++----------
 block/backup.c                   |  2 +-
 block/mirror.c                   | 33 +++++++++++++++++---------------
 blockdev.c                       | 13 +++++++++++--
 job-qmp.c                        |  2 +-
 job.c                            | 27 ++++++++++++++------------
 tests/unit/test-bdrv-drain.c     |  2 +-
 tests/unit/test-block-iothread.c |  2 +-
 tests/unit/test-blockjob-txn.c   |  8 ++++----
 tests/unit/test-blockjob.c       |  4 ++--
 10 files changed, 62 insertions(+), 50 deletions(-)

-- 
2.29.2

[PATCH 1/3] job: add job_complete_ex with do_graph_change argument

[Vladimir Sementsov-Ogievskiy]

It's an alternative for soft-cancelling mirror job after READY: mirror
should finish all in-flight requests, but don't change block graph in
any way.

To be used in the next commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/qemu/job.h               |  5 ++++-
 block/mirror.c                   | 20 ++++++++++++++------
 job.c                            |  9 +++++++--
 tests/unit/test-bdrv-drain.c     |  2 +-
 tests/unit/test-block-iothread.c |  2 +-
 tests/unit/test-blockjob.c       |  4 ++--
 6 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/include/qemu/job.h b/include/qemu/job.h
index 41162ed494..3dfb79cee6 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -211,7 +211,7 @@ struct JobDriver {
      * Optional callback for job types whose completion must be triggered
      * manually.
      */
-    void (*complete)(Job *job, Error **errp);
+    void (*complete)(Job *job, bool do_graph_change, Error **errp);
 
     /**
      * If the callback is not NULL, prepare will be invoked when all the jobs
@@ -492,6 +492,9 @@ void job_transition_to_ready(Job *job);
 /** Asynchronously complete the specified @job. */
 void job_complete(Job *job, Error **errp);
 
+/** Asynchronously complete the specified @job. */
+void job_complete_ex(Job *job, bool do_graph_change, Error **errp);
+
 /**
  * Asynchronously cancel the specified @job. If @force is true, the job should
  * be cancelled immediately without waiting for a consistent state.
diff --git a/block/mirror.c b/block/mirror.c
index 98fc66eabf..ad9736eb5e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -44,6 +44,11 @@ typedef struct MirrorBlockJob {
     BlockDriverState *base;
     BlockDriverState *base_overlay;
 
+    /*
+     * Do final graph changes. True at start, may be changed by
+     * mirror_complete().
+     */
+    bool do_graph_change;
     /* The name of the graph node to replace */
     char *replaces;
     /* The BDS to replace */
@@ -648,7 +653,7 @@ static int mirror_exit_common(Job *job)
     BlockDriverState *target_bs;
     BlockDriverState *mirror_top_bs;
     Error *local_err = NULL;
-    bool abort = job->ret < 0;
+    bool do_graph_change = s->do_graph_change && job->ret >= 0;
     int ret = 0;
 
     if (s->prepared) {
@@ -689,7 +694,7 @@ static int mirror_exit_common(Job *job)
     bs_opaque->stop = true;
     bdrv_child_refresh_perms(mirror_top_bs, mirror_top_bs->backing,
                              &error_abort);
-    if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
+    if (do_graph_change && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
         BlockDriverState *backing = s->is_none_mode ? src : s->base;
         BlockDriverState *unfiltered_target = bdrv_skip_filters(target_bs);
 
@@ -701,7 +706,7 @@ static int mirror_exit_common(Job *job)
                 ret = -EPERM;
             }
         }
-    } else if (!abort && s->backing_mode == MIRROR_OPEN_BACKING_CHAIN) {
+    } else if (do_graph_change && s->backing_mode == MIRROR_OPEN_BACKING_CHAIN) {
         assert(!bdrv_backing_chain_next(target_bs));
         ret = bdrv_open_backing_file(bdrv_skip_filters(target_bs), NULL,
                                      "backing", &local_err);
@@ -716,7 +721,7 @@ static int mirror_exit_common(Job *job)
         aio_context_acquire(replace_aio_context);
     }
 
-    if (s->should_complete && !abort) {
+    if (s->should_complete && do_graph_change) {
         BlockDriverState *to_replace = s->to_replace ?: src;
         bool ro = bdrv_is_read_only(to_replace);
 
@@ -1124,7 +1129,7 @@ immediate_exit:
     return ret;
 }
 
-static void mirror_complete(Job *job, Error **errp)
+static void mirror_complete(Job *job, bool do_graph_change, Error **errp)
 {
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
 
@@ -1134,8 +1139,10 @@ static void mirror_complete(Job *job, Error **errp)
         return;
     }
 
+    s->do_graph_change = do_graph_change;
+
     /* block all operations on to_replace bs */
-    if (s->replaces) {
+    if (s->do_graph_change && s->replaces) {
         AioContext *replace_aio_context;
 
         s->to_replace = bdrv_find_node(s->replaces);
@@ -1737,6 +1744,7 @@ static BlockJob *mirror_start_job(
     blk_set_allow_aio_context_change(s->target, true);
     blk_set_disable_request_queuing(s->target, true);
 
+    s->do_graph_change = true;
     s->replaces = g_strdup(replaces);
     s->on_source_error = on_source_error;
     s->on_target_error = on_target_error;
diff --git a/job.c b/job.c
index e7a5d28854..52127dd6bd 100644
--- a/job.c
+++ b/job.c
@@ -987,7 +987,7 @@ int job_complete_sync(Job *job, Error **errp)
     return job_finish_sync(job, job_complete, errp);
 }
 
-void job_complete(Job *job, Error **errp)
+void job_complete_ex(Job *job, bool do_graph_change, Error **errp)
 {
     /* Should not be reachable via external interface for internal jobs */
     assert(job->id);
@@ -1000,7 +1000,12 @@ void job_complete(Job *job, Error **errp)
         return;
     }
 
-    job->driver->complete(job, errp);
+    job->driver->complete(job, true, errp);
+}
+
+void job_complete(Job *job, Error **errp)
+{
+    job_complete_ex(job, false, errp);
 }
 
 int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp)
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index ce071b5fc5..b754eca27b 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -823,7 +823,7 @@ static int coroutine_fn test_job_run(Job *job, Error **errp)
     return s->run_ret;
 }
 
-static void test_job_complete(Job *job, Error **errp)
+static void test_job_complete(Job *job, bool do_graph_change, Error **errp)
 {
     TestBlockJob *s = container_of(job, TestBlockJob, common.job);
     s->should_complete = true;
diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c
index c39e70b2f5..d07ba69aee 100644
--- a/tests/unit/test-block-iothread.c
+++ b/tests/unit/test-block-iothread.c
@@ -385,7 +385,7 @@ static int coroutine_fn test_job_run(Job *job, Error **errp)
     return 0;
 }
 
-static void test_job_complete(Job *job, Error **errp)
+static void test_job_complete(Job *job, bool do_graph_change, Error **errp)
 {
     TestBlockJob *s = container_of(job, TestBlockJob, common.job);
     s->should_complete = true;
diff --git a/tests/unit/test-blockjob.c b/tests/unit/test-blockjob.c
index dcacfa6c7c..b2653a3733 100644
--- a/tests/unit/test-blockjob.c
+++ b/tests/unit/test-blockjob.c
@@ -165,7 +165,7 @@ typedef struct CancelJob {
     bool should_complete;
 } CancelJob;
 
-static void cancel_job_complete(Job *job, Error **errp)
+static void cancel_job_complete(Job *job, bool do_graph_change, Error **errp)
 {
     CancelJob *s = container_of(job, CancelJob, common.job);
     s->should_complete = true;
@@ -382,7 +382,7 @@ typedef struct YieldingJob {
     bool should_complete;
 } YieldingJob;
 
-static void yielding_job_complete(Job *job, Error **errp)
+static void yielding_job_complete(Job *job, bool do_graph_change, Error **errp)
 {
     YieldingJob *s = container_of(job, YieldingJob, common.job);
     s->should_complete = true;
-- 
2.29.2

[PATCH 2/3] job: use complete(do_graph_change=false) to handle soft cancel

[Vladimir Sementsov-Ogievskiy]

Soft cancel of READY mirror is more like completion than cancelling. We
have bugs and misunderstanding because of this feature. Now, let's
handle it the other way to drop force cancelling at all in the
following commit.

This makes internal implementation cleaner. Still, we should deprecate
and drop old interface (through block-job-cancel) as a separate step.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 blockdev.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 3d8ac368a1..c4ee5f02f4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3366,7 +3366,16 @@ void qmp_block_job_cancel(const char *device,
     }
 
     trace_qmp_block_job_cancel(job);
-    job_user_cancel(&job->job, force, errp);
+    if (!force && job_is_ready(&job->job)) {
+        /*
+         * Hack to support old mirror soft-cancel. Please add new API to do
+         * complete with disabled graph-change, deprecate soft-cancel and
+         * finally drop this code.
+         */
+        job_complete_ex(&job->job, false, errp);
+    } else {
+        job_user_cancel(&job->job, force, errp);
+    }
 out:
     aio_context_release(aio_context);
 }
-- 
2.29.2

[PATCH 3/3] job: drop force argument of *job*cancel

[Vladimir Sementsov-Ogievskiy]

Now, when soft-cancelling READY mirror handled in
qmp_block_job_cancel(), no other functions need to care of it:
cancel is always force.

So drop unused code paths.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/qemu/job.h             | 14 ++++----------
 block/backup.c                 |  2 +-
 block/mirror.c                 | 13 ++++---------
 blockdev.c                     |  4 ++--
 job-qmp.c                      |  2 +-
 job.c                          | 18 ++++++++----------
 tests/unit/test-blockjob-txn.c |  8 ++++----
 7 files changed, 24 insertions(+), 37 deletions(-)

diff --git a/include/qemu/job.h b/include/qemu/job.h
index 3dfb79cee6..0e30665fed 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -103,12 +103,6 @@ typedef struct Job {
      */
     bool cancelled;
 
-    /**
-     * Set to true if the job should abort immediately without waiting
-     * for data to be in sync.
-     */
-    bool force_cancel;
-
     /** Set to true when the job has deferred work to the main loop. */
     bool deferred_to_main_loop;
 
@@ -254,7 +248,7 @@ struct JobDriver {
     /**
      * If the callback is not NULL, it will be invoked in job_cancel_async
      */
-    void (*cancel)(Job *job, bool force);
+    void (*cancel)(Job *job);
 
 
     /** Called when the job is freed */
@@ -496,16 +490,16 @@ void job_complete(Job *job, Error **errp);
 void job_complete_ex(Job *job, bool do_graph_change, Error **errp);
 
 /**
- * Asynchronously cancel the specified @job. If @force is true, the job should
+ * Asynchronously cancel the specified @job.
  * be cancelled immediately without waiting for a consistent state.
  */
-void job_cancel(Job *job, bool force);
+void job_cancel(Job *job);
 
 /**
  * Cancels the specified job like job_cancel(), but may refuse to do so if the
  * operation isn't meaningful in the current state of the job.
  */
-void job_user_cancel(Job *job, bool force, Error **errp);
+void job_user_cancel(Job *job, Error **errp);
 
 /**
  * Synchronously cancel the @job.  The completion callback is called
diff --git a/block/backup.c b/block/backup.c
index bd3614ce70..6cf2f974aa 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -331,7 +331,7 @@ static void coroutine_fn backup_set_speed(BlockJob *job, int64_t speed)
     }
 }
 
-static void backup_cancel(Job *job, bool force)
+static void backup_cancel(Job *job)
 {
     BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
 
diff --git a/block/mirror.c b/block/mirror.c
index ad9736eb5e..06a07baf46 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1095,9 +1095,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
         }
         trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
         job_sleep_ns(&s->common.job, delay_ns);
-        if (job_is_cancelled(&s->common.job) &&
-            (!s->synced || s->common.job.force_cancel))
-        {
+        if (job_is_cancelled(&s->common.job)) {
             break;
         }
         s->last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
@@ -1109,8 +1107,7 @@ immediate_exit:
          * or it was cancelled prematurely so that we do not guarantee that
          * the target is a copy of the source.
          */
-        assert(ret < 0 || ((s->common.job.force_cancel || !s->synced) &&
-               job_is_cancelled(&s->common.job)));
+        assert(ret < 0 || job_is_cancelled(&s->common.job));
         assert(need_drain);
         mirror_wait_for_all_io(s);
     }
@@ -1197,14 +1194,12 @@ static bool mirror_drained_poll(BlockJob *job)
     return !!s->in_flight;
 }
 
-static void mirror_cancel(Job *job, bool force)
+static void mirror_cancel(Job *job)
 {
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
     BlockDriverState *target = blk_bs(s->target);
 
-    if (force || !job_is_ready(job)) {
-        bdrv_cancel_in_flight(target);
-    }
+    bdrv_cancel_in_flight(target);
 }
 
 static const BlockJobDriver mirror_job_driver = {
diff --git a/blockdev.c b/blockdev.c
index c4ee5f02f4..cc424a451f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -150,7 +150,7 @@ void blockdev_mark_auto_del(BlockBackend *blk)
             AioContext *aio_context = job->job.aio_context;
             aio_context_acquire(aio_context);
 
-            job_cancel(&job->job, false);
+            job_cancel(&job->job);
 
             aio_context_release(aio_context);
         }
@@ -3374,7 +3374,7 @@ void qmp_block_job_cancel(const char *device,
          */
         job_complete_ex(&job->job, false, errp);
     } else {
-        job_user_cancel(&job->job, force, errp);
+        job_user_cancel(&job->job, errp);
     }
 out:
     aio_context_release(aio_context);
diff --git a/job-qmp.c b/job-qmp.c
index 829a28aa70..272837bd1f 100644
--- a/job-qmp.c
+++ b/job-qmp.c
@@ -58,7 +58,7 @@ void qmp_job_cancel(const char *id, Error **errp)
     }
 
     trace_qmp_job_cancel(job);
-    job_user_cancel(job, true, errp);
+    job_user_cancel(job, errp);
     aio_context_release(aio_context);
 }
 
diff --git a/job.c b/job.c
index 52127dd6bd..04437ce438 100644
--- a/job.c
+++ b/job.c
@@ -716,10 +716,10 @@ static int job_finalize_single(Job *job)
     return 0;
 }
 
-static void job_cancel_async(Job *job, bool force)
+static void job_cancel_async(Job *job)
 {
     if (job->driver->cancel) {
-        job->driver->cancel(job, force);
+        job->driver->cancel(job);
     }
     if (job->user_paused) {
         /* Do not call job_enter here, the caller will handle it.  */
@@ -731,8 +731,6 @@ static void job_cancel_async(Job *job, bool force)
         job->pause_count--;
     }
     job->cancelled = true;
-    /* To prevent 'force == false' overriding a previous 'force == true' */
-    job->force_cancel |= force;
 }
 
 static void job_completed_txn_abort(Job *job)
@@ -763,7 +761,7 @@ static void job_completed_txn_abort(Job *job)
         if (other_job != job) {
             ctx = other_job->aio_context;
             aio_context_acquire(ctx);
-            job_cancel_async(other_job, false);
+            job_cancel_async(other_job);
             aio_context_release(ctx);
         }
     }
@@ -932,13 +930,13 @@ void job_start(Job *job)
     aio_co_enter(job->aio_context, job->co);
 }
 
-void job_cancel(Job *job, bool force)
+void job_cancel(Job *job)
 {
     if (job->status == JOB_STATUS_CONCLUDED) {
         job_do_dismiss(job);
         return;
     }
-    job_cancel_async(job, force);
+    job_cancel_async(job);
     if (!job_started(job)) {
         job_completed(job);
     } else if (job->deferred_to_main_loop) {
@@ -948,12 +946,12 @@ void job_cancel(Job *job, bool force)
     }
 }
 
-void job_user_cancel(Job *job, bool force, Error **errp)
+void job_user_cancel(Job *job, Error **errp)
 {
     if (job_apply_verb(job, JOB_VERB_CANCEL, errp)) {
         return;
     }
-    job_cancel(job, force);
+    job_cancel(job);
 }
 
 /* A wrapper around job_cancel() taking an Error ** parameter so it may be
@@ -961,7 +959,7 @@ void job_user_cancel(Job *job, bool force, Error **errp)
  * pointer casts there. */
 static void job_cancel_err(Job *job, Error **errp)
 {
-    job_cancel(job, false);
+    job_cancel(job);
 }
 
 int job_cancel_sync(Job *job)
diff --git a/tests/unit/test-blockjob-txn.c b/tests/unit/test-blockjob-txn.c
index 8bd13b9949..5a995c59c7 100644
--- a/tests/unit/test-blockjob-txn.c
+++ b/tests/unit/test-blockjob-txn.c
@@ -125,7 +125,7 @@ static void test_single_job(int expected)
     job_start(&job->job);
 
     if (expected == -ECANCELED) {
-        job_cancel(&job->job, false);
+        job_cancel(&job->job);
     }
 
     while (result == -EINPROGRESS) {
@@ -171,10 +171,10 @@ static void test_pair_jobs(int expected1, int expected2)
     job_txn_unref(txn);
 
     if (expected1 == -ECANCELED) {
-        job_cancel(&job1->job, false);
+        job_cancel(&job1->job);
     }
     if (expected2 == -ECANCELED) {
-        job_cancel(&job2->job, false);
+        job_cancel(&job2->job);
     }
 
     while (result1 == -EINPROGRESS || result2 == -EINPROGRESS) {
@@ -227,7 +227,7 @@ static void test_pair_jobs_fail_cancel_race(void)
     job_start(&job1->job);
     job_start(&job2->job);
 
-    job_cancel(&job1->job, false);
+    job_cancel(&job1->job);
 
     /* Now make job2 finish before the main loop kicks jobs.  This simulates
      * the race between a pending kick and another job completing.
-- 
2.29.2

Re: [PATCH RFC 0/3] mirror: rework soft-cancelling READY mirror

[Vladimir Sementsov-Ogievskiy]

27.07.2021 19:47, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> That's an alternative to (part of) Max's
> "[PATCH for-6.1? v2 0/7] mirror: Handle errors after READY cancel"
> and shows' my idea of handling soft-cancelling READY mirror case
> directly in qmp_block_job_cancel. And cleanup all other job cancelling
> functions.
> 
> That's untested draft, don't take it to heart :)


Side idea:

Instead of this all we can do the following:

1. Add new interface as alternative to soft-cancelling READY mirror.

It may be argument to job-complete qmp command, or some separate command to change job parameters, or just an option for blockdev-mirror command (may be our users actually know what they want when they start the job).

2. Deprecate block-job-cancel command (with recommendation to use job-cancel and new interface [1] instead)

3. Wait for 3 releases and apply patch 3, improved by dropping block-job-cancel at all.


This way, deprecated interface remains buggy until dropped, but that's not bad. It's good actually :)


> 
> Vladimir Sementsov-Ogievskiy (3):
>    job: add job_complete_ex with do_graph_change argument
>    job: use complete(do_graph_change=false) to handle soft cancel
>    job: drop force argument of *job*cancel
> 
>   include/qemu/job.h               | 19 ++++++++----------
>   block/backup.c                   |  2 +-
>   block/mirror.c                   | 33 +++++++++++++++++---------------
>   blockdev.c                       | 13 +++++++++++--
>   job-qmp.c                        |  2 +-
>   job.c                            | 27 ++++++++++++++------------
>   tests/unit/test-bdrv-drain.c     |  2 +-
>   tests/unit/test-block-iothread.c |  2 +-
>   tests/unit/test-blockjob-txn.c   |  8 ++++----
>   tests/unit/test-blockjob.c       |  4 ++--
>   10 files changed, 62 insertions(+), 50 deletions(-)
> 


-- 
Best regards,
Vladimir

Re: [PATCH RFC 0/3] mirror: rework soft-cancelling READY mirror

[Max Reitz]

On 27.07.21 18:47, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> That's an alternative to (part of) Max's
> "[PATCH for-6.1? v2 0/7] mirror: Handle errors after READY cancel"
> and shows' my idea of handling soft-cancelling READY mirror case
> directly in qmp_block_job_cancel. And cleanup all other job cancelling
> functions.
>
> That's untested draft, don't take it to heart :)

Well, I would have preferred it if you’d rebased this on top of that 
series, precisely because it’s an alternative to only part of it. And if 
it’s just an untested draft, that would have been even better, because 
it would’ve given a better idea on what the cleanup looks like.

There are also things like this series making cancel internally always a 
force-cancel, where I’m not sure whether we want that in the replication 
driver or not[1].  With my series, we add an explicit parameter, so 
we’re forced to think about it, and then in this series on top we can 
just drop the parameter for all force-cancel invocations again, and for 
all non-force-cancel invocations we would have to think a bit more.

Specifically as for this series, I don’t like job_complete_ex() very 
much, I think the parameter should be part of job_complete() itself.  If 
we think that’s too specific of a mirror parameter to include in normal 
job_complete(), well, then there shouldn’t be a job_complete_ex() 
either, and do_graph_change should be a property of the mirror job 
(perhaps as pivot_on_completion) that’s cleared by 
qmp_block_job_cancel() before invoking job_complete().

Max

[1] Although looking at it again now, it probably wants force-cancel.

Re: [PATCH RFC 0/3] mirror: rework soft-cancelling READY mirror

[Vladimir Sementsov-Ogievskiy]

28.07.2021 10:00, Max Reitz wrote:
> On 27.07.21 18:47, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> That's an alternative to (part of) Max's
>> "[PATCH for-6.1? v2 0/7] mirror: Handle errors after READY cancel"
>> and shows' my idea of handling soft-cancelling READY mirror case
>> directly in qmp_block_job_cancel. And cleanup all other job cancelling
>> functions.
>>
>> That's untested draft, don't take it to heart :)
> 
> Well, I would have preferred it if you’d rebased this on top of that series, precisely because it’s an alternative to only part of it. And if it’s just an untested draft, that would have been even better, because it would’ve given a better idea on what the cleanup looks like.
> 
> There are also things like this series making cancel internally always a force-cancel, where I’m not sure whether we want that in the replication driver or not[1].  With my series, we add an explicit parameter, so we’re forced to think about it, and then in this series on top we can just drop the parameter for all force-cancel invocations again, and for all non-force-cancel invocations we would have to think a bit more.

I now don't sure that patch 5 of your series is correct (see my last answer to it), that's why I decided to not base on it. My series has the benefit of handling soft-mirror-cancel case the other way and handles mirror finalization in case of soft-cancel properly.

> 
> Specifically as for this series, I don’t like job_complete_ex() very much, I think the parameter should be part of job_complete() itself.

That was my idea. But job_complete is passed as function pointer, so changing its prototype would be more work.. But I think it's possible.

>  If we think that’s too specific of a mirror parameter to include in normal job_complete(), well, then there shouldn’t be a job_complete_ex() either, and do_graph_change should be a property of the mirror job (perhaps as pivot_on_completion) that’s cleared by qmp_block_job_cancel() before invoking job_complete().

This way users will lose a way to make a decision during job running.. But probably they don't need actually. Moving the option to mirror job parameter seems a good option to me.

> 
> Max
> 
> [1] Although looking at it again now, it probably wants force-cancel.
> 


What do you think of my idea to keep old bugs as is and just deprecate block-job-cancel and add a new interface for "no-graph-change mirror" case?

-- 
Best regards,
Vladimir

Re: [PATCH RFC 0/3] mirror: rework soft-cancelling READY mirror

[Max Reitz]

On 29.07.21 12:02, Vladimir Sementsov-Ogievskiy wrote:
> 28.07.2021 10:00, Max Reitz wrote:
>> On 27.07.21 18:47, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> That's an alternative to (part of) Max's
>>> "[PATCH for-6.1? v2 0/7] mirror: Handle errors after READY cancel"
>>> and shows' my idea of handling soft-cancelling READY mirror case
>>> directly in qmp_block_job_cancel. And cleanup all other job cancelling
>>> functions.
>>>
>>> That's untested draft, don't take it to heart :)
>>
>> Well, I would have preferred it if you’d rebased this on top of that 
>> series, precisely because it’s an alternative to only part of it. And 
>> if it’s just an untested draft, that would have been even better, 
>> because it would’ve given a better idea on what the cleanup looks like.
>>
>> There are also things like this series making cancel internally 
>> always a force-cancel, where I’m not sure whether we want that in the 
>> replication driver or not[1].  With my series, we add an explicit 
>> parameter, so we’re forced to think about it, and then in this series 
>> on top we can just drop the parameter for all force-cancel 
>> invocations again, and for all non-force-cancel invocations we would 
>> have to think a bit more.
>
> I now don't sure that patch 5 of your series is correct (see my last 
> answer to it), that's why I decided to not base on it.

Well, we can always take patch 5 from v1.  (Where I changed any 
job_is_cancelled() to job_cancel_requested() when it influenced the 
external interface.)

> My series has the benefit of handling soft-mirror-cancel case the 
> other way and handles mirror finalization in case of soft-cancel 
> properly.
>
>>
>> Specifically as for this series, I don’t like job_complete_ex() very 
>> much, I think the parameter should be part of job_complete() itself.
>
> That was my idea. But job_complete is passed as function pointer, so 
> changing its prototype would be more work.. But I think it's possible.
>
>>   If we think that’s too specific of a mirror parameter to include in 
>> normal job_complete(), well, then there shouldn’t be a 
>> job_complete_ex() either, and do_graph_change should be a property of 
>> the mirror job (perhaps as pivot_on_completion) that’s cleared by 
>> qmp_block_job_cancel() before invoking job_complete().
>
> This way users will lose a way to make a decision during job running..

On the contrary, it would be a precursor to letting the user change this 
property explicitly with a new QMP command.

> But probably they don't need actually. Moving the option to mirror job 
> parameter seems a good option to me.
>
>>
>> Max
>>
>> [1] Although looking at it again now, it probably wants force-cancel.
>>
>
>
> What do you think of my idea to keep old bugs as is and just deprecate 
> block-job-cancel and add a new interface for "no-graph-change mirror" 
> case?

I don’t see a reason for that.  The fix isn’t that complicated.

Also, honestly, I don’t see a good reason for deprecating anything.

Max

Re: [PATCH RFC 0/3] mirror: rework soft-cancelling READY mirror

[Vladimir Sementsov-Ogievskiy]

29.07.2021 13:38, Max Reitz wrote:
> On 29.07.21 12:02, Vladimir Sementsov-Ogievskiy wrote:
>> 28.07.2021 10:00, Max Reitz wrote:
>>> On 27.07.21 18:47, Vladimir Sementsov-Ogievskiy wrote:
>>>> Hi all!
>>>>
>>>> That's an alternative to (part of) Max's
>>>> "[PATCH for-6.1? v2 0/7] mirror: Handle errors after READY cancel"
>>>> and shows' my idea of handling soft-cancelling READY mirror case
>>>> directly in qmp_block_job_cancel. And cleanup all other job cancelling
>>>> functions.
>>>>
>>>> That's untested draft, don't take it to heart :)
>>>
>>> Well, I would have preferred it if you’d rebased this on top of that series, precisely because it’s an alternative to only part of it. And if it’s just an untested draft, that would have been even better, because it would’ve given a better idea on what the cleanup looks like.
>>>
>>> There are also things like this series making cancel internally always a force-cancel, where I’m not sure whether we want that in the replication driver or not[1].  With my series, we add an explicit parameter, so we’re forced to think about it, and then in this series on top we can just drop the parameter for all force-cancel invocations again, and for all non-force-cancel invocations we would have to think a bit more.
>>
>> I now don't sure that patch 5 of your series is correct (see my last answer to it), that's why I decided to not base on it.
> 
> Well, we can always take patch 5 from v1.  (Where I changed any job_is_cancelled() to job_cancel_requested() when it influenced the external interface.)
> 
>> My series has the benefit of handling soft-mirror-cancel case the other way and handles mirror finalization in case of soft-cancel properly.
>>
>>>
>>> Specifically as for this series, I don’t like job_complete_ex() very much, I think the parameter should be part of job_complete() itself.
>>
>> That was my idea. But job_complete is passed as function pointer, so changing its prototype would be more work.. But I think it's possible.
>>
>>>   If we think that’s too specific of a mirror parameter to include in normal job_complete(), well, then there shouldn’t be a job_complete_ex() either, and do_graph_change should be a property of the mirror job (perhaps as pivot_on_completion) that’s cleared by qmp_block_job_cancel() before invoking job_complete().
>>
>> This way users will lose a way to make a decision during job running..
> 
> On the contrary, it would be a precursor to letting the user change this property explicitly with a new QMP command.
> 
>> But probably they don't need actually. Moving the option to mirror job parameter seems a good option to me.
>>
>>>
>>> Max
>>>
>>> [1] Although looking at it again now, it probably wants force-cancel.
>>>
>>
>>
>> What do you think of my idea to keep old bugs as is and just deprecate block-job-cancel and add a new interface for "no-graph-change mirror" case?
> 
> I don’t see a reason for that.  The fix isn’t that complicated.
> 
> Also, honestly, I don’t see a good reason for deprecating anything.
> 

Current interface lead to mess in the code, that's bad. Cancellation mode that is actually a kind of completion (and having comments in many places about that) - that shows for me that interface is not good.. It's a question of terminology, what to call "cancel". Also, that's not the first time this question arise. Remember my recent cancel-in-flight-requests series, when I thought that "cancel is cancel" and didn't consider soft-cancel of mirror.. And reviewers didn't caught it. I don't think that interface is good, it will always confuse new developers and users. But that's just my opinion, I don't impose it )

If not deprecate, i.e. if we consider old interface to be good, than no reason for this my series and for introducing new interface :)

-- 
Best regards,
Vladimir

Re: [PATCH RFC 0/3] mirror: rework soft-cancelling READY mirror

[Max Reitz]

On 29.07.21 13:35, Vladimir Sementsov-Ogievskiy wrote:
> 29.07.2021 13:38, Max Reitz wrote:
>> On 29.07.21 12:02, Vladimir Sementsov-Ogievskiy wrote:
>>> 28.07.2021 10:00, Max Reitz wrote:
>>>> On 27.07.21 18:47, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Hi all!
>>>>>
>>>>> That's an alternative to (part of) Max's
>>>>> "[PATCH for-6.1? v2 0/7] mirror: Handle errors after READY cancel"
>>>>> and shows' my idea of handling soft-cancelling READY mirror case
>>>>> directly in qmp_block_job_cancel. And cleanup all other job 
>>>>> cancelling
>>>>> functions.
>>>>>
>>>>> That's untested draft, don't take it to heart :)
>>>>
>>>> Well, I would have preferred it if you’d rebased this on top of 
>>>> that series, precisely because it’s an alternative to only part of 
>>>> it. And if it’s just an untested draft, that would have been even 
>>>> better, because it would’ve given a better idea on what the cleanup 
>>>> looks like.
>>>>
>>>> There are also things like this series making cancel internally 
>>>> always a force-cancel, where I’m not sure whether we want that in 
>>>> the replication driver or not[1].  With my series, we add an 
>>>> explicit parameter, so we’re forced to think about it, and then in 
>>>> this series on top we can just drop the parameter for all 
>>>> force-cancel invocations again, and for all non-force-cancel 
>>>> invocations we would have to think a bit more.
>>>
>>> I now don't sure that patch 5 of your series is correct (see my last 
>>> answer to it), that's why I decided to not base on it.
>>
>> Well, we can always take patch 5 from v1.  (Where I changed any 
>> job_is_cancelled() to job_cancel_requested() when it influenced the 
>> external interface.)
>>
>>> My series has the benefit of handling soft-mirror-cancel case the 
>>> other way and handles mirror finalization in case of soft-cancel 
>>> properly.
>>>
>>>>
>>>> Specifically as for this series, I don’t like job_complete_ex() 
>>>> very much, I think the parameter should be part of job_complete() 
>>>> itself.
>>>
>>> That was my idea. But job_complete is passed as function pointer, so 
>>> changing its prototype would be more work.. But I think it's possible.
>>>
>>>>   If we think that’s too specific of a mirror parameter to include 
>>>> in normal job_complete(), well, then there shouldn’t be a 
>>>> job_complete_ex() either, and do_graph_change should be a property 
>>>> of the mirror job (perhaps as pivot_on_completion) that’s cleared 
>>>> by qmp_block_job_cancel() before invoking job_complete().
>>>
>>> This way users will lose a way to make a decision during job running..
>>
>> On the contrary, it would be a precursor to letting the user change 
>> this property explicitly with a new QMP command.
>>
>>> But probably they don't need actually. Moving the option to mirror 
>>> job parameter seems a good option to me.
>>>
>>>>
>>>> Max
>>>>
>>>> [1] Although looking at it again now, it probably wants force-cancel.
>>>>
>>>
>>>
>>> What do you think of my idea to keep old bugs as is and just 
>>> deprecate block-job-cancel and add a new interface for 
>>> "no-graph-change mirror" case?
>>
>> I don’t see a reason for that.  The fix isn’t that complicated.
>>
>> Also, honestly, I don’t see a good reason for deprecating anything.
>>
>
> Current interface lead to mess in the code, that's bad. Cancellation 
> mode that is actually a kind of completion (and having comments in 
> many places about that) - that shows for me that interface is not 
> good.. It's a question of terminology, what to call "cancel". Also, 
> that's not the first time this question arise. Remember my recent 
> cancel-in-flight-requests series, when I thought that "cancel is 
> cancel" and didn't consider soft-cancel of mirror.. And reviewers 
> didn't caught it. I don't think that interface is good, it will always 
> confuse new developers and users. But that's just my opinion, I don't 
> impose it )
>
> If not deprecate, i.e. if we consider old interface to be good, than 
> no reason for this my series and for introducing new interface :)

I’m not against a better interface, I’m against using this current bug 
as an excuse to improve the interface.  We’ve known we want to improve 
the interface for quite a long time now, we don’t need an excuse for that.

If we use this bug as an excuse, I’m afraid of becoming hung up on 
interface discussions instead of just getting the bug fixed.  And we 
must get the bug fixed, it’s real, it’s kind of bad, and saying “it 
won’t appear with the new interface, let’s not worry about the old one” 
is not something I like.

OTOH, if we use this bug as an excuse, I’m also afraid of trying to rush 
the design instead of actually implementing the interface that we’ve 
always desired, i.e. where the user gets to choose the completion mode 
via yet-to-be-implemented some job property setter function.

As a final note (but this is precisely the interface discussion that I 
want to avoid for now), I said I don’t see a good reason for deprecating 
anything, because `job-cancel force=false` can just internally do 
`set-job-property .pivot_on_completion=false; job-complete`.  From an 
implementation perspective, that should be simple.

I understand that for users the existence of the `force` flag may still 
be confusing and so we might want to deprecate it for that reason, but 
again, this is the interface discussion that I think we don’t actually 
need right now.

And that’s why I’d prefer for any clean-up to come on top of fixing the 
existing bug, so we can have separate discussions and take our time with 
the interface without feeling driven by the need to fix a bug.

Max

Re: [PATCH RFC 0/3] mirror: rework soft-cancelling READY mirror

[Vladimir Sementsov-Ogievskiy]

29.07.2021 16:47, Max Reitz wrote:
> On 29.07.21 13:35, Vladimir Sementsov-Ogievskiy wrote:
>> 29.07.2021 13:38, Max Reitz wrote:
>>> On 29.07.21 12:02, Vladimir Sementsov-Ogievskiy wrote:
>>>> 28.07.2021 10:00, Max Reitz wrote:
>>>>> On 27.07.21 18:47, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> Hi all!
>>>>>>
>>>>>> That's an alternative to (part of) Max's
>>>>>> "[PATCH for-6.1? v2 0/7] mirror: Handle errors after READY cancel"
>>>>>> and shows' my idea of handling soft-cancelling READY mirror case
>>>>>> directly in qmp_block_job_cancel. And cleanup all other job cancelling
>>>>>> functions.
>>>>>>
>>>>>> That's untested draft, don't take it to heart :)
>>>>>
>>>>> Well, I would have preferred it if you’d rebased this on top of that series, precisely because it’s an alternative to only part of it. And if it’s just an untested draft, that would have been even better, because it would’ve given a better idea on what the cleanup looks like.
>>>>>
>>>>> There are also things like this series making cancel internally always a force-cancel, where I’m not sure whether we want that in the replication driver or not[1].  With my series, we add an explicit parameter, so we’re forced to think about it, and then in this series on top we can just drop the parameter for all force-cancel invocations again, and for all non-force-cancel invocations we would have to think a bit more.
>>>>
>>>> I now don't sure that patch 5 of your series is correct (see my last answer to it), that's why I decided to not base on it.
>>>
>>> Well, we can always take patch 5 from v1.  (Where I changed any job_is_cancelled() to job_cancel_requested() when it influenced the external interface.)
>>>
>>>> My series has the benefit of handling soft-mirror-cancel case the other way and handles mirror finalization in case of soft-cancel properly.
>>>>
>>>>>
>>>>> Specifically as for this series, I don’t like job_complete_ex() very much, I think the parameter should be part of job_complete() itself.
>>>>
>>>> That was my idea. But job_complete is passed as function pointer, so changing its prototype would be more work.. But I think it's possible.
>>>>
>>>>>   If we think that’s too specific of a mirror parameter to include in normal job_complete(), well, then there shouldn’t be a job_complete_ex() either, and do_graph_change should be a property of the mirror job (perhaps as pivot_on_completion) that’s cleared by qmp_block_job_cancel() before invoking job_complete().
>>>>
>>>> This way users will lose a way to make a decision during job running..
>>>
>>> On the contrary, it would be a precursor to letting the user change this property explicitly with a new QMP command.
>>>
>>>> But probably they don't need actually. Moving the option to mirror job parameter seems a good option to me.
>>>>
>>>>>
>>>>> Max
>>>>>
>>>>> [1] Although looking at it again now, it probably wants force-cancel.
>>>>>
>>>>
>>>>
>>>> What do you think of my idea to keep old bugs as is and just deprecate block-job-cancel and add a new interface for "no-graph-change mirror" case?
>>>
>>> I don’t see a reason for that.  The fix isn’t that complicated.
>>>
>>> Also, honestly, I don’t see a good reason for deprecating anything.
>>>
>>
>> Current interface lead to mess in the code, that's bad. Cancellation mode that is actually a kind of completion (and having comments in many places about that) - that shows for me that interface is not good.. It's a question of terminology, what to call "cancel". Also, that's not the first time this question arise. Remember my recent cancel-in-flight-requests series, when I thought that "cancel is cancel" and didn't consider soft-cancel of mirror.. And reviewers didn't caught it. I don't think that interface is good, it will always confuse new developers and users. But that's just my opinion, I don't impose it )
>>
>> If not deprecate, i.e. if we consider old interface to be good, than no reason for this my series and for introducing new interface :)
> 
> I’m not against a better interface, I’m against using this current bug as an excuse to improve the interface.  We’ve known we want to improve the interface for quite a long time now, we don’t need an excuse for that.
> 
> If we use this bug as an excuse, I’m afraid of becoming hung up on interface discussions instead of just getting the bug fixed.  And we must get the bug fixed, it’s real, it’s kind of bad, and saying “it won’t appear with the new interface, let’s not worry about the old one” is not something I like.
> 
> OTOH, if we use this bug as an excuse, I’m also afraid of trying to rush the design instead of actually implementing the interface that we’ve always desired, i.e. where the user gets to choose the completion mode via yet-to-be-implemented some job property setter function.
> 
> As a final note (but this is precisely the interface discussion that I want to avoid for now), I said I don’t see a good reason for deprecating anything, because `job-cancel force=false` can just internally do `set-job-property .pivot_on_completion=false; job-complete`.  From an implementation perspective, that should be simple.
> 
> I understand that for users the existence of the `force` flag may still be confusing and so we might want to deprecate it for that reason, but again, this is the interface discussion that I think we don’t actually need right now.
> 
> And that’s why I’d prefer for any clean-up to come on top of fixing the existing bug, so we can have separate discussions and take our time with the interface without feeling driven by the need to fix a bug.
> 

OK, I see your point. Let's just fix a bug with current interface and anything else is a separate step. Are you going to send v3?



-- 
Best regards,
Vladimir

Re: [PATCH RFC 0/3] mirror: rework soft-cancelling READY mirror

[Max Reitz]

On 29.07.21 18:29, Vladimir Sementsov-Ogievskiy wrote:
> 29.07.2021 16:47, Max Reitz wrote:
>> On 29.07.21 13:35, Vladimir Sementsov-Ogievskiy wrote:
>>> 29.07.2021 13:38, Max Reitz wrote:
>>>> On 29.07.21 12:02, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 28.07.2021 10:00, Max Reitz wrote:
>>>>>> On 27.07.21 18:47, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>> Hi all!
>>>>>>>
>>>>>>> That's an alternative to (part of) Max's
>>>>>>> "[PATCH for-6.1? v2 0/7] mirror: Handle errors after READY cancel"
>>>>>>> and shows' my idea of handling soft-cancelling READY mirror case
>>>>>>> directly in qmp_block_job_cancel. And cleanup all other job 
>>>>>>> cancelling
>>>>>>> functions.
>>>>>>>
>>>>>>> That's untested draft, don't take it to heart :)
>>>>>>
>>>>>> Well, I would have preferred it if you’d rebased this on top of 
>>>>>> that series, precisely because it’s an alternative to only part 
>>>>>> of it. And if it’s just an untested draft, that would have been 
>>>>>> even better, because it would’ve given a better idea on what the 
>>>>>> cleanup looks like.
>>>>>>
>>>>>> There are also things like this series making cancel internally 
>>>>>> always a force-cancel, where I’m not sure whether we want that in 
>>>>>> the replication driver or not[1].  With my series, we add an 
>>>>>> explicit parameter, so we’re forced to think about it, and then 
>>>>>> in this series on top we can just drop the parameter for all 
>>>>>> force-cancel invocations again, and for all non-force-cancel 
>>>>>> invocations we would have to think a bit more.
>>>>>
>>>>> I now don't sure that patch 5 of your series is correct (see my 
>>>>> last answer to it), that's why I decided to not base on it.
>>>>
>>>> Well, we can always take patch 5 from v1.  (Where I changed any 
>>>> job_is_cancelled() to job_cancel_requested() when it influenced the 
>>>> external interface.)
>>>>
>>>>> My series has the benefit of handling soft-mirror-cancel case the 
>>>>> other way and handles mirror finalization in case of soft-cancel 
>>>>> properly.
>>>>>
>>>>>>
>>>>>> Specifically as for this series, I don’t like job_complete_ex() 
>>>>>> very much, I think the parameter should be part of job_complete() 
>>>>>> itself.
>>>>>
>>>>> That was my idea. But job_complete is passed as function pointer, 
>>>>> so changing its prototype would be more work.. But I think it's 
>>>>> possible.
>>>>>
>>>>>>   If we think that’s too specific of a mirror parameter to 
>>>>>> include in normal job_complete(), well, then there shouldn’t be a 
>>>>>> job_complete_ex() either, and do_graph_change should be a 
>>>>>> property of the mirror job (perhaps as pivot_on_completion) 
>>>>>> that’s cleared by qmp_block_job_cancel() before invoking 
>>>>>> job_complete().
>>>>>
>>>>> This way users will lose a way to make a decision during job 
>>>>> running..
>>>>
>>>> On the contrary, it would be a precursor to letting the user change 
>>>> this property explicitly with a new QMP command.
>>>>
>>>>> But probably they don't need actually. Moving the option to mirror 
>>>>> job parameter seems a good option to me.
>>>>>
>>>>>>
>>>>>> Max
>>>>>>
>>>>>> [1] Although looking at it again now, it probably wants 
>>>>>> force-cancel.
>>>>>>
>>>>>
>>>>>
>>>>> What do you think of my idea to keep old bugs as is and just 
>>>>> deprecate block-job-cancel and add a new interface for 
>>>>> "no-graph-change mirror" case?
>>>>
>>>> I don’t see a reason for that.  The fix isn’t that complicated.
>>>>
>>>> Also, honestly, I don’t see a good reason for deprecating anything.
>>>>
>>>
>>> Current interface lead to mess in the code, that's bad. Cancellation 
>>> mode that is actually a kind of completion (and having comments in 
>>> many places about that) - that shows for me that interface is not 
>>> good.. It's a question of terminology, what to call "cancel". Also, 
>>> that's not the first time this question arise. Remember my recent 
>>> cancel-in-flight-requests series, when I thought that "cancel is 
>>> cancel" and didn't consider soft-cancel of mirror.. And reviewers 
>>> didn't caught it. I don't think that interface is good, it will 
>>> always confuse new developers and users. But that's just my opinion, 
>>> I don't impose it )
>>>
>>> If not deprecate, i.e. if we consider old interface to be good, than 
>>> no reason for this my series and for introducing new interface :)
>>
>> I’m not against a better interface, I’m against using this current 
>> bug as an excuse to improve the interface.  We’ve known we want to 
>> improve the interface for quite a long time now, we don’t need an 
>> excuse for that.
>>
>> If we use this bug as an excuse, I’m afraid of becoming hung up on 
>> interface discussions instead of just getting the bug fixed. And we 
>> must get the bug fixed, it’s real, it’s kind of bad, and saying “it 
>> won’t appear with the new interface, let’s not worry about the old 
>> one” is not something I like.
>>
>> OTOH, if we use this bug as an excuse, I’m also afraid of trying to 
>> rush the design instead of actually implementing the interface that 
>> we’ve always desired, i.e. where the user gets to choose the 
>> completion mode via yet-to-be-implemented some job property setter 
>> function.
>>
>> As a final note (but this is precisely the interface discussion that 
>> I want to avoid for now), I said I don’t see a good reason for 
>> deprecating anything, because `job-cancel force=false` can just 
>> internally do `set-job-property .pivot_on_completion=false; 
>> job-complete`.  From an implementation perspective, that should be 
>> simple.
>>
>> I understand that for users the existence of the `force` flag may 
>> still be confusing and so we might want to deprecate it for that 
>> reason, but again, this is the interface discussion that I think we 
>> don’t actually need right now.
>>
>> And that’s why I’d prefer for any clean-up to come on top of fixing 
>> the existing bug, so we can have separate discussions and take our 
>> time with the interface without feeling driven by the need to fix a bug.
>>
>
> OK, I see your point. Let's just fix a bug with current interface and 
> anything else is a separate step. Are you going to send v3?

Yep, sure!

(Still want to look into the exact job->ret details, though. Perhaps I 
can do better than just to keep it in the v1 state.)

Max