Re: [Qemu-devel] [PULL 18/33] vhost-user-scsi: Introduce vhost-user-scsi host device

[Paolo Bonzini]

On 08/06/2017 13:05, Felipe Franciosi wrote:
>> Weird.  Doesn't QEMU wait for response to GET_VRING_BASE? I think
>> it does since we migrate the returned value.
>
> It does and that's what I said on my first message. On
> GET_VRING_BASE, a vhost-user-scsi backend application will stop
> picking up new requests and, _additionally_, quiesce the VQ (by
> waiting for any outstanding I/O to complete or cancelling them)
> before returning the last_avail_idx.

Yes, I understand now.

> Such an application therefore doesn't _necessarily_ need a drain
> call. However, we could extend vhost-user to include a message for
> draining (which can be implemented as "wait for pending I/O to
> complete" or "cancel all outstanding" depending on the
> implementation).

Ok, and I see that vhost_dev_stop also calls get_vring_base so my
worries are appeased. :)

Paolo

Re: [Qemu-devel] [PULL 18/33] vhost-user-scsi: Introduce vhost-user-scsi host device

[Felipe Franciosi]

> On 8 Jun 2017, at 01:31, Michael S. Tsirkin  wrote:
> 
> On Wed, Jun 07, 2017 at 02:56:57PM -0400, Paolo Bonzini wrote:
>> 
 This could be documented,
>>> 
>>> It is documented AFAIK. Pls take a look at the spec documentation.
>> 
>> Found it now.  It's not under GET_VRING_BASE, it's under "starting
>> and stopping rings"---fair enough.
>> 
>> In the case of vhost-user-scsi, however, QEMU also must not proceed
>> until vhost-user-scsi has drained the pending I/O---and this pending
>> I/O would be completed _after_ QEMU has sent GET_VRING_BASE.
> 
> Weird.  Doesn't QEMU wait for response to GET_VRING_BASE?
> I think it does since we migrate the returned value.

It does and that's what I said on my first message. On GET_VRING_BASE, a 
vhost-user-scsi backend application will stop picking up new requests and, 
_additionally_, quiesce the VQ (by waiting for any outstanding I/O to complete 
or cancelling them) before returning the last_avail_idx.

Such an application therefore doesn't _necessarily_ need a drain call. However, 
we could extend vhost-user to include a message for draining (which can be 
implemented as "wait for pending I/O to complete" or "cancel all outstanding" 
depending on the implementation).

> 
> Spec says:
>   Client must only process each ring when it is started.
> so this isn't expected. I guess whoever wrote vhost-user-scsi
> understood "process" as "start processing".
> What was intended is reading or writing any part of ring.
> The used ring must not be updated after ring is stopped.
> A spec clarification might in order.
> 
>> Is this handled by VHOST_USER_PROTOCOL_F_REPLY_ACK already?  If so,
>> migration would be denied if the server lacks that protocol feature.
>> 
>> Paolo
> 
> GET_VRING_BASE does not need an ack, after respoding ring should
> be stopped.

Precisely. To clarify further: the REPLY_ACK feature was added as an extension 
only for messages that do not demand a reply. This is to cope with a vhost-user 
protocol deficiency where commands like SET_MEM_TABLE are asynchronous. Qemu 
sends them and carry on executing before the backend can ACK new regions were 
configured. The original vhost(-kernel) doesn't suffer that problem as messages 
are sent via an ioctl() which blocks until the backend is done.

 but perhaps it's best to add a START_STOP
 feature and message to the vhost-user protocol?
>>> 
>>> We just never need to GET_VRING_BASE if ring keeps going -
>>> makes no sense since base gets invalidated immediately.
>>> 
>>> 
>>> 
 The feature then can be optional for vhost-user-net and mandatory for
 vhost-user-scsi.  When this is done we can remove .unmigratable.
 
 Thanks,
 
 Paolo
>>> 
>>> If vhost-user-scsi does not stop the ring after responding to
>>> GET_VRING_BASE, it's just a bug that needs to be fixed.

Any backend application written for vhost-user-scsi needs to adhere to the spec 
and stop picking up new requests upon a GET_VRING_BASE. Depending on the 
storage backend, it can decide whether to also quiesce the VQ or potentially 
cancel any outstanding request.

Worth noting, this is very different in networking as packets can just be 
dropped.

Thanks,
Felipe

