[Qemu-devel] [PATCH] blk: fix aio context loss on media change

[Qemu-devel] [PATCH] blk: fix aio context loss on media change

[Vladimir Sementsov-Ogievskiy]

If we have separate iothread for cdrom, we lose connection to it on
qmp_blockdev_change_medium, as aio_context is on bds which is dropped
and switched with new one.

As an example result, after such media change we have crash on
virtio_scsi_ctx_check: Assertion `blk_get_aio_context(d->conf.blk) == s->ctx' failed.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---

Hi all!

We've faced into this assert, and there some kind of fix. I don't sure that
such fix doesn't break some conceptions, in this case, I hope, someone will
propose a true-way solution.

======
Also, on master branch I can't reproduce it as vm crashed earlier, without any
eject/change, on assert(s->ctx && s->dataplane_started) in
virtio_scsi_data_plane_handle_ctrl(). It looks like race with
virtio_scsi_dataplane_start(), and for test (to reproduce assert described above),
I've "fixed" it with just:

@@ -63,6 +63,7 @@ static bool virtio_scsi_data_plane_handle_ctrl(VirtIODevice *vdev,
 {
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
 
+    sleep(10);
     assert(s->ctx && s->dataplane_started);
     return virtio_scsi_handle_ctrl_vq(s, vq);
 }

This race is not reproduced for me in our 2.6 based branch.

 block/block-backend.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 5742c09c2c..6d5044228e 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -65,6 +65,8 @@ struct BlockBackend {
     bool allow_write_beyond_eof;
 
     NotifierList remove_bs_notifiers, insert_bs_notifiers;
+
+    AioContext *aio_context;
 };
 
 typedef struct BlockBackendAIOCB {
@@ -559,6 +561,10 @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
     }
     bdrv_ref(bs);
 
+    if (blk->aio_context != NULL) {
+        bdrv_set_aio_context(bs, blk->aio_context);
+    }
+
     notifier_list_notify(&blk->insert_bs_notifiers, blk);
     if (blk->public.throttle_state) {
         throttle_timers_attach_aio_context(
@@ -1607,6 +1613,7 @@ void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
 {
     BlockDriverState *bs = blk_bs(blk);
 
+    blk->aio_context = new_context;
     if (bs) {
         if (blk->public.throttle_state) {
             throttle_timers_detach_aio_context(&blk->public.throttle_timers);
-- 
2.11.1

Re: [Qemu-devel] [PATCH] blk: fix aio context loss on media change

[Kevin Wolf]

Am 14.03.2017 um 18:11 hat Vladimir Sementsov-Ogievskiy geschrieben:
> If we have separate iothread for cdrom, we lose connection to it on
> qmp_blockdev_change_medium, as aio_context is on bds which is dropped
> and switched with new one.
> 
> As an example result, after such media change we have crash on
> virtio_scsi_ctx_check: Assertion `blk_get_aio_context(d->conf.blk) == s->ctx' failed.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 
> Hi all!
> 
> We've faced into this assert, and there some kind of fix. I don't sure that
> such fix doesn't break some conceptions, in this case, I hope, someone will
> propose a true-way solution.

The "true way" would be proper AioContext management in the sense that
all users of a BDS can specify a specific AioContext that they need and
if they all agree, callbacks are invoked to change everyone to that
AioContext. If they conflict, attaching the new user would have to error
out.

But we discussed this earlier, and while I'm not completely sure any
more about the details, I seem to remeber that Paolo said something
along the lines that AioContext is going away anyway and building the
code for proper management would be wasted time.

Stefan, Paolo, do you remember the details why we didn't even do a
simple fix like the one below? I think there were some patches on the
list, no?

Kevin

