12.08.2019 16:23, Kevin Wolf wrote: > Am 09.08.2019 um 15:18 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 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() >> >> ... >> >> 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... > > Do the patches to switch backup to a filter node solve this > automatically because that node would be inserted in > backup_job_create()? >
Hmm, great, looks like they should. At least it moves scope of the problem to do_drive_backup and do_blockdev_backup functions.. Am I right that aio_context_acquire/aio_context_release guarantees no new request created during the section? Or should we add drained_begin/drained_end pair, or at least drain() at start of qmp_blockdev_backup and qmp_drive_backup? Assume scenario like the this, 1. fsfreeze 2. qmp backup 3. fsthaw to make sure that backup starting point is consistent. So in our qmp command we should: 1. complete all current requests to make drives corresponding to fsfreeze point 2. initialize write-notifiers or filter before any new guest request, i.e. before fsthaw, i.e. before qmp command return. Transactions should be OK, as they use drained_begin/drained_end pairs, and additional aio_context_acquire/aio_context_release pairs. -- Best regards, Vladimir
