Re: [Qemu-devel] [PATCH v3 5/6] blockjob: refactor backup_start as backup_job_create

[John Snow <jsnow@redhat.com>]

On 11/08/2016 04:11 AM, Kevin Wolf wrote:
> Am 08.11.2016 um 06:41 hat John Snow geschrieben:
>> On 11/03/2016 09:17 AM, Kevin Wolf wrote:
>>> Am 02.11.2016 um 18:50 hat John Snow geschrieben:
>>>> Refactor backup_start as backup_job_create, which only creates the job,
>>>> but does not automatically start it. The old interface, 'backup_start',
>>>> is not kept in favor of limiting the number of nearly-identical interfaces
>>>> that would have to be edited to keep up with QAPI changes in the future.
>>>>
>>>> Callers that wish to synchronously start the backup_block_job can
>>>> instead just call block_job_start immediately after calling
>>>> backup_job_create.
>>>>
>>>> Transactions are updated to use the new interface, calling block_job_start
>>>> only during the .commit phase, which helps prevent race conditions where
>>>> jobs may finish before we even finish building the transaction. This may
>>>> happen, for instance, during empty block backup jobs.
>>>>
>>>> Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>
>>>> +static void drive_backup_commit(BlkActionState *common)
>>>> +{
>>>> +    DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
>>>> +    if (state->job) {
>>>> +        block_job_start(state->job);
>>>> +    }
>>>> }
>>>
>>> How could state->job ever be NULL?
>>>
>>
>> Mechanical thinking. It can't. (I definitely didn't copy paste from
>> the .abort routines. Definitely.)
>>
>>> Same question for abort, and for blockdev_backup_commit/abort.
>>>
>>
>> Abort ... we may not have created the job successfully. Abort gets
>> called whether or not we made it to or through the matching
>> .prepare.
>
> Ah, yes, I always forget about this. It's so counterintuitive (and
> bdrv_reopen() actually works differently, it only aborts entries that
> have successfully been prepared).
>
> Is there a good reason why qmp_transaction() works this way, especially
> since we have a separate .clean function?
>
> Kevin
>

We just don't track which actions have succeeded or not, so we loop 
through all actions on each phase regardless.

I could add a little state enumeration (or boolean) to each action and I 
could adjust abort to only run on actions that either completed or 
failed, but in this case I think it still wouldn't change the text for 
.abort, because an action may fail before it got to creating the job, 
for instance.

Unless you'd propose undoing .prepare IN .prepare in failure cases, but 
why write abort code twice? I don't mind it living in .abort, personally.

--js