On 01/31/2013 07:13 PM, Michael S. Tsirkin wrote: > On Thu, Jan 31, 2013 at 05:28:21PM +0800, Asias He wrote: >> Hello Nicholas, >> >> On 01/31/2013 03:33 PM, 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. >>> >>> Signed-off-by: Asias He <asias@xxxxxxxxxx> >>> --- >>> drivers/vhost/tcm_vhost.c | 115 ++++++++++++++++++++++++++++------------------ >>> drivers/vhost/tcm_vhost.h | 4 +- >>> 2 files changed, 74 insertions(+), 45 deletions(-) >>> >>> diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c >>> index 218deb6..d50cb95 100644 >>> --- a/drivers/vhost/tcm_vhost.c >>> +++ b/drivers/vhost/tcm_vhost.c >>> @@ -59,13 +59,18 @@ 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]; >>> struct vhost_dev dev; >>> struct vhost_virtqueue vqs[3]; >>> >>> struct vhost_work vs_completion_work; /* cmd completion work item */ >>> struct llist_head vs_completion_list; /* cmd completion queue */ >>> + char vs_vhost_wwpn[TRANSPORT_IQN_LEN]; >>> + int vs_num_target; >>> }; >>> >>> /* Local pointer to allocated TCM configfs fabric module */ >>> @@ -564,13 +569,7 @@ 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; >>> - >>> - /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */ >>> - tv_tpg = vs->vs_tpg; >>> - if (unlikely(!tv_tpg)) { >>> - pr_err("%s endpoint not set\n", __func__); >>> - return; >>> - } >>> + u8 target; >>> >>> mutex_lock(&vq->mutex); >>> vhost_disable_notify(&vs->dev, vq); >>> @@ -637,6 +636,35 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs) >>> break; >>> } >>> >>> + /* Extract the tpgt */ >>> + target = v_req.lun[1]; >>> + >>> + /* Target does not exit, fail the request */ >>> + if (unlikely(target >= vs->vs_num_target)) { >>> + 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; >>> + } >>> + >>> + tv_tpg = vs->vs_tpg[target]; >>> + if (unlikely(!tv_tpg)) { >>> + /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */ >>> + pr_err("endpoint not set, target = %d\n", target); >>> + vhost_discard_vq_desc(vq, 1); >>> + break; >>> + } >>> + >>> exp_data_len = 0; >>> for (i = 0; i < data_num; i++) >>> exp_data_len += vq->iov[data_first + i].iov_len; >>> @@ -771,14 +799,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 - 1]) { >>> mutex_unlock(&vs->dev.mutex); >>> mutex_lock(&tv_tpg->tv_tpg_mutex); >>> tv_tpg->tv_tpg_vhost_count--; >>> @@ -786,15 +811,17 @@ static int vhost_scsi_set_endpoint( >>> return -EEXIST; >>> } >>> >>> - vs->vs_tpg = tv_tpg; >>> + vs->vs_tpg[tv_tpg->tport_tpgt - 1] = tv_tpg; >> >> >> tv_tpg->tport_tpgt starts from 0, right? I thought it starts from 1, >> because I always got it starts from 1 in targetcli. >> >> o- vhost >> o- naa.6001405bd4e8476d >> o- tpg1 >> o- luns >> o- lun0 >> o- tpg2 >> o- luns >> o- lun0 >> o- tpg3 >> o- luns >> o- lun0 >> o- tpg4 >> o- luns >> o- lun0 >> >> If it is true. I will cook v2 of this patch. >> >> Also, the tv_tpg->tport_tpgt can be none-continuous. e.g. >> >> o- vhost >> o- naa.6001405bd4e8476d >> o- tpg1 >> o- luns >> o- lun0 >> o- tpg2 >> o- luns >> o- lun0 >> o- tpg4 >> o- luns >> o- lun0 >> >> I will handle this in v2. >> >> >> And we need to fix another problem mentioned by Paolo. >> Otherwise we can not use this to tell if a target exists or the endpoint >> is not setup. >> >> target = v_req.lun[1]; >> tv_tpg = vs->vs_tpg[target]; >> if (tv_tpg) >> /* target exist */ >> else >> /* target does not exist*/ >> > > Why should there be a difference? With multi-target, if tv_tpg is NULL, there might be two reasons: 1. backend is not setup 2. this target does not exist. we need another flag to indicate if the endpoint is setup or not. Set/Clear it in vhost_scsi_set_endpoint()/vhost_scsi_clear_endpoint(), Test it in vhost_scsi_handle_vq(). > >> """ from paolo >> Another small bug I found is an ordering problem between >> VHOST_SET_VRING_KICK and VHOST_SCSI_SET_ENDPOINT. Starting the vq >> causes a "vhost_scsi_handle_vq endpoint not set" error in dmesg. >> Because of this I added the first two patches, which let me do >> VHOST_SCSI_SET_ENDPOINT before VHOST_SET_VRING_KICK but after setting >> up the vring. >> """ >> >> >>> smp_mb__after_atomic_inc(); >>> + if (!vs->vs_num_target++) >>> + memcpy(vs->vs_vhost_wwpn, t->vhost_wwpn, >>> + sizeof(vs->vs_vhost_wwpn)); >>> mutex_unlock(&vs->dev.mutex); >>> - return 0; >>> } >>> mutex_unlock(&tv_tpg->tv_tpg_mutex); >>> } >>> mutex_unlock(&tcm_vhost_mutex); >>> - return -EINVAL; >>> + return 0; >>> } >>> >>> static int vhost_scsi_clear_endpoint( >>> @@ -803,7 +830,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 +841,32 @@ static int vhost_scsi_clear_endpoint( >>> goto err; >>> } >>> } >>> + for (i = 0; i < vs->vs_num_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) { >>> + ret = -ENODEV; >>> + goto err; >>> + } >>> + 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; >>> } >>> - tv_tpg->tv_tpg_vhost_count--; >>> - vs->vs_tpg = NULL; >>> mutex_unlock(&vs->dev.mutex); >>> - >>> return 0; >>> >>> err: >>> @@ -868,16 +901,10 @@ 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); >>> - } >>> - >>> + memcpy(t.vhost_wwpn, s->vs_vhost_wwpn, sizeof(t.vhost_wwpn)); >>> + vhost_scsi_clear_endpoint(s, &t); >>> 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 >>> >>> struct vhost_scsi_target { >>> int abi_version; >>> >> >> >> -- >> Asias -- Asias -- 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