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 <address@hidden> 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