Re: [Qemu-devel] [RFC 5/9] block: add block job transactions

[Max Reitz <mreitz@redhat.com>]

On 12.06.2015 12:09, Stefan Hajnoczi wrote:
> Sometimes block jobs must execute as a transaction group.  Finishing
> jobs wait until all other jobs are ready to complete successfully.
> Failure or cancellation of one job cancels the other jobs in the group.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   blockjob.c                | 160 ++++++++++++++++++++++++++++++++++++++++++++++
>   include/block/block.h     |   1 +
>   include/block/block_int.h |   3 +-
>   include/block/blockjob.h  |  49 ++++++++++++++
>   trace-events              |   4 ++
>   5 files changed, 216 insertions(+), 1 deletion(-)
>
> diff --git a/blockjob.c b/blockjob.c
> index 2755465..ff622f5 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -399,3 +399,163 @@ void block_job_defer_to_main_loop(BlockJob *job,
>   
>       qemu_bh_schedule(data->bh);
>   }
> +
> +/* Transactional group of block jobs */
> +struct BlockJobTxn {
> +    /* Jobs may be in different AioContexts so protect all fields */
> +    QemuMutex lock;
> +
> +    /* Reference count for txn object */
> +    unsigned int ref;
> +
> +    /* Is this txn cancelling its jobs? */
> +    bool aborting;
> +
> +    /* Number of jobs still running */
> +    unsigned int jobs_pending;
> +
> +    /* List of jobs */
> +    QLIST_HEAD(, BlockJob) jobs;
> +};
> +
> +BlockJobTxn *block_job_txn_new(void)
> +{
> +    BlockJobTxn *txn = g_new(BlockJobTxn, 1);
> +    qemu_mutex_init(&txn->lock);
> +    txn->ref = 1; /* dropped by block_job_txn_begin() */
> +    txn->aborting = false;
> +    txn->jobs_pending = 0;
> +    QLIST_INIT(&txn->jobs);
> +    return txn;
> +}
> +
> +static void block_job_txn_unref(BlockJobTxn *txn)
> +{
> +    qemu_mutex_lock(&txn->lock);
> +
> +    if (--txn->ref > 0) {
> +        qemu_mutex_unlock(&txn->lock);
> +        return;
> +    }
> +
> +    qemu_mutex_unlock(&txn->lock);
> +    qemu_mutex_destroy(&txn->lock);
> +    g_free(txn);
> +}
> +
> +/* The purpose of this is to keep txn alive until all jobs have been added */
> +void block_job_txn_begin(BlockJobTxn *txn)
> +{
> +    block_job_txn_unref(txn);
> +}
> +
> +void block_job_txn_add_job(BlockJobTxn *txn, BlockJob *job)
> +{
> +    if (!txn) {
> +        return;
> +    }

Do you plan on making use of this case? I'm asking because while I'm 
usually in favor of handling everything gracefully as long as it's easy 
to implement, here I can't think of a case where using NULL with this 
function makes sense, that is to me it would seem like the caller made 
some bad mistake. This in turn would mean that dereferencing a NULL 
pointer or failing an assertion were preferable to hiding that mistake.

Other than this small thing and that it doesn't compile (until patch 7, 
I presume), looks good.

Max