Re: [Qemu-devel] [PULL 18/33] vhost-user-scsi: Introduce vhost-user-scsi host device

[Michael S. Tsirkin]

On Wed, Jun 07, 2017 at 02:56:57PM -0400, Paolo Bonzini wrote:
> 
> > > This could be documented,
> > 
> > It is documented AFAIK. Pls take a look at the spec documentation.
> 
> Found it now.  It's not under GET_VRING_BASE, it's under "starting
> and stopping rings"---fair enough.
> 
> In the case of vhost-user-scsi, however, QEMU also must not proceed
> until vhost-user-scsi has drained the pending I/O---and this pending
> I/O would be completed _after_ QEMU has sent GET_VRING_BASE.

Weird.  Doesn't QEMU wait for response to GET_VRING_BASE?
I think it does since we migrate the returned value.

Spec says:
Client must only process each ring when it is started.
so this isn't expected. I guess whoever wrote vhost-user-scsi
understood "process" as "start processing".
What was intended is reading or writing any part of ring.
The used ring must not be updated after ring is stopped.
A spec clarification might in order.

> Is this handled by VHOST_USER_PROTOCOL_F_REPLY_ACK already?  If so,
> migration would be denied if the server lacks that protocol feature.
> 
> Paolo

GET_VRING_BASE does not need an ack, after respoding ring should
be stopped.

> > > but perhaps it's best to add a START_STOP
> > > feature and message to the vhost-user protocol?
> > 
> > We just never need to GET_VRING_BASE if ring keeps going -
> > makes no sense since base gets invalidated immediately.
> > 
> > 
> > 
> > > The feature then can be optional for vhost-user-net and mandatory for
> > > vhost-user-scsi.  When this is done we can remove .unmigratable.
> > > 
> > > Thanks,
> > > 
> > > Paolo
> > 
> > If vhost-user-scsi does not stop the ring after responding to
> > GET_VRING_BASE, it's just a bug that needs to be fixed.
> > 
> > > > > 
> > > > > Can you please send a version of your patch that uses .unmigratable?
> > > > 
> > > > Sure I can do that. We can work on the migration later on.
> > > > 
> > > > > 
> > > > > I'll send a v6 that momentarily drops vhost-scsi, but I intend to
> > > > > include it again in the next pull request.
> > > > 
> > > > Sounds good to me.
> > > > 
> > > > Felipe
> > > > 
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > Paolo
> > > > 
> > > > 
> > 

Re: [Qemu-devel] [PULL 18/33] vhost-user-scsi: Introduce vhost-user-scsi host device

[Paolo Bonzini]

> > This could be documented,
> 
> It is documented AFAIK. Pls take a look at the spec documentation.

Found it now.  It's not under GET_VRING_BASE, it's under "starting
and stopping rings"---fair enough.

In the case of vhost-user-scsi, however, QEMU also must not proceed
until vhost-user-scsi has drained the pending I/O---and this pending
I/O would be completed _after_ QEMU has sent GET_VRING_BASE.

Is this handled by VHOST_USER_PROTOCOL_F_REPLY_ACK already?  If so,
migration would be denied if the server lacks that protocol feature.

Paolo

> > but perhaps it's best to add a START_STOP
> > feature and message to the vhost-user protocol?
> 
> We just never need to GET_VRING_BASE if ring keeps going -
> makes no sense since base gets invalidated immediately.
> 
> 
> 
> > The feature then can be optional for vhost-user-net and mandatory for
> > vhost-user-scsi.  When this is done we can remove .unmigratable.
> > 
> > Thanks,
> > 
> > Paolo
> 
> If vhost-user-scsi does not stop the ring after responding to
> GET_VRING_BASE, it's just a bug that needs to be fixed.
> 
> > > > 
> > > > Can you please send a version of your patch that uses .unmigratable?
> > > 
> > > Sure I can do that. We can work on the migration later on.
> > > 
> > > > 
> > > > I'll send a v6 that momentarily drops vhost-scsi, but I intend to
> > > > include it again in the next pull request.
> > > 
> > > Sounds good to me.
> > > 
> > > Felipe
> > > 
> > > > 
> > > > Thanks,
> > > > 
> > > > Paolo
> > > 
> > > 
> 

Re: [Qemu-devel] [PULL 18/33] vhost-user-scsi: Introduce vhost-user-scsi host device

[Michael S. Tsirkin]

