From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41142) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YuNMf-0007eG-VG for qemu-devel@nongnu.org; Mon, 18 May 2015 11:54:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YuNMf-0000MO-3O for qemu-devel@nongnu.org; Mon, 18 May 2015 11:54:01 -0400 Message-ID: <555A0B13.6010304@redhat.com> Date: Mon, 18 May 2015 11:53:55 -0400 From: John Snow MIME-Version: 1.0 References: <1431385466-4868-1-git-send-email-jsnow@redhat.com> <1431385466-4868-10-git-send-email-jsnow@redhat.com> <20150518153539.GA27654@stefanha-thinkpad.redhat.com> In-Reply-To: <20150518153539.GA27654@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 09/11] block: drive_backup transaction callback support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, famz@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org, mreitz@redhat.com, vsementsov@parallels.com On 05/18/2015 11:35 AM, Stefan Hajnoczi wrote: > On Mon, May 11, 2015 at 07:04:24PM -0400, John Snow wrote: >> +static void drive_backup_cb(BlkActionState *common) +{ + >> BlkActionCallbackData *cb_data =3D common->cb_data; + >> BlockDriverState *bs =3D cb_data->opaque; + DriveBackupState >> *state =3D DO_UPCAST(DriveBackupState, common, common); + + >> assert(state->bs =3D=3D bs); + if (bs->job) { + >> assert(state->job =3D=3D bs->job); + } >=20 > What is the purpose of the if statement? >=20 > Why is it not okay for a new job to have started? >=20 Hmm, maybe it's fine -- It was just my thought that it probably /shouldn't/ occur under normal circumstances. I think my assumption was that we want to impose an ordering that job cleanup occurs before another job launches, in general. I think, though, that you wanted to start allowing non-conflicting jobs to run concurrently, though, so I'll just eye over this series again to make sure it's okay for cleanup to happen after another job starts ... ...Provided the second job does not fiddle with bitmaps, of course. We should clean those up before another bitmap job starts, definitely. >> + + state->aio_context =3D bdrv_get_aio_context(bs); + >> aio_context_acquire(state->aio_context); >=20 > The bs->job access above should be protected by > aio_context_acquire(). >=20 Thanks, --js