Re: [PATCH V2 0/6] tcm_vhost hotplug/hotunplug support and locking/flushing fix

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Mar 12, 2013 at 10:31:09AM +0800, Asias He wrote:
> On Mon, Mar 11, 2013 at 02:03:27PM +0200, Michael S. Tsirkin wrote:
> > On Fri, Mar 08, 2013 at 10:21:41AM +0800, Asias He wrote:
> > > Changes in v2:
> > > - Remove code duplication in tcm_vhost_{hotplug,hotunplug}
> > > - Fix racing of vs_events_nr
> > > - Add flush fix patch to this series
> > > Asias He (6):
> > >   tcm_vhost: Add missed lock in vhost_scsi_clear_endpoint()
> > >   tcm_vhost: Introduce tcm_vhost_check_feature()
> > >   tcm_vhost: Introduce tcm_vhost_check_endpoint()
> > >   tcm_vhost: Fix vs->vs_endpoint checking in vhost_scsi_handle_vq()
> > >   tcm_vhost: Add hotplug/hotunplug support
> > >   tcm_vhost: Flush vhost_work in vhost_scsi_flush()
> > > 
> > 
> > Are all these patches bugfixes?
> 
> Nope.
> 
> > Please don't add any more features in kernel until qemu starts using
> > this driver. 
> 
> Why? Why should qemu prevent from improving the driver and the benefit
> to the other user of the driver? Kvm tool is already using it since last
> Aug. This adds the missing disk hotplug support to kvm tool.

Basically we shouldn't add stuff to kernel for a single user.
Before we commit to support features forever, let's take the time
to see that the interfaces satisfy the requirements of multiple users.
It doesn't have to be qemu but we need to see several users.

> Plus, this
> is not a pure "feature", it is designed in virtio-scsi spec. 

The spec doesn't say it's a mandatory feature or did I miss something?

> > We put it in kernel after at the KVM forum it looked like
> > everyone agrees it's useful and qemu and kvmtool will use it, but it has
> > been in kernel since July and qemu patches are still outstanding.
> 
> I am working on the qemu bits where Paolo and Nicholas left.

Great.

> > If only part of the patches are bugfixes could you please separate them
> > out and submit for 3.9?
> 
> I will separate patch 2 and 5 for hotplug support and others for
> bugfixes.

Thanks.

> > Thanks,
> > 
> > >  drivers/vhost/tcm_vhost.c | 243 ++++++++++++++++++++++++++++++++++++++++++++--
> > >  drivers/vhost/tcm_vhost.h |  10 ++
> > >  2 files changed, 247 insertions(+), 6 deletions(-)
> > > 
> > > -- 
> > > 1.8.1.4
> 
> -- 
> Asias