[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH] blockjob: drain all job nodes in block_job_drai
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-block] [PATCH] blockjob: drain all job nodes in block_job_drain |
Date: |
Fri, 21 Jun 2019 08:41:18 +0000 |
19.06.2019 19:37, Max Reitz wrote:
> On 19.06.19 16:44, Vladimir Sementsov-Ogievskiy wrote:
>> Instead of draining additional nodes in each job code, let's do it in
>> common block_job_drain, draining just all job's children.
>>
>> It's also a first step to finally get rid of blockjob->blk.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>
>> Hi all!
>>
>> As a follow-up for "block: drop bs->job" recently merged, I'm now trying
>> to drop BlockJob.blk pointer, jobs really works with several nodes and
>> now reason to keep special blk for one of the children, and no reason to
>> handle nodes differently in, for example, backup code..
>>
>> And as a first step I need to sort out block_job_drain, and here is my
>> suggestion on it.
>>
>> block/backup.c | 18 +-----------------
>> block/mirror.c | 26 +++-----------------------
>> blockjob.c | 7 ++++++-
>> 3 files changed, 10 insertions(+), 41 deletions(-)
>
> Looks good to me. Two questions though:
>
> Would it make sense to remove BlockJobDriver.drain() now? I think
> everything that isn’t “drain the attached nodes” should be handled by
> JobDriver.pause(), no?
Hmm, anyway it becomes unused, and it's usually better to not leave unused
things,
which we can repair in future if needed. Will do.
>
>> diff --git a/blockjob.c b/blockjob.c
>> index 458ae76f51..0cabdc867d 100644
>> --- a/blockjob.c
>> +++ b/blockjob.c
>> @@ -94,8 +94,13 @@ void block_job_drain(Job *job)
>> BlockJob *bjob = container_of(job, BlockJob, job);
>> const JobDriver *drv = job->driver;
>> BlockJobDriver *bjdrv = container_of(drv, BlockJobDriver, job_driver);
>> + GSList *l;
>> +
>> + for (l = bjob->nodes; l; l = l->next) {
>> + BdrvChild *c = l->data;
>> + bdrv_drain(c->bs);
>> + }
>
> Could it be more efficient to bdrv_drained_begin() all nodes in one loop
> and then bdrv_drained_end() them all in a second one?
>
> (Draining a node means draining its parents, and that is quicker if
> they’re drained already. If the nodes are in a chain, just using
> bdrv_drain() may mean some nodes are drained and undrained a couple of
> times.)
>
Understand, OK, will do and resend today. Thanks for reviewing!
>>
>> - blk_drain(bjob->blk);
>> if (bjdrv->drain) {
>> bjdrv->drain(bjob);
>> }
>>
>
>
--
Best regards,
Vladimir