Re: [Qemu-block] [PATCH v5 33/42] blockdev: Fix active commit choice

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> We have to perform an active commit whenever the top node has a parent
> that has taken the WRITE permission on it.
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>   blockdev.c | 24 +++++++++++++++++++++---
>   1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index a464cabf9e..5370f3b738 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3294,6 +3294,7 @@ void qmp_block_commit(bool has_job_id, const char 
> *job_id, const char *device,
>        */
>       BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
>       int job_flags = JOB_DEFAULT;
> +    uint64_t top_perm, top_shared;
>   
>       if (!has_speed) {
>           speed = 0;
> @@ -3406,14 +3407,31 @@ void qmp_block_commit(bool has_job_id, const char 
> *job_id, const char *device,
>           goto out;
>       }
>   
> -    if (top_bs == bs) {
> +    /*
> +     * Active commit is required if and only if someone has taken a
> +     * WRITE permission on the top node.  Historically, we have always
> +     * used active commit for top nodes, so continue that practice.
> +     * (Active commit is never really wrong.)

Hmm, if we start active commit when nobody has write access, than
we leave a possibility to someone to get this access during commit. And during
passive commit write access is blocked. So, may be right way is do active commit
always? Benefits:
1. One code path. and it shouldn't be worse when no writers, without guest 
writes
mirror code shouldn't work worse than passive commit, if it is, it should be 
fixed.
2. Possibility of write access if user needs it during commit
3. I'm sure that active commit (mirror code) actually works faster, as it uses
async requests and smarter handling of block status.

> +     */
> +    bdrv_get_cumulative_perm(top_bs, &top_perm, &top_shared);
> +    if (top_perm & BLK_PERM_WRITE ||
> +        bdrv_skip_rw_filters(top_bs) == bdrv_skip_rw_filters(bs))
> +    {
>           if (has_backing_file) {
>               error_setg(errp, "'backing-file' specified,"
>                                " but 'top' is the active layer");
>               goto out;
>           }
> -        commit_active_start(has_job_id ? job_id : NULL, bs, base_bs,
> -                            job_flags, speed, on_error,
> +        if (!has_job_id) {
> +            /*
> +             * Emulate here what block_job_create() does, because it
> +             * is possible that @bs != @top_bs (the block job should
> +             * be named after @bs, even if @top_bs is the actual
> +             * source)
> +             */
> +            job_id = bdrv_get_device_name(bs);
> +        }
> +        commit_active_start(job_id, top_bs, base_bs, job_flags, speed, 
> on_error,
>                               filter_node_name, NULL, NULL, false, 
> &local_err);
>       } else {
>           BlockDriverState *overlay_bs = bdrv_find_overlay(bs, top_bs);
> 


-- 
Best regards,
Vladimir