Re: [Qemu-devel] [PATCH v3 10/15] block: add block job transactions

[Stefan Hajnoczi]

On Fri, Jul 10, 2015 at 11:46:47AM +0800, Fam Zheng wrote:
> From: Stefan Hajnoczi <address@hidden>

Please take authorship, most of the code is yours.

> +static void block_job_completed_txn(BlockJobTxn *txn, BlockJob *job, int ret)
> +{
> +    AioContext *ctx;
> +    BlockJob *other_job, *next;
> +    if (ret < 0 || block_job_is_cancelled(job)) {
> +        if (!txn->aborting) {
> +            /* We are the first failed job. Cancel other jobs. */
> +            txn->aborting = true;
> +            QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) {
> +                if (other_job == job || other_job->completed) {

You didn't leave any clues as to why other_job->complete is safe to
access outside its AioContext.

Perhaps because it's only modified from the main loop and we are in the
main loop too?

Please either access it inside the AioContext or add a comment
explaining why it is safe.

> +                    continue;
> +                }
> +                ctx = bdrv_get_aio_context(other_job->bs);
> +                aio_context_acquire(ctx);
> +                block_job_cancel_sync(other_job);
> +                assert(other_job->completed);
> +                aio_context_release(ctx);
> +            }
> +            QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) {
> +                if (other_job->driver->txn_abort) {
> +                    other_job->driver->txn_abort(other_job);
> +                }
> +                other_job->cb(other_job->opaque,
> +                              other_job->ret ? : -ECANCELED);

Please don't add ECANCELED here since block_job_completed() doesn't do
that either.  We should be consistent: currently cb() needs to check
block_job_is_cancelled() to determine whether the job was cancelled.

Also, if a job finished but another job aborts, then we need to mark the
first job as cancelled (other_job->cancelled == true).

> +                block_job_release(other_job->bs);
> +            }

No AioContext acquire/release in this loop?  Remember the job's bs can
still be accessed from another thread while we're running.

> +        } else {
> +            /*
> +             * We are cancelled by another job, who will handle everything.
> +             */
> +            return;
> +        }
> +    } else {
> +        /*
> +         * Successful completion, see if there is other running jobs in this

s/is/are/ (plural)

> +         * txn. */
> +        QLIST_FOREACH(other_job, &txn->jobs, txn_list) {
> +            if (!other_job->completed) {
> +                return;
> +            }
> +        }
> +        /* We are the last completed job, commit the transaction. */
> +        QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) {
> +            if (other_job->driver->txn_commit) {
> +                other_job->driver->txn_commit(other_job);
> +            }
> +            assert(other_job->ret == 0);
> +            other_job->cb(other_job->opaque, 0);
> +            block_job_release(other_job->bs);
> +        }

More other_job field accesses outside AioContext that need to be checked
and probably protected using acquire/release.

> +/**
> + * block_job_txn_new:
> + *
> + * Allocate and return a new block job transaction.  Jobs can be added to the
> + * transaction using block_job_txn_add_job().
> + *
> + * All jobs in the transaction either complete successfully or fail/cancel 
> as a
> + * group.  Jobs wait for each other before completing.  Cancelling one job
> + * cancels all jobs in the transaction.
> + */
> +BlockJobTxn *block_job_txn_new(void);

The doc comments says "Allocate and return a new block job transaction"
but does not explain how/when the object is freed.  Please add a
sentence along the lines of: The transaction is automatically freed when
the last job completes or is cancelled.

pgp3jYuISxeHL.pgp
Description: PGP signature