[Qemu-devel] [question] savevm/delvm: Is it neccesary to perform bdrv_drain_all before savevm and delvm?

[Qemu-devel] [question] savevm/delvm: Is it neccesary to perform bdrv_drain_all before savevm and delvm?

[Zhang Haoyu]

Hi,

I noticed that bdrv_drain_all is performed in load_vmstate before bdrv_snapshot_goto,
and bdrv_drain_all is performed in qmp_transaction before internal_snapshot_prepare,
so is it also neccesary to perform bdrv_drain_all in savevm and delvm?

Thanks,
Zhang Haoyu

Re: [Qemu-devel] [question] savevm/delvm: Is it neccesary to perform bdrv_drain_all before savevm and delvm?

[Kevin Wolf]

Am 20.10.2014 um 15:48 hat Zhang Haoyu geschrieben:
> Hi,
> 
> I noticed that bdrv_drain_all is performed in load_vmstate before bdrv_snapshot_goto,
> and bdrv_drain_all is performed in qmp_transaction before internal_snapshot_prepare,
> so is it also neccesary to perform bdrv_drain_all in savevm and delvm?

Definitely yes for savevm. do_savevm() calls it indirectly via
vm_stop(), so that part looks okay.

delvm doesn't affect the currently running VM, and therefore doesn't
interfere with guest requests that are in flight. So I think that a
bdrv_drain_all() isn't needed there.

Kevin

Re: [Qemu-devel] [question] savevm/delvm: Is it necessary to perform bdrv_drain_all before savevm and delvm?

[Zhang Haoyu]

 >> Hi,
 >>
 >> I noticed that bdrv_drain_all is performed in load_vmstate before 
bdrv_snapshot_goto,
 >> and bdrv_drain_all is performed in qmp_transaction before 
internal_snapshot_prepare,
 >> so is it also neccesary to perform bdrv_drain_all in savevm and delvm?
 >
 >Definitely yes for savevm. do_savevm() calls it indirectly via
 >vm_stop(), so that part looks okay.
 >
Yes, you are right.

 >delvm doesn't affect the currently running VM, and therefore doesn't
 >interfere with guest requests that are in flight. So I think that a
 >bdrv_drain_all() isn't needed there.
 >
I'm worried about that there are still pending IOs while deleting snapshot,
then is it possible that there is concurrency problem between the 
process of deleting snapshot
and the coroutine of io read/write(bdrv_co_do_rw) invoked by the pending 
IOs?
This coroutine is also in main thread.
Am I missing something?

Thanks,
Zhang Haoyu

 >Kevin

Re: [Qemu-devel] [question] savevm/delvm: Is it necessary to perform bdrv_drain_all before savevm and delvm?

[Kevin Wolf]

Am 20.10.2014 um 17:09 hat Zhang Haoyu geschrieben:
> >> Hi,
> >>
> >> I noticed that bdrv_drain_all is performed in load_vmstate before
> bdrv_snapshot_goto,
> >> and bdrv_drain_all is performed in qmp_transaction before
> internal_snapshot_prepare,
> >> so is it also neccesary to perform bdrv_drain_all in savevm and delvm?
> >
> >Definitely yes for savevm. do_savevm() calls it indirectly via
> >vm_stop(), so that part looks okay.
> >
> Yes, you are right.
> 
> >delvm doesn't affect the currently running VM, and therefore doesn't
> >interfere with guest requests that are in flight. So I think that a
> >bdrv_drain_all() isn't needed there.
> >
> I'm worried about that there are still pending IOs while deleting snapshot,
> then is it possible that there is concurrency problem between the
> process of deleting snapshot
> and the coroutine of io read/write(bdrv_co_do_rw) invoked by the
> pending IOs?
> This coroutine is also in main thread.
> Am I missing something?

What kind of concurrency problem are you thinking of?

I do see that there might be a chance of concurrency, but that doesn't
automatically mean the requests are conflicting.

Would you feel better with taking s->lock in qcow2_snapshot_delete()?
This might actually be a valid concern.

Kevin

Re: [Qemu-devel] [question] savevm/delvm: Is it necessary to perform bdrv_drain_all before savevm and delvm?

