Hi Asias, On Fri, 2013-02-01 at 16:16 +0800, Asias He wrote: > In order to take advantages of Paolo's multi-queue virito-scsi, we need > multi-target support in tcm_vhost first. Otherwise all the requests go > to one queue and other queues are idle. > > This patch makes: > > 1. All the targets under the wwpn is seen and can be used by guest. > 2. No need to pass the tpgt number in struct vhost_scsi_target to > tcm_vhost.ko. Only wwpn is needed. > 3. We can always pass max_target = 255 to guest now, since we abort the > request who's target id does not exist. > > Changes in v2: > - Handle non-contiguous tpgt > > Signed-off-by: Asias He <asias@xxxxxxxxxx> > --- So this patch is looks pretty good. Just a few points below.. > drivers/vhost/tcm_vhost.c | 117 ++++++++++++++++++++++++++++++---------------- > drivers/vhost/tcm_vhost.h | 4 +- > 2 files changed, 79 insertions(+), 42 deletions(-) > > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c > index 218deb6..f1481f0 100644 > --- a/drivers/vhost/tcm_vhost.c > +++ b/drivers/vhost/tcm_vhost.c > @@ -59,8 +59,14 @@ enum { > VHOST_SCSI_VQ_IO = 2, > }; > > +#define VHOST_SCSI_MAX_TARGET 256 > + > struct vhost_scsi { > - struct tcm_vhost_tpg *vs_tpg; /* Protected by vhost_scsi->dev.mutex */ > + /* Protected by vhost_scsi->dev.mutex */ > + struct tcm_vhost_tpg *vs_tpg[VHOST_SCSI_MAX_TARGET]; > + char vs_vhost_wwpn[TRANSPORT_IQN_LEN]; > + bool vs_endpoint; > + > struct vhost_dev dev; > struct vhost_virtqueue vqs[3]; > > @@ -564,13 +570,11 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs) > u32 exp_data_len, data_first, data_num, data_direction; > unsigned out, in, i; > int head, ret; > + u8 target; > > /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */ > - tv_tpg = vs->vs_tpg; > - if (unlikely(!tv_tpg)) { > - pr_err("%s endpoint not set\n", __func__); > + if (unlikely(!vs->vs_endpoint)) > return; > - } > > mutex_lock(&vq->mutex); > vhost_disable_notify(&vs->dev, vq); > @@ -637,6 +641,28 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs) > break; > } > > + /* Extract the tpgt */ > + target = v_req.lun[1]; > + tv_tpg = vs->vs_tpg[target]; > + > + /* Target does not exist, fail the request */ > + if (unlikely(!tv_tpg)) { > + struct virtio_scsi_cmd_resp __user *resp; > + struct virtio_scsi_cmd_resp rsp; > + > + memset(&rsp, 0, sizeof(rsp)); > + rsp.response = VIRTIO_SCSI_S_BAD_TARGET; > + resp = vq->iov[out].iov_base; > + ret = __copy_to_user(resp, &rsp, sizeof(rsp)); > + if (!ret) > + vhost_add_used_and_signal(&vs->dev, > + &vs->vqs[2], head, 0); > + else > + pr_err("Faulted on virtio_scsi_cmd_resp\n"); > + > + continue; > + } > + > exp_data_len = 0; > for (i = 0; i < data_num; i++) > exp_data_len += vq->iov[data_first + i].iov_len; > @@ -745,6 +771,7 @@ static int vhost_scsi_set_endpoint( > { > struct tcm_vhost_tport *tv_tport; > struct tcm_vhost_tpg *tv_tpg; > + bool match = false; > int index; > > mutex_lock(&vs->dev.mutex); > @@ -771,14 +798,11 @@ static int vhost_scsi_set_endpoint( > } > tv_tport = tv_tpg->tport; > > - if (!strcmp(tv_tport->tport_name, t->vhost_wwpn) && > - (tv_tpg->tport_tpgt == t->vhost_tpgt)) { > + if (!strcmp(tv_tport->tport_name, t->vhost_wwpn)) { > tv_tpg->tv_tpg_vhost_count++; > - mutex_unlock(&tv_tpg->tv_tpg_mutex); > - mutex_unlock(&tcm_vhost_mutex); > > mutex_lock(&vs->dev.mutex); > - if (vs->vs_tpg) { > + if (vs->vs_tpg[tv_tpg->tport_tpgt]) { > mutex_unlock(&vs->dev.mutex); > mutex_lock(&tv_tpg->tv_tpg_mutex); > tv_tpg->tv_tpg_vhost_count--; > @@ -786,15 +810,24 @@ static int vhost_scsi_set_endpoint( > return -EEXIST; > } > > - vs->vs_tpg = tv_tpg; > + vs->vs_tpg[tv_tpg->tport_tpgt] = tv_tpg; > smp_mb__after_atomic_inc(); > + match = true; > mutex_unlock(&vs->dev.mutex); > - return 0; > } > mutex_unlock(&tv_tpg->tv_tpg_mutex); > } > mutex_unlock(&tcm_vhost_mutex); > - return -EINVAL; > + > + mutex_lock(&vs->dev.mutex); > + if (match) { > + memcpy(vs->vs_vhost_wwpn, t->vhost_wwpn, > + sizeof(vs->vs_vhost_wwpn)); > + vs->vs_endpoint = true; > + } > + mutex_unlock(&vs->dev.mutex); > + This looks racy to me.. Why not just set these while holding vs->dev.mutex above, instead of releasing + re-aquire..? > + return 0; > } > > static int vhost_scsi_clear_endpoint( > @@ -803,7 +836,8 @@ static int vhost_scsi_clear_endpoint( > { > struct tcm_vhost_tport *tv_tport; > struct tcm_vhost_tpg *tv_tpg; > - int index, ret; > + int index, ret, i; > + u8 target; > > mutex_lock(&vs->dev.mutex); > /* Verify that ring has been setup correctly. */ > @@ -813,27 +847,32 @@ static int vhost_scsi_clear_endpoint( > goto err; > } > } > + for (i = 0; i < VHOST_SCSI_MAX_TARGET; i++) { > + target = i; > > - if (!vs->vs_tpg) { > - ret = -ENODEV; > - goto err; > - } > - tv_tpg = vs->vs_tpg; > - tv_tport = tv_tpg->tport; > - > - if (strcmp(tv_tport->tport_name, t->vhost_wwpn) || > - (tv_tpg->tport_tpgt != t->vhost_tpgt)) { > - pr_warn("tv_tport->tport_name: %s, tv_tpg->tport_tpgt: %hu" > - " does not match t->vhost_wwpn: %s, t->vhost_tpgt: %hu\n", > - tv_tport->tport_name, tv_tpg->tport_tpgt, > - t->vhost_wwpn, t->vhost_tpgt); > - ret = -EINVAL; > - goto err; > + tv_tpg = vs->vs_tpg[target]; > + if (!tv_tpg) > + continue; > + > + tv_tport = tv_tpg->tport; > + if (!tv_tport) { > + ret = -ENODEV; > + goto err; > + } > + > + if (strcmp(tv_tport->tport_name, t->vhost_wwpn)) { > + pr_warn("tv_tport->tport_name: %s, tv_tpg->tport_tpgt: %hu" > + " does not match t->vhost_wwpn: %s, t->vhost_tpgt: %hu\n", > + tv_tport->tport_name, tv_tpg->tport_tpgt, > + t->vhost_wwpn, t->vhost_tpgt); > + ret = -EINVAL; > + goto err; > + } > + tv_tpg->tv_tpg_vhost_count--; > + vs->vs_tpg[target] = NULL; > + vs->vs_endpoint = false; > } > - tv_tpg->tv_tpg_vhost_count--; > - vs->vs_tpg = NULL; > mutex_unlock(&vs->dev.mutex); > - > return 0; > > err: > @@ -868,16 +907,12 @@ static int vhost_scsi_open(struct inode *inode, struct file *f) > static int vhost_scsi_release(struct inode *inode, struct file *f) > { > struct vhost_scsi *s = f->private_data; > + struct vhost_scsi_target t; > > - if (s->vs_tpg && s->vs_tpg->tport) { > - struct vhost_scsi_target backend; > - > - memcpy(backend.vhost_wwpn, s->vs_tpg->tport->tport_name, > - sizeof(backend.vhost_wwpn)); > - backend.vhost_tpgt = s->vs_tpg->tport_tpgt; > - vhost_scsi_clear_endpoint(s, &backend); > - } > - > + mutex_lock(&s->dev.mutex); > + memcpy(t.vhost_wwpn, s->vs_vhost_wwpn, sizeof(t.vhost_wwpn)); > + mutex_unlock(&s->dev.mutex); > + vhost_scsi_clear_endpoint(s, &t); I think vhost_scsi_release() logic still needs a check here to prevent vhost_scsi_clear_endpoint() from being called when no endpoint has been configured, no..? > vhost_dev_stop(&s->dev); > vhost_dev_cleanup(&s->dev, false); > kfree(s); > diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h > index 47ee80b..519a550 100644 > --- a/drivers/vhost/tcm_vhost.h > +++ b/drivers/vhost/tcm_vhost.h > @@ -93,9 +93,11 @@ struct tcm_vhost_tport { > * > * ABI Rev 0: July 2012 version starting point for v3.6-rc merge candidate + > * RFC-v2 vhost-scsi userspace. Add GET_ABI_VERSION ioctl usage > + * ABI Rev 1: January 2013. Ignore vhost_tpgt filed in struct vhost_scsi_target. > + * All the targets under vhost_wwpn can be seen and used by guset. > */ > > -#define VHOST_SCSI_ABI_VERSION 0 > +#define VHOST_SCSI_ABI_VERSION 1 > So Paolo's preference was to not bump the ABI rev for this change. I don't really have a preference either way, so I'll defer to his and MST's judgment here. --nab -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html