Re: [Qemu-devel] [PATCH 04/21] block/commit: utilize job_exit shim

[John Snow <jsnow@redhat.com>]

On 08/08/2018 12:29 PM, Kevin Wolf wrote:
> Am 07.08.2018 um 06:33 hat John Snow geschrieben:
>> Change the manual deferment to commit_complete into the implicit
>> callback to job_exit.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> There is one tricky thing in this patch that the commit message could be
> a bit more explicit about, which is moving job_completed() to a later
> point.
> 
> This is the code that happens between the old call of job_completed()
> and the new one:
> 
>     /* If bdrv_drop_intermediate() didn't already do that, remove the commit
>      * filter driver from the backing chain. Do this as the final step so that
>      * the 'consistent read' permission can be granted.  */
>     if (remove_commit_top_bs) {
>         bdrv_child_try_set_perm(commit_top_bs->backing, 0, BLK_PERM_ALL,
>                                 &error_abort);
>         bdrv_replace_node(commit_top_bs, backing_bs(commit_top_bs),
>                           &error_abort);
>     }
> 
>     bdrv_unref(commit_top_bs);
>     bdrv_unref(top);
> 
> As the comment states, bdrv_replace_node() requires that the permission
> restrictions that the commit job made are already lifted. The most
> important part is done by the explicit block_job_remove_all_bdrv() call
> right before this hunk. It still leaves bjob->blk around, which could
> have implications, but luckily we didn't take any permissions for that
> one:
> 
>     s = block_job_create(job_id, &commit_job_driver, NULL, bs, 0, BLK_PERM_ALL,
>                          speed, JOB_DEFAULT, NULL, NULL, errp);
> 
> So I think we got everything out of the way and bdrv_replace_node() can
> do what it wants to do.
> 
> Kevin
> 

I suppose it will be up to the author of a job to be aware of any
permissions they pick up at creation time that might have an effect on
the cleanup they wish to do during completion time.

--js