On Wed, Jun 07, 2017 at 02:22:24PM -0400, Paolo Bonzini wrote:
> > > How, since there is no synchronization point between the vhost-user
> > > server on the source and the destination?
> > 
> > The idea is that the backend should both stop picking up new requests and
> > also quiesce outstanding requests upon a GET_VRING_BASE vhost message.
> 
> This could be documented,

It is documented AFAIK. Pls take a look at the spec documentation.

> but perhaps it's best to add a START_STOP
> feature and message to the vhost-user protocol?

We just never need to GET_VRING_BASE if ring keeps going -
makes no sense since base gets invalidated immediately.



> The feature then can be optional for vhost-user-net and mandatory for
> vhost-user-scsi.  When this is done we can remove .unmigratable.
> 
> Thanks,
> 
> Paolo

If vhost-user-scsi does not stop the ring after responding to
GET_VRING_BASE, it's just a bug that needs to be fixed.

> > > 
> > > Can you please send a version of your patch that uses .unmigratable?
> > 
> > Sure I can do that. We can work on the migration later on.
> > 
> > > 
> > > I'll send a v6 that momentarily drops vhost-scsi, but I intend to
> > > include it again in the next pull request.
> > 
> > Sounds good to me.
> > 
> > Felipe
> > 
> > > 
> > > Thanks,
> > > 
> > > Paolo
> > 
> > 

Re: [Qemu-devel] [PULL 18/33] vhost-user-scsi: Introduce vhost-user-scsi host device

[Felipe Franciosi]

> On 7 Jun 2017, at 16:47, Peter Maydell  wrote:
> 
> On 7 June 2017 at 16:39, Felipe Franciosi  wrote:
>> 
>>> On 7 Jun 2017, at 16:37, Peter Maydell  wrote:
>>> 
>>> On 7 June 2017 at 16:28, Paolo Bonzini  wrote:
 From: Felipe Franciosi 
 
 This commit introduces a vhost-user device for SCSI. This is based
 on the existing vhost-scsi implementation, but done over vhost-user
 instead. It also uses a chardev to connect to the backend. Unlike
 vhost-scsi (today), VMs using vhost-user-scsi can be live migrated.
 
 To use it, start Qemu with a command line equivalent to:
 
 qemu-system-x86_64 \
  -chardev socket,id=vus0,path=/tmp/vus.sock \
  -device vhost-user-scsi-pci,chardev=vus0,bus=pci.0,addr=...
 
 A separate commit presents a sample application linked with libiscsi to
 provide a backend for vhost-user-scsi.
 
 Signed-off-by: Felipe Franciosi 
 Message-Id: <1488479153-21203-4-git-send-email-fel...@nutanix.com>
 [Disable migration for now, since it does not support bdrv_drain. - Paolo]
>>> 
>>> I was expecting this to mean a VMStateDescription with a
>>> ".unmigratable = 1" field, but it doesn't seem to have one.
>>> Does it disable migration some other way?
>> 
>> Hi Peter,
>> 
>> vhost-user-scsi supports migration.
> 
> Paolo's change comment in the commit message quoted above
> says it does not, which is what I was remarking on.
> 
> (Your original patches use register_savevm(), which is a function
> that has just gone away. They'd need to use VMStateDescription
> structs instead to support migration.)

Oh thanks for pointing that out, I missed it at the bottom of the commit 
message.

F.

> 
> thanks
> -- PMM

Re: [Qemu-devel] [PULL 18/33] vhost-user-scsi: Introduce vhost-user-scsi host device

[Paolo Bonzini]

> > How, since there is no synchronization point between the vhost-user
> > server on the source and the destination?
> 
> The idea is that the backend should both stop picking up new requests and
> also quiesce outstanding requests upon a GET_VRING_BASE vhost message.

This could be documented, but perhaps it's best to add a START_STOP
feature and message to the vhost-user protocol?

The feature then can be optional for vhost-user-net and mandatory for
vhost-user-scsi.  When this is done we can remove .unmigratable.

Thanks,

Paolo

> > 
> > Can you please send a version of your patch that uses .unmigratable?
> 
> Sure I can do that. We can work on the migration later on.
> 
> > 
> > I'll send a v6 that momentarily drops vhost-scsi, but I intend to
> > include it again in the next pull request.
> 
> Sounds good to me.
> 
> Felipe
> 
> > 
> > Thanks,
> > 
> > Paolo
> 
> 

