Re: [PATCH 0/2] block-backend: Retain permissions after migration
On 11/25/2021 9:53 PM, Hanna Reitz wrote: > Hi, > > Peng Liang has reported an issue regarding migration of raw images here: > https://lists.nongnu.org/archive/html/qemu-block/2021-11/msg00673.html > > It turns out that after migrating, all permissions are shared when they > weren’t before. The cause of the problem is that we deliberately delay > restricting the shared permissions until migration is really done (until > the runstate is no longer INMIGRATE) and first share all permissions; > but this causes us to lose the original shared permission mask and > overwrites it with BLK_PERM_ALL, so once we do try to restrict the > shared permissions, we only again share them all. > > Fix this by saving the set of shared permissions through the first > blk_perm_set() call that shares all; and add a regression test. > > > I don’t believe we have to fix this in 6.2, because I think this bug has > existed for four years now. (I.e. it isn’t critical, and it’s no > regression.) > > > Hanna Reitz (2): > block-backend: Retain permissions after migration > iotests/migration-permissions: New test > > block/block-backend.c | 11 ++ > .../qemu-iotests/tests/migration-permissions | 101 ++ > .../tests/migration-permissions.out | 5 + > 3 files changed, 117 insertions(+) > create mode 100755 tests/qemu-iotests/tests/migration-permissions > create mode 100644 tests/qemu-iotests/tests/migration-permissions.out > Hi Hanna, QEMU 6.3 development tree has been opened. Will this fix be merged in 6.3? Thanks, Peng
Re: [PATCH 1/2] block-backend: Retain permissions after migration
On 11/25/2021 9:53 PM, Hanna Reitz wrote: > After migration, the permissions the guest device wants to impose on its > BlockBackend are stored in blk->perm and blk->shared_perm. In > blk_root_activate(), we take our permissions, but keep all shared > permissions open by calling `blk_set_perm(blk->perm, BLK_PERM_ALL)`. > > Only afterwards (immediately or later, depending on the runstate) do we > restrict the shared permissions by calling > `blk_set_perm(blk->perm, blk->shared_perm)`. Unfortunately, our first > call with shared_perm=BLK_PERM_ALL has overwritten blk->shared_perm to > be BLK_PERM_ALL, so this is a no-op and the set of shared permissions is > not restricted. > > Fix this bug by saving the set of shared permissions before invoking > blk_set_perm() with BLK_PERM_ALL and restoring it afterwards. > > Fixes: 5f7772c4d0cf32f4e779fcd5a69ae4dae24aeebf >("block-backend: Defer shared_perm tightening migration >completion") > Reported-by: Peng Liang > Signed-off-by: Hanna Reitz > --- > block/block-backend.c | 11 +++ > 1 file changed, 11 insertions(+) > Thanks for your patch! Tested-by: Peng Liang
Questions about losing the write lock of raw-format disks after migration
Hi folks, When we test migration with raw-format disk, we found that the QEMU process in the dst will lose the write lock after migration. However, the QEMU process in the dst will still hold the write lock for qcow2-format disk. After reading some block layer's code, I found that the first blk_set_perm in blk_root_activate will set blk->shared_perm to BLK_PERM_ALL (disable all shared permissions?). Then in blk_vm_state_changed, blk_set_perm will set shared_perm to blk->shared_perm, which is BLK_PERM_ALL. And it makes raw_handle_perm_lock not to get the write lock. So I try the following patch and it will fix the problem: diff --git a/block/block-backend.c b/block/block-backend.c index 12ef80ea17..96518fd1f0 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -197,13 +197,6 @@ static void blk_root_activate(BdrvChild *child, Error **errp) blk->disable_perm = false; -blk_set_perm(blk, blk->perm, BLK_PERM_ALL, &local_err); -if (local_err) { -error_propagate(errp, local_err); -blk->disable_perm = true; -return; -} - if (runstate_check(RUN_STATE_INMIGRATE)) { /* Activation can happen when migration process is still active, for * example when nbd_server_add is called during non-shared storage I'm new to the block layer and I'm not sure that it's a right fix to the problem. Any idea about the problem and the patch? Thanks, Peng