[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v6 30/33] include/block/block_int-common.h: introduce bdrv_am

From: Hanna Reitz
Subject: Re: [PATCH v6 30/33] include/block/block_int-common.h: introduce bdrv_amend_pre_run and bdrv_amend_clean
Date: Wed, 26 Jan 2022 15:54:53 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0

On 21.01.22 18:05, Emanuele Giuseppe Esposito wrote:
These two callbacks will be invoked by job callbacks to execute
driver-specific code while still being in BQL.
In this example, we want the amend JobDriver to execute the
permission check (bdrv_child_refresh_perms) currently only
done in block/crypto.c block_crypto_amend_options_generic_luks()
to all its bdrv.
This is achieved by introducing callbacks in the JobDriver, but
we also need to make sure that crypto->updating_keys is true
before refreshing the permissions the first time, so that
WRITE perm is temporarly given to qcrypto_block_amend_options(),
and set it to false when permissions are restored.

Therefore bdrv_amend_pre_run() and bdrv_amend_clean() will take care of
just temporarly setting the crypto-specific updating_keys flag.

Note that at this stage, they are not yet invoked.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
  block/crypto.c                   | 16 ++++++++++++++++
  include/block/block_int-common.h | 13 +++++++++++++
  2 files changed, 29 insertions(+)

diff --git a/block/crypto.c b/block/crypto.c
index c8ba4681e2..f5e0c7b7c0 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -777,6 +777,20 @@ block_crypto_get_specific_info_luks(BlockDriverState *bs, 
Error **errp)
      return spec_info;
+static void
+block_crypto_amend_pre_run(BlockDriverState *bs)
+    BlockCrypto *crypto = bs->opaque;
+    crypto->updating_keys = true;

So I see you decided to leave actually updating the permissions to the caller, i.e. blockdev_amend_pre_run().  Why?

I’m asking because:

(1) If the .bdrv_amend_pre_run() isn’t implemented, I don’t think refreshing the permissions in blockdev_amend_pre_run() is necessary.  So if it is implemented, the implementation might as well do it itself.

(2) The way you implemented it, it’s actually kind of hard to see why this is even necessary.  Both of these functions (block_crypto_amend_{pre_run,cleanup}()) don’t require the BQL, so the explanation for .bdrv_amend_pre_run() (“useful to perform driver-specific initialization code under BQL”) doesn’t really apply.  If you want to explain (and we should) why this is necessary, then the .bdrv_amend_pre_run() documentation needs to state that we will refresh the permissions after this function has run and before .bdrv_co_amend() will run, and so it’s also useful to put code here that will change the permissions on permission update, but that just really gets complicated to explain.  Naively, I find it simpler to just say “Put BQL code here, this will run before .bdrv_co_amend()” and have every implementation that needs it refresh the permissions itself.

(3) In patch 32, you add block_crypto_amend_options_{prepare,cleanup}().  If the functions added here (block_crypto_amend_{pre_run,cleanup}()) would refresh the permissions by themselves, they’d be exactly the same functions, so you could reuse the ones from here in patch 32.

+static void
+block_crypto_amend_cleanup(BlockDriverState *bs)
+    BlockCrypto *crypto = bs->opaque;
+    crypto->updating_keys = false;
  static int
  block_crypto_amend_options_generic_luks(BlockDriverState *bs,
@@ -931,6 +945,8 @@ static BlockDriver bdrv_crypto_luks = {
      .bdrv_get_specific_info = block_crypto_get_specific_info_luks,
      .bdrv_amend_options = block_crypto_amend_options_luks,
      .bdrv_co_amend      = block_crypto_co_amend_luks,
+    .bdrv_amend_pre_run       = block_crypto_amend_pre_run,
+    .bdrv_amend_clean         = block_crypto_amend_cleanup,
.is_format = true, diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index cc8c8835ba..9d28396978 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -189,6 +189,19 @@ struct BlockDriver {
       * the GS API.
+ /*
+     * Called inside job->pre_run() callback, it is useful
+     * to perform driver-specific initialization code under
+     * BQL, like setting up specific permission flags.

I wouldn’t necessarily state that this function is called by `job->pre_run()`, but rather paint the picture of how it’s used together with `.bdrv_co_amend()`, e.g. something like:

“This function is invoked under the BQL before .bdrv_co_amend() (which in contrast does not necessarily run under the BQL) to allow driver-specific initialization code that requires the BQL, like setting up specific permission flags.”

(Though this comment should be much more specific if we keep updating the permissions in the caller, because then the comment also needs to explain that we do that.)

+     */
+    void (*bdrv_amend_pre_run)(BlockDriverState *bs);
+    /*
+     * Called inside job->clean() callback, it undoes
+     * the driver-specific initialization code done in amend_pre_run.
+     * Also this function is under BQL.

Here too I’d omit the job reference and just say that (e.g.)

“This function is invoked under the BQL after .bdrv_co_amend() to allow cleaning up what was done in .bdrv_amend_pre_run().”


+     */
+    void (*bdrv_amend_clean)(BlockDriverState *bs);
       * Return true if @to_replace can be replaced by a BDS with the
       * same data as @bs without it affecting @bs's behavior (that is,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]