On 11/04/2017 03:17, John Snow wrote:
> One thing that I wonder about a little is the push-down of whether or
> not to reset iostatus falling to block_job_cancel_async; it seemed to me
> as if txn_abort really had the best knowledge as to whether or not we
> wanted to reset iostatus, but as it stands it doesn't really make a
> difference.
Actually, there was no iostatus reset in txn_abort; it was done in
block_job_cancel only. But the BLOCK_JOB_CANCELLED event doesn't
publish the iostatus actually, so I could have also done the opposite:
remove the reset in block_job_cancel---it shouldn't make a difference in
theory.
On the other hand, I like respecting the invariant then the coroutine
always runs with BLOCK_DEVICE_IO_STATUS_OK iostatus, so I think that
it's right for cancel_async to reset the iostatus. Speaking about
iostatus and invariants, probably we should also set "job->user_paused =
false" in block_job_cancel_async.
> ACK for now, because it's still not perfectly obvious to me how this
> will wind up helping, though I do believe you :)
It just helps me making sure that the locking is correct later. Maybe
it will help you too!
Paolo
>> static void block_job_completed_txn_success(BlockJob *job)
>> @@ -502,37 +577,6 @@ void block_job_cancel(BlockJob *job)
>> }
>> }
>>
>> -static int block_job_finish_sync(BlockJob *job,
>> - void (*finish)(BlockJob *, Error **errp),
>> - Error **errp)
>> -{
>> - Error *local_err = NULL;
>> - int ret;
>> -
>> - assert(blk_bs(job->blk)->job == job);
>> -
>> - block_job_ref(job);
>> -
>> - finish(job, &local_err);
>> - if (local_err) {
>> - error_propagate(errp, local_err);
>> - block_job_unref(job);
>> - return -EBUSY;
>> - }
>> - /* block_job_drain calls block_job_enter, and it should be enough to
>> - * induce progress until the job completes or moves to the main thread.
>> - */
>> - while (!job->deferred_to_main_loop && !job->completed) {
>> - block_job_drain(job);
>> - }
>> - while (!job->completed) {
>> - aio_poll(qemu_get_aio_context(), true);
>> - }
>> - ret = (job->cancelled && job->ret == 0) ? -ECANCELED : job->ret;
>> - block_job_unref(job);
>> - return ret;
>> -}
>> -
>> /* A wrapper around block_job_cancel() taking an Error ** parameter so it
>> may be
>> * used with block_job_finish_sync() without the need for (rather nasty)
>> * function pointer casts there. */
>> @@ -856,36 +900,3 @@ void block_job_defer_to_main_loop(BlockJob *job,
>> aio_bh_schedule_oneshot(qemu_get_aio_context(),
>> block_job_defer_to_main_loop_bh, data);
>> }
>
> And everything following is pure movement.
>
>> -
>> -BlockJobTxn *block_job_txn_new(void)
>> -{
>> - BlockJobTxn *txn = g_new0(BlockJobTxn, 1);
>> - QLIST_INIT(&txn->jobs);
>> - txn->refcnt = 1;
>> - return txn;
>> -}
>> -
>> -static void block_job_txn_ref(BlockJobTxn *txn)
>> -{
>> - txn->refcnt++;
>> -}
>> -
>> -void block_job_txn_unref(BlockJobTxn *txn)
>> -{
>> - if (txn && --txn->refcnt == 0) {
>> - g_free(txn);
>> - }
>> -}
>> -
>> -void block_job_txn_add_job(BlockJobTxn *txn, BlockJob *job)
>> -{
>> - if (!txn) {
>> - return;
>> - }
>> -
>> - assert(!job->txn);
>> - job->txn = txn;
>> -
>> - QLIST_INSERT_HEAD(&txn->jobs, job, txn_list);
>> - block_job_txn_ref(txn);
>> -}
>>
