Re: [PATCH 4/5] virtio-scsi: Add start/stop functionality for vhost-scsi

On Tue, Sep 11, 2012 at 08:46:34AM -0500, Anthony Liguori wrote:

On 09/10/2012 01:24 AM, Michael S. Tsirkin wrote:

quoted

On Mon, Sep 10, 2012 at 08:16:54AM +0200, Paolo Bonzini wrote:

quoted

Il 09/09/2012 00:40, Michael S. Tsirkin ha scritto:

quoted

On Fri, Sep 07, 2012 at 06:00:50PM +0200, Paolo Bonzini wrote:

quoted

Il 07/09/2012 08:48, Nicholas A. Bellinger ha scritto:

quoted

Cc: Stefan Hajnoczi<redacted>
Cc: Zhi Yong Wu<redacted>
Cc: Michael S. Tsirkin<mst@redhat.com>
Cc: Paolo Bonzini<pbonzini@redhat.com>
Signed-off-by: Nicholas Bellinger<redacted>
---
 hw/virtio-pci.c  |    2 ++
 hw/virtio-scsi.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-scsi.h |    1 +
 3 files changed, 52 insertions(+), 0 deletions(-)

Please create a completely separate device vhost-scsi-pci instead (or
virtio-scsi-tcm-pci, or something like that).  It is used completely
differently from virtio-scsi-pci, it does not make sense to conflate the
two.

Ideally the name would say how it is different, not what backend it
uses. Any good suggestions?

I chose the backend name because, ideally, there would be no other
difference.  QEMU _could_ implement all the goodies in vhost-scsi (such
as reservations or ALUA), it just doesn't do that yet.

Paolo

Then why do you say "It is used completely differently from
virtio-scsi-pci"?  Isn't it just a different backend?

If yes then it should be a backend option, like it is
for virtio-net.

I don't mean to bike shed here so don't take this as a nack on
making it a backend option, but in retrospect, the way we did
vhost-net was a mistake even though I strongly advocated for it to
be a backend option.

The code to do it is really, really ugly.  I think it would have
made a lot more sense to just make it a device and then have it not
use a netdev backend or any other kind of backend split.

For instance:

qemu -device vhost-net-pci,tapfd=X

We'd have to duplicate all tap options such as upscript then,
and educate users that vhost-net-pci is in fact same as virtio-net-pci
just faster. They have enough trouble guessing "-net-pci"
in virtio-net-pci.

IMHO a simple -device virtio-net-pci,vhost=on
would have been the right thing to do in retrospect.

since vhost-net absolutely requires a tap fd, I think it's better in
the long run to not abuse the netdev backend to prevent user
confusion.

In practice adding an option (even if it was in the wrong place)
did not result in user confusion. Also in practice, renaming "virtio"
to virtio-net-pci etc did create confusion.

Having a dedicated backend type that only has one
possible option and can only be used by one device is a bit silly
too.

By now we can just enable it by default. This was always the
idea.

So I would be in favor of dropping/squashing 3/5 and radically
simplifying how this was exposed to the user.

Let's just make sure we don't have implementation tail wagging
the user interface dog :)

I would just take qemu_vhost_scsi_opts and make them device properties.

Regards,

Anthony Liguori

quoted