[Zhang Haoyu]

>> >> Hi,
>> >>
>> >> I noticed that bdrv_drain_all is performed in load_vmstate before
>> bdrv_snapshot_goto,
>> >> and bdrv_drain_all is performed in qmp_transaction before
>> internal_snapshot_prepare,
>> >> so is it also neccesary to perform bdrv_drain_all in savevm and delvm?
>> >
>> >Definitely yes for savevm. do_savevm() calls it indirectly via
>> >vm_stop(), so that part looks okay.
>> >
>> Yes, you are right.
>> 
>> >delvm doesn't affect the currently running VM, and therefore doesn't
>> >interfere with guest requests that are in flight. So I think that a
>> >bdrv_drain_all() isn't needed there.
>> >
>> I'm worried about that there are still pending IOs while deleting snapshot,
>> then is it possible that there is concurrency problem between the
>> process of deleting snapshot
>> and the coroutine of io read/write(bdrv_co_do_rw) invoked by the
>> pending IOs?
>> This coroutine is also in main thread.
>> Am I missing something?
>
>What kind of concurrency problem are you thinking of?
>
I have encountered two problem,
1) double-free of Qcow2DiscardRegion in qcow2_process_discards
   please see the discussing mail: [PATCH] qcow2: fix double-free of Qcow2DiscardRegion in qcow2_process_discards
2) qcow2 image is truncated to very huge size, but the size shown by qemu-img info is normal
   please see the discussing mail:  
   [question] is it possible that big-endian l1 table offset referenced by other I/O while updating l1 table offset in qcow2_update_snapshot_refcount?
   
I suspect that both of the two problems are concurrency problem mentioned above, 
please see the discussing mail.


>I do see that there might be a chance of concurrency, but that doesn't
>automatically mean the requests are conflicting.
>
>Would you feel better with taking s->lock in qcow2_snapshot_delete()?
Both deleting snapshot and the coroutine of pending io read/write(bdrv_co_do_rw)
are performed in main thread, could BDRVQcowState.lock work?

Thanks,
Zhang Haoyu
>This might actually be a valid concern.
>
>Kevin

Re: [Qemu-devel] [question] savevm/delvm: Is it necessary to perform bdrv_drain_all before savevm and delvm?

[Kevin Wolf]

Am 21.10.2014 um 03:13 hat Zhang Haoyu geschrieben:
> >> >> Hi,
> >> >>
> >> >> I noticed that bdrv_drain_all is performed in load_vmstate before
> >> bdrv_snapshot_goto,
> >> >> and bdrv_drain_all is performed in qmp_transaction before
> >> internal_snapshot_prepare,
> >> >> so is it also neccesary to perform bdrv_drain_all in savevm and delvm?
> >> >
> >> >Definitely yes for savevm. do_savevm() calls it indirectly via
> >> >vm_stop(), so that part looks okay.
> >> >
> >> Yes, you are right.
> >> 
> >> >delvm doesn't affect the currently running VM, and therefore doesn't
> >> >interfere with guest requests that are in flight. So I think that a
> >> >bdrv_drain_all() isn't needed there.
> >> >
> >> I'm worried about that there are still pending IOs while deleting snapshot,
> >> then is it possible that there is concurrency problem between the
> >> process of deleting snapshot
> >> and the coroutine of io read/write(bdrv_co_do_rw) invoked by the
> >> pending IOs?
> >> This coroutine is also in main thread.
> >> Am I missing something?
> >
> >What kind of concurrency problem are you thinking of?
> >
> I have encountered two problem,
> 1) double-free of Qcow2DiscardRegion in qcow2_process_discards
>    please see the discussing mail: [PATCH] qcow2: fix double-free of Qcow2DiscardRegion in qcow2_process_discards
> 2) qcow2 image is truncated to very huge size, but the size shown by qemu-img info is normal
>    please see the discussing mail:  
>    [question] is it possible that big-endian l1 table offset referenced by other I/O while updating l1 table offset in qcow2_update_snapshot_refcount?

Did you verify that the invalid value actually makes sense if
byteswapped? For example, that there is no reserved bit set then?