> ======
> Also, on master branch I can't reproduce it as vm crashed earlier, without any
> eject/change, on assert(s->ctx && s->dataplane_started) in
> virtio_scsi_data_plane_handle_ctrl(). It looks like race with
> virtio_scsi_dataplane_start(), and for test (to reproduce assert described above),
> I've "fixed" it with just:
> 
> @@ -63,6 +63,7 @@ static bool virtio_scsi_data_plane_handle_ctrl(VirtIODevice *vdev,
>  {
>      VirtIOSCSI *s = VIRTIO_SCSI(vdev);
>  
> +    sleep(10);
>      assert(s->ctx && s->dataplane_started);
>      return virtio_scsi_handle_ctrl_vq(s, vq);
>  }
> 
> This race is not reproduced for me in our 2.6 based branch.
> 
>  block/block-backend.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 5742c09c2c..6d5044228e 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -65,6 +65,8 @@ struct BlockBackend {
>      bool allow_write_beyond_eof;
>  
>      NotifierList remove_bs_notifiers, insert_bs_notifiers;
> +
> +    AioContext *aio_context;
>  };
>  
>  typedef struct BlockBackendAIOCB {
> @@ -559,6 +561,10 @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
>      }
>      bdrv_ref(bs);
>  
> +    if (blk->aio_context != NULL) {
> +        bdrv_set_aio_context(bs, blk->aio_context);
> +    }
> +
>      notifier_list_notify(&blk->insert_bs_notifiers, blk);
>      if (blk->public.throttle_state) {
>          throttle_timers_attach_aio_context(
> @@ -1607,6 +1613,7 @@ void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
>  {
>      BlockDriverState *bs = blk_bs(blk);
>  
> +    blk->aio_context = new_context;
>      if (bs) {
>          if (blk->public.throttle_state) {
>              throttle_timers_detach_aio_context(&blk->public.throttle_timers);
> -- 
> 2.11.1
> 

Re: [Qemu-devel] [PATCH] blk: fix aio context loss on media change

[Fam Zheng]

On Wed, 03/15 12:03, Kevin Wolf wrote:
> Am 14.03.2017 um 18:11 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > If we have separate iothread for cdrom, we lose connection to it on
> > qmp_blockdev_change_medium, as aio_context is on bds which is dropped
> > and switched with new one.
> > 
> > As an example result, after such media change we have crash on
> > virtio_scsi_ctx_check: Assertion `blk_get_aio_context(d->conf.blk) == s->ctx' failed.
> > 
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> > 
> > Hi all!
> > 
> > We've faced into this assert, and there some kind of fix. I don't sure that
> > such fix doesn't break some conceptions, in this case, I hope, someone will
> > propose a true-way solution.
> 
> The "true way" would be proper AioContext management in the sense that
> all users of a BDS can specify a specific AioContext that they need and
> if they all agree, callbacks are invoked to change everyone to that
> AioContext. If they conflict, attaching the new user would have to error
> out.
> 
> But we discussed this earlier, and while I'm not completely sure any
> more about the details, I seem to remeber that Paolo said something
> along the lines that AioContext is going away anyway and building the
> code for proper management would be wasted time.

Matches my impression.

> 
> Stefan, Paolo, do you remember the details why we didn't even do a
> simple fix like the one below? I think there were some patches on the
> list, no?

ISTM the concern was mostly "what about other BB in the graph?"

Should the new op blocker API be used in this one (a new type of perm)?

> 
> Kevin
> 
> > ======
> > Also, on master branch I can't reproduce it as vm crashed earlier, without any
> > eject/change, on assert(s->ctx && s->dataplane_started) in
> > virtio_scsi_data_plane_handle_ctrl(). It looks like race with
> > virtio_scsi_dataplane_start(), and for test (to reproduce assert described above),
> > I've "fixed" it with just:
> > 
> > @@ -63,6 +63,7 @@ static bool virtio_scsi_data_plane_handle_ctrl(VirtIODevice *vdev,
> >  {
> >      VirtIOSCSI *s = VIRTIO_SCSI(vdev);
> >  
> > +    sleep(10);

This will be fixed by 

https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg02659.html

Fam

Re: [Qemu-devel] [PATCH] blk: fix aio context loss on media change

[Kevin Wolf]

Am 15.03.2017 um 12:14 hat Fam Zheng geschrieben:
> On Wed, 03/15 12:03, Kevin Wolf wrote:
> > Am 14.03.2017 um 18:11 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > If we have separate iothread for cdrom, we lose connection to it on
> > > qmp_blockdev_change_medium, as aio_context is on bds which is dropped
> > > and switched with new one.
> > > 
> > > As an example result, after such media change we have crash on
> > > virtio_scsi_ctx_check: Assertion `blk_get_aio_context(d->conf.blk) == s->ctx' failed.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > ---
> > > 
> > > Hi all!
> > > 
> > > We've faced into this assert, and there some kind of fix. I don't sure that
> > > such fix doesn't break some conceptions, in this case, I hope, someone will
> > > propose a true-way solution.
> > 
> > The "true way" would be proper AioContext management in the sense that
> > all users of a BDS can specify a specific AioContext that they need and
> > if they all agree, callbacks are invoked to change everyone to that
> > AioContext. If they conflict, attaching the new user would have to error
> > out.
> > 
> > But we discussed this earlier, and while I'm not completely sure any
> > more about the details, I seem to remeber that Paolo said something
> > along the lines that AioContext is going away anyway and building the
> > code for proper management would be wasted time.
> 
> Matches my impression.
> 
> > 
> > Stefan, Paolo, do you remember the details why we didn't even do a
> > simple fix like the one below? I think there were some patches on the
> > list, no?
> 
> ISTM the concern was mostly "what about other BB in the graph?"
> 
> Should the new op blocker API be used in this one (a new type of perm)?

If we know what operation to block, that's an option. Would "change the
node's AioContext" work for it?

I think it would effectively mean that you need to attach the device
first and then jobs etc. respect the AioContext, whereas the opposite
order breaks because they don't have callbacks to adjust the AioContext
after the fact.

This seems to match what's actually safe, so it might really be as easy
as this.

Kevin

Re: [Qemu-devel] [PATCH] blk: fix aio context loss on media change

[Fam Zheng]

On Wed, 03/15 13:06, Kevin Wolf wrote:
> > > 
> > > Stefan, Paolo, do you remember the details why we didn't even do a
> > > simple fix like the one below? I think there were some patches on the
> > > list, no?
> > 
> > ISTM the concern was mostly "what about other BB in the graph?"
> > 
> > Should the new op blocker API be used in this one (a new type of perm)?
> 
> If we know what operation to block, that's an option. Would "change the
> node's AioContext" work for it?
> 
> I think it would effectively mean that you need to attach the device
> first and then jobs etc. respect the AioContext, whereas the opposite
> order breaks because they don't have callbacks to adjust the AioContext
> after the fact.
> 
> This seems to match what's actually safe, so it might really be as easy
> as this.

Yes, this sounds good to me.

Vladimir, would you like to implement this? It would be good to have this fixed
in 2.9.

Fam

Re: [Qemu-devel] [PATCH] blk: fix aio context loss on media change

[Vladimir Sementsov-Ogievskiy]

15.03.2017 16:13, Fam Zheng wrote:
> On Wed, 03/15 13:06, Kevin Wolf wrote:
>>>> Stefan, Paolo, do you remember the details why we didn't even do a
>>>> simple fix like the one below? I think there were some patches on the
>>>> list, no?
>>> ISTM the concern was mostly "what about other BB in the graph?"
>>>
>>> Should the new op blocker API be used in this one (a new type of perm)?
>> If we know what operation to block, that's an option. Would "change the
>> node's AioContext" work for it?

we can start with empty cdrom, so there is no context at start

>>
>> I think it would effectively mean that you need to attach the device
>> first and then jobs etc. respect the AioContext, whereas the opposite
>> order breaks because they don't have callbacks to adjust the AioContext
>> after the fact.
>>
>> This seems to match what's actually safe, so it might really be as easy
>> as this.
> Yes, this sounds good to me.
>
> Vladimir, would you like to implement this? It would be good to have this fixed
> in 2.9.

I don't sure about how to do it, so, I don't mind someone else to 
implement this..

>
> Fam


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] blk: fix aio context loss on media change

[Fam Zheng]

On Wed, 03/15 17:04, Vladimir Sementsov-Ogievskiy wrote:
> 15.03.2017 16:13, Fam Zheng wrote:
> > On Wed, 03/15 13:06, Kevin Wolf wrote:
> > > > > Stefan, Paolo, do you remember the details why we didn't even do a
> > > > > simple fix like the one below? I think there were some patches on the
> > > > > list, no?
> > > > ISTM the concern was mostly "what about other BB in the graph?"
> > > > 
> > > > Should the new op blocker API be used in this one (a new type of perm)?
> > > If we know what operation to block, that's an option. Would "change the
> > > node's AioContext" work for it?
> 
> we can start with empty cdrom, so there is no context at start

Practically, I agree this patch is fine for now because callers don't
arbitrarily move BDS from one context to another, only a small set of operations
exists - start/stop data plane, block job callbacks. The problem is that new
code in the future can cause trouble if different BBs in the same graph don't
agree on what context they can run on, especially with dynamical
reconfiguration. So the previous discussions were more about an API that works
generally.

> 
> > > 
> > > I think it would effectively mean that you need to attach the device
> > > first and then jobs etc. respect the AioContext, whereas the opposite
> > > order breaks because they don't have callbacks to adjust the AioContext
> > > after the fact.
> > > 
> > > This seems to match what's actually safe, so it might really be as easy
> > > as this.
> > Yes, this sounds good to me.
> > 
> > Vladimir, would you like to implement this? It would be good to have this fixed
> > in 2.9.
> 
> I don't sure about how to do it, so, I don't mind someone else to implement
> this..

OK, I can take a look at this tomorrow.

Fam

Re: [Qemu-devel] [PATCH] blk: fix aio context loss on media change

[Paolo Bonzini]

On 15/03/2017 12:03, Kevin Wolf wrote:
> But we discussed this earlier, and while I'm not completely sure any
> more about the details, I seem to remeber that Paolo said something
> along the lines that AioContext is going away anyway and building the
> code for proper management would be wasted time.

AioContext is going to stay, but everybody will be able to send
operations to a BB/BDS from any AioContext.  The BDS AioContext will
only matter for network devices, since they have to attach the file
descriptor handlers somewhere.  For files it won't matter at all because
you can use multiple Linux AIO context or thread pools at the same time.

There should be a policy on which BB sets AioContext on the BDS (e.g.
only the device does it), but apart from that, it should not be an issue.

Paolo

> Stefan, Paolo, do you remember the details why we didn't even do a
> simple fix like the one below? I think there were some patches on the
> list, no?
> 
> Kevin
> 
>> ======
>> Also, on master branch I can't reproduce it as vm crashed earlier, without any
>> eject/change, on assert(s->ctx && s->dataplane_started) in
>> virtio_scsi_data_plane_handle_ctrl(). It looks like race with
>> virtio_scsi_dataplane_start(), and for test (to reproduce assert described above),
>> I've "fixed" it with just:
>>
>> @@ -63,6 +63,7 @@ static bool virtio_scsi_data_plane_handle_ctrl(VirtIODevice *vdev,
>>  {
>>      VirtIOSCSI *s = VIRTIO_SCSI(vdev);
>>  
>> +    sleep(10);
>>      assert(s->ctx && s->dataplane_started);
>>      return virtio_scsi_handle_ctrl_vq(s, vq);
>>  }
>>
>> This race is not reproduced for me in our 2.6 based branch.
>>
>>  block/block-backend.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 5742c09c2c..6d5044228e 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -65,6 +65,8 @@ struct BlockBackend {
>>      bool allow_write_beyond_eof;
>>  
>>      NotifierList remove_bs_notifiers, insert_bs_notifiers;
>> +
>> +    AioContext *aio_context;
>>  };
>>  
>>  typedef struct BlockBackendAIOCB {
>> @@ -559,6 +561,10 @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
>>      }
>>      bdrv_ref(bs);
>>  
>> +    if (blk->aio_context != NULL) {
>> +        bdrv_set_aio_context(bs, blk->aio_context);
>> +    }
>> +
>>      notifier_list_notify(&blk->insert_bs_notifiers, blk);
>>      if (blk->public.throttle_state) {
>>          throttle_timers_attach_aio_context(
>> @@ -1607,6 +1613,7 @@ void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
>>  {
>>      BlockDriverState *bs = blk_bs(blk);
>>  
>> +    blk->aio_context = new_context;
>>      if (bs) {
>>          if (blk->public.throttle_state) {
>>              throttle_timers_detach_aio_context(&blk->public.throttle_timers);
>> -- 
>> 2.11.1
>>

Re: [Qemu-devel] [PATCH] blk: fix aio context loss on media change

[Kevin Wolf]

Am 15.03.2017 um 14:39 hat Paolo Bonzini geschrieben:
> On 15/03/2017 12:03, Kevin Wolf wrote:
> > But we discussed this earlier, and while I'm not completely sure any
> > more about the details, I seem to remeber that Paolo said something
> > along the lines that AioContext is going away anyway and building the
> > code for proper management would be wasted time.
> 
> AioContext is going to stay, but everybody will be able to send
> operations to a BB/BDS from any AioContext.  The BDS AioContext will
> only matter for network devices, since they have to attach the file
> descriptor handlers somewhere.  For files it won't matter at all because
> you can use multiple Linux AIO context or thread pools at the same time.

Should the iothread option then become a -blockdev option rather than a
-device one?

> There should be a policy on which BB sets AioContext on the BDS (e.g.
> only the device does it), but apart from that, it should not be an issue.

We don't know which BBs are going to be attached. We don't necessarily
have a device at all, or we could have two of them.

Though maybe we should try to keep a BDS and its children in the same
AioContext anyway if that's possible? Will it make a difference?

Kevin

Re: [Qemu-devel] [PATCH] blk: fix aio context loss on media change

[Paolo Bonzini]

On 15/03/2017 15:30, Kevin Wolf wrote:
> Am 15.03.2017 um 14:39 hat Paolo Bonzini geschrieben:
>> On 15/03/2017 12:03, Kevin Wolf wrote:
>>> But we discussed this earlier, and while I'm not completely sure any
>>> more about the details, I seem to remeber that Paolo said something
>>> along the lines that AioContext is going away anyway and building the
>>> code for proper management would be wasted time.
>>
>> AioContext is going to stay, but everybody will be able to send
>> operations to a BB/BDS from any AioContext.  The BDS AioContext will
>> only matter for network devices, since they have to attach the file
>> descriptor handlers somewhere.  For files it won't matter at all because
>> you can use multiple Linux AIO context or thread pools at the same time.
> 
> Should the iothread option then become a -blockdev option rather than a
> -device one?

Well, both.  The device also needs an I/O thread to attach its ioeventfd
handler.  And it makes sense to use the -device I/O thread if -blockdev
specified none.

>> There should be a policy on which BB sets AioContext on the BDS (e.g.
>> only the device does it), but apart from that, it should not be an issue.
> 
> We don't know which BBs are going to be attached. We don't necessarily
> have a device at all, or we could have two of them.

Wow, can we really have two? :-O

> Though maybe we should try to keep a BDS and its children in the same
> AioContext anyway if that's possible? Will it make a difference?

Everything can make sense---but yes, keeping the whole hierarchy in the
same AioContext makes sense more often.

Paolo

Re: [Qemu-devel] [PATCH] blk: fix aio context loss on media change

[Kevin Wolf]

Am 15.03.2017 um 15:43 hat Paolo Bonzini geschrieben:
> On 15/03/2017 15:30, Kevin Wolf wrote:
> > Am 15.03.2017 um 14:39 hat Paolo Bonzini geschrieben:
> >> On 15/03/2017 12:03, Kevin Wolf wrote:
> >>> But we discussed this earlier, and while I'm not completely sure any
> >>> more about the details, I seem to remeber that Paolo said something
> >>> along the lines that AioContext is going away anyway and building the
> >>> code for proper management would be wasted time.
> >>
> >> AioContext is going to stay, but everybody will be able to send
> >> operations to a BB/BDS from any AioContext.  The BDS AioContext will
> >> only matter for network devices, since they have to attach the file
> >> descriptor handlers somewhere.  For files it won't matter at all because
> >> you can use multiple Linux AIO context or thread pools at the same time.
> > 
> > Should the iothread option then become a -blockdev option rather than a
> > -device one?
> 
> Well, both.  The device also needs an I/O thread to attach its ioeventfd
> handler.  And it makes sense to use the -device I/O thread if -blockdev
> specified none.

Right, that makes sense. I just wasn't aware until now that we would get
a per-node option, so that's good to know.

> >> There should be a policy on which BB sets AioContext on the BDS (e.g.
> >> only the device does it), but apart from that, it should not be an issue.
> > 
> > We don't know which BBs are going to be attached. We don't necessarily
> > have a device at all, or we could have two of them.
> 
> Wow, can we really have two? :-O

What would prevent you from doing this? The whole blockdev work was
about making the block layer more flexible, so now we have this
flexibility of attaching more or less anything to anything (unless op
blockers prevent it, which is why they are important for actually
supporting blockdev).

> > Though maybe we should try to keep a BDS and its children in the same
> > AioContext anyway if that's possible? Will it make a difference?
> 
> Everything can make sense---but yes, keeping the whole hierarchy in the
> same AioContext makes sense more often.

So I take this to mean that it does make a difference. :-)

If we want to keep users and their child nodes in the same AioContext by
default, we'll probably still need to implement all of the callbacks
that we would need for proper AioContext management today.

Kevin

Re: [Qemu-devel] [PATCH] blk: fix aio context loss on media change

[Paolo Bonzini]

On 15/03/2017 16:02, Kevin Wolf wrote:
> Am 15.03.2017 um 15:43 hat Paolo Bonzini geschrieben:
>> On 15/03/2017 15:30, Kevin Wolf wrote:
>>> Am 15.03.2017 um 14:39 hat Paolo Bonzini geschrieben:
>>>> On 15/03/2017 12:03, Kevin Wolf wrote:
>>>>> But we discussed this earlier, and while I'm not completely sure any
>>>>> more about the details, I seem to remeber that Paolo said something
>>>>> along the lines that AioContext is going away anyway and building the
>>>>> code for proper management would be wasted time.
>>>>
>>>> AioContext is going to stay, but everybody will be able to send
>>>> operations to a BB/BDS from any AioContext.  The BDS AioContext will
>>>> only matter for network devices, since they have to attach the file
>>>> descriptor handlers somewhere.  For files it won't matter at all because
>>>> you can use multiple Linux AIO context or thread pools at the same time.
>>>
>>> Should the iothread option then become a -blockdev option rather than a
>>> -device one?
>>
>> Well, both.  The device also needs an I/O thread to attach its ioeventfd
>> handler.  And it makes sense to use the -device I/O thread if -blockdev
>> specified none.
> 
> Right, that makes sense. I just wasn't aware until now that we would get
> a per-node option, so that's good to know.

I hadn't thought about it either. :)

>>>> There should be a policy on which BB sets AioContext on the BDS (e.g.
>>>> only the device does it), but apart from that, it should not be an issue.
>>>
>>> We don't know which BBs are going to be attached. We don't necessarily
>>> have a device at all, or we could have two of them.
>>
>> Wow, can we really have two? :-O
> 
> What would prevent you from doing this? The whole blockdev work was
> about making the block layer more flexible, so now we have this
> flexibility of attaching more or less anything to anything (unless op
> blockers prevent it, which is why they are important for actually
> supporting blockdev).

Yeah, the actual question was more "will the blockers allow two" devices
behind the same BDS.  But I suppose there's no reason to prevent that
(emulating multipath, for example).

>>> Though maybe we should try to keep a BDS and its children in the same
>>> AioContext anyway if that's possible? Will it make a difference?
>>
>> Everything can make sense---but yes, keeping the whole hierarchy in the
>> same AioContext makes sense more often.
> 
> So I take this to mean that it does make a difference. :-)
> 
> If we want to keep users and their child nodes in the same AioContext by
> default, we'll probably still need to implement all of the callbacks
> that we would need for proper AioContext management today.

Or just assume that in the common case people won't specify iothread on
-blockdev, only on -device.

Paolo

Re: [Qemu-devel] [PATCH] blk: fix aio context loss on media change

[Kevin Wolf]

Am 15.03.2017 um 16:09 hat Paolo Bonzini geschrieben:
> >>>> There should be a policy on which BB sets AioContext on the BDS (e.g.
> >>>> only the device does it), but apart from that, it should not be an issue.
> >>>
> >>> We don't know which BBs are going to be attached. We don't necessarily
> >>> have a device at all, or we could have two of them.
> >>
> >> Wow, can we really have two? :-O
> > 
> > What would prevent you from doing this? The whole blockdev work was
> > about making the block layer more flexible, so now we have this
> > flexibility of attaching more or less anything to anything (unless op
> > blockers prevent it, which is why they are important for actually
> > supporting blockdev).
> 
> Yeah, the actual question was more "will the blockers allow two" devices
> behind the same BDS.  But I suppose there's no reason to prevent that
> (emulating multipath, for example).

For read-only devices, there's no reason anyway. For writable ones, you
have to set the share-rw=on qdev property, but then it's allowed.

> >>> Though maybe we should try to keep a BDS and its children in the same
> >>> AioContext anyway if that's possible? Will it make a difference?
> >>
> >> Everything can make sense---but yes, keeping the whole hierarchy in the
> >> same AioContext makes sense more often.
> > 
> > So I take this to mean that it does make a difference. :-)
> > 
> > If we want to keep users and their child nodes in the same AioContext by
> > default, we'll probably still need to implement all of the callbacks
> > that we would need for proper AioContext management today.
> 
> Or just assume that in the common case people won't specify iothread on
> -blockdev, only on -device.

Right, but then attaching a new device could mean that the BDS moves to
a different AioContext even though the other users are still on the old
one. Not sure if this is bad, but it might be unexpected.

Kevin