Re: [Qemu-devel] [PULL 18/33] vhost-user-scsi: Introduce vhost-user-scsi host device

[Felipe Franciosi]

> On 7 Jun 2017, at 17:21, Paolo Bonzini  wrote:
> 
> 
> 
> On 07/06/2017 17:39, Felipe Franciosi wrote:
>> 
>>> On 7 Jun 2017, at 16:37, Peter Maydell  wrote:
>>> 
>>> On 7 June 2017 at 16:28, Paolo Bonzini  wrote:
 From: Felipe Franciosi 
 
 This commit introduces a vhost-user device for SCSI. This is based
 on the existing vhost-scsi implementation, but done over vhost-user
 instead. It also uses a chardev to connect to the backend. Unlike
 vhost-scsi (today), VMs using vhost-user-scsi can be live migrated.
 
 To use it, start Qemu with a command line equivalent to:
 
 qemu-system-x86_64 \
  -chardev socket,id=vus0,path=/tmp/vus.sock \
  -device vhost-user-scsi-pci,chardev=vus0,bus=pci.0,addr=...
 
 A separate commit presents a sample application linked with libiscsi to
 provide a backend for vhost-user-scsi.
 
 Signed-off-by: Felipe Franciosi 
 Message-Id: <1488479153-21203-4-git-send-email-fel...@nutanix.com>
 [Disable migration for now, since it does not support bdrv_drain. - Paolo]
>>> 
>>> I was expecting this to mean a VMStateDescription with a
>>> ".unmigratable = 1" field, but it doesn't seem to have one.
>>> Does it disable migration some other way?
>> 
>> Hi Peter,
>> 
>> vhost-user-scsi supports migration.
> 
> How, since there is no synchronization point between the vhost-user
> server on the source and the destination?

The idea is that the backend should both stop picking up new requests and also 
quiesce outstanding requests upon a GET_VRING_BASE vhost message.

> 
> Can you please send a version of your patch that uses .unmigratable?

Sure I can do that. We can work on the migration later on.

> 
> I'll send a v6 that momentarily drops vhost-scsi, but I intend to
> include it again in the next pull request.

Sounds good to me.

Felipe

> 
> Thanks,
> 
> Paolo

Re: [Qemu-devel] [PULL 18/33] vhost-user-scsi: Introduce vhost-user-scsi host device

[Paolo Bonzini]

On 07/06/2017 17:39, Felipe Franciosi wrote:
> 
>> On 7 Jun 2017, at 16:37, Peter Maydell  wrote:
>>
>> On 7 June 2017 at 16:28, Paolo Bonzini  wrote:
>>> From: Felipe Franciosi 
>>>
>>> This commit introduces a vhost-user device for SCSI. This is based
>>> on the existing vhost-scsi implementation, but done over vhost-user
>>> instead. It also uses a chardev to connect to the backend. Unlike
>>> vhost-scsi (today), VMs using vhost-user-scsi can be live migrated.
>>>
>>> To use it, start Qemu with a command line equivalent to:
>>>
>>> qemu-system-x86_64 \
>>>   -chardev socket,id=vus0,path=/tmp/vus.sock \
>>>   -device vhost-user-scsi-pci,chardev=vus0,bus=pci.0,addr=...
>>>
>>> A separate commit presents a sample application linked with libiscsi to
>>> provide a backend for vhost-user-scsi.
>>>
>>> Signed-off-by: Felipe Franciosi 
>>> Message-Id: <1488479153-21203-4-git-send-email-fel...@nutanix.com>
>>> [Disable migration for now, since it does not support bdrv_drain. - Paolo]
>>
>> I was expecting this to mean a VMStateDescription with a
>> ".unmigratable = 1" field, but it doesn't seem to have one.
>> Does it disable migration some other way?
>
> Hi Peter,
> 
> vhost-user-scsi supports migration.

How, since there is no synchronization point between the vhost-user
server on the source and the destination?

Can you please send a version of your patch that uses .unmigratable?

I'll send a v6 that momentarily drops vhost-scsi, but I intend to
include it again in the next pull request.

Thanks,

Paolo

Re: [Qemu-devel] [PULL 18/33] vhost-user-scsi: Introduce vhost-user-scsi host device

[Peter Maydell]

