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 = common->cb_data; + >> BlockDriverState *bs = cb_data->opaque; + DriveBackupState >> *state = DO_UPCAST(DriveBackupState, common, common); + + >> assert(state->bs == bs); + if (bs->job) { + >> assert(state->job == bs->job); + } > > What is the purpose of the if statement? > > Why is it not okay for a new job to have started? >
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 = bdrv_get_aio_context(bs); + >> aio_context_acquire(state->aio_context); > > The bs->job access above should be protected by > aio_context_acquire(). > Thanks, --js