[Qemu-devel] [PATCH v2 0/2] introduce pinned blk

[Qemu-devel] [PATCH v2 0/2] introduce pinned blk

[Vladimir Sementsov-Ogievskiy]

Hi all.

Here is a proposal of replacing workaround in mirror, when
we have to move filter node back to block-job blk after
bdrv_replace_node.

v2: rebased on updated blk_new, with aio context paramter.


Vladimir Sementsov-Ogievskiy (2):
  block: introduce pinned blk
  blockjob: use blk_new_pinned in block_job_create

 include/block/block_int.h      |  6 ++++++
 include/sysemu/block-backend.h |  2 ++
 block.c                        |  2 +-
 block/block-backend.c          | 25 ++++++++++++++++++++++++-
 block/mirror.c                 |  6 +-----
 blockjob.c                     |  2 +-
 6 files changed, 35 insertions(+), 8 deletions(-)

-- 
2.18.0

[Qemu-devel] [PATCH v2 1/2] block: introduce pinned blk

[Vladimir Sementsov-Ogievskiy]

Add stay_at_node fields to BlockBackend and BdrvChild, for the same
behavior as stay_at_node field of BdrvChildRole. It will be used for
block-job blk.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block_int.h      |  6 ++++++
 include/sysemu/block-backend.h |  2 ++
 block.c                        |  2 +-
 block/block-backend.c          | 25 ++++++++++++++++++++++++-
 4 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 06df2bda1b..1a2eebd904 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -729,6 +729,12 @@ struct BdrvChild {
      */
     bool frozen;
 
+    /*
+     * This link should not be modified in bdrv_replace_node process. Used by
+     * should_update_child()
+     */
+    bool stay_at_node;
+
     QLIST_ENTRY(BdrvChild) next;
     QLIST_ENTRY(BdrvChild) next_parent;
 };
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 733c4957eb..fb248be977 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -77,6 +77,8 @@ typedef struct BlockBackendPublic {
 } BlockBackendPublic;
 
 BlockBackend *blk_new(AioContext *ctx, uint64_t perm, uint64_t shared_perm);
+BlockBackend *blk_new_pinned(AioContext *ctx,
+                             uint64_t perm, uint64_t shared_perm);
 BlockBackend *blk_new_open(const char *filename, const char *reference,
                            QDict *options, int flags, Error **errp);
 int blk_get_refcnt(BlockBackend *blk);
diff --git a/block.c b/block.c
index e3e77feee0..fda92c8629 100644
--- a/block.c
+++ b/block.c
@@ -3971,7 +3971,7 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
     GHashTable *found;
     bool ret;
 
