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

[Kevin Wolf]

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