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 <vsement...@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); > } >
signature.asc
Description: OpenPGP digital signature