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

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

[Vladimir Sementsov-Ogievskiy]

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

diff --git a/block/backup.c b/block/backup.c
index 715e1d3be8..7930004bbd 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -320,21 +320,6 @@ void backup_do_checkpoint(BlockJob *job, Error **errp)
     hbitmap_set(backup_job->copy_bitmap, 0, backup_job->len);
 }
 
-static void backup_drain(BlockJob *job)
-{
-    BackupBlockJob *s = container_of(job, BackupBlockJob, common);
-
-    /* Need to keep a reference in case blk_drain triggers execution
-     * of backup_complete...
-     */
-    if (s->target) {
-        BlockBackend *target = s->target;
-        blk_ref(target);
-        blk_drain(target);
-        blk_unref(target);
-    }
-}
-
 static BlockErrorAction backup_error_action(BackupBlockJob *job,
                                             bool read, int error)
 {
@@ -493,8 +478,7 @@ static const BlockJobDriver backup_job_driver = {
         .commit                 = backup_commit,
         .abort                  = backup_abort,
         .clean                  = backup_clean,
-    },
-    .drain                  = backup_drain,
+    }
 };
 
 static int64_t backup_calculate_cluster_size(BlockDriverState *target,
diff --git a/block/mirror.c b/block/mirror.c
index d17be4cdbc..6bea99558f 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -644,14 +644,11 @@ static int mirror_exit_common(Job *job)
     bdrv_ref(mirror_top_bs);
     bdrv_ref(target_bs);
 
-    /* Remove target parent that still uses BLK_PERM_WRITE/RESIZE before
+    /*
+     * Remove target parent that still uses BLK_PERM_WRITE/RESIZE before
      * inserting target_bs at s->to_replace, where we might not be able to get
      * these permissions.
-     *
-     * Note that blk_unref() alone doesn't necessarily drop permissions because
-     * we might be running nested inside mirror_drain(), which takes an extra
-     * reference, so use an explicit blk_set_perm() first. */
-    blk_set_perm(s->target, 0, BLK_PERM_ALL, &error_abort);
+     */
     blk_unref(s->target);
     s->target = NULL;
 
@@ -1143,21 +1140,6 @@ static bool mirror_drained_poll(BlockJob *job)
     return !!s->in_flight;
 }
 
-static void mirror_drain(BlockJob *job)
-{
-    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
-
-    /* Need to keep a reference in case blk_drain triggers execution
-     * of mirror_complete...
-     */
-    if (s->target) {
-        BlockBackend *target = s->target;
-        blk_ref(target);
-        blk_drain(target);
-        blk_unref(target);
-    }
-}
-
 static const BlockJobDriver mirror_job_driver = {
     .job_driver = {
         .instance_size          = sizeof(MirrorBlockJob),
@@ -1172,7 +1154,6 @@ static const BlockJobDriver mirror_job_driver = {
         .complete               = mirror_complete,
     },
     .drained_poll           = mirror_drained_poll,
-    .drain                  = mirror_drain,
 };
 
 static const BlockJobDriver commit_active_job_driver = {
@@ -1189,7 +1170,6 @@ static const BlockJobDriver commit_active_job_driver = {
         .complete               = mirror_complete,
     },
     .drained_poll           = mirror_drained_poll,
-    .drain                  = mirror_drain,
 };
 
 static void coroutine_fn
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);
+    }
 
-    blk_drain(bjob->blk);
     if (bjdrv->drain) {
         bjdrv->drain(bjob);
     }
-- 
2.18.0

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

[Vladimir Sementsov-Ogievskiy]

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

diff --git a/block/backup.c b/block/backup.c
index 715e1d3be8..7930004bbd 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -320,21 +320,6 @@ void backup_do_checkpoint(BlockJob *job, Error **errp)
     hbitmap_set(backup_job->copy_bitmap, 0, backup_job->len);
 }
 
-static void backup_drain(BlockJob *job)
-{
-    BackupBlockJob *s = container_of(job, BackupBlockJob, common);
-
-    /* Need to keep a reference in case blk_drain triggers execution
-     * of backup_complete...
-     */
-    if (s->target) {
-        BlockBackend *target = s->target;
-        blk_ref(target);
-        blk_drain(target);
-        blk_unref(target);
-    }
-}
-
 static BlockErrorAction backup_error_action(BackupBlockJob *job,
                                             bool read, int error)
 {
@@ -493,8 +478,7 @@ static const BlockJobDriver backup_job_driver = {
         .commit                 = backup_commit,
         .abort                  = backup_abort,
         .clean                  = backup_clean,
-    },
-    .drain                  = backup_drain,
+    }
 };
 
 static int64_t backup_calculate_cluster_size(BlockDriverState *target,
diff --git a/block/mirror.c b/block/mirror.c
index d17be4cdbc..6bea99558f 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -644,14 +644,11 @@ static int mirror_exit_common(Job *job)
     bdrv_ref(mirror_top_bs);
     bdrv_ref(target_bs);
 
-    /* Remove target parent that still uses BLK_PERM_WRITE/RESIZE before
+    /*
+     * Remove target parent that still uses BLK_PERM_WRITE/RESIZE before
      * inserting target_bs at s->to_replace, where we might not be able to get
      * these permissions.
-     *
-     * Note that blk_unref() alone doesn't necessarily drop permissions because
-     * we might be running nested inside mirror_drain(), which takes an extra
-     * reference, so use an explicit blk_set_perm() first. */
-    blk_set_perm(s->target, 0, BLK_PERM_ALL, &error_abort);
+     */
     blk_unref(s->target);
     s->target = NULL;
 
@@ -1143,21 +1140,6 @@ static bool mirror_drained_poll(BlockJob *job)
     return !!s->in_flight;
 }
 
