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

[Hanna Reitz <hreitz@redhat.com>]

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,
>                                           QCryptoBlockAmendOptions *amend_options,
> @@ -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().”

Hanna

> +     */
> +    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,