On Fri, 08/11 18:48, Kevin Wolf wrote:
> The patch below implements what I was thinking of, and it seems to fix
> this problem. However, you'll immediately run into the next one, which
> is a message like 'Conflicts with use by ide0-hd0 as 'root', which does
> not allow 'write' on #block172'.
>
> The reason for this is that since commit 4417ab7adf1,
> bdrv_invalidate_cache() not only activates the image, but also is the
> signal for guest device BlockBackends that migration has completed and
> they should now request exclusive access to the image.
>
> As the NBD server shows, this assumption is wrong. Images can be
> activated even before migration completes. Maybe this really needs to
> be done in a VM state change handler.
>
> We can't simply revert commit 4417ab7adf1 because for image file
> locking, it is important that we drop locks for inactive images, so
> BdrvChildRole.activate/inactivate will probably stay. Maybe only one
> bool blk->disable_perm isn't enough, we might need a different one for
> devices before migration completed, or at least a counter.
I'll make your diff a formal patch and add a VM state change handler for 2.10.
I'm not very confident with a counter because it's not obviour to me that
inactivate/activate pairs are always balanced.
>
> I'll be on vacation starting tomorrow, so someone else needs to tackle
> this. Fam, I think you are relatively familiar with the op blockers?
>
> Kevin
>
>
> diff --git a/nbd/server.c b/nbd/server.c
> index 82a78bf439..b68b6fb535 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1045,11 +1045,22 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t
> dev_offset, off_t size,
> bool writethrough, BlockBackend *on_eject_blk,
> Error **errp)
> {
> + AioContext *ctx;
> BlockBackend *blk;
> NBDExport *exp = g_malloc0(sizeof(NBDExport));
> uint64_t perm;
> int ret;
>
> + /*
> + * NBD exports are used for non-shared storage migration. Make sure
> + * that BDRV_O_INACTIVE is cleared and the image is ready for write
> + * access since the export could be available before migration handover.
> + */
> + ctx = bdrv_get_aio_context(bs);
> + aio_context_acquire(ctx);
> + bdrv_invalidate_cache(bs, NULL);
> + aio_context_release(ctx);
> +
> /* Don't allow resize while the NBD server is running, otherwise we don't
> * care what happens with the node. */
> perm = BLK_PERM_CONSISTENT_READ;
> @@ -1087,15 +1098,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t
> dev_offset, off_t size,
> exp->eject_notifier.notify = nbd_eject_notifier;
> blk_add_remove_bs_notifier(on_eject_blk, &exp->eject_notifier);
> }
> -
> - /*
> - * NBD exports are used for non-shared storage migration. Make sure
> - * that BDRV_O_INACTIVE is cleared and the image is ready for write
> - * access since the export could be available before migration handover.
> - */
> - aio_context_acquire(exp->ctx);
> - blk_invalidate_cache(blk, NULL);
> - aio_context_release(exp->ctx);
> return exp;
>
> fail:
Fam
