[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] blockjob: drain all job nodes in block_job_drai
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH] blockjob: drain all job nodes in block_job_drain |
Date: |
Wed, 19 Jun 2019 18:37:25 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 |
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?
> 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);
> }
>
signature.asc
Description: OpenPGP digital signature