Reordered the patch, so patch 1 can go to v3.9. Asias He (2): tcm_vhost: Initialize vq->last_used_idx when set endpoint tcm_vhost: Use vq->private_data to indicate if the endpoint is setup drivers/vhost/tcm_vhost.c | 47 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 6 deletions(-) -- 1.8.1.4
Asias He
2013-Mar-28 02:17 UTC
[PATCH V2 1/2] tcm_vhost: Initialize vq->last_used_idx when set endpoint
This patch fixes guest hang when booting seabios and guest.
[ 0.576238] scsi0 : Virtio SCSI HBA
[ 0.616754] virtio_scsi virtio1: request:id 0 is not a head!
vq->last_used_idx is initialized only when /dev/vhost-scsi is
opened or closed.
vhost_scsi_open -> vhost_dev_init() -> vhost_vq_reset()
vhost_scsi_release() -> vhost_dev_cleanup -> vhost_vq_reset()
So, when guest talks to tcm_vhost after seabios does, vq->last_used_idx
still contains the old valule for seabios. This confuses guest.
Fix this by calling vhost_init_used() to init vq->last_used_idx when
we set endpoint.
Signed-off-by: Asias He <asias at redhat.com>
---
drivers/vhost/tcm_vhost.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 43fb11e..5e3d4487 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -781,8 +781,9 @@ static int vhost_scsi_set_endpoint(
{
struct tcm_vhost_tport *tv_tport;
struct tcm_vhost_tpg *tv_tpg;
+ struct vhost_virtqueue *vq;
bool match = false;
- int index, ret;
+ int index, ret, i;
mutex_lock(&vs->dev.mutex);
/* Verify that ring has been setup correctly. */
@@ -826,6 +827,12 @@ static int vhost_scsi_set_endpoint(
if (match) {
memcpy(vs->vs_vhost_wwpn, t->vhost_wwpn,
sizeof(vs->vs_vhost_wwpn));
+ for (i = 0; i < VHOST_SCSI_MAX_VQ; i++) {
+ vq = &vs->vqs[i];
+ mutex_lock(&vq->mutex);
+ vhost_init_used(vq);
+ mutex_unlock(&vq->mutex);
+ }
vs->vs_endpoint = true;
ret = 0;
} else {
--
1.8.1.4
Asias He
2013-Mar-28 02:17 UTC
[PATCH V2 2/2] tcm_vhost: Use vq->private_data to indicate if the endpoint is setup
Currently, vs->vs_endpoint is used indicate if the endpoint is setup or
not. It is set or cleared in vhost_scsi_set_endpoint() or
vhost_scsi_clear_endpoint() under the vs->dev.mutex lock. However, when
we check it in vhost_scsi_handle_vq(), we ignored the lock.
Instead of using the vs->vs_endpoint and the vs->dev.mutex lock to
indicate the status of the endpoint, we use per virtqueue
vq->private_data to indicate it. In this way, we can only take the
vq->mutex lock which is per queue and make the concurrent multiqueue
process having less lock contention. Further, in the read side of
vq->private_data, we can even do not take only lock if it is accessed in
the vhost worker thread, because it is protected by "vhost rcu".
Signed-off-by: Asias He <asias at redhat.com>
---
drivers/vhost/tcm_vhost.c | 38 +++++++++++++++++++++++++++++++++-----
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 5e3d4487..0524267 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -67,7 +67,6 @@ struct vhost_scsi {
/* 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[VHOST_SCSI_MAX_VQ];
@@ -91,6 +90,24 @@ static int iov_num_pages(struct iovec *iov)
((unsigned long)iov->iov_base & PAGE_MASK)) >> PAGE_SHIFT;
}
+static bool tcm_vhost_check_endpoint(struct vhost_virtqueue *vq)
+{
+ bool ret = false;
+
+ /*
+ * We can handle the vq only after the endpoint is setup by calling the
+ * VHOST_SCSI_SET_ENDPOINT ioctl.
+ *
+ * TODO: Check that we are running from vhost_worker which acts
+ * as read-side critical section for vhost kind of RCU.
+ * See the comments in struct vhost_virtqueue in drivers/vhost/vhost.h
+ */
+ if (rcu_dereference_check(vq->private_data, 1))
+ ret = true;
+
+ return ret;
+}
+
static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
{
return 1;
@@ -581,8 +598,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
int head, ret;
u8 target;
- /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
- if (unlikely(!vs->vs_endpoint))
+ if (!tcm_vhost_check_endpoint(vq))
return;
mutex_lock(&vq->mutex);
@@ -829,11 +845,12 @@ static int vhost_scsi_set_endpoint(
sizeof(vs->vs_vhost_wwpn));
for (i = 0; i < VHOST_SCSI_MAX_VQ; i++) {
vq = &vs->vqs[i];
+ /* Flushing the vhost_work acts as synchronize_rcu */
mutex_lock(&vq->mutex);
+ rcu_assign_pointer(vq->private_data, vs);
vhost_init_used(vq);
mutex_unlock(&vq->mutex);
}
- vs->vs_endpoint = true;
ret = 0;
} else {
ret = -EEXIST;
@@ -849,6 +866,8 @@ static int vhost_scsi_clear_endpoint(
{
struct tcm_vhost_tport *tv_tport;
struct tcm_vhost_tpg *tv_tpg;
+ struct vhost_virtqueue *vq;
+ bool match = false;
int index, ret, i;
u8 target;
@@ -884,9 +903,18 @@ static int vhost_scsi_clear_endpoint(
}
tv_tpg->tv_tpg_vhost_count--;
vs->vs_tpg[target] = NULL;
- vs->vs_endpoint = false;
+ match = true;
mutex_unlock(&tv_tpg->tv_tpg_mutex);
}
+ if (match) {
+ for (i = 0; i < VHOST_SCSI_MAX_VQ; i++) {
+ vq = &vs->vqs[i];
+ /* Flushing the vhost_work acts as synchronize_rcu */
+ mutex_lock(&vq->mutex);
+ rcu_assign_pointer(vq->private_data, NULL);
+ mutex_unlock(&vq->mutex);
+ }
+ }
mutex_unlock(&vs->dev.mutex);
return 0;
--
1.8.1.4
Nicholas A. Bellinger
2013-Mar-28 02:54 UTC
[PATCH V2 1/2] tcm_vhost: Initialize vq->last_used_idx when set endpoint
Hi Asias, On Thu, 2013-03-28 at 10:17 +0800, Asias He wrote:> This patch fixes guest hang when booting seabios and guest. > > [ 0.576238] scsi0 : Virtio SCSI HBA > [ 0.616754] virtio_scsi virtio1: request:id 0 is not a head! > > vq->last_used_idx is initialized only when /dev/vhost-scsi is > opened or closed. > > vhost_scsi_open -> vhost_dev_init() -> vhost_vq_reset() > vhost_scsi_release() -> vhost_dev_cleanup -> vhost_vq_reset() > > So, when guest talks to tcm_vhost after seabios does, vq->last_used_idx > still contains the old valule for seabios. This confuses guest. > > Fix this by calling vhost_init_used() to init vq->last_used_idx when > we set endpoint. > > Signed-off-by: Asias He <asias at redhat.com> > --- > drivers/vhost/tcm_vhost.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c > index 43fb11e..5e3d4487 100644 > --- a/drivers/vhost/tcm_vhost.c > +++ b/drivers/vhost/tcm_vhost.c > @@ -781,8 +781,9 @@ static int vhost_scsi_set_endpoint( > { > struct tcm_vhost_tport *tv_tport; > struct tcm_vhost_tpg *tv_tpg; > + struct vhost_virtqueue *vq; > bool match = false; > - int index, ret; > + int index, ret, i; > > mutex_lock(&vs->dev.mutex); > /* Verify that ring has been setup correctly. */ > @@ -826,6 +827,12 @@ static int vhost_scsi_set_endpoint( > if (match) { > memcpy(vs->vs_vhost_wwpn, t->vhost_wwpn, > sizeof(vs->vs_vhost_wwpn)); > + for (i = 0; i < VHOST_SCSI_MAX_VQ; i++) { > + vq = &vs->vqs[i]; > + mutex_lock(&vq->mutex); > + vhost_init_used(vq); > + mutex_unlock(&vq->mutex); > + }Already tried a similar patch earlier today, but as vhost_init_used() depends upon a vq->private_data being set it does not actually re-initialize ->last_used_idx..
Asias He
2013-Mar-28 03:21 UTC
[PATCH V2 1/2] tcm_vhost: Initialize vq->last_used_idx when set endpoint
On Wed, Mar 27, 2013 at 07:54:07PM -0700, Nicholas A. Bellinger wrote:> Hi Asias, > > On Thu, 2013-03-28 at 10:17 +0800, Asias He wrote: > > This patch fixes guest hang when booting seabios and guest. > > > > [ 0.576238] scsi0 : Virtio SCSI HBA > > [ 0.616754] virtio_scsi virtio1: request:id 0 is not a head! > > > > vq->last_used_idx is initialized only when /dev/vhost-scsi is > > opened or closed. > > > > vhost_scsi_open -> vhost_dev_init() -> vhost_vq_reset() > > vhost_scsi_release() -> vhost_dev_cleanup -> vhost_vq_reset() > > > > So, when guest talks to tcm_vhost after seabios does, vq->last_used_idx > > still contains the old valule for seabios. This confuses guest. > > > > Fix this by calling vhost_init_used() to init vq->last_used_idx when > > we set endpoint. > > > > Signed-off-by: Asias He <asias at redhat.com> > > --- > > drivers/vhost/tcm_vhost.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c > > index 43fb11e..5e3d4487 100644 > > --- a/drivers/vhost/tcm_vhost.c > > +++ b/drivers/vhost/tcm_vhost.c > > @@ -781,8 +781,9 @@ static int vhost_scsi_set_endpoint( > > { > > struct tcm_vhost_tport *tv_tport; > > struct tcm_vhost_tpg *tv_tpg; > > + struct vhost_virtqueue *vq; > > bool match = false; > > - int index, ret; > > + int index, ret, i; > > > > mutex_lock(&vs->dev.mutex); > > /* Verify that ring has been setup correctly. */ > > @@ -826,6 +827,12 @@ static int vhost_scsi_set_endpoint( > > if (match) { > > memcpy(vs->vs_vhost_wwpn, t->vhost_wwpn, > > sizeof(vs->vs_vhost_wwpn)); > > + for (i = 0; i < VHOST_SCSI_MAX_VQ; i++) { > > + vq = &vs->vqs[i]; > > + mutex_lock(&vq->mutex); > > + vhost_init_used(vq); > > + mutex_unlock(&vq->mutex); > > + } > > Already tried a similar patch earlier today, but as vhost_init_used() > depends upon a vq->private_data being set it does not actually > re-initialize ->last_used_idx.. >Sigh... Ah, We have this in vhost_init_used if (!vq->private_data) return 0; Michael, how bad if we let the original patch 1/2 and 2/2 go to 3.9. -- Asias