Asias He (4): tcm_vhost: Add missed lock in vhost_scsi_clear_endpoint() tcm_vhost: Introduce tcm_vhost_check_endpoint() tcm_vhost: Fix vs->vs_endpoint checking in vhost_scsi_handle_vq() tcm_vhost: Flush vhost_work in vhost_scsi_flush() drivers/vhost/tcm_vhost.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) -- 1.8.1.4
Asias He
2013-Mar-12 02:42 UTC
[PATCH 1/4] tcm_vhost: Add missed lock in vhost_scsi_clear_endpoint()
tv_tpg->tv_tpg_vhost_count should be protected by tv_tpg->tv_tpg_mutex.
Signed-off-by: Asias He <asias at redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha at redhat.com>
---
drivers/vhost/tcm_vhost.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 9951297..b3e50d7 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -860,9 +860,11 @@ static int vhost_scsi_clear_endpoint(
if (!tv_tpg)
continue;
+ mutex_lock(&tv_tpg->tv_tpg_mutex);
tv_tport = tv_tpg->tport;
if (!tv_tport) {
ret = -ENODEV;
+ mutex_unlock(&tv_tpg->tv_tpg_mutex);
goto err;
}
@@ -872,11 +874,13 @@ static int vhost_scsi_clear_endpoint(
tv_tport->tport_name, tv_tpg->tport_tpgt,
t->vhost_wwpn, t->vhost_tpgt);
ret = -EINVAL;
+ mutex_unlock(&tv_tpg->tv_tpg_mutex);
goto err;
}
tv_tpg->tv_tpg_vhost_count--;
vs->vs_tpg[target] = NULL;
vs->vs_endpoint = false;
+ mutex_unlock(&tv_tpg->tv_tpg_mutex);
}
mutex_unlock(&vs->dev.mutex);
return 0;
--
1.8.1.4
Asias He
2013-Mar-12 02:42 UTC
[PATCH 2/4] tcm_vhost: Introduce tcm_vhost_check_endpoint()
This helper is useful to check if vs->vs_endpoint is setup by
vhost_scsi_set_endpoint()
Signed-off-by: Asias He <asias at redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha at redhat.com>
---
drivers/vhost/tcm_vhost.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index b3e50d7..29612bc 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -91,6 +91,18 @@ 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_scsi *vs)
+{
+ bool ret = false;
+
+ mutex_lock(&vs->dev.mutex);
+ if (vs->vs_endpoint)
+ ret = true;
+ mutex_unlock(&vs->dev.mutex);
+
+ return ret;
+}
+
static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
{
return 1;
--
1.8.1.4
Asias He
2013-Mar-12 02:42 UTC
[PATCH 3/4] tcm_vhost: Fix vs->vs_endpoint checking in vhost_scsi_handle_vq()
vs->vs_endpoint is protected by the vs->dev.mutex. Use tcm_vhost_check_endpoint() to do check. The helper does the needed locking for us. Signed-off-by: Asias He <asias at redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha at redhat.com> --- drivers/vhost/tcm_vhost.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c index 29612bc..61093d1 100644 --- a/drivers/vhost/tcm_vhost.c +++ b/drivers/vhost/tcm_vhost.c @@ -594,7 +594,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs, u8 target; /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */ - if (unlikely(!vs->vs_endpoint)) + if (!tcm_vhost_check_endpoint(vs)) return; mutex_lock(&vq->mutex); -- 1.8.1.4
Asias He
2013-Mar-12 02:42 UTC
[PATCH 4/4] tcm_vhost: Flush vhost_work in vhost_scsi_flush()
We also need to flush the vhost_works. One is the completion vhost_work, the other is event vhost_work. Signed-off-by: Asias He <asias at redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha at redhat.com> --- drivers/vhost/tcm_vhost.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c index 61093d1..c5157cd 100644 --- a/drivers/vhost/tcm_vhost.c +++ b/drivers/vhost/tcm_vhost.c @@ -953,6 +953,7 @@ static void vhost_scsi_flush(struct vhost_scsi *vs) for (i = 0; i < VHOST_SCSI_MAX_VQ; i++) vhost_scsi_flush_vq(vs, i); + vhost_work_flush(&vs->dev, &vs->vs_completion_work); } static int vhost_scsi_set_features(struct vhost_scsi *vs, u64 features) -- 1.8.1.4
Paolo Bonzini
2013-Mar-12 08:26 UTC
[PATCH 2/4] tcm_vhost: Introduce tcm_vhost_check_endpoint()
Il 12/03/2013 03:42, Asias He ha scritto:> This helper is useful to check if vs->vs_endpoint is setup by > vhost_scsi_set_endpoint() > > Signed-off-by: Asias He <asias at redhat.com> > Reviewed-by: Stefan Hajnoczi <stefanha at redhat.com> > --- > drivers/vhost/tcm_vhost.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c > index b3e50d7..29612bc 100644 > --- a/drivers/vhost/tcm_vhost.c > +++ b/drivers/vhost/tcm_vhost.c > @@ -91,6 +91,18 @@ 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_scsi *vs) > +{ > + bool ret = false; > + > + mutex_lock(&vs->dev.mutex); > + if (vs->vs_endpoint) > + ret = true; > + mutex_unlock(&vs->dev.mutex);The return value is invalid as soon as mutex_unlock is called, i.e. before tcm_vhost_check_endpoint returns. Instead, check vs->vs_endpoint in the caller while the mutex is taken. Paolo> + return ret; > +} > + > static int tcm_vhost_check_true(struct se_portal_group *se_tpg) > { > return 1; >
Il 12/03/2013 03:42, Asias He ha scritto:> Asias He (4): > tcm_vhost: Add missed lock in vhost_scsi_clear_endpoint() > tcm_vhost: Introduce tcm_vhost_check_endpoint() > tcm_vhost: Fix vs->vs_endpoint checking in vhost_scsi_handle_vq() > tcm_vhost: Flush vhost_work in vhost_scsi_flush() > > drivers/vhost/tcm_vhost.c | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) >Patch 1, 2, 4: Reviewed-by: Paolo Bonzini <pbonzini at redhat.com>
Michael S. Tsirkin
2013-Mar-12 11:11 UTC
[PATCH 3/4] tcm_vhost: Fix vs->vs_endpoint checking in vhost_scsi_handle_vq()
On Tue, Mar 12, 2013 at 10:42:50AM +0800, Asias He wrote:> vs->vs_endpoint is protected by the vs->dev.mutex. Use > tcm_vhost_check_endpoint() to do check. The helper does the needed > locking for us. > > Signed-off-by: Asias He <asias at redhat.com> > Reviewed-by: Stefan Hajnoczi <stefanha at redhat.com>This takes dev mutex on data path which will introduce contention esp for multiqueue. How about storing the endpoint as part of vq private data and protecting with vq mutex?> --- > drivers/vhost/tcm_vhost.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c > index 29612bc..61093d1 100644 > --- a/drivers/vhost/tcm_vhost.c > +++ b/drivers/vhost/tcm_vhost.c > @@ -594,7 +594,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs, > u8 target; > > /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */ > - if (unlikely(!vs->vs_endpoint)) > + if (!tcm_vhost_check_endpoint(vs)) > return; > > mutex_lock(&vq->mutex); > -- > 1.8.1.4
On Tue, Mar 12, 2013 at 09:26:42AM +0100, Paolo Bonzini wrote:> Il 12/03/2013 03:42, Asias He ha scritto: > > Asias He (4): > > tcm_vhost: Add missed lock in vhost_scsi_clear_endpoint() > > tcm_vhost: Introduce tcm_vhost_check_endpoint() > > tcm_vhost: Fix vs->vs_endpoint checking in vhost_scsi_handle_vq() > > tcm_vhost: Flush vhost_work in vhost_scsi_flush() > > > > drivers/vhost/tcm_vhost.c | 19 ++++++++++++++++++- > > 1 file changed, 18 insertions(+), 1 deletion(-) > > > > Patch 1, 2, 4: > > Reviewed-by: Paolo Bonzini <pbonzini at redhat.com>How is 2 useful without 3?
Michael S. Tsirkin
2013-Mar-12 11:13 UTC
[PATCH 1/4] tcm_vhost: Add missed lock in vhost_scsi_clear_endpoint()
On Tue, Mar 12, 2013 at 10:42:48AM +0800, Asias He wrote:> tv_tpg->tv_tpg_vhost_count should be protected by tv_tpg->tv_tpg_mutex. > > Signed-off-by: Asias He <asias at redhat.com> > Reviewed-by: Stefan Hajnoczi <stefanha at redhat.com> > --- > drivers/vhost/tcm_vhost.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c > index 9951297..b3e50d7 100644 > --- a/drivers/vhost/tcm_vhost.c > +++ b/drivers/vhost/tcm_vhost.c > @@ -860,9 +860,11 @@ static int vhost_scsi_clear_endpoint( > if (!tv_tpg) > continue; > > + mutex_lock(&tv_tpg->tv_tpg_mutex); > tv_tport = tv_tpg->tport; > if (!tv_tport) { > ret = -ENODEV; > + mutex_unlock(&tv_tpg->tv_tpg_mutex); > goto err; > } > > @@ -872,11 +874,13 @@ static int vhost_scsi_clear_endpoint( > tv_tport->tport_name, tv_tpg->tport_tpgt, > t->vhost_wwpn, t->vhost_tpgt); > ret = -EINVAL; > + mutex_unlock(&tv_tpg->tv_tpg_mutex); > goto err; > } > tv_tpg->tv_tpg_vhost_count--; > vs->vs_tpg[target] = NULL; > vs->vs_endpoint = false; > + mutex_unlock(&tv_tpg->tv_tpg_mutex); > } > mutex_unlock(&vs->dev.mutex); > return 0;Does the error handling have to be so messy? How about another label which does unlock and goto there?> -- > 1.8.1.4
Il 12/03/2013 12:12, Michael S. Tsirkin ha scritto:> On Tue, Mar 12, 2013 at 09:26:42AM +0100, Paolo Bonzini wrote: >> Il 12/03/2013 03:42, Asias He ha scritto: >>> Asias He (4): >>> tcm_vhost: Add missed lock in vhost_scsi_clear_endpoint() >>> tcm_vhost: Introduce tcm_vhost_check_endpoint() >>> tcm_vhost: Fix vs->vs_endpoint checking in vhost_scsi_handle_vq() >>> tcm_vhost: Flush vhost_work in vhost_scsi_flush() >>> >>> drivers/vhost/tcm_vhost.c | 19 ++++++++++++++++++- >>> 1 file changed, 18 insertions(+), 1 deletion(-) >> >> Patch 1, 2, 4: >> >> Reviewed-by: Paolo Bonzini <pbonzini at redhat.com> > > How is 2 useful without 3?I can still say I reviewed it and Asias can include the tag in the next respin. But I screwed up, patch 2 is exactly the one that I said makes little sense and should be folded into patch 3. Paolo
Asias He
2013-Mar-13 02:54 UTC
[PATCH 1/4] tcm_vhost: Add missed lock in vhost_scsi_clear_endpoint()
On Tue, Mar 12, 2013 at 01:13:44PM +0200, Michael S. Tsirkin wrote:> On Tue, Mar 12, 2013 at 10:42:48AM +0800, Asias He wrote: > > tv_tpg->tv_tpg_vhost_count should be protected by tv_tpg->tv_tpg_mutex. > > > > Signed-off-by: Asias He <asias at redhat.com> > > Reviewed-by: Stefan Hajnoczi <stefanha at redhat.com> > > --- > > drivers/vhost/tcm_vhost.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c > > index 9951297..b3e50d7 100644 > > --- a/drivers/vhost/tcm_vhost.c > > +++ b/drivers/vhost/tcm_vhost.c > > @@ -860,9 +860,11 @@ static int vhost_scsi_clear_endpoint( > > if (!tv_tpg) > > continue; > > > > + mutex_lock(&tv_tpg->tv_tpg_mutex); > > tv_tport = tv_tpg->tport; > > if (!tv_tport) { > > ret = -ENODEV; > > + mutex_unlock(&tv_tpg->tv_tpg_mutex); > > goto err; > > } > > > > @@ -872,11 +874,13 @@ static int vhost_scsi_clear_endpoint( > > tv_tport->tport_name, tv_tpg->tport_tpgt, > > t->vhost_wwpn, t->vhost_tpgt); > > ret = -EINVAL; > > + mutex_unlock(&tv_tpg->tv_tpg_mutex); > > goto err; > > } > > tv_tpg->tv_tpg_vhost_count--; > > vs->vs_tpg[target] = NULL; > > vs->vs_endpoint = false; > > + mutex_unlock(&tv_tpg->tv_tpg_mutex); > > } > > mutex_unlock(&vs->dev.mutex); > > return 0; > > Does the error handling have to be so messy? > How about another label which does unlock and goto there?Well, I will do it.> > -- > > 1.8.1.4-- Asias
Asias He
2013-Mar-13 03:02 UTC
[PATCH 2/4] tcm_vhost: Introduce tcm_vhost_check_endpoint()
On Tue, Mar 12, 2013 at 09:26:18AM +0100, Paolo Bonzini wrote:> Il 12/03/2013 03:42, Asias He ha scritto: > > This helper is useful to check if vs->vs_endpoint is setup by > > vhost_scsi_set_endpoint() > > > > Signed-off-by: Asias He <asias at redhat.com> > > Reviewed-by: Stefan Hajnoczi <stefanha at redhat.com> > > --- > > drivers/vhost/tcm_vhost.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c > > index b3e50d7..29612bc 100644 > > --- a/drivers/vhost/tcm_vhost.c > > +++ b/drivers/vhost/tcm_vhost.c > > @@ -91,6 +91,18 @@ 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_scsi *vs) > > +{ > > + bool ret = false; > > + > > + mutex_lock(&vs->dev.mutex); > > + if (vs->vs_endpoint) > > + ret = true; > > + mutex_unlock(&vs->dev.mutex); > > The return value is invalid as soon as mutex_unlock is called, i.e. > before tcm_vhost_check_endpoint returns. Instead, check vs->vs_endpoint > in the caller while the mutex is taken.Do you mean 1) or 2)? 1) vhost_scsi_handle_vq() { mutex_lock(&vs->dev.mutex); check vs->vs_endpoint mutex_unlock(&vs->dev.mutex); handle vq } 2) vhost_scsi_handle_vq() { lock vs->dev.mutex check vs->vs_endpoint handle vq unlock vs->dev.mutex } 1) does not make any difference with the original one right? 2) would be too heavy. This might not be a problem in current 1 thread per vhost model. But if we want concurrent multiqueue, this will be killing us. Anyway, the current one is not good. Need to think.> Paolo > > > + return ret; > > +} > > + > > static int tcm_vhost_check_true(struct se_portal_group *se_tpg) > > { > > return 1; > > >-- Asias
On Tue, Mar 12, 2013 at 12:14:56PM +0100, Paolo Bonzini wrote:> Il 12/03/2013 12:12, Michael S. Tsirkin ha scritto: > > On Tue, Mar 12, 2013 at 09:26:42AM +0100, Paolo Bonzini wrote: > >> Il 12/03/2013 03:42, Asias He ha scritto: > >>> Asias He (4): > >>> tcm_vhost: Add missed lock in vhost_scsi_clear_endpoint() > >>> tcm_vhost: Introduce tcm_vhost_check_endpoint() > >>> tcm_vhost: Fix vs->vs_endpoint checking in vhost_scsi_handle_vq() > >>> tcm_vhost: Flush vhost_work in vhost_scsi_flush() > >>> > >>> drivers/vhost/tcm_vhost.c | 19 ++++++++++++++++++- > >>> 1 file changed, 18 insertions(+), 1 deletion(-) > >> > >> Patch 1, 2, 4: > >> > >> Reviewed-by: Paolo Bonzini <pbonzini at redhat.com> > > > > How is 2 useful without 3? > > I can still say I reviewed it and Asias can include the tag in the next > respin. But I screwed up, patch 2 is exactly the one that I said makes > little sense and should be folded into patch 3.Thanks, forgot to include your Reviewed-by tag in v2. Including it. And 2 and 3 are folded. -- Asias