> I suspect that both of the two problems are concurrency problem mentioned above, 
> please see the discussing mail.
> 
> 
> >I do see that there might be a chance of concurrency, but that doesn't
> >automatically mean the requests are conflicting.
> >
> >Would you feel better with taking s->lock in qcow2_snapshot_delete()?
> Both deleting snapshot and the coroutine of pending io read/write(bdrv_co_do_rw)
> are performed in main thread, could BDRVQcowState.lock work?

Yes. s->lock is not a mutex for threads, but a coroutine based one.

The problem, however, is that qcow2_snapshot_delete() isn't execute in a
coroutine, so we can't take s->lock here. We would either need to move
it into a coroutine or add a bdrv_drain_all() indeed.

This also means that we probably need to review all other cases where
non-coroutine callbacks from BlockDriver might interfere with running
requests. The original assumption that they are safe as long as they are
not running in a coroutine seems to be wrong.

Kevin

Re: [Qemu-devel] [question] savevm/delvm: Is it necessary to performbdrv_drain_all before savevm and delvm?

[Zhang Haoyu]

> >> >> Hi,
> >> >>
> >> >> I noticed that bdrv_drain_all is performed in load_vmstate before
> >> bdrv_snapshot_goto,
>> >> >> and bdrv_drain_all is performed in qmp_transaction before
>> >> internal_snapshot_prepare,
>> >> >> so is it also neccesary to perform bdrv_drain_all in savevm and delvm?
>> >> >
>> >> >Definitely yes for savevm. do_savevm() calls it indirectly via
>> >> >vm_stop(), so that part looks okay.
>> >> >
>> >> Yes, you are right.
>> >> 
>> >> >delvm doesn't affect the currently running VM, and therefore doesn't
>> >> >interfere with guest requests that are in flight. So I think that a
>> >> >bdrv_drain_all() isn't needed there.
>> >> >
>> >> I'm worried about that there are still pending IOs while deleting snapshot,
>> >> then is it possible that there is concurrency problem between the
>> >> process of deleting snapshot
>> >> and the coroutine of io read/write(bdrv_co_do_rw) invoked by the
>> >> pending IOs?
>> >> This coroutine is also in main thread.
>> >> Am I missing something?
>> >
>> >What kind of concurrency problem are you thinking of?
>> >
>> I have encountered two problem,
>> 1) double-free of Qcow2DiscardRegion in qcow2_process_discards
>>    please see the discussing mail: [PATCH] qcow2: fix double-free of Qcow2DiscardRegion in qcow2_process_discards
>> 2) qcow2 image is truncated to very huge size, but the size shown by qemu-img info is normal
>>    please see the discussing mail:  
>>    [question] is it possible that big-endian l1 table offset referenced by other I/O while updating l1 table offset in qcow2_update_snapshot_refcount?
>
>Did you verify that the invalid value actually makes sense if
>byteswapped? For example, that there is no reserved bit set then?
>
Yes, exactly, I have verified that those l2 table offset are invalid value if byte-swapped.

>> I suspect that both of the two problems are concurrency problem mentioned above, 
>> please see the discussing mail.
>> 
>> 
>> >I do see that there might be a chance of concurrency, but that doesn't
>> >automatically mean the requests are conflicting.
>> >
>> >Would you feel better with taking s->lock in qcow2_snapshot_delete()?
>> Both deleting snapshot and the coroutine of pending io read/write(bdrv_co_do_rw)
>> are performed in main thread, could BDRVQcowState.lock work?
>
>Yes. s->lock is not a mutex for threads, but a coroutine based one.
>
Yes, you are right.

>The problem, however, is that qcow2_snapshot_delete() isn't execute in a
>coroutine, so we can't take s->lock here. We would either need to move
>it into a coroutine or add a bdrv_drain_all() indeed.
>
I'm inclined to add bdrv_drain_all(), just keeping consistent with the other 
snapshot-related operations, like savevm, loadvm, internal_snapshot_prepare, etc.

Thanks,
Zhang Haoyu

>This also means that we probably need to review all other cases where
>non-coroutine callbacks from BlockDriver might interfere with running
>requests. The original assumption that they are safe as long as they are
>not running in a coroutine seems to be wrong.
Agreed.
>
>Kevin