| Message ID | 1491405126-30591-2-git-send-email-den@openvz.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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/ ? > 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. > -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?
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 :)
diff --git a/block/parallels.c b/block/parallels.c index 90acf79..9dea09e 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -728,18 +728,35 @@ fail_options: goto fail; } - -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) { + return err; + } + + err = bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS); + if (err < 0) { + return err; } - if (bs->open_flags & BDRV_O_RDWR) { - bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS); + return bdrv_flush(bs->file->bs); +} + +static void parallels_close(BlockDriverState *bs) +{ + BDRVParallelsState *s = bs->opaque; + + if (!(bs->open_flags & BDRV_O_INACTIVE)) { + parallels_inactivate(bs); } g_free(s->bat_dirty_bmap); @@ -777,6 +794,7 @@ static BlockDriver bdrv_parallels = { .bdrv_co_flush_to_os = parallels_co_flush_to_os, .bdrv_co_readv = parallels_co_readv, .bdrv_co_writev = parallels_co_writev, + .bdrv_inactivate = parallels_inactivate, .bdrv_create = parallels_create, .bdrv_check = parallels_check,
We should avoid to image file at migration end when BDRV_O_INACTIVE 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. Signed-off-by: Denis V. Lunev <den@openvz.org> CC: Stefan Hajnoczi <stefanha@redhat.com> --- block/parallels.c | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-)