-    if (c->role->stay_at_node) {
+    if (c->stay_at_node || c->role->stay_at_node) {
         return false;
     }
 
diff --git a/block/block-backend.c b/block/block-backend.c
index f5d9407d20..cd59f98e51 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -88,6 +88,11 @@ struct BlockBackend {
      * Accessed with atomic ops.
      */
     unsigned int in_flight;
+
+    /*
+     * On blk_insert_bs() new child will inherit  @stay_at_node.
+     */
+    bool stay_at_node;
 };
 
 typedef struct BlockBackendAIOCB {
@@ -321,9 +326,14 @@ static const BdrvChildRole child_root = {
  * to other users of the attached node.
  * Both sets of permissions can be changed later using blk_set_perm().
  *
+ * @stay_at_node is used to set stay_at_node field of child, attached in
+ * blk_insert_bs(). If true, child bs will not be updated on bdrv_replace_node.
+ *
  * Return the new BlockBackend on success, null on failure.
  */
-BlockBackend *blk_new(AioContext *ctx, uint64_t perm, uint64_t shared_perm)
+static BlockBackend *blk_new_common(AioContext *ctx,
+                                    uint64_t perm, uint64_t shared_perm,
+                                    bool stay_at_node)
 {
     BlockBackend *blk;
 
@@ -332,6 +342,7 @@ BlockBackend *blk_new(AioContext *ctx, uint64_t perm, uint64_t shared_perm)
     blk->ctx = ctx;
     blk->perm = perm;
     blk->shared_perm = shared_perm;
+    blk->stay_at_node = stay_at_node;
     blk_set_enable_write_cache(blk, true);
 
     blk->on_read_error = BLOCKDEV_ON_ERROR_REPORT;
@@ -347,6 +358,17 @@ BlockBackend *blk_new(AioContext *ctx, uint64_t perm, uint64_t shared_perm)
     return blk;
 }
 
+BlockBackend *blk_new(AioContext *ctx, uint64_t perm, uint64_t shared_perm)
+{
+    return blk_new_common(ctx, perm, shared_perm, false);
+}
+
+BlockBackend *blk_new_pinned(AioContext *ctx,
+                             uint64_t perm, uint64_t shared_perm)
+{
+    return blk_new_common(ctx, perm, shared_perm, true);
+}
+
 /*
  * Creates a new BlockBackend, opens a new BlockDriverState, and connects both.
  * The new BlockBackend is in the main AioContext.
@@ -808,6 +830,7 @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
     if (blk->root == NULL) {
         return -EPERM;
     }
+    blk->root->stay_at_node = blk->stay_at_node;
 
     notifier_list_notify(&blk->insert_bs_notifiers, blk);
     if (tgm->throttle_state) {
-- 
2.18.0

[Qemu-devel] [PATCH v2 2/2] blockjob: use blk_new_pinned in block_job_create

[Vladimir Sementsov-Ogievskiy]

child_role job already has .stay_at_node=true, so on bdrv_replace_node
operation these child are unchanged. Make block job blk behave in same
manner, to avoid inconsistent intermediate graph states and workarounds
like in mirror.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/mirror.c | 6 +-----
 blockjob.c     | 2 +-
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index f8bdb5b21b..23443116e4 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -713,12 +713,8 @@ static int mirror_exit_common(Job *job)
                             &error_abort);
     bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
 
-    /* We just changed the BDS the job BB refers to (with either or both of the
-     * bdrv_replace_node() calls), so switch the BB back so the cleanup does
-     * the right thing. We don't need any permissions any more now. */
-    blk_remove_bs(bjob->blk);
+    /* We don't need any permissions any more now. */
     blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort);
-    blk_insert_bs(bjob->blk, mirror_top_bs, &error_abort);
 
     bs_opaque->job = NULL;
 
diff --git a/blockjob.c b/blockjob.c
index 931d675c0c..f5c8d31491 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -398,7 +398,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
         job_id = bdrv_get_device_name(bs);
     }
 
-    blk = blk_new(bdrv_get_aio_context(bs), perm, shared_perm);
+    blk = blk_new_pinned(bdrv_get_aio_context(bs), perm, shared_perm);
     ret = blk_insert_bs(blk, bs, errp);
     if (ret < 0) {
         blk_unref(blk);
-- 
2.18.0

Re: [Qemu-devel] [PATCH v2 2/2] blockjob: use blk_new_pinned in block_job_create

[Kevin Wolf]

Am 05.06.2019 um 14:32 hat Vladimir Sementsov-Ogievskiy geschrieben:
> child_role job already has .stay_at_node=true, so on bdrv_replace_node
> operation these child are unchanged. Make block job blk behave in same
> manner, to avoid inconsistent intermediate graph states and workarounds
> like in mirror.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

This feels dangerous. It does what you want it to do if the only graph
change below the BlockBackend is the one in mirror_exit_common. But the
user could also take a snapshot, or in the future hopefully insert a
filter node, and you would then want the BlockBackend to move.

To be honest, even BdrvChildRole.stay_at_node is a bit of a hack. But at
least it's only used for permissions and not for the actual data flow.

Kevin

Re: [Qemu-devel] [PATCH v2 2/2] blockjob: use blk_new_pinned in block_job_create

[Vladimir Sementsov-Ogievskiy]

05.06.2019 20:11, Kevin Wolf wrote:
> Am 05.06.2019 um 14:32 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> child_role job already has .stay_at_node=true, so on bdrv_replace_node
>> operation these child are unchanged. Make block job blk behave in same
>> manner, to avoid inconsistent intermediate graph states and workarounds
>> like in mirror.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> This feels dangerous. It does what you want it to do if the only graph
> change below the BlockBackend is the one in mirror_exit_common. But the
> user could also take a snapshot, or in the future hopefully insert a
> filter node, and you would then want the BlockBackend to move.
> 
> To be honest, even BdrvChildRole.stay_at_node is a bit of a hack. But at
> least it's only used for permissions and not for the actual data flow.
> 

Hmm. Than, may be just add a parameter to bdrv_replace_node, which parents
to ignore? Would it work?


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 2/2] blockjob: use blk_new_pinned in block_job_create

[Kevin Wolf]

Am 05.06.2019 um 19:16 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 05.06.2019 20:11, Kevin Wolf wrote:
> > Am 05.06.2019 um 14:32 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> child_role job already has .stay_at_node=true, so on bdrv_replace_node
> >> operation these child are unchanged. Make block job blk behave in same
> >> manner, to avoid inconsistent intermediate graph states and workarounds
> >> like in mirror.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > 
> > This feels dangerous. It does what you want it to do if the only graph
> > change below the BlockBackend is the one in mirror_exit_common. But the
> > user could also take a snapshot, or in the future hopefully insert a
> > filter node, and you would then want the BlockBackend to move.
> > 
> > To be honest, even BdrvChildRole.stay_at_node is a bit of a hack. But at
> > least it's only used for permissions and not for the actual data flow.
> 
> Hmm. Than, may be just add a parameter to bdrv_replace_node, which parents
> to ignore? Would it work?

I would have to think a bit more about it, but it does sound safer.

Or we take a step back and ask why it's even a problem for the mirror
block job if the BlockBackend is moved to a different node. The main
reason I see is because of bs->job that is set for the root node of the
BlockBackend and needs to be unset for the same node.

Maybe we can just finally get rid of bs->job? It doesn't have many users
any more.

Kevin

Re: [Qemu-devel] [PATCH v2 2/2] blockjob: use blk_new_pinned in block_job_create

[Vladimir Sementsov-Ogievskiy]

06.06.2019 13:05, Kevin Wolf wrote:
> Am 05.06.2019 um 19:16 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 05.06.2019 20:11, Kevin Wolf wrote:
>>> Am 05.06.2019 um 14:32 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> child_role job already has .stay_at_node=true, so on bdrv_replace_node
>>>> operation these child are unchanged. Make block job blk behave in same
>>>> manner, to avoid inconsistent intermediate graph states and workarounds
>>>> like in mirror.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>
>>> This feels dangerous. It does what you want it to do if the only graph
>>> change below the BlockBackend is the one in mirror_exit_common. But the
>>> user could also take a snapshot, or in the future hopefully insert a
>>> filter node, and you would then want the BlockBackend to move.
>>>
>>> To be honest, even BdrvChildRole.stay_at_node is a bit of a hack. But at
>>> least it's only used for permissions and not for the actual data flow.
>>
>> Hmm. Than, may be just add a parameter to bdrv_replace_node, which parents
>> to ignore? Would it work?
> 
> I would have to think a bit more about it, but it does sound safer.
> 
> Or we take a step back and ask why it's even a problem for the mirror
> block job if the BlockBackend is moved to a different node. The main
> reason I see is because of bs->job that is set for the root node of the
> BlockBackend and needs to be unset for the same node.
> 
> Maybe we can just finally get rid of bs->job? It doesn't have many users
> any more.
> 

Hmm, looked at it. Not sure what should be refactored around job to get rid
of "main node" concept.. Which seems to be in a bad relation with starting
job on implicit filters as a main node..

But about just removing bs->job pointer, I don't know at least what to do with
blk_iostatus_reset and blockdev_mark_auto_del..


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 2/2] blockjob: use blk_new_pinned in block_job_create

