21.06.2019 16:26, Vladimir Sementsov-Ogievskiy wrote:
> 19.06.2019 18:59, Max Reitz wrote:
>> 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
>
> OK, so this is for future.. Then:
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Strange, I have this mail automatically returned back. Did you receive it?
>
>>
>>>> + */
>>>> + 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