Re: [Qemu-block] [Qemu-devel] backup bug or question

[Vladimir Sementsov-Ogievskiy]

09.08.2019 23:13, John Snow wrote:
> 
> 
> On 8/9/19 9:18 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Hi!
>>
>> Hmm, hacking around backup I have a question:
>>
>> What prevents guest write request after job_start but before setting
>> write notifier?
>>
>> code path:
>>
>> qmp_drive_backup or transaction with backup
>>
>>      job_start
>>         aio_co_enter(job_co_entry) /* may only schedule execution, isn't it 
>> ? */
>>
>> ....
>>
>> job_co_entry
>>      job_pause_point() /* it definitely yields, isn't it bad? */
>>      job->driver->run() /* backup_run */
>>
>> ----
>>
>> backup_run()
>>      bdrv_add_before_write_notifier()
>>
>> ...
>>
> 
> I think you're right... :(
> 
> 
> We create jobs like this:
> 
> job->paused        = true;
> job->pause_count   = 1;
> 
> 
> And then job_start does this:
> 
> job->co = qemu_coroutine_create(job_co_entry, job);
> job->pause_count--;
> job->busy = true;
> job->paused = false;
> 
> 
> Which means that job_co_entry is being called before we lift the pause:
> 
> assert(job && job->driver && job->driver->run);
> job_pause_point(job);
> job->ret = job->driver->run(job, &job->err);
> 
> ...Which means that we are definitely yielding in job_pause_point.
> 
> Yeah, that's a race condition waiting to happen.
> 
>> And what guarantees we give to the user? Is it guaranteed that write 
>> notifier is
>> set when qmp command returns?
>>
>> And I guess, if we start several backups in a transaction it should be 
>> guaranteed
>> that the set of backups is consistent and correspond to one point in time...
>>
> 
> I would have hoped that maybe the drain_all coupled with the individual
> jobs taking drain_start and drain_end would save us, but I guess we
> simply don't have a guarantee that all backup jobs WILL have installed
> their handler by the time the transaction ends.
> 
> Or, if there is that guarantee, I don't know what provides it, so I
> think we shouldn't count on it accidentally working anymore.
> 
> 
> 
> I think we should do two things:
> 
> 1. Move the handler installation to creation time.
> 2. Modify backup_before_write_notify to return without invoking
> backup_do_cow if the job isn't started yet.
> 

Hmm, I don't see, how it helps.. No-op write-notifier will not save as from
guest write, is it?


-- 
Best regards,
Vladimir