[0/5] block/amend: Fix failures seen in iotest 296

Message

Hanna Czenczek March 4, 2022, 3:37 p.m. UTC

Hi,

I’ve tried basing my block branch on Kevin’s and noticed that after
“crypto: perform permission checks under BQL”, iotest 296 was failing.
I/We have debugged those failures and here are fixes for it.

Hence, this series is based on Kevin’s block branch
(efa33ed9b298d39e2b8c19c5f4bdd80a3b632260 at the time of writing this
cover letter).  I’ve pushed it here:

  https://gitlab.com/hreitz/qemu/-/commits/amend-job-fixes-v1

Patch 1 adds clean-up of the amend job in an error path that said commit
adds to qmp_x_blockdev_amend().

Patch 2 changes the type of a JobDriver callback added in that commit;
together with patch 3, this is kind of a matter of style, but it can
also replace patch 3 and fix the bug that it fixes in another way.

Patch 3 fixes a permission bug: When changing the permissions fails
before amend, block/crypto will still keep updating_keys to be true.
Without patch 2, that will remains so indefinitely and then
block_crypto_child_perms() will continue to unshare the CONSISTENT_READ
permission, which is wrong.  (Patch 2 fixes this problem, too,
specifically because with it, block_crypto_amend_cleanup() will always
be called when the job is dismissed, and so updating_keys will be reset
at least then.)

Patch 4 fixes an issue that’s not related to “crypto: perform permission
checks under BQL”, but it became appearent only while debugging the
other issues here, so it’s part of this series, too.

Patch 5 fixes the test itself.  It expects permission-related errors to
occur when the job is already running, not as an immediate result of the
QMP x-blockdev-amend command.  “crypto: perform permission checks under
BQL” has changed this, so the test needs to take that into account.


Ideally, I believe the following patches should be squashed into
“crypto: perform permission checks under BQL” lest bisect breaks:
- Patch 1
- Patch 2 or 3 (or both)
- Patch 5

But if that isn’t feasible, we can just take the whole series on top.


Hanna Reitz (5):
  block/amend: Clean up job on early failure
  block/amend: Always call .bdrv_amend_clean()
  block/crypto: Reset updating_keys on perm failure
  block/amend: Keep strong reference to BDS
  iotests/296: Accept early failure

 block/amend.c              |  8 ++++++--
 block/crypto.c             |  8 +++++++-
 tests/qemu-iotests/296     |  8 ++++++--
 tests/qemu-iotests/296.out | 17 +++++------------
 4 files changed, 24 insertions(+), 17 deletions(-)

Comments

Kevin Wolf March 4, 2022, 4:16 p.m. UTC | #1

Am 04.03.2022 um 16:37 hat Hanna Reitz geschrieben:
> Hi,
> 
> I’ve tried basing my block branch on Kevin’s and noticed that after
> “crypto: perform permission checks under BQL”, iotest 296 was failing.
> I/We have debugged those failures and here are fixes for it.
> 
> Hence, this series is based on Kevin’s block branch
> (efa33ed9b298d39e2b8c19c5f4bdd80a3b632260 at the time of writing this
> cover letter).  I’ve pushed it here:
> 
>   https://gitlab.com/hreitz/qemu/-/commits/amend-job-fixes-v1
> 
> Patch 1 adds clean-up of the amend job in an error path that said commit
> adds to qmp_x_blockdev_amend().
> 
> Patch 2 changes the type of a JobDriver callback added in that commit;
> together with patch 3, this is kind of a matter of style, but it can
> also replace patch 3 and fix the bug that it fixes in another way.
> 
> Patch 3 fixes a permission bug: When changing the permissions fails
> before amend, block/crypto will still keep updating_keys to be true.
> Without patch 2, that will remains so indefinitely and then
> block_crypto_child_perms() will continue to unshare the CONSISTENT_READ
> permission, which is wrong.  (Patch 2 fixes this problem, too,
> specifically because with it, block_crypto_amend_cleanup() will always
> be called when the job is dismissed, and so updating_keys will be reset
> at least then.)
> 
> Patch 4 fixes an issue that’s not related to “crypto: perform permission
> checks under BQL”, but it became appearent only while debugging the
> other issues here, so it’s part of this series, too.
> 
> Patch 5 fixes the test itself.  It expects permission-related errors to
> occur when the job is already running, not as an immediate result of the
> QMP x-blockdev-amend command.  “crypto: perform permission checks under
> BQL” has changed this, so the test needs to take that into account.
> 
> 
> Ideally, I believe the following patches should be squashed into
> “crypto: perform permission checks under BQL” lest bisect breaks:
> - Patch 1
> - Patch 2 or 3 (or both)
> - Patch 5
> 
> But if that isn’t feasible, we can just take the whole series on top.

Thanks, I'm squashing in 1, 3 and 5 and taking 2 and 4 on top.

Kevin