On 04/06/2017 06:14 PM, Stefan Hajnoczi wrote: > On Wed, Apr 05, 2017 at 06:12:05PM +0300, Denis V. Lunev wrote: >> We should avoid to image file at migration end when BDRV_O_INACTIVE > s/avoid to image/avoid writing to the image/ ? yes
>> is set on the block driver state. All modifications should be done >> in advance, i.e. in bdrv_inactivate. >> >> The patch also adds final bdrv_flush() to be sure that changes have >> reached disk finally before other side will access the image. > If migration fails/cancels on the source after .bdrv_inactivate() we > need to return to the "activated" state. .bdrv_invalidate_cache() will > be called so I think it needs to be implemented by parallels to set > s->header->inuse again if BDRV_O_RDWR. > > .bdrv_invalidate_cache() is also called on the destination at live > migration handover. That's okay - it makes sense for the destination to > set the inuse field on success. hmm. sounds like a good catch.. >> -static void parallels_close(BlockDriverState *bs) >> +static int parallels_inactivate(BlockDriverState *bs) >> { >> + int err; >> BDRVParallelsState *s = bs->opaque; >> >> - if (bs->open_flags & BDRV_O_RDWR) { >> - s->header->inuse = 0; >> - parallels_update_header(bs); >> + if (!(bs->open_flags & BDRV_O_RDWR)) { >> + return 0; >> + } >> + >> + s->header->inuse = 0; >> + err = parallels_update_header(bs); >> + if (err < 0) { > Should we set s->header->inuse again in all error paths in case > something calls parallels_hupdate_header() again later? yes. thank you :)