From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41511) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZExRC-0005lg-T7 for qemu-devel@nongnu.org; Tue, 14 Jul 2015 06:27:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZExR9-0002N4-FL for qemu-devel@nongnu.org; Tue, 14 Jul 2015 06:27:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49061) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZExR9-0002Mh-8Y for qemu-devel@nongnu.org; Tue, 14 Jul 2015 06:27:43 -0400 Date: Tue, 14 Jul 2015 11:27:40 +0100 From: Stefan Hajnoczi Message-ID: <20150714102740.GF17927@stefanha-thinkpad.redhat.com> References: <1436500012-32593-1-git-send-email-famz@redhat.com> <1436500012-32593-11-git-send-email-famz@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Zi0sgQQBxRFxMTsj" Content-Disposition: inline In-Reply-To: <1436500012-32593-11-git-send-email-famz@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 10/15] block: add block job transactions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: Kevin Wolf , Jeff Cody , qemu-devel@nongnu.org, Max Reitz , vsementsov@parallels.com, John Snow --Zi0sgQQBxRFxMTsj Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Jul 10, 2015 at 11:46:47AM +0800, Fam Zheng wrote: > From: Stefan Hajnoczi 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. --Zi0sgQQBxRFxMTsj Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVpOQcAAoJEJykq7OBq3PI5kcH/A99gSL4cZUQQ2LMkJ4WsNgQ L6PJSmH4WDd+8UDmdJz+pEJoHI+XBS41losXycYYTF+qTE0bg3pvUHFm93jwuquo HjvqCJZoiXvFe6azTrZnPv8WyRrbGP/A8kcPk2GqC63TnNHfA8yQzJIDrrKz4RlN 6YljwqsWacNGD/qgLw/mjAms9041o+A/0TIo+ksJA+nE8QHRhGEdDu/tmFEQzFee ZJk9B7UsOgXmKbGYzLf5ILNORGbX6Yle+8QBjl7vZCjqrzTSeIP9eVS3SH3H92zo mruBnYV7vAD48znR3DQrj9dVFloovMIE2qtlPj8ZbzTMT0gEbe2YT+RjhHpiyjE= =rode -----END PGP SIGNATURE----- --Zi0sgQQBxRFxMTsj--