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 <mre...@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 <vsement...@virtuozzo.com> > >>> + */ >>> + 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