[Kevin Wolf]

Am 06.06.2019 um 14:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 06.06.2019 13:05, Kevin Wolf wrote:
> > Am 05.06.2019 um 19:16 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> 05.06.2019 20:11, Kevin Wolf wrote:
> >>> Am 05.06.2019 um 14:32 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >>>> child_role job already has .stay_at_node=true, so on bdrv_replace_node
> >>>> operation these child are unchanged. Make block job blk behave in same
> >>>> manner, to avoid inconsistent intermediate graph states and workarounds
> >>>> like in mirror.
> >>>>
> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >>>
> >>> This feels dangerous. It does what you want it to do if the only graph
> >>> change below the BlockBackend is the one in mirror_exit_common. But the
> >>> user could also take a snapshot, or in the future hopefully insert a
> >>> filter node, and you would then want the BlockBackend to move.
> >>>
> >>> To be honest, even BdrvChildRole.stay_at_node is a bit of a hack. But at
> >>> least it's only used for permissions and not for the actual data flow.
> >>
> >> Hmm. Than, may be just add a parameter to bdrv_replace_node, which parents
> >> to ignore? Would it work?
> > 
> > I would have to think a bit more about it, but it does sound safer.
> > 
> > Or we take a step back and ask why it's even a problem for the mirror
> > block job if the BlockBackend is moved to a different node. The main
> > reason I see is because of bs->job that is set for the root node of the
> > BlockBackend and needs to be unset for the same node.
> > 
> > Maybe we can just finally get rid of bs->job? It doesn't have many users
> > any more.
> > 
> 
> Hmm, looked at it. Not sure what should be refactored around job to get rid
> of "main node" concept.. Which seems to be in a bad relation with starting
> job on implicit filters as a main node..
> 
> But about just removing bs->job pointer, I don't know at least what to do with
> blk_iostatus_reset and blockdev_mark_auto_del..

