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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3530 bytes --]

On 19.06.19 11:31, Vladimir Sementsov-Ogievskiy wrote:
> 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 <mreitz@redhat.com>
>> ---
>>   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.

Isn’t that blocked by the commit filter?  If it isn’t, it should be.

> 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.

Disadvantage: Something may break because the basic commit job does not
emit BLOCK_JOB_READY and thus does not require block-job-complete.

Technically everything should expect jobs to potentially emit
BLOCK_JOB_READY, but who knows.  I think we’d want at least a
deprecation period.

Max

>> +     */
>> +    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);
>>
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]