On 7 June 2017 at 16:39, Felipe Franciosi  wrote:
>
>> On 7 Jun 2017, at 16:37, Peter Maydell  wrote:
>>
>> On 7 June 2017 at 16:28, Paolo Bonzini  wrote:
>>> From: Felipe Franciosi 
>>>
>>> This commit introduces a vhost-user device for SCSI. This is based
>>> on the existing vhost-scsi implementation, but done over vhost-user
>>> instead. It also uses a chardev to connect to the backend. Unlike
>>> vhost-scsi (today), VMs using vhost-user-scsi can be live migrated.
>>>
>>> To use it, start Qemu with a command line equivalent to:
>>>
>>> qemu-system-x86_64 \
>>>   -chardev socket,id=vus0,path=/tmp/vus.sock \
>>>   -device vhost-user-scsi-pci,chardev=vus0,bus=pci.0,addr=...
>>>
>>> A separate commit presents a sample application linked with libiscsi to
>>> provide a backend for vhost-user-scsi.
>>>
>>> Signed-off-by: Felipe Franciosi 
>>> Message-Id: <1488479153-21203-4-git-send-email-fel...@nutanix.com>
>>> [Disable migration for now, since it does not support bdrv_drain. - Paolo]
>>
>> I was expecting this to mean a VMStateDescription with a
>> ".unmigratable = 1" field, but it doesn't seem to have one.
>> Does it disable migration some other way?
>
> Hi Peter,
>
> vhost-user-scsi supports migration.

Paolo's change comment in the commit message quoted above
says it does not, which is what I was remarking on.

(Your original patches use register_savevm(), which is a function
that has just gone away. They'd need to use VMStateDescription
structs instead to support migration.)

thanks
-- PMM

Re: [Qemu-devel] [PULL 18/33] vhost-user-scsi: Introduce vhost-user-scsi host device

[Felipe Franciosi]

> On 7 Jun 2017, at 16:37, Peter Maydell  wrote:
> 
> On 7 June 2017 at 16:28, Paolo Bonzini  wrote:
>> From: Felipe Franciosi 
>> 
>> This commit introduces a vhost-user device for SCSI. This is based
>> on the existing vhost-scsi implementation, but done over vhost-user
>> instead. It also uses a chardev to connect to the backend. Unlike
>> vhost-scsi (today), VMs using vhost-user-scsi can be live migrated.
>> 
>> To use it, start Qemu with a command line equivalent to:
>> 
>> qemu-system-x86_64 \
>>   -chardev socket,id=vus0,path=/tmp/vus.sock \
>>   -device vhost-user-scsi-pci,chardev=vus0,bus=pci.0,addr=...
>> 
>> A separate commit presents a sample application linked with libiscsi to
>> provide a backend for vhost-user-scsi.
>> 
>> Signed-off-by: Felipe Franciosi 
>> Message-Id: <1488479153-21203-4-git-send-email-fel...@nutanix.com>
>> [Disable migration for now, since it does not support bdrv_drain. - Paolo]
> 
> I was expecting this to mean a VMStateDescription with a
> ".unmigratable = 1" field, but it doesn't seem to have one.
> Does it disable migration some other way?

Hi Peter,

vhost-user-scsi supports migration.

Thanks,
Felipe

> 
> thanks
> -- PMM

Re: [Qemu-devel] [PULL 18/33] vhost-user-scsi: Introduce vhost-user-scsi host device

[Peter Maydell]

On 7 June 2017 at 16:28, Paolo Bonzini  wrote:
> From: Felipe Franciosi 
>
> This commit introduces a vhost-user device for SCSI. This is based
> on the existing vhost-scsi implementation, but done over vhost-user
> instead. It also uses a chardev to connect to the backend. Unlike
> vhost-scsi (today), VMs using vhost-user-scsi can be live migrated.
>
> To use it, start Qemu with a command line equivalent to:
>
> qemu-system-x86_64 \
>-chardev socket,id=vus0,path=/tmp/vus.sock \
>-device vhost-user-scsi-pci,chardev=vus0,bus=pci.0,addr=...
>
> A separate commit presents a sample application linked with libiscsi to
> provide a backend for vhost-user-scsi.
>
> Signed-off-by: Felipe Franciosi 
> Message-Id: <1488479153-21203-4-git-send-email-fel...@nutanix.com>
> [Disable migration for now, since it does not support bdrv_drain. - Paolo]

I was expecting this to mean a VMStateDescription with a
".unmigratable = 1" field, but it doesn't seem to have one.
Does it disable migration some other way?

thanks
-- PMM