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 <[email protected]> >> --- >> 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);
>>
>
>
signature.asc
Description: OpenPGP digital signature
