Change the manual deferment to commit_complete into the implicit
callback to job_exit.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/commit.c | 19 +++++--------------
1 file changed, 5 insertions(+), 14 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index 620666161b..5bed098d5f 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -68,19 +68,14 @@ static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base,
return 0;
}
-typedef struct {
- int ret;
-} CommitCompleteData;
-
-static void commit_complete(Job *job, void *opaque)
+static void commit_exit(Job *job)
{
CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
BlockJob *bjob = &s->common;
- CommitCompleteData *data = opaque;
BlockDriverState *top = blk_bs(s->top);
BlockDriverState *base = blk_bs(s->base);
BlockDriverState *commit_top_bs = s->commit_top_bs;
- int ret = data->ret;
+ int ret = job->ret;
bool remove_commit_top_bs = false;
/* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
@@ -117,9 +112,6 @@ static void commit_complete(Job *job, void *opaque)
* bdrv_set_backing_hd() to fail. */
block_job_remove_all_bdrv(bjob);
- job_completed(job, ret);
- g_free(data);
-
/* 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. */
@@ -132,12 +124,12 @@ static void commit_complete(Job *job, void *opaque)
bdrv_unref(commit_top_bs);
bdrv_unref(top);
+ job->ret = ret;
}
static void coroutine_fn commit_run(void *opaque)
{
CommitBlockJob *s = opaque;
- CommitCompleteData *data;
int64_t offset;
uint64_t delay_ns = 0;
int ret = 0;
@@ -210,9 +202,7 @@ static void coroutine_fn commit_run(void *opaque)
out:
qemu_vfree(buf);
- data = g_malloc(sizeof(*data));
- data->ret = ret;
- job_defer_to_main_loop(&s->common.job, commit_complete, data);
+ s->common.job.ret = ret;
}
static const BlockJobDriver commit_job_driver = {
@@ -223,6 +213,7 @@ static const BlockJobDriver commit_job_driver = {
.user_resume = block_job_user_resume,
.drain = block_job_drain,
.start = commit_run,
+ .exit = commit_exit,
},
};
--
2.14.4
On Tue, Aug 07, 2018 at 12:33:32AM -0400, John Snow wrote:
> Change the manual deferment to commit_complete into the implicit
> callback to job_exit.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/commit.c | 19 +++++--------------
> 1 file changed, 5 insertions(+), 14 deletions(-)
>
> diff --git a/block/commit.c b/block/commit.c
> index 620666161b..5bed098d5f 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -68,19 +68,14 @@ static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base,
> return 0;
> }
>
> -typedef struct {
> - int ret;
> -} CommitCompleteData;
> -
> -static void commit_complete(Job *job, void *opaque)
> +static void commit_exit(Job *job)
> {
> CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> BlockJob *bjob = &s->common;
> - CommitCompleteData *data = opaque;
> BlockDriverState *top = blk_bs(s->top);
> BlockDriverState *base = blk_bs(s->base);
> BlockDriverState *commit_top_bs = s->commit_top_bs;
> - int ret = data->ret;
> + int ret = job->ret;
> bool remove_commit_top_bs = false;
>
> /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
Since 'ret' is only assigned to 'job->ret' outside of this line, we could
drop the local 'ret' and assign job->ret to the output of
brdrv_drop_intermediate() in this code below:
if (!job_is_cancelled(job) && ret == 0) {
/* success */
ret = bdrv_drop_intermediate(s->commit_top_bs, base,
s->backing_file_str);
}
> @@ -117,9 +112,6 @@ static void commit_complete(Job *job, void *opaque)
> * bdrv_set_backing_hd() to fail. */
> block_job_remove_all_bdrv(bjob);
>
> - job_completed(job, ret);
> - g_free(data);
> -
> /* 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. */
> @@ -132,12 +124,12 @@ static void commit_complete(Job *job, void *opaque)
>
> bdrv_unref(commit_top_bs);
> bdrv_unref(top);
> + job->ret = ret;
We could then drop this line as well.
(This may fall under bike shedding, so I don't feel strongly about it):
Reviewed-by: Jeff Cody <jcody@redhat.com>
> }
>
> static void coroutine_fn commit_run(void *opaque)
> {
> CommitBlockJob *s = opaque;
> - CommitCompleteData *data;
> int64_t offset;
> uint64_t delay_ns = 0;
> int ret = 0;
> @@ -210,9 +202,7 @@ static void coroutine_fn commit_run(void *opaque)
> out:
> qemu_vfree(buf);
>
> - data = g_malloc(sizeof(*data));
> - data->ret = ret;
> - job_defer_to_main_loop(&s->common.job, commit_complete, data);
> + s->common.job.ret = ret;
> }
>
> static const BlockJobDriver commit_job_driver = {
> @@ -223,6 +213,7 @@ static const BlockJobDriver commit_job_driver = {
> .user_resume = block_job_user_resume,
> .drain = block_job_drain,
> .start = commit_run,
> + .exit = commit_exit,
> },
> };
>
> --
> 2.14.4
>
On 08/07/2018 11:41 PM, Jeff Cody wrote:
> On Tue, Aug 07, 2018 at 12:33:32AM -0400, John Snow wrote:
>> Change the manual deferment to commit_complete into the implicit
>> callback to job_exit.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> block/commit.c | 19 +++++--------------
>> 1 file changed, 5 insertions(+), 14 deletions(-)
>>
>> diff --git a/block/commit.c b/block/commit.c
>> index 620666161b..5bed098d5f 100644
>> --- a/block/commit.c
>> +++ b/block/commit.c
>> @@ -68,19 +68,14 @@ static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base,
>> return 0;
>> }
>>
>> -typedef struct {
>> - int ret;
>> -} CommitCompleteData;
>> -
>> -static void commit_complete(Job *job, void *opaque)
>> +static void commit_exit(Job *job)
>> {
>> CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
>> BlockJob *bjob = &s->common;
>> - CommitCompleteData *data = opaque;
>> BlockDriverState *top = blk_bs(s->top);
>> BlockDriverState *base = blk_bs(s->base);
>> BlockDriverState *commit_top_bs = s->commit_top_bs;
>> - int ret = data->ret;
>> + int ret = job->ret;
>> bool remove_commit_top_bs = false;
>>
>> /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
>
> Since 'ret' is only assigned to 'job->ret' outside of this line, we could
> drop the local 'ret' and assign job->ret to the output of
> brdrv_drop_intermediate() in this code below:
>
> if (!job_is_cancelled(job) && ret == 0) {
> /* success */
> ret = bdrv_drop_intermediate(s->commit_top_bs, base,
> s->backing_file_str);
> }
>
>> @@ -117,9 +112,6 @@ static void commit_complete(Job *job, void *opaque)
>> * bdrv_set_backing_hd() to fail. */
>> block_job_remove_all_bdrv(bjob);
>>
>> - job_completed(job, ret);
>> - g_free(data);
>> -
>> /* 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. */
>> @@ -132,12 +124,12 @@ static void commit_complete(Job *job, void *opaque)
>>
>> bdrv_unref(commit_top_bs);
>> bdrv_unref(top);
>> + job->ret = ret;
>
> We could then drop this line as well.
>
> (This may fall under bike shedding, so I don't feel strongly about it):
You're right; though most of it gets blown out of the water in #16
anyway, so it doesn't stay a problem for long.
--js
>
> Reviewed-by: Jeff Cody <jcody@redhat.com>
>
Thanks!
>> }
>>
>> static void coroutine_fn commit_run(void *opaque)
>> {
>> CommitBlockJob *s = opaque;
>> - CommitCompleteData *data;
>> int64_t offset;
>> uint64_t delay_ns = 0;
>> int ret = 0;
>> @@ -210,9 +202,7 @@ static void coroutine_fn commit_run(void *opaque)
>> out:
>> qemu_vfree(buf);
>>
>> - data = g_malloc(sizeof(*data));
>> - data->ret = ret;
>> - job_defer_to_main_loop(&s->common.job, commit_complete, data);
>> + s->common.job.ret = ret;
>> }
>>
>> static const BlockJobDriver commit_job_driver = {
>> @@ -223,6 +213,7 @@ static const BlockJobDriver commit_job_driver = {
>> .user_resume = block_job_user_resume,
>> .drain = block_job_drain,
>> .start = commit_run,
>> + .exit = commit_exit,
>> },
>> };
>>
>> --
>> 2.14.4
>>
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
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
Am 15.08.2018 um 22:52 hat John Snow geschrieben:
> 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.
I'm really only talking about the commit job, the specific conversion
in this patch and the terse commit message.
Kevin
© 2016 - 2025 Red Hat, Inc.