blk_iostatus_reset() looks easy. It has only two callers:

1. blk_attach_dev(). This doesn't have anything to do with jobs and
   attaching a new guest device won't solve any problem the job
   encountered, so no reason to reset the iostatus for the job.

2. qmp_cont(). This resets the iostatus for everything. We can just
   call block_job_iostatus_reset() for all block jobs instead of going
   through BlockBackend.

blockdev_mark_auto_del() might be a bit trickier. The whole idea of the
function is: When a guest device gets unplugged, automatically remove
its root block node, too. Commit 12bde0eed6b made it cancel a block job
because that should happen immediately when the device is actually
released by the guest and not only after the job finishes and gives up
its reference. I would like to just change the behaviour, but I'm afraid
we can't do this because of compatibility.

However, just checking bs->job is really only one special case of
another user of the node to be deleted. Maybe we can extend it a little
so that any block jobs that contain the node in job->nodes are
cancelled.

Kevin

Re: [Qemu-devel] [PATCH v2 2/2] blockjob: use blk_new_pinned in block_job_create

[Vladimir Sementsov-Ogievskiy]

06.06.2019 16:06, Kevin Wolf wrote:
> Am 06.06.2019 um 14:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 06.06.2019 13:05, Kevin Wolf wrote:
>>> Am 05.06.2019 um 19:16 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> 05.06.2019 20:11, Kevin Wolf wrote:
>>>>> Am 05.06.2019 um 14:32 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>>> child_role job already has .stay_at_node=true, so on bdrv_replace_node
>>>>>> operation these child are unchanged. Make block job blk behave in same
>>>>>> manner, to avoid inconsistent intermediate graph states and workarounds
>>>>>> like in mirror.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>
>>>>> This feels dangerous. It does what you want it to do if the only graph
>>>>> change below the BlockBackend is the one in mirror_exit_common. But the
>>>>> user could also take a snapshot, or in the future hopefully insert a
>>>>> filter node, and you would then want the BlockBackend to move.
>>>>>
>>>>> To be honest, even BdrvChildRole.stay_at_node is a bit of a hack. But at
>>>>> least it's only used for permissions and not for the actual data flow.
>>>>
>>>> Hmm. Than, may be just add a parameter to bdrv_replace_node, which parents
>>>> to ignore? Would it work?
>>>
>>> I would have to think a bit more about it, but it does sound safer.
>>>
>>> Or we take a step back and ask why it's even a problem for the mirror
>>> block job if the BlockBackend is moved to a different node. The main
>>> reason I see is because of bs->job that is set for the root node of the
>>> BlockBackend and needs to be unset for the same node.
>>>
>>> Maybe we can just finally get rid of bs->job? It doesn't have many users
>>> any more.
>>>
>>
>> Hmm, looked at it. Not sure what should be refactored around job to get rid
>> of "main node" concept.. Which seems to be in a bad relation with starting
>> job on implicit filters as a main node..
>>
>> But about just removing bs->job pointer, I don't know at least what to do with
>> blk_iostatus_reset and blockdev_mark_auto_del..
> 
> blk_iostatus_reset() looks easy. It has only two callers:
> 
> 1. blk_attach_dev(). This doesn't have anything to do with jobs and
>     attaching a new guest device won't solve any problem the job
>     encountered, so no reason to reset the iostatus for the job.
> 
> 2. qmp_cont(). This resets the iostatus for everything. We can just
>     call block_job_iostatus_reset() for all block jobs instead of going
>     through BlockBackend.
> 
> blockdev_mark_auto_del() might be a bit trickier. The whole idea of the
> function is: When a guest device gets unplugged, automatically remove
> its root block node, too. Commit 12bde0eed6b made it cancel a block job
> because that should happen immediately when the device is actually
> released by the guest and not only after the job finishes and gives up
> its reference. I would like to just change the behaviour, but I'm afraid
> we can't do this because of compatibility.
> 
> However, just checking bs->job is really only one special case of
> another user of the node to be deleted. Maybe we can extend it a little
> so that any block jobs that contain the node in job->nodes are
> cancelled.
> 

OK, thanks. I'll try this way


-- 
Best regards,
Vladimir