Re: [Qemu-devel] [PATCH] blockjob: drain all job nodes in block_job_drain - Vladimir Sementsov-Ogievskiy

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Max Reitz <mreitz@redhat.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"qemu-block@nongnu.org" <qemu-block@nongnu.org>
Cc: "kwolf@redhat.com" <kwolf@redhat.com>,
	"jsnow@redhat.com" <jsnow@redhat.com>
Subject: Re: [Qemu-devel] [PATCH] blockjob: drain all job nodes in block_job_drain
Date: Fri, 21 Jun 2019 08:41:18 +0000	[thread overview]
Message-ID: <3316404f-cfc7-6fdc-4427-1bb06d559f01@virtuozzo.com> (raw)
In-Reply-To: <94df77c0-c44a-49c8-ab16-6a637c55088c@redhat.com>

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 <vsementsov@virtuozzo.com>
>> ---
>>
>> 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