Re: [PATCH 5/5] tcm_vhost: Add hotplug/hotunplug support

[Asias He <asias@redhat.com>]

On Wed, Mar 06, 2013 at 10:21:09AM +0100, Stefan Hajnoczi wrote:
> On Wed, Mar 06, 2013 at 02:16:30PM +0800, Asias He wrote:
> > +static struct tcm_vhost_evt *tcm_vhost_allocate_evt(struct vhost_scsi *vs,
> > +	u32 event, u32 reason)
> > +{
> > +	struct tcm_vhost_evt *evt;
> > +
> > +	if (atomic_read(&vs->vs_events_nr) > VHOST_SCSI_MAX_EVENT)
> > +		return NULL;
> > +
> > +	evt = kzalloc(sizeof(*evt), GFP_KERNEL);
> > +
> > +	if (evt) {
> > +		atomic_inc(&vs->vs_events_nr);
> 
> This looks suspicious: checking vs_events_nr > VHOST_SCSI_MAX_EVENT
> first and then incrementing later isn't atomic!

This does not matter. (1) and (2) are okay. In case (3), the other side
can only decrease the number of event, the limit will not be exceeded.

(1) 
   		   atomic_dec()
   atomic_read() 
   atomic_inc()
(2)
   atomic_read() 
   atomic_inc()
   		   atomic_dec()

(3)
   atomic_read() 
   		   atomic_dec()
   atomic_inc()

          
> > +static int tcm_vhost_hotunplug(struct tcm_vhost_tpg *tpg, struct se_lun *lun)
> > +{
> > +	struct vhost_scsi *vs = tpg->vhost_scsi;
> > +
> > +	mutex_lock(&tpg->tv_tpg_mutex);
> > +	vs = tpg->vhost_scsi;
> > +	mutex_unlock(&tpg->tv_tpg_mutex);
> > +	if (!vs)
> > +		return -EOPNOTSUPP;
> > +
> > +	if (!tcm_vhost_check_feature(vs, VIRTIO_SCSI_F_HOTPLUG))
> > +		return -EOPNOTSUPP;
> > +
> > +	return tcm_vhost_send_evt(vs, tpg, lun,
> > +			VIRTIO_SCSI_T_TRANSPORT_RESET,
> > +			VIRTIO_SCSI_EVT_RESET_REMOVED);
> > +}
> 
> tcm_vhost_hotplug() and tcm_vhost_hotunplug() are the same function
> except for VIRTIO_SCSI_EVT_RESET_RESCAN vs
> VIRTIO_SCSI_EVT_RESET_REMOVED.  That can be passed in as an argument and
> the code duplication can be eliminated.

I thought about this also. We can have a tcm_vhost_do_hotplug() helper.

   tcm_vhost_do_hotplug(tpg, lun, plug)
   
   tcm_vhost_hotplug() {
   	tcm_vhost_do_hotplug(tpg, lun, true)
   }
   
   tcm_vhost_hotunplug() {
   	tcm_vhost_do_hotplug(tpg, lun, false)
   }

The reason I did not do that is I do not like the true/false argument
but anyway this could remove duplication. I will do it.

-- 
Asias