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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2124 bytes --]

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?

> 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.)

Max

>  
> -    blk_drain(bjob->blk);
>      if (bjdrv->drain) {
>          bjdrv->drain(bjob);
>      }
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]