Re: [Qemu-devel] [PATCH 09/10] blockjob: reorganize block_job_completed_txn_abort

[Paolo Bonzini]

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);
>> -}
>>