Re: [Qemu-devel] [PATCH v4 09/11] block: drive_backup transaction callback support

[John Snow <jsnow@redhat.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 = 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