-static void mirror_drain(BlockJob *job)
-{
-    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
-
-    /* Need to keep a reference in case blk_drain triggers execution
-     * of mirror_complete...
-     */
-    if (s->target) {
-        BlockBackend *target = s->target;
-        blk_ref(target);
-        blk_drain(target);
-        blk_unref(target);
-    }
-}
-
 static const BlockJobDriver mirror_job_driver = {
     .job_driver = {
         .instance_size          = sizeof(MirrorBlockJob),
@@ -1172,7 +1154,6 @@ static const BlockJobDriver mirror_job_driver = {
         .complete               = mirror_complete,
     },
     .drained_poll           = mirror_drained_poll,
-    .drain                  = mirror_drain,
 };
 
 static const BlockJobDriver commit_active_job_driver = {
@@ -1189,7 +1170,6 @@ static const BlockJobDriver commit_active_job_driver = {
         .complete               = mirror_complete,
     },
     .drained_poll           = mirror_drained_poll,
-    .drain                  = mirror_drain,
 };
 
 static void coroutine_fn
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);
+    }
 
-    blk_drain(bjob->blk);
     if (bjdrv->drain) {
         bjdrv->drain(bjob);
     }
-- 
2.18.0

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

[Vladimir Sementsov-Ogievskiy]

19.06.2019 17: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.

Oops, sorry for double-sending.

-- 
Best regards,
Vladimir

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

[Max Reitz]

[-- 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 --]

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

[Vladimir Sementsov-Ogievskiy]

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

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

[Vladimir Sementsov-Ogievskiy]

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

diff --git a/block/backup.c b/block/backup.c
index 715e1d3be8..7930004bbd 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -320,21 +320,6 @@ void backup_do_checkpoint(BlockJob *job, Error **errp)
     hbitmap_set(backup_job->copy_bitmap, 0, backup_job->len);
 }
 
-static void backup_drain(BlockJob *job)
-{
-    BackupBlockJob *s = container_of(job, BackupBlockJob, common);
-
-    /* Need to keep a reference in case blk_drain triggers execution
-     * of backup_complete...
-     */
-    if (s->target) {
-        BlockBackend *target = s->target;
-        blk_ref(target);
-        blk_drain(target);
-        blk_unref(target);
-    }
-}
-
 static BlockErrorAction backup_error_action(BackupBlockJob *job,
                                             bool read, int error)
 {
@@ -493,8 +478,7 @@ static const BlockJobDriver backup_job_driver = {
         .commit                 = backup_commit,
         .abort                  = backup_abort,
         .clean                  = backup_clean,
-    },
-    .drain                  = backup_drain,
+    }
 };
 
 static int64_t backup_calculate_cluster_size(BlockDriverState *target,
diff --git a/block/mirror.c b/block/mirror.c
index d17be4cdbc..6bea99558f 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -644,14 +644,11 @@ static int mirror_exit_common(Job *job)
     bdrv_ref(mirror_top_bs);
     bdrv_ref(target_bs);
 
-    /* Remove target parent that still uses BLK_PERM_WRITE/RESIZE before
+    /*
+     * Remove target parent that still uses BLK_PERM_WRITE/RESIZE before
      * inserting target_bs at s->to_replace, where we might not be able to get
      * these permissions.
-     *
-     * Note that blk_unref() alone doesn't necessarily drop permissions because
-     * we might be running nested inside mirror_drain(), which takes an extra
-     * reference, so use an explicit blk_set_perm() first. */
-    blk_set_perm(s->target, 0, BLK_PERM_ALL, &error_abort);
+     */
     blk_unref(s->target);
     s->target = NULL;
 
@@ -1143,21 +1140,6 @@ static bool mirror_drained_poll(BlockJob *job)
     return !!s->in_flight;
 }
 
-static void mirror_drain(BlockJob *job)
-{
-    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
-
-    /* Need to keep a reference in case blk_drain triggers execution
-     * of mirror_complete...
-     */
-    if (s->target) {
-        BlockBackend *target = s->target;
-        blk_ref(target);
-        blk_drain(target);
-        blk_unref(target);
-    }
-}
-
 static const BlockJobDriver mirror_job_driver = {
     .job_driver = {
         .instance_size          = sizeof(MirrorBlockJob),
@@ -1172,7 +1154,6 @@ static const BlockJobDriver mirror_job_driver = {
         .complete               = mirror_complete,
     },
     .drained_poll           = mirror_drained_poll,
-    .drain                  = mirror_drain,
 };
 
 static const BlockJobDriver commit_active_job_driver = {
@@ -1189,7 +1170,6 @@ static const BlockJobDriver commit_active_job_driver = {
         .complete               = mirror_complete,
     },
     .drained_poll           = mirror_drained_poll,
-    .drain                  = mirror_drain,
 };
 
 static void coroutine_fn
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);
+    }
 
-    blk_drain(bjob->blk);
     if (bjdrv->drain) {
         bjdrv->drain(bjob);
     }
-- 
2.18.0