[PATCH v2] tcm_vhost: Multi-target support

[PATCH v2] tcm_vhost: Multi-target support

[Asias He]

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@redhat.com>
---
 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);
+
+	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);
 	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;
-- 
1.8.1

Re: [PATCH v2] tcm_vhost: Multi-target support

[Nicholas A. Bellinger]

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@redhat.com>
> ---

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

Re: [PATCH v2] tcm_vhost: Multi-target support

[Asias He]

On 02/05/2013 04:48 AM, Nicholas A. Bellinger wrote:
> 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@redhat.com>
>> ---
> 
> So this patch is looks pretty good.  Just a few points below..

Thanks.

> 
>>  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..?

I wanted to set vs->vs_endpoint after all the vs->vs_tpg[] are set.
However, we can take and release the vs->dev.mutex lock once. Will do it
in v3.


>> +	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..?

Yes. we need the check, but it is done in vhost_scsi_clear_endpoint now.

-->
                tv_tpg = vs->vs_tpg[target];
                if (!tv_tpg)
                        continue;

                tv_tport = tv_tpg->tport;
                if (!tv_tport) {
                        ret = -ENODEV;
                        goto err;
                }

>>  	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.

I am ok with either way also.

-- 
Asias