* [RFC PATCH 0/6] vhost-user: Shutdown/Flush slave channel properly
@ 2021-01-25 18:01 ` Vivek Goyal
0 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-25 18:01 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: marcandre.lureau, stefanha, dgilbert, vgoyal, mst
Hi,
We are working on DAX support in virtiofs and have some patches out of
the tree hosted here.
https://gitlab.com/virtio-fs/qemu/-/commits/virtio-fs-dev
These patches have not been proposed for merge yet, becasue David
Gilbert noticed that we can run into a deadlock during an emergency
reboot of guest kernel. (echo b > /proc/sysrq-trigger).
I have provided details of deadlock in 4th path of the series with
subject "qemu, vhost-user: Extend protocol to start/stop/flush slave
channel".
Basic problem seems to be that we don't have a proper mechanism to
shutdown slave channel when vhost-user device is stopping. This means
there might be pending messages in slave channel and slave is blocked
and waiting for response.
This is an RFC patch series to enhance vhost-user protocol to
properly shutdown/flush slave channel and avoid the deadlock. Though
we faced the issue in the context of virtiofs, any vhost-user
device using slave channel can potentially run into issues and
can benefit from these patches.
Any feedback is welcome. Currently patches are based on out of
tree code but after I get some feedback, I can only take pieces
which are relevant to upstream and post separately.
Thanks
Vivek
Vivek Goyal (6):
virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
libvhost-user: Use slave_mutex in all slave messages
vhost-user: Return error code from slave_read()
qemu, vhost-user: Extend protocol to start/stop/flush slave channel
libvhost-user: Add support to start/stop/flush slave channel
virtiofsd: Opt in for slave start/stop/shutdown functionality
hw/virtio/vhost-user.c | 151 +++++++++++++++++++++-
subprojects/libvhost-user/libvhost-user.c | 147 +++++++++++++++++----
subprojects/libvhost-user/libvhost-user.h | 8 +-
tools/virtiofsd/fuse_virtio.c | 20 +++
4 files changed, 294 insertions(+), 32 deletions(-)
--
2.25.4
^ permalink raw reply [flat|nested] 52+ messages in thread
* [Virtio-fs] [RFC PATCH 0/6] vhost-user: Shutdown/Flush slave channel properly
@ 2021-01-25 18:01 ` Vivek Goyal
0 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-25 18:01 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: marcandre.lureau, vgoyal, mst
Hi,
We are working on DAX support in virtiofs and have some patches out of
the tree hosted here.
https://gitlab.com/virtio-fs/qemu/-/commits/virtio-fs-dev
These patches have not been proposed for merge yet, becasue David
Gilbert noticed that we can run into a deadlock during an emergency
reboot of guest kernel. (echo b > /proc/sysrq-trigger).
I have provided details of deadlock in 4th path of the series with
subject "qemu, vhost-user: Extend protocol to start/stop/flush slave
channel".
Basic problem seems to be that we don't have a proper mechanism to
shutdown slave channel when vhost-user device is stopping. This means
there might be pending messages in slave channel and slave is blocked
and waiting for response.
This is an RFC patch series to enhance vhost-user protocol to
properly shutdown/flush slave channel and avoid the deadlock. Though
we faced the issue in the context of virtiofs, any vhost-user
device using slave channel can potentially run into issues and
can benefit from these patches.
Any feedback is welcome. Currently patches are based on out of
tree code but after I get some feedback, I can only take pieces
which are relevant to upstream and post separately.
Thanks
Vivek
Vivek Goyal (6):
virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
libvhost-user: Use slave_mutex in all slave messages
vhost-user: Return error code from slave_read()
qemu, vhost-user: Extend protocol to start/stop/flush slave channel
libvhost-user: Add support to start/stop/flush slave channel
virtiofsd: Opt in for slave start/stop/shutdown functionality
hw/virtio/vhost-user.c | 151 +++++++++++++++++++++-
subprojects/libvhost-user/libvhost-user.c | 147 +++++++++++++++++----
subprojects/libvhost-user/libvhost-user.h | 8 +-
tools/virtiofsd/fuse_virtio.c | 20 +++
4 files changed, 294 insertions(+), 32 deletions(-)
--
2.25.4
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 1/6] virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
2021-01-25 18:01 ` [Virtio-fs] " Vivek Goyal
@ 2021-01-25 18:01 ` Vivek Goyal
-1 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-25 18:01 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: marcandre.lureau, stefanha, dgilbert, vgoyal, mst
When we are shutting down virtqueues, virtio_loop() receives a message
VHOST_USER_GET_VRING_BASE from master. We acquire ->vu_dispatch_rwlock
and get into the process of shutting down virtqueue. In one of the
final steps, we are waiting for fv_queue_thread() to exit/finish and
wait with ->vu_dispatch_rwlock held.
But it is possible that fv_queue_thread() itself is waiting to get
->vu_dispatch_rwlock (With --thread-pool=0 option). If requests
are being processed by fv_queue_worker(), then fv_queue_worker()
can wait for the ->vu_dispatch_rwlock, and fv_queue_thread() will
wait for fv_queue_worker() before thread pool can be stopped.
IOW, if guest is shutdown uncleanly (some sort of emergency reboot),
it is possible that virtiofsd is processing a fs request and
qemu initiates device shutdown sequence. In that case there seem
to be two options. Either abort the existing request completely or
let existing request finish.
This patch is taking second approach. That is drop the ->vu_dispatch_rwlock
temporarily so that fv_queue_thread() can finish and deadlock does not
happen.
->vu_dispatch_rwlock provides mutual exclusion between virtio_loop()
(handling vhost-user protocol messages) and fv_queue_thread() (handling
fuse filesystem requests). Rational seems to be that protocol message
might change queue memory mappings, so we don't want both to proceed
at the same time.
In this case queue is shutting down, so I hope it is fine for fv_queue_thread() to send response back while virtio_loop() is still waiting (and not handling
any further vho-user protocol messages).
IOW, assumption here is that while virto_loop is blocked processing
VHOST_USER_GET_VRING_BASE message, it is still ok to send back the
response on vq by fv_queue_thread().
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
tools/virtiofsd/fuse_virtio.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 9577eaa68d..6805d8ba01 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -813,11 +813,20 @@ static void fv_queue_cleanup_thread(struct fv_VuDev *vud, int qidx)
fuse_log(FUSE_LOG_ERR, "Eventfd_write for queue %d: %s\n",
qidx, strerror(errno));
}
+
+ /*
+ * Drop ->vu_dispath_rwlock and reacquire. We are about to wait for
+ * for fv_queue_thread() and that might require ->vu_dispatch_rwlock
+ * to finish.
+ */
+ pthread_rwlock_unlock(&vud->vu_dispatch_rwlock);
ret = pthread_join(ourqi->thread, NULL);
if (ret) {
fuse_log(FUSE_LOG_ERR, "%s: Failed to join thread idx %d err %d\n",
__func__, qidx, ret);
}
+ pthread_rwlock_wrlock(&vud->vu_dispatch_rwlock);
+
pthread_mutex_destroy(&ourqi->vq_lock);
close(ourqi->kill_fd);
ourqi->kick_fd = -1;
@@ -952,7 +961,11 @@ int virtio_loop(struct fuse_session *se)
/*
* Make sure all fv_queue_thread()s quit on exit, as we're about to
* free virtio dev and fuse session, no one should access them anymore.
+ * Hold ->vu_dispatch_rwlock in write mode as fv_queue_cleanup_thread()
+ * assumes mutex is locked and unlocks/re-locks it.
*/
+
+ pthread_rwlock_wrlock(&se->virtio_dev->vu_dispatch_rwlock);
for (int i = 0; i < se->virtio_dev->nqueues; i++) {
if (!se->virtio_dev->qi[i]) {
continue;
@@ -961,6 +974,7 @@ int virtio_loop(struct fuse_session *se)
fuse_log(FUSE_LOG_INFO, "%s: Stopping queue %d thread\n", __func__, i);
fv_queue_cleanup_thread(se->virtio_dev, i);
}
+ pthread_rwlock_unlock(&se->virtio_dev->vu_dispatch_rwlock);
fuse_log(FUSE_LOG_INFO, "%s: Exit\n", __func__);
--
2.25.4
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Virtio-fs] [PATCH 1/6] virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
@ 2021-01-25 18:01 ` Vivek Goyal
0 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-25 18:01 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: marcandre.lureau, vgoyal, mst
When we are shutting down virtqueues, virtio_loop() receives a message
VHOST_USER_GET_VRING_BASE from master. We acquire ->vu_dispatch_rwlock
and get into the process of shutting down virtqueue. In one of the
final steps, we are waiting for fv_queue_thread() to exit/finish and
wait with ->vu_dispatch_rwlock held.
But it is possible that fv_queue_thread() itself is waiting to get
->vu_dispatch_rwlock (With --thread-pool=0 option). If requests
are being processed by fv_queue_worker(), then fv_queue_worker()
can wait for the ->vu_dispatch_rwlock, and fv_queue_thread() will
wait for fv_queue_worker() before thread pool can be stopped.
IOW, if guest is shutdown uncleanly (some sort of emergency reboot),
it is possible that virtiofsd is processing a fs request and
qemu initiates device shutdown sequence. In that case there seem
to be two options. Either abort the existing request completely or
let existing request finish.
This patch is taking second approach. That is drop the ->vu_dispatch_rwlock
temporarily so that fv_queue_thread() can finish and deadlock does not
happen.
->vu_dispatch_rwlock provides mutual exclusion between virtio_loop()
(handling vhost-user protocol messages) and fv_queue_thread() (handling
fuse filesystem requests). Rational seems to be that protocol message
might change queue memory mappings, so we don't want both to proceed
at the same time.
In this case queue is shutting down, so I hope it is fine for fv_queue_thread() to send response back while virtio_loop() is still waiting (and not handling
any further vho-user protocol messages).
IOW, assumption here is that while virto_loop is blocked processing
VHOST_USER_GET_VRING_BASE message, it is still ok to send back the
response on vq by fv_queue_thread().
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
tools/virtiofsd/fuse_virtio.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 9577eaa68d..6805d8ba01 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -813,11 +813,20 @@ static void fv_queue_cleanup_thread(struct fv_VuDev *vud, int qidx)
fuse_log(FUSE_LOG_ERR, "Eventfd_write for queue %d: %s\n",
qidx, strerror(errno));
}
+
+ /*
+ * Drop ->vu_dispath_rwlock and reacquire. We are about to wait for
+ * for fv_queue_thread() and that might require ->vu_dispatch_rwlock
+ * to finish.
+ */
+ pthread_rwlock_unlock(&vud->vu_dispatch_rwlock);
ret = pthread_join(ourqi->thread, NULL);
if (ret) {
fuse_log(FUSE_LOG_ERR, "%s: Failed to join thread idx %d err %d\n",
__func__, qidx, ret);
}
+ pthread_rwlock_wrlock(&vud->vu_dispatch_rwlock);
+
pthread_mutex_destroy(&ourqi->vq_lock);
close(ourqi->kill_fd);
ourqi->kick_fd = -1;
@@ -952,7 +961,11 @@ int virtio_loop(struct fuse_session *se)
/*
* Make sure all fv_queue_thread()s quit on exit, as we're about to
* free virtio dev and fuse session, no one should access them anymore.
+ * Hold ->vu_dispatch_rwlock in write mode as fv_queue_cleanup_thread()
+ * assumes mutex is locked and unlocks/re-locks it.
*/
+
+ pthread_rwlock_wrlock(&se->virtio_dev->vu_dispatch_rwlock);
for (int i = 0; i < se->virtio_dev->nqueues; i++) {
if (!se->virtio_dev->qi[i]) {
continue;
@@ -961,6 +974,7 @@ int virtio_loop(struct fuse_session *se)
fuse_log(FUSE_LOG_INFO, "%s: Stopping queue %d thread\n", __func__, i);
fv_queue_cleanup_thread(se->virtio_dev, i);
}
+ pthread_rwlock_unlock(&se->virtio_dev->vu_dispatch_rwlock);
fuse_log(FUSE_LOG_INFO, "%s: Exit\n", __func__);
--
2.25.4
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH 2/6] libvhost-user: Use slave_mutex in all slave messages
2021-01-25 18:01 ` [Virtio-fs] " Vivek Goyal
@ 2021-01-25 18:01 ` Vivek Goyal
-1 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-25 18:01 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: marcandre.lureau, stefanha, dgilbert, vgoyal, mst
dev->slave_mutex needs to be taken when sending messages on slave_fd.
Currently _vu_queue_notify() does not do that.
Introduce a helper vu_message_slave_send_receive() which sends as well
as receive response. Use this helper in all the paths which send
message on slave_fd channel.
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
subprojects/libvhost-user/libvhost-user.c | 50 ++++++++++++-----------
1 file changed, 27 insertions(+), 23 deletions(-)
diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 4cf4aef63d..7a56c56dc8 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -403,7 +403,7 @@ vu_send_reply(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
* Processes a reply on the slave channel.
* Entered with slave_mutex held and releases it before exit.
* Returns true on success.
- * *payload is written on success
+ * *payload is written on success, if payload is not NULL.
*/
static bool
vu_process_message_reply(VuDev *dev, const VhostUserMsg *vmsg,
@@ -427,7 +427,9 @@ vu_process_message_reply(VuDev *dev, const VhostUserMsg *vmsg,
goto out;
}
- *payload = msg_reply.payload.u64;
+ if (payload) {
+ *payload = msg_reply.payload.u64;
+ }
result = true;
out:
@@ -435,6 +437,25 @@ out:
return result;
}
+/* Returns true on success, false otherwise */
+static bool
+vu_message_slave_send_receive(VuDev *dev, VhostUserMsg *vmsg, uint64_t *payload)
+{
+ pthread_mutex_lock(&dev->slave_mutex);
+ if (!vu_message_write(dev, dev->slave_fd, vmsg)) {
+ pthread_mutex_unlock(&dev->slave_mutex);
+ return false;
+ }
+
+ if ((vmsg->flags & VHOST_USER_NEED_REPLY_MASK) == 0) {
+ pthread_mutex_unlock(&dev->slave_mutex);
+ return true;
+ }
+
+ /* Also unlocks the slave_mutex */
+ return vu_process_message_reply(dev, vmsg, payload);
+}
+
/* Kick the log_call_fd if required. */
static void
vu_log_kick(VuDev *dev)
@@ -1340,16 +1361,8 @@ bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq *vq, int fd,
return false;
}
- pthread_mutex_lock(&dev->slave_mutex);
- if (!vu_message_write(dev, dev->slave_fd, &vmsg)) {
- pthread_mutex_unlock(&dev->slave_mutex);
- return false;
- }
-
- /* Also unlocks the slave_mutex */
- res = vu_process_message_reply(dev, &vmsg, &payload);
+ res = vu_message_slave_send_receive(dev, &vmsg, &payload);
res = res && (payload == 0);
-
return res;
}
@@ -2395,10 +2408,7 @@ static void _vu_queue_notify(VuDev *dev, VuVirtq *vq, bool sync)
vmsg.flags |= VHOST_USER_NEED_REPLY_MASK;
}
- vu_message_write(dev, dev->slave_fd, &vmsg);
- if (ack) {
- vu_message_read_default(dev, dev->slave_fd, &vmsg);
- }
+ vu_message_slave_send_receive(dev, &vmsg, NULL);
return;
}
@@ -2942,17 +2952,11 @@ int64_t vu_fs_cache_request(VuDev *dev, VhostUserSlaveRequest req, int fd,
return -EINVAL;
}
- pthread_mutex_lock(&dev->slave_mutex);
- if (!vu_message_write(dev, dev->slave_fd, &vmsg)) {
- pthread_mutex_unlock(&dev->slave_mutex);
- return -EIO;
- }
-
- /* Also unlocks the slave_mutex */
- res = vu_process_message_reply(dev, &vmsg, &payload);
+ res = vu_message_slave_send_receive(dev, &vmsg, &payload);
if (!res) {
return -EIO;
}
+
/*
* Payload is delivered as uint64_t but is actually signed for
* errors.
--
2.25.4
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Virtio-fs] [PATCH 2/6] libvhost-user: Use slave_mutex in all slave messages
@ 2021-01-25 18:01 ` Vivek Goyal
0 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-25 18:01 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: marcandre.lureau, vgoyal, mst
dev->slave_mutex needs to be taken when sending messages on slave_fd.
Currently _vu_queue_notify() does not do that.
Introduce a helper vu_message_slave_send_receive() which sends as well
as receive response. Use this helper in all the paths which send
message on slave_fd channel.
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
subprojects/libvhost-user/libvhost-user.c | 50 ++++++++++++-----------
1 file changed, 27 insertions(+), 23 deletions(-)
diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 4cf4aef63d..7a56c56dc8 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -403,7 +403,7 @@ vu_send_reply(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
* Processes a reply on the slave channel.
* Entered with slave_mutex held and releases it before exit.
* Returns true on success.
- * *payload is written on success
+ * *payload is written on success, if payload is not NULL.
*/
static bool
vu_process_message_reply(VuDev *dev, const VhostUserMsg *vmsg,
@@ -427,7 +427,9 @@ vu_process_message_reply(VuDev *dev, const VhostUserMsg *vmsg,
goto out;
}
- *payload = msg_reply.payload.u64;
+ if (payload) {
+ *payload = msg_reply.payload.u64;
+ }
result = true;
out:
@@ -435,6 +437,25 @@ out:
return result;
}
+/* Returns true on success, false otherwise */
+static bool
+vu_message_slave_send_receive(VuDev *dev, VhostUserMsg *vmsg, uint64_t *payload)
+{
+ pthread_mutex_lock(&dev->slave_mutex);
+ if (!vu_message_write(dev, dev->slave_fd, vmsg)) {
+ pthread_mutex_unlock(&dev->slave_mutex);
+ return false;
+ }
+
+ if ((vmsg->flags & VHOST_USER_NEED_REPLY_MASK) == 0) {
+ pthread_mutex_unlock(&dev->slave_mutex);
+ return true;
+ }
+
+ /* Also unlocks the slave_mutex */
+ return vu_process_message_reply(dev, vmsg, payload);
+}
+
/* Kick the log_call_fd if required. */
static void
vu_log_kick(VuDev *dev)
@@ -1340,16 +1361,8 @@ bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq *vq, int fd,
return false;
}
- pthread_mutex_lock(&dev->slave_mutex);
- if (!vu_message_write(dev, dev->slave_fd, &vmsg)) {
- pthread_mutex_unlock(&dev->slave_mutex);
- return false;
- }
-
- /* Also unlocks the slave_mutex */
- res = vu_process_message_reply(dev, &vmsg, &payload);
+ res = vu_message_slave_send_receive(dev, &vmsg, &payload);
res = res && (payload == 0);
-
return res;
}
@@ -2395,10 +2408,7 @@ static void _vu_queue_notify(VuDev *dev, VuVirtq *vq, bool sync)
vmsg.flags |= VHOST_USER_NEED_REPLY_MASK;
}
- vu_message_write(dev, dev->slave_fd, &vmsg);
- if (ack) {
- vu_message_read_default(dev, dev->slave_fd, &vmsg);
- }
+ vu_message_slave_send_receive(dev, &vmsg, NULL);
return;
}
@@ -2942,17 +2952,11 @@ int64_t vu_fs_cache_request(VuDev *dev, VhostUserSlaveRequest req, int fd,
return -EINVAL;
}
- pthread_mutex_lock(&dev->slave_mutex);
- if (!vu_message_write(dev, dev->slave_fd, &vmsg)) {
- pthread_mutex_unlock(&dev->slave_mutex);
- return -EIO;
- }
-
- /* Also unlocks the slave_mutex */
- res = vu_process_message_reply(dev, &vmsg, &payload);
+ res = vu_message_slave_send_receive(dev, &vmsg, &payload);
if (!res) {
return -EIO;
}
+
/*
* Payload is delivered as uint64_t but is actually signed for
* errors.
--
2.25.4
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH 3/6] vhost-user: Return error code from slave_read()
2021-01-25 18:01 ` [Virtio-fs] " Vivek Goyal
@ 2021-01-25 18:01 ` Vivek Goyal
-1 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-25 18:01 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: marcandre.lureau, stefanha, dgilbert, vgoyal, mst
Right now slave_read() is called through main event loop and does not
return error. In next few patches I want to call slave_read() from
vhost device shutdown path as well and want to know if an error
happened so that caller can give up and return error accordingly.
Hence, create helper function do_slave_read(), which returns an
integer. Success is 0 and negative number is error code. slave_read()
calls do_slave_read() and ignores error code.
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
hw/virtio/vhost-user.c | 43 ++++++++++++++++++++++++++++++++++++------
1 file changed, 37 insertions(+), 6 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index d95dbc39e3..867cac034f 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1401,7 +1401,7 @@ static uint64_t vhost_user_slave_handle_vring_host_notifier(
return false;
}
-static void slave_read(void *opaque)
+static int do_slave_read(void *opaque)
{
struct vhost_dev *dev = opaque;
struct vhost_user *u = dev->opaque;
@@ -1432,13 +1432,22 @@ static void slave_read(void *opaque)
size = recvmsg(u->slave_fd, &msgh, 0);
} while (size < 0 && (errno == EINTR || errno == EAGAIN));
- if (size != VHOST_USER_HDR_SIZE) {
+ if (size < 0) {
+ ret = -errno;
error_report("Failed to read from slave.");
goto err;
}
+ if (size != VHOST_USER_HDR_SIZE) {
+ error_report("Failed to read %lu bytes from slave.",
+ VHOST_USER_HDR_SIZE);
+ ret = -EBADMSG;
+ goto err;
+ }
+
if (msgh.msg_flags & MSG_CTRUNC) {
error_report("Truncated message.");
+ ret = -EBADMSG;
goto err;
}
@@ -1456,6 +1465,7 @@ static void slave_read(void *opaque)
error_report("Failed to read msg header."
" Size %d exceeds the maximum %zu.", hdr.size,
VHOST_USER_PAYLOAD_SIZE);
+ ret = -EBADMSG;
goto err;
}
@@ -1464,8 +1474,15 @@ static void slave_read(void *opaque)
size = read(u->slave_fd, &payload, hdr.size);
} while (size < 0 && (errno == EINTR || errno == EAGAIN));
- if (size != hdr.size) {
+ if (size == -1) {
error_report("Failed to read payload from slave.");
+ ret = errno;
+ goto err;
+ }
+
+ if (size != hdr.size) {
+ error_report("Failed to read %d payload bytes from slave.", hdr.size);
+ ret = -EBADMSG;
goto err;
}
@@ -1529,13 +1546,22 @@ static void slave_read(void *opaque)
size = writev(u->slave_fd, iovec, ARRAY_SIZE(iovec));
} while (size < 0 && (errno == EINTR || errno == EAGAIN));
- if (size != VHOST_USER_HDR_SIZE + hdr.size) {
+ if (size == -1) {
error_report("Failed to send msg reply to slave.");
+ ret = -errno;
+ goto err;
+ }
+
+ if (size != VHOST_USER_HDR_SIZE + hdr.size) {
+ error_report("Failed to send msg reply to slave. Wrote %d bytes"
+ " expected %lu bytes.", size,
+ VHOST_USER_HDR_SIZE + hdr.size);
+ ret = -EIO;
goto err;
}
}
- return;
+ return 0;
err:
qemu_set_fd_handler(u->slave_fd, NULL, NULL, NULL);
@@ -1546,7 +1572,12 @@ err:
close(fd[i]);
}
}
- return;
+ return ret;
+}
+
+static void slave_read(void *opaque)
+{
+ do_slave_read(opaque);
}
static int vhost_setup_slave_channel(struct vhost_dev *dev)
--
2.25.4
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Virtio-fs] [PATCH 3/6] vhost-user: Return error code from slave_read()
@ 2021-01-25 18:01 ` Vivek Goyal
0 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-25 18:01 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: marcandre.lureau, vgoyal, mst
Right now slave_read() is called through main event loop and does not
return error. In next few patches I want to call slave_read() from
vhost device shutdown path as well and want to know if an error
happened so that caller can give up and return error accordingly.
Hence, create helper function do_slave_read(), which returns an
integer. Success is 0 and negative number is error code. slave_read()
calls do_slave_read() and ignores error code.
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
hw/virtio/vhost-user.c | 43 ++++++++++++++++++++++++++++++++++++------
1 file changed, 37 insertions(+), 6 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index d95dbc39e3..867cac034f 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1401,7 +1401,7 @@ static uint64_t vhost_user_slave_handle_vring_host_notifier(
return false;
}
-static void slave_read(void *opaque)
+static int do_slave_read(void *opaque)
{
struct vhost_dev *dev = opaque;
struct vhost_user *u = dev->opaque;
@@ -1432,13 +1432,22 @@ static void slave_read(void *opaque)
size = recvmsg(u->slave_fd, &msgh, 0);
} while (size < 0 && (errno == EINTR || errno == EAGAIN));
- if (size != VHOST_USER_HDR_SIZE) {
+ if (size < 0) {
+ ret = -errno;
error_report("Failed to read from slave.");
goto err;
}
+ if (size != VHOST_USER_HDR_SIZE) {
+ error_report("Failed to read %lu bytes from slave.",
+ VHOST_USER_HDR_SIZE);
+ ret = -EBADMSG;
+ goto err;
+ }
+
if (msgh.msg_flags & MSG_CTRUNC) {
error_report("Truncated message.");
+ ret = -EBADMSG;
goto err;
}
@@ -1456,6 +1465,7 @@ static void slave_read(void *opaque)
error_report("Failed to read msg header."
" Size %d exceeds the maximum %zu.", hdr.size,
VHOST_USER_PAYLOAD_SIZE);
+ ret = -EBADMSG;
goto err;
}
@@ -1464,8 +1474,15 @@ static void slave_read(void *opaque)
size = read(u->slave_fd, &payload, hdr.size);
} while (size < 0 && (errno == EINTR || errno == EAGAIN));
- if (size != hdr.size) {
+ if (size == -1) {
error_report("Failed to read payload from slave.");
+ ret = errno;
+ goto err;
+ }
+
+ if (size != hdr.size) {
+ error_report("Failed to read %d payload bytes from slave.", hdr.size);
+ ret = -EBADMSG;
goto err;
}
@@ -1529,13 +1546,22 @@ static void slave_read(void *opaque)
size = writev(u->slave_fd, iovec, ARRAY_SIZE(iovec));
} while (size < 0 && (errno == EINTR || errno == EAGAIN));
- if (size != VHOST_USER_HDR_SIZE + hdr.size) {
+ if (size == -1) {
error_report("Failed to send msg reply to slave.");
+ ret = -errno;
+ goto err;
+ }
+
+ if (size != VHOST_USER_HDR_SIZE + hdr.size) {
+ error_report("Failed to send msg reply to slave. Wrote %d bytes"
+ " expected %lu bytes.", size,
+ VHOST_USER_HDR_SIZE + hdr.size);
+ ret = -EIO;
goto err;
}
}
- return;
+ return 0;
err:
qemu_set_fd_handler(u->slave_fd, NULL, NULL, NULL);
@@ -1546,7 +1572,12 @@ err:
close(fd[i]);
}
}
- return;
+ return ret;
+}
+
+static void slave_read(void *opaque)
+{
+ do_slave_read(opaque);
}
static int vhost_setup_slave_channel(struct vhost_dev *dev)
--
2.25.4
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH 4/6] qemu, vhost-user: Extend protocol to start/stop/flush slave channel
2021-01-25 18:01 ` [Virtio-fs] " Vivek Goyal
@ 2021-01-25 18:01 ` Vivek Goyal
-1 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-25 18:01 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: marcandre.lureau, stefanha, dgilbert, vgoyal, mst
Currently we don't have a mechanism to flush slave channel while shutting
down vhost-user device and that can result a deadlock. Consider following
scenario.
1. Slave gets a request from guest on virtqueue (say index 1, vq1), to map
a portion of file in qemu address space.
2. Thread serving vq1 receives this request and sends a message to qemu on
slave channel/fd and gets blocked in waiting for a response from qemu.
3. In the mean time, user does "echo b > /proc/sysrq-trigger" in guest. This
leads to qemu reset and ultimately in main thread we end up in
vhost_dev_stop() which is trying to shutdown virtqueues for the device.
4. Slave gets VHOST_USER_GET_VRING_BASE message to shutdown a virtqueue on
unix socket being used for communication.
5. Slave tries to shutdown the thread serving vq1 and waits for it to
terminate. But vq1 thread can't terminate because it is waiting for
a response from qemu on slave_fd. And qemu is not processing slave_fd
anymore as qemu is ressing (and not running main event loop anymore)
and is waiting for a response to VHOST_USER_GET_VRING_BASE.
So we have a deadlock. Qemu is waiting on slave to response to
VHOST_USER_GET_VRING_BASE message and slave is waiting on qemu to
respond to request it sent on slave_fd.
I can reliably reproduce this race with virtio-fs.
Hence here is the proposal to solve this problem. Enhance vhost-user
protocol to properly shutdown slave_fd channel. And if there are pending
requests, flush the channel completely before sending the request to
shutdown virtqueues.
New workflow to shutdown slave channel is.
- Qemu sends VHOST_USER_STOP_SLAVE_CHANNEL request to slave. It waits
for an reply from guest.
- Then qemu sits in a tight loop waiting for
VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE message from slave on slave_fd.
And while waiting for this message, qemu continues to process requests
on slave_fd to flush any pending requests. This will unblock threads
in slave and allow slave to shutdown slave channel.
- Once qemu gets VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE message, it knows
no more requests will come on slave_fd and it can continue to shutdown
virtqueues now.
- Once device starts again, qemu will send VHOST_USER_START_SLAVE_CHANNEL
message to slave to open the slave channel and receive requests.
IOW, this allows for proper shutdown of slave channel, making sure
no threads in slave are blocked on sending/receiving message. And
this in-turn allows for shutting down of virtqueues, hence resolving
the deadlock.
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
hw/virtio/vhost-user.c | 108 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 108 insertions(+)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 867cac034f..56be61d4e2 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -80,6 +80,7 @@ enum VhostUserProtocolFeature {
VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
/* Feature 14 reserved for VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS. */
VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
+ VHOST_USER_PROTOCOL_F_SLAVE_CH_START_STOP = 16,
VHOST_USER_PROTOCOL_F_MAX
};
@@ -125,6 +126,8 @@ typedef enum VhostUserRequest {
VHOST_USER_GET_MAX_MEM_SLOTS = 36,
VHOST_USER_ADD_MEM_REG = 37,
VHOST_USER_REM_MEM_REG = 38,
+ VHOST_USER_START_SLAVE_CHANNEL = 39,
+ VHOST_USER_STOP_SLAVE_CHANNEL = 40,
VHOST_USER_MAX
} VhostUserRequest;
@@ -139,6 +142,7 @@ typedef enum VhostUserSlaveRequest {
VHOST_USER_SLAVE_FS_UNMAP = 7,
VHOST_USER_SLAVE_FS_SYNC = 8,
VHOST_USER_SLAVE_FS_IO = 9,
+ VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE = 10,
VHOST_USER_SLAVE_MAX
} VhostUserSlaveRequest;
@@ -246,6 +250,7 @@ struct vhost_user {
/* Shared between vhost devs of the same virtio device */
VhostUserState *user;
int slave_fd;
+ bool slave_channel_open;
NotifierWithReturn postcopy_notifier;
struct PostCopyFD postcopy_fd;
uint64_t postcopy_client_bases[VHOST_USER_MAX_RAM_SLOTS];
@@ -1511,6 +1516,10 @@ static int do_slave_read(void *opaque)
ret = vhost_user_fs_slave_io(dev, &payload.fs, fd[0]);
break;
#endif
+ case VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE:
+ u->slave_channel_open = false;
+ ret = 0;
+ break;
default:
error_report("Received unexpected msg type: %d.", hdr.request);
ret = (uint64_t)-EINVAL;
@@ -1580,6 +1589,70 @@ static void slave_read(void *opaque)
do_slave_read(opaque);
}
+static int vhost_start_slave_channel(struct vhost_dev *dev)
+{
+ struct vhost_user *u = dev->opaque;
+ VhostUserMsg msg = {
+ .hdr.request = VHOST_USER_START_SLAVE_CHANNEL,
+ .hdr.flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK,
+ };
+ int ret = 0;
+
+ if (!virtio_has_feature(dev->protocol_features,
+ VHOST_USER_PROTOCOL_F_SLAVE_CH_START_STOP)) {
+ return 0;
+ }
+
+ ret = vhost_user_write(dev, &msg, NULL, 0);
+ if (ret) {
+ return ret;
+ }
+
+ ret = process_message_reply(dev, &msg);
+ if (ret)
+ return ret;
+
+ u->slave_channel_open = true;
+ return ret;
+}
+
+static int vhost_stop_slave_channel(struct vhost_dev *dev)
+{
+ struct vhost_user *u = dev->opaque;
+ VhostUserMsg msg = {
+ .hdr.request = VHOST_USER_STOP_SLAVE_CHANNEL,
+ .hdr.flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK,
+ };
+ int ret = 0;
+
+ if (!virtio_has_feature(dev->protocol_features,
+ VHOST_USER_PROTOCOL_F_SLAVE_CH_START_STOP)) {
+ return 0;
+ }
+
+ ret = vhost_user_write(dev, &msg, NULL, 0);
+ if (ret) {
+ return ret;
+ }
+
+ ret = process_message_reply(dev, &msg);
+ if (ret) {
+ return ret;
+ }
+
+ /*
+ * Wait for flush finish message from slave. And continue to process
+ * slave messages till we get flush finish.
+ */
+ while (u->slave_channel_open) {
+ ret = do_slave_read(dev);
+ if (ret)
+ break;
+ }
+
+ return ret;
+}
+
static int vhost_setup_slave_channel(struct vhost_dev *dev)
{
VhostUserMsg msg = {
@@ -1860,6 +1933,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
u = g_new0(struct vhost_user, 1);
u->user = opaque;
u->slave_fd = -1;
+ u->slave_channel_open = false;
u->dev = dev;
dev->opaque = u;
@@ -1934,6 +2008,17 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
u->user->memory_slots = MIN(ram_slots, VHOST_USER_MAX_RAM_SLOTS);
}
+
+ if (virtio_has_feature(dev->protocol_features,
+ VHOST_USER_PROTOCOL_F_SLAVE_CH_START_STOP) &&
+ !(virtio_has_feature(dev->protocol_features,
+ VHOST_USER_PROTOCOL_F_SLAVE_REQ) &&
+ virtio_has_feature(dev->protocol_features,
+ VHOST_USER_PROTOCOL_F_REPLY_ACK))) {
+ error_report("Slave channel start/stop support requires reply-ack"
+ " and slave-req protocol features.");
+ return -1;
+ }
}
if (dev->migration_blocker == NULL &&
@@ -1949,6 +2034,10 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
if (err < 0) {
return err;
}
+ err = vhost_start_slave_channel(dev);
+ if (err < 0) {
+ return err;
+ }
}
u->postcopy_notifier.notify = vhost_user_postcopy_notifier;
@@ -2408,6 +2497,24 @@ void vhost_user_cleanup(VhostUserState *user)
user->chr = NULL;
}
+static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
+{
+ int ret;
+
+ if (!started) {
+ ret = vhost_stop_slave_channel(dev);
+ if (ret < 0) {
+ return ret;
+ }
+ } else {
+ ret = vhost_start_slave_channel(dev);
+ if (ret < 0) {
+ return ret;
+ }
+ }
+ return 0;
+}
+
const VhostOps user_ops = {
.backend_type = VHOST_BACKEND_TYPE_USER,
.vhost_backend_init = vhost_user_backend_init,
@@ -2434,6 +2541,7 @@ const VhostOps user_ops = {
.vhost_net_set_mtu = vhost_user_net_set_mtu,
.vhost_set_iotlb_callback = vhost_user_set_iotlb_callback,
.vhost_send_device_iotlb_msg = vhost_user_send_device_iotlb_msg,
+ .vhost_dev_start = vhost_user_dev_start,
.vhost_get_config = vhost_user_get_config,
.vhost_set_config = vhost_user_set_config,
.vhost_crypto_create_session = vhost_user_crypto_create_session,
--
2.25.4
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Virtio-fs] [PATCH 4/6] qemu, vhost-user: Extend protocol to start/stop/flush slave channel
@ 2021-01-25 18:01 ` Vivek Goyal
0 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-25 18:01 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: marcandre.lureau, vgoyal, mst
Currently we don't have a mechanism to flush slave channel while shutting
down vhost-user device and that can result a deadlock. Consider following
scenario.
1. Slave gets a request from guest on virtqueue (say index 1, vq1), to map
a portion of file in qemu address space.
2. Thread serving vq1 receives this request and sends a message to qemu on
slave channel/fd and gets blocked in waiting for a response from qemu.
3. In the mean time, user does "echo b > /proc/sysrq-trigger" in guest. This
leads to qemu reset and ultimately in main thread we end up in
vhost_dev_stop() which is trying to shutdown virtqueues for the device.
4. Slave gets VHOST_USER_GET_VRING_BASE message to shutdown a virtqueue on
unix socket being used for communication.
5. Slave tries to shutdown the thread serving vq1 and waits for it to
terminate. But vq1 thread can't terminate because it is waiting for
a response from qemu on slave_fd. And qemu is not processing slave_fd
anymore as qemu is ressing (and not running main event loop anymore)
and is waiting for a response to VHOST_USER_GET_VRING_BASE.
So we have a deadlock. Qemu is waiting on slave to response to
VHOST_USER_GET_VRING_BASE message and slave is waiting on qemu to
respond to request it sent on slave_fd.
I can reliably reproduce this race with virtio-fs.
Hence here is the proposal to solve this problem. Enhance vhost-user
protocol to properly shutdown slave_fd channel. And if there are pending
requests, flush the channel completely before sending the request to
shutdown virtqueues.
New workflow to shutdown slave channel is.
- Qemu sends VHOST_USER_STOP_SLAVE_CHANNEL request to slave. It waits
for an reply from guest.
- Then qemu sits in a tight loop waiting for
VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE message from slave on slave_fd.
And while waiting for this message, qemu continues to process requests
on slave_fd to flush any pending requests. This will unblock threads
in slave and allow slave to shutdown slave channel.
- Once qemu gets VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE message, it knows
no more requests will come on slave_fd and it can continue to shutdown
virtqueues now.
- Once device starts again, qemu will send VHOST_USER_START_SLAVE_CHANNEL
message to slave to open the slave channel and receive requests.
IOW, this allows for proper shutdown of slave channel, making sure
no threads in slave are blocked on sending/receiving message. And
this in-turn allows for shutting down of virtqueues, hence resolving
the deadlock.
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
hw/virtio/vhost-user.c | 108 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 108 insertions(+)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 867cac034f..56be61d4e2 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -80,6 +80,7 @@ enum VhostUserProtocolFeature {
VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
/* Feature 14 reserved for VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS. */
VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
+ VHOST_USER_PROTOCOL_F_SLAVE_CH_START_STOP = 16,
VHOST_USER_PROTOCOL_F_MAX
};
@@ -125,6 +126,8 @@ typedef enum VhostUserRequest {
VHOST_USER_GET_MAX_MEM_SLOTS = 36,
VHOST_USER_ADD_MEM_REG = 37,
VHOST_USER_REM_MEM_REG = 38,
+ VHOST_USER_START_SLAVE_CHANNEL = 39,
+ VHOST_USER_STOP_SLAVE_CHANNEL = 40,
VHOST_USER_MAX
} VhostUserRequest;
@@ -139,6 +142,7 @@ typedef enum VhostUserSlaveRequest {
VHOST_USER_SLAVE_FS_UNMAP = 7,
VHOST_USER_SLAVE_FS_SYNC = 8,
VHOST_USER_SLAVE_FS_IO = 9,
+ VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE = 10,
VHOST_USER_SLAVE_MAX
} VhostUserSlaveRequest;
@@ -246,6 +250,7 @@ struct vhost_user {
/* Shared between vhost devs of the same virtio device */
VhostUserState *user;
int slave_fd;
+ bool slave_channel_open;
NotifierWithReturn postcopy_notifier;
struct PostCopyFD postcopy_fd;
uint64_t postcopy_client_bases[VHOST_USER_MAX_RAM_SLOTS];
@@ -1511,6 +1516,10 @@ static int do_slave_read(void *opaque)
ret = vhost_user_fs_slave_io(dev, &payload.fs, fd[0]);
break;
#endif
+ case VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE:
+ u->slave_channel_open = false;
+ ret = 0;
+ break;
default:
error_report("Received unexpected msg type: %d.", hdr.request);
ret = (uint64_t)-EINVAL;
@@ -1580,6 +1589,70 @@ static void slave_read(void *opaque)
do_slave_read(opaque);
}
+static int vhost_start_slave_channel(struct vhost_dev *dev)
+{
+ struct vhost_user *u = dev->opaque;
+ VhostUserMsg msg = {
+ .hdr.request = VHOST_USER_START_SLAVE_CHANNEL,
+ .hdr.flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK,
+ };
+ int ret = 0;
+
+ if (!virtio_has_feature(dev->protocol_features,
+ VHOST_USER_PROTOCOL_F_SLAVE_CH_START_STOP)) {
+ return 0;
+ }
+
+ ret = vhost_user_write(dev, &msg, NULL, 0);
+ if (ret) {
+ return ret;
+ }
+
+ ret = process_message_reply(dev, &msg);
+ if (ret)
+ return ret;
+
+ u->slave_channel_open = true;
+ return ret;
+}
+
+static int vhost_stop_slave_channel(struct vhost_dev *dev)
+{
+ struct vhost_user *u = dev->opaque;
+ VhostUserMsg msg = {
+ .hdr.request = VHOST_USER_STOP_SLAVE_CHANNEL,
+ .hdr.flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK,
+ };
+ int ret = 0;
+
+ if (!virtio_has_feature(dev->protocol_features,
+ VHOST_USER_PROTOCOL_F_SLAVE_CH_START_STOP)) {
+ return 0;
+ }
+
+ ret = vhost_user_write(dev, &msg, NULL, 0);
+ if (ret) {
+ return ret;
+ }
+
+ ret = process_message_reply(dev, &msg);
+ if (ret) {
+ return ret;
+ }
+
+ /*
+ * Wait for flush finish message from slave. And continue to process
+ * slave messages till we get flush finish.
+ */
+ while (u->slave_channel_open) {
+ ret = do_slave_read(dev);
+ if (ret)
+ break;
+ }
+
+ return ret;
+}
+
static int vhost_setup_slave_channel(struct vhost_dev *dev)
{
VhostUserMsg msg = {
@@ -1860,6 +1933,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
u = g_new0(struct vhost_user, 1);
u->user = opaque;
u->slave_fd = -1;
+ u->slave_channel_open = false;
u->dev = dev;
dev->opaque = u;
@@ -1934,6 +2008,17 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
u->user->memory_slots = MIN(ram_slots, VHOST_USER_MAX_RAM_SLOTS);
}
+
+ if (virtio_has_feature(dev->protocol_features,
+ VHOST_USER_PROTOCOL_F_SLAVE_CH_START_STOP) &&
+ !(virtio_has_feature(dev->protocol_features,
+ VHOST_USER_PROTOCOL_F_SLAVE_REQ) &&
+ virtio_has_feature(dev->protocol_features,
+ VHOST_USER_PROTOCOL_F_REPLY_ACK))) {
+ error_report("Slave channel start/stop support requires reply-ack"
+ " and slave-req protocol features.");
+ return -1;
+ }
}
if (dev->migration_blocker == NULL &&
@@ -1949,6 +2034,10 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
if (err < 0) {
return err;
}
+ err = vhost_start_slave_channel(dev);
+ if (err < 0) {
+ return err;
+ }
}
u->postcopy_notifier.notify = vhost_user_postcopy_notifier;
@@ -2408,6 +2497,24 @@ void vhost_user_cleanup(VhostUserState *user)
user->chr = NULL;
}
+static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
+{
+ int ret;
+
+ if (!started) {
+ ret = vhost_stop_slave_channel(dev);
+ if (ret < 0) {
+ return ret;
+ }
+ } else {
+ ret = vhost_start_slave_channel(dev);
+ if (ret < 0) {
+ return ret;
+ }
+ }
+ return 0;
+}
+
const VhostOps user_ops = {
.backend_type = VHOST_BACKEND_TYPE_USER,
.vhost_backend_init = vhost_user_backend_init,
@@ -2434,6 +2541,7 @@ const VhostOps user_ops = {
.vhost_net_set_mtu = vhost_user_net_set_mtu,
.vhost_set_iotlb_callback = vhost_user_set_iotlb_callback,
.vhost_send_device_iotlb_msg = vhost_user_send_device_iotlb_msg,
+ .vhost_dev_start = vhost_user_dev_start,
.vhost_get_config = vhost_user_get_config,
.vhost_set_config = vhost_user_set_config,
.vhost_crypto_create_session = vhost_user_crypto_create_session,
--
2.25.4
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH 5/6] libvhost-user: Add support to start/stop/flush slave channel
2021-01-25 18:01 ` [Virtio-fs] " Vivek Goyal
@ 2021-01-25 18:01 ` Vivek Goyal
-1 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-25 18:01 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: marcandre.lureau, stefanha, dgilbert, vgoyal, mst
This patch adds support to start/stop/flush slave channel functionality.
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
subprojects/libvhost-user/libvhost-user.c | 103 ++++++++++++++++++++--
subprojects/libvhost-user/libvhost-user.h | 8 +-
2 files changed, 105 insertions(+), 6 deletions(-)
diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 7a56c56dc8..b4c795c63e 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -140,6 +140,8 @@ vu_request_to_string(unsigned int req)
REQ(VHOST_USER_GET_MAX_MEM_SLOTS),
REQ(VHOST_USER_ADD_MEM_REG),
REQ(VHOST_USER_REM_MEM_REG),
+ REQ(VHOST_USER_START_SLAVE_CHANNEL),
+ REQ(VHOST_USER_STOP_SLAVE_CHANNEL),
REQ(VHOST_USER_MAX),
};
#undef REQ
@@ -437,11 +439,11 @@ out:
return result;
}
-/* Returns true on success, false otherwise */
+/* slave mutex should be held. Will be unlocked upon return */
static bool
-vu_message_slave_send_receive(VuDev *dev, VhostUserMsg *vmsg, uint64_t *payload)
+vu_message_slave_send_receive_locked(VuDev *dev, VhostUserMsg *vmsg,
+ uint64_t *payload)
{
- pthread_mutex_lock(&dev->slave_mutex);
if (!vu_message_write(dev, dev->slave_fd, vmsg)) {
pthread_mutex_unlock(&dev->slave_mutex);
return false;
@@ -456,6 +458,46 @@ vu_message_slave_send_receive(VuDev *dev, VhostUserMsg *vmsg, uint64_t *payload)
return vu_process_message_reply(dev, vmsg, payload);
}
+/* Returns true on success, false otherwise */
+static bool
+vu_message_slave_send_receive(VuDev *dev, VhostUserMsg *vmsg,
+ uint64_t *payload)
+{
+ pthread_mutex_lock(&dev->slave_mutex);
+ if (!dev->slave_channel_open) {
+ pthread_mutex_unlock(&dev->slave_mutex);
+ return false;
+ }
+ return vu_message_slave_send_receive_locked(dev, vmsg, payload);
+}
+
+static bool
+vu_finish_stop_slave(VuDev *dev)
+{
+ bool res;
+ uint64_t payload = 0;
+ VhostUserMsg vmsg = {
+ .request = VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE,
+ .flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK,
+ .size = sizeof(vmsg.payload.u64),
+ .payload.u64 = 0,
+ };
+
+ /*
+ * Once we get slave_mutex, this should make sure no other caller is
+ * currently in the process of sending or receiving message on slave_fd.
+ * And setting slave_channel_open to false now will make sure any new
+ * callers will not send message and instead get error back. So it
+ * is now safe to send stop finished message to master.
+ */
+ pthread_mutex_lock(&dev->slave_mutex);
+ dev->slave_channel_open = false;
+ /* This also drops slave_mutex */
+ res = vu_message_slave_send_receive_locked(dev, &vmsg, &payload);
+ res = res && (payload == 0);
+ return res;
+}
+
/* Kick the log_call_fd if required. */
static void
vu_log_kick(VuDev *dev)
@@ -1529,6 +1571,35 @@ vu_set_slave_req_fd(VuDev *dev, VhostUserMsg *vmsg)
return false;
}
+static bool
+vu_slave_channel_start(VuDev *dev, VhostUserMsg *vmsg)
+{
+ pthread_mutex_lock(&dev->slave_mutex);
+ dev->slave_channel_open = true;
+ pthread_mutex_unlock(&dev->slave_mutex);
+ /* Caller (vu_dispatch()) will send a reply */
+ return false;
+}
+
+static bool
+vu_slave_channel_stop(VuDev *dev, VhostUserMsg *vmsg, bool *reply_sent,
+ bool *reply_status)
+{
+ vmsg_set_reply_u64(vmsg, 0);
+ *reply_sent = true;
+ *reply_status = false;
+ if (!vu_send_reply(dev, dev->sock, vmsg)) {
+ return false;
+ }
+
+ if (!vu_finish_stop_slave(dev)) {
+ return false;
+ }
+
+ *reply_status = true;
+ return false;
+}
+
static bool
vu_get_config(VuDev *dev, VhostUserMsg *vmsg)
{
@@ -1823,7 +1894,8 @@ static bool vu_handle_get_max_memslots(VuDev *dev, VhostUserMsg *vmsg)
}
static bool
-vu_process_message(VuDev *dev, VhostUserMsg *vmsg)
+vu_process_message(VuDev *dev, VhostUserMsg *vmsg, bool *reply_sent,
+ bool *reply_status)
{
int do_reply = 0;
@@ -1843,6 +1915,14 @@ vu_process_message(VuDev *dev, VhostUserMsg *vmsg)
DPRINT("\n");
}
+ if (reply_sent) {
+ *reply_sent = false;
+ }
+
+ if (reply_status) {
+ *reply_status = false;
+ }
+
if (dev->iface->process_msg &&
dev->iface->process_msg(dev, vmsg, &do_reply)) {
return do_reply;
@@ -1912,6 +1992,10 @@ vu_process_message(VuDev *dev, VhostUserMsg *vmsg)
return vu_add_mem_reg(dev, vmsg);
case VHOST_USER_REM_MEM_REG:
return vu_rem_mem_reg(dev, vmsg);
+ case VHOST_USER_START_SLAVE_CHANNEL:
+ return vu_slave_channel_start(dev, vmsg);
+ case VHOST_USER_STOP_SLAVE_CHANNEL:
+ return vu_slave_channel_stop(dev, vmsg, reply_sent, reply_status);
default:
vmsg_close_fds(vmsg);
vu_panic(dev, "Unhandled request: %d", vmsg->request);
@@ -1926,6 +2010,7 @@ vu_dispatch(VuDev *dev)
VhostUserMsg vmsg = { 0, };
int reply_requested;
bool need_reply, success = false;
+ bool reply_sent = false, reply_status = false;
if (!dev->read_msg(dev, dev->sock, &vmsg)) {
goto end;
@@ -1933,7 +2018,14 @@ vu_dispatch(VuDev *dev)
need_reply = vmsg.flags & VHOST_USER_NEED_REPLY_MASK;
- reply_requested = vu_process_message(dev, &vmsg);
+ reply_requested = vu_process_message(dev, &vmsg, &reply_sent,
+ &reply_status);
+ /* reply has already been sent, if needed */
+ if (reply_sent) {
+ success = reply_status;
+ goto end;
+ }
+
if (!reply_requested && need_reply) {
vmsg_set_reply_u64(&vmsg, 0);
reply_requested = 1;
@@ -2051,6 +2143,7 @@ vu_init(VuDev *dev,
dev->log_call_fd = -1;
pthread_mutex_init(&dev->slave_mutex, NULL);
dev->slave_fd = -1;
+ dev->slave_channel_open = false;
dev->max_queues = max_queues;
dev->vq = malloc(max_queues * sizeof(dev->vq[0]));
diff --git a/subprojects/libvhost-user/libvhost-user.h b/subprojects/libvhost-user/libvhost-user.h
index ee75d4931f..1d0ef54f69 100644
--- a/subprojects/libvhost-user/libvhost-user.h
+++ b/subprojects/libvhost-user/libvhost-user.h
@@ -64,6 +64,7 @@ enum VhostUserProtocolFeature {
VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD = 12,
VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14,
VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
+ VHOST_USER_PROTOCOL_F_SLAVE_CH_START_STOP = 16,
VHOST_USER_PROTOCOL_F_MAX
};
@@ -109,6 +110,8 @@ typedef enum VhostUserRequest {
VHOST_USER_GET_MAX_MEM_SLOTS = 36,
VHOST_USER_ADD_MEM_REG = 37,
VHOST_USER_REM_MEM_REG = 38,
+ VHOST_USER_START_SLAVE_CHANNEL = 39,
+ VHOST_USER_STOP_SLAVE_CHANNEL = 40,
VHOST_USER_MAX
} VhostUserRequest;
@@ -123,6 +126,7 @@ typedef enum VhostUserSlaveRequest {
VHOST_USER_SLAVE_FS_UNMAP = 7,
VHOST_USER_SLAVE_FS_SYNC = 8,
VHOST_USER_SLAVE_FS_IO = 9,
+ VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE = 10,
VHOST_USER_SLAVE_MAX
} VhostUserSlaveRequest;
@@ -405,9 +409,11 @@ struct VuDev {
VuVirtq *vq;
VuDevInflightInfo inflight_info;
int log_call_fd;
- /* Must be held while using slave_fd */
+ /* Must be held while using slave_fd, slave_channel_open */
pthread_mutex_t slave_mutex;
int slave_fd;
+ /* If not set, do not send more requests on slave fd. */
+ bool slave_channel_open;
uint64_t log_size;
uint8_t *log_table;
uint64_t features;
--
2.25.4
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Virtio-fs] [PATCH 5/6] libvhost-user: Add support to start/stop/flush slave channel
@ 2021-01-25 18:01 ` Vivek Goyal
0 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-25 18:01 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: marcandre.lureau, vgoyal, mst
This patch adds support to start/stop/flush slave channel functionality.
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
subprojects/libvhost-user/libvhost-user.c | 103 ++++++++++++++++++++--
subprojects/libvhost-user/libvhost-user.h | 8 +-
2 files changed, 105 insertions(+), 6 deletions(-)
diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 7a56c56dc8..b4c795c63e 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -140,6 +140,8 @@ vu_request_to_string(unsigned int req)
REQ(VHOST_USER_GET_MAX_MEM_SLOTS),
REQ(VHOST_USER_ADD_MEM_REG),
REQ(VHOST_USER_REM_MEM_REG),
+ REQ(VHOST_USER_START_SLAVE_CHANNEL),
+ REQ(VHOST_USER_STOP_SLAVE_CHANNEL),
REQ(VHOST_USER_MAX),
};
#undef REQ
@@ -437,11 +439,11 @@ out:
return result;
}
-/* Returns true on success, false otherwise */
+/* slave mutex should be held. Will be unlocked upon return */
static bool
-vu_message_slave_send_receive(VuDev *dev, VhostUserMsg *vmsg, uint64_t *payload)
+vu_message_slave_send_receive_locked(VuDev *dev, VhostUserMsg *vmsg,
+ uint64_t *payload)
{
- pthread_mutex_lock(&dev->slave_mutex);
if (!vu_message_write(dev, dev->slave_fd, vmsg)) {
pthread_mutex_unlock(&dev->slave_mutex);
return false;
@@ -456,6 +458,46 @@ vu_message_slave_send_receive(VuDev *dev, VhostUserMsg *vmsg, uint64_t *payload)
return vu_process_message_reply(dev, vmsg, payload);
}
+/* Returns true on success, false otherwise */
+static bool
+vu_message_slave_send_receive(VuDev *dev, VhostUserMsg *vmsg,
+ uint64_t *payload)
+{
+ pthread_mutex_lock(&dev->slave_mutex);
+ if (!dev->slave_channel_open) {
+ pthread_mutex_unlock(&dev->slave_mutex);
+ return false;
+ }
+ return vu_message_slave_send_receive_locked(dev, vmsg, payload);
+}
+
+static bool
+vu_finish_stop_slave(VuDev *dev)
+{
+ bool res;
+ uint64_t payload = 0;
+ VhostUserMsg vmsg = {
+ .request = VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE,
+ .flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK,
+ .size = sizeof(vmsg.payload.u64),
+ .payload.u64 = 0,
+ };
+
+ /*
+ * Once we get slave_mutex, this should make sure no other caller is
+ * currently in the process of sending or receiving message on slave_fd.
+ * And setting slave_channel_open to false now will make sure any new
+ * callers will not send message and instead get error back. So it
+ * is now safe to send stop finished message to master.
+ */
+ pthread_mutex_lock(&dev->slave_mutex);
+ dev->slave_channel_open = false;
+ /* This also drops slave_mutex */
+ res = vu_message_slave_send_receive_locked(dev, &vmsg, &payload);
+ res = res && (payload == 0);
+ return res;
+}
+
/* Kick the log_call_fd if required. */
static void
vu_log_kick(VuDev *dev)
@@ -1529,6 +1571,35 @@ vu_set_slave_req_fd(VuDev *dev, VhostUserMsg *vmsg)
return false;
}
+static bool
+vu_slave_channel_start(VuDev *dev, VhostUserMsg *vmsg)
+{
+ pthread_mutex_lock(&dev->slave_mutex);
+ dev->slave_channel_open = true;
+ pthread_mutex_unlock(&dev->slave_mutex);
+ /* Caller (vu_dispatch()) will send a reply */
+ return false;
+}
+
+static bool
+vu_slave_channel_stop(VuDev *dev, VhostUserMsg *vmsg, bool *reply_sent,
+ bool *reply_status)
+{
+ vmsg_set_reply_u64(vmsg, 0);
+ *reply_sent = true;
+ *reply_status = false;
+ if (!vu_send_reply(dev, dev->sock, vmsg)) {
+ return false;
+ }
+
+ if (!vu_finish_stop_slave(dev)) {
+ return false;
+ }
+
+ *reply_status = true;
+ return false;
+}
+
static bool
vu_get_config(VuDev *dev, VhostUserMsg *vmsg)
{
@@ -1823,7 +1894,8 @@ static bool vu_handle_get_max_memslots(VuDev *dev, VhostUserMsg *vmsg)
}
static bool
-vu_process_message(VuDev *dev, VhostUserMsg *vmsg)
+vu_process_message(VuDev *dev, VhostUserMsg *vmsg, bool *reply_sent,
+ bool *reply_status)
{
int do_reply = 0;
@@ -1843,6 +1915,14 @@ vu_process_message(VuDev *dev, VhostUserMsg *vmsg)
DPRINT("\n");
}
+ if (reply_sent) {
+ *reply_sent = false;
+ }
+
+ if (reply_status) {
+ *reply_status = false;
+ }
+
if (dev->iface->process_msg &&
dev->iface->process_msg(dev, vmsg, &do_reply)) {
return do_reply;
@@ -1912,6 +1992,10 @@ vu_process_message(VuDev *dev, VhostUserMsg *vmsg)
return vu_add_mem_reg(dev, vmsg);
case VHOST_USER_REM_MEM_REG:
return vu_rem_mem_reg(dev, vmsg);
+ case VHOST_USER_START_SLAVE_CHANNEL:
+ return vu_slave_channel_start(dev, vmsg);
+ case VHOST_USER_STOP_SLAVE_CHANNEL:
+ return vu_slave_channel_stop(dev, vmsg, reply_sent, reply_status);
default:
vmsg_close_fds(vmsg);
vu_panic(dev, "Unhandled request: %d", vmsg->request);
@@ -1926,6 +2010,7 @@ vu_dispatch(VuDev *dev)
VhostUserMsg vmsg = { 0, };
int reply_requested;
bool need_reply, success = false;
+ bool reply_sent = false, reply_status = false;
if (!dev->read_msg(dev, dev->sock, &vmsg)) {
goto end;
@@ -1933,7 +2018,14 @@ vu_dispatch(VuDev *dev)
need_reply = vmsg.flags & VHOST_USER_NEED_REPLY_MASK;
- reply_requested = vu_process_message(dev, &vmsg);
+ reply_requested = vu_process_message(dev, &vmsg, &reply_sent,
+ &reply_status);
+ /* reply has already been sent, if needed */
+ if (reply_sent) {
+ success = reply_status;
+ goto end;
+ }
+
if (!reply_requested && need_reply) {
vmsg_set_reply_u64(&vmsg, 0);
reply_requested = 1;
@@ -2051,6 +2143,7 @@ vu_init(VuDev *dev,
dev->log_call_fd = -1;
pthread_mutex_init(&dev->slave_mutex, NULL);
dev->slave_fd = -1;
+ dev->slave_channel_open = false;
dev->max_queues = max_queues;
dev->vq = malloc(max_queues * sizeof(dev->vq[0]));
diff --git a/subprojects/libvhost-user/libvhost-user.h b/subprojects/libvhost-user/libvhost-user.h
index ee75d4931f..1d0ef54f69 100644
--- a/subprojects/libvhost-user/libvhost-user.h
+++ b/subprojects/libvhost-user/libvhost-user.h
@@ -64,6 +64,7 @@ enum VhostUserProtocolFeature {
VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD = 12,
VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14,
VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
+ VHOST_USER_PROTOCOL_F_SLAVE_CH_START_STOP = 16,
VHOST_USER_PROTOCOL_F_MAX
};
@@ -109,6 +110,8 @@ typedef enum VhostUserRequest {
VHOST_USER_GET_MAX_MEM_SLOTS = 36,
VHOST_USER_ADD_MEM_REG = 37,
VHOST_USER_REM_MEM_REG = 38,
+ VHOST_USER_START_SLAVE_CHANNEL = 39,
+ VHOST_USER_STOP_SLAVE_CHANNEL = 40,
VHOST_USER_MAX
} VhostUserRequest;
@@ -123,6 +126,7 @@ typedef enum VhostUserSlaveRequest {
VHOST_USER_SLAVE_FS_UNMAP = 7,
VHOST_USER_SLAVE_FS_SYNC = 8,
VHOST_USER_SLAVE_FS_IO = 9,
+ VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE = 10,
VHOST_USER_SLAVE_MAX
} VhostUserSlaveRequest;
@@ -405,9 +409,11 @@ struct VuDev {
VuVirtq *vq;
VuDevInflightInfo inflight_info;
int log_call_fd;
- /* Must be held while using slave_fd */
+ /* Must be held while using slave_fd, slave_channel_open */
pthread_mutex_t slave_mutex;
int slave_fd;
+ /* If not set, do not send more requests on slave fd. */
+ bool slave_channel_open;
uint64_t log_size;
uint8_t *log_table;
uint64_t features;
--
2.25.4
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH 6/6] virtiofsd: Opt in for slave start/stop/shutdown functionality
2021-01-25 18:01 ` [Virtio-fs] " Vivek Goyal
@ 2021-01-25 18:01 ` Vivek Goyal
-1 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-25 18:01 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: marcandre.lureau, stefanha, dgilbert, vgoyal, mst
This is an opt-in feature not enabled by default. Enable it in
protocol features by setting VHOST_USER_PROTOCOL_F_SLAVE_CH_START_STOP.
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
tools/virtiofsd/fuse_virtio.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 6805d8ba01..9328c8fac6 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -103,6 +103,11 @@ static void fv_set_features(VuDev *dev, uint64_t features)
{
}
+static uint64_t fv_get_protocol_features(VuDev *dev)
+{
+ return 1ULL << VHOST_USER_PROTOCOL_F_SLAVE_CH_START_STOP;
+}
+
/*
* Callback from libvhost-user if there's a new fd we're supposed to listen
* to, typically a queue kick?
@@ -902,6 +907,7 @@ static const VuDevIface fv_iface = {
.get_features = fv_get_features,
.set_features = fv_set_features,
+ .get_protocol_features = fv_get_protocol_features,
/* Don't need process message, we've not got any at vhost-user level */
.queue_set_started = fv_queue_set_started,
--
2.25.4
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Virtio-fs] [PATCH 6/6] virtiofsd: Opt in for slave start/stop/shutdown functionality
@ 2021-01-25 18:01 ` Vivek Goyal
0 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-25 18:01 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: marcandre.lureau, vgoyal, mst
This is an opt-in feature not enabled by default. Enable it in
protocol features by setting VHOST_USER_PROTOCOL_F_SLAVE_CH_START_STOP.
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
tools/virtiofsd/fuse_virtio.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 6805d8ba01..9328c8fac6 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -103,6 +103,11 @@ static void fv_set_features(VuDev *dev, uint64_t features)
{
}
+static uint64_t fv_get_protocol_features(VuDev *dev)
+{
+ return 1ULL << VHOST_USER_PROTOCOL_F_SLAVE_CH_START_STOP;
+}
+
/*
* Callback from libvhost-user if there's a new fd we're supposed to listen
* to, typically a queue kick?
@@ -902,6 +907,7 @@ static const VuDevIface fv_iface = {
.get_features = fv_get_features,
.set_features = fv_set_features,
+ .get_protocol_features = fv_get_protocol_features,
/* Don't need process message, we've not got any at vhost-user level */
.queue_set_started = fv_queue_set_started,
--
2.25.4
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH 1/6] virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
2021-01-25 18:01 ` [Virtio-fs] " Vivek Goyal
@ 2021-01-26 15:56 ` Greg Kurz
-1 siblings, 0 replies; 52+ messages in thread
From: Greg Kurz @ 2021-01-26 15:56 UTC (permalink / raw)
To: Vivek Goyal
Cc: mst, qemu-devel, dgilbert, virtio-fs, stefanha, marcandre.lureau
On Mon, 25 Jan 2021 13:01:10 -0500
Vivek Goyal <vgoyal@redhat.com> wrote:
> When we are shutting down virtqueues, virtio_loop() receives a message
> VHOST_USER_GET_VRING_BASE from master. We acquire ->vu_dispatch_rwlock
> and get into the process of shutting down virtqueue. In one of the
> final steps, we are waiting for fv_queue_thread() to exit/finish and
> wait with ->vu_dispatch_rwlock held.
>
> But it is possible that fv_queue_thread() itself is waiting to get
> ->vu_dispatch_rwlock (With --thread-pool=0 option). If requests
> are being processed by fv_queue_worker(), then fv_queue_worker()
> can wait for the ->vu_dispatch_rwlock, and fv_queue_thread() will
> wait for fv_queue_worker() before thread pool can be stopped.
>
> IOW, if guest is shutdown uncleanly (some sort of emergency reboot),
> it is possible that virtiofsd is processing a fs request and
> qemu initiates device shutdown sequence. In that case there seem
> to be two options. Either abort the existing request completely or
> let existing request finish.
>
> This patch is taking second approach. That is drop the ->vu_dispatch_rwlock
> temporarily so that fv_queue_thread() can finish and deadlock does not
> happen.
>
> ->vu_dispatch_rwlock provides mutual exclusion between virtio_loop()
> (handling vhost-user protocol messages) and fv_queue_thread() (handling
> fuse filesystem requests). Rational seems to be that protocol message
> might change queue memory mappings, so we don't want both to proceed
> at the same time.
>
> In this case queue is shutting down, so I hope it is fine for fv_queue_thread() to send response back while virtio_loop() is still waiting (and not handling
It looks this lacks a \n after "fine for"
> any further vho-user protocol messages).
>
> IOW, assumption here is that while virto_loop is blocked processing
> VHOST_USER_GET_VRING_BASE message, it is still ok to send back the
> response on vq by fv_queue_thread().
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
> tools/virtiofsd/fuse_virtio.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index 9577eaa68d..6805d8ba01 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -813,11 +813,20 @@ static void fv_queue_cleanup_thread(struct fv_VuDev *vud, int qidx)
> fuse_log(FUSE_LOG_ERR, "Eventfd_write for queue %d: %s\n",
> qidx, strerror(errno));
> }
> +
> + /*
> + * Drop ->vu_dispath_rwlock and reacquire. We are about to wait for
> + * for fv_queue_thread() and that might require ->vu_dispatch_rwlock
> + * to finish.
> + */
> + pthread_rwlock_unlock(&vud->vu_dispatch_rwlock);
> ret = pthread_join(ourqi->thread, NULL);
> if (ret) {
> fuse_log(FUSE_LOG_ERR, "%s: Failed to join thread idx %d err %d\n",
> __func__, qidx, ret);
> }
> + pthread_rwlock_wrlock(&vud->vu_dispatch_rwlock);
> +
So this is assuming that fv_queue_cleanup_thread() is called with
vu_dispatch_rwlock already taken for writing, but there are no
clear evidence in the code why it should care for the locking at
all in the first place.
On the contrary, one of its two callers is a vhost-user callback,
in which we can reasonably have this assumption, while we can
have the opposite assumption for the other one in virtio_loop().
This makes me think that the drop/reacquire trick should only
be done in fv_queue_set_started(), instead of...
> pthread_mutex_destroy(&ourqi->vq_lock);
> close(ourqi->kill_fd);
> ourqi->kick_fd = -1;
> @@ -952,7 +961,11 @@ int virtio_loop(struct fuse_session *se)
> /*
> * Make sure all fv_queue_thread()s quit on exit, as we're about to
> * free virtio dev and fuse session, no one should access them anymore.
> + * Hold ->vu_dispatch_rwlock in write mode as fv_queue_cleanup_thread()
> + * assumes mutex is locked and unlocks/re-locks it.
> */
> +
> + pthread_rwlock_wrlock(&se->virtio_dev->vu_dispatch_rwlock);
... artificially introducing another critical section here.
The issue isn't even specific to vu_dispatch_rwlock actually :
fv_queue_cleanup_thread() shouldn't be called with any lock
held because it might sleep in pthread_join() and cause a
deadlock all the same. So I'd rather document that instead :
drop all locks before calling fv_queue_cleanup_thread().
Also, since pthread_rwlock_wrlock() can fail, I think we should
always check it's return value, at least with an assert() like
already done elsewhere.
> for (int i = 0; i < se->virtio_dev->nqueues; i++) {
> if (!se->virtio_dev->qi[i]) {
> continue;
> @@ -961,6 +974,7 @@ int virtio_loop(struct fuse_session *se)
> fuse_log(FUSE_LOG_INFO, "%s: Stopping queue %d thread\n", __func__, i);
> fv_queue_cleanup_thread(se->virtio_dev, i);
> }
> + pthread_rwlock_unlock(&se->virtio_dev->vu_dispatch_rwlock);
>
> fuse_log(FUSE_LOG_INFO, "%s: Exit\n", __func__);
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Virtio-fs] [PATCH 1/6] virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
@ 2021-01-26 15:56 ` Greg Kurz
0 siblings, 0 replies; 52+ messages in thread
From: Greg Kurz @ 2021-01-26 15:56 UTC (permalink / raw)
To: Vivek Goyal; +Cc: mst, qemu-devel, virtio-fs, marcandre.lureau
On Mon, 25 Jan 2021 13:01:10 -0500
Vivek Goyal <vgoyal@redhat.com> wrote:
> When we are shutting down virtqueues, virtio_loop() receives a message
> VHOST_USER_GET_VRING_BASE from master. We acquire ->vu_dispatch_rwlock
> and get into the process of shutting down virtqueue. In one of the
> final steps, we are waiting for fv_queue_thread() to exit/finish and
> wait with ->vu_dispatch_rwlock held.
>
> But it is possible that fv_queue_thread() itself is waiting to get
> ->vu_dispatch_rwlock (With --thread-pool=0 option). If requests
> are being processed by fv_queue_worker(), then fv_queue_worker()
> can wait for the ->vu_dispatch_rwlock, and fv_queue_thread() will
> wait for fv_queue_worker() before thread pool can be stopped.
>
> IOW, if guest is shutdown uncleanly (some sort of emergency reboot),
> it is possible that virtiofsd is processing a fs request and
> qemu initiates device shutdown sequence. In that case there seem
> to be two options. Either abort the existing request completely or
> let existing request finish.
>
> This patch is taking second approach. That is drop the ->vu_dispatch_rwlock
> temporarily so that fv_queue_thread() can finish and deadlock does not
> happen.
>
> ->vu_dispatch_rwlock provides mutual exclusion between virtio_loop()
> (handling vhost-user protocol messages) and fv_queue_thread() (handling
> fuse filesystem requests). Rational seems to be that protocol message
> might change queue memory mappings, so we don't want both to proceed
> at the same time.
>
> In this case queue is shutting down, so I hope it is fine for fv_queue_thread() to send response back while virtio_loop() is still waiting (and not handling
It looks this lacks a \n after "fine for"
> any further vho-user protocol messages).
>
> IOW, assumption here is that while virto_loop is blocked processing
> VHOST_USER_GET_VRING_BASE message, it is still ok to send back the
> response on vq by fv_queue_thread().
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
> tools/virtiofsd/fuse_virtio.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index 9577eaa68d..6805d8ba01 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -813,11 +813,20 @@ static void fv_queue_cleanup_thread(struct fv_VuDev *vud, int qidx)
> fuse_log(FUSE_LOG_ERR, "Eventfd_write for queue %d: %s\n",
> qidx, strerror(errno));
> }
> +
> + /*
> + * Drop ->vu_dispath_rwlock and reacquire. We are about to wait for
> + * for fv_queue_thread() and that might require ->vu_dispatch_rwlock
> + * to finish.
> + */
> + pthread_rwlock_unlock(&vud->vu_dispatch_rwlock);
> ret = pthread_join(ourqi->thread, NULL);
> if (ret) {
> fuse_log(FUSE_LOG_ERR, "%s: Failed to join thread idx %d err %d\n",
> __func__, qidx, ret);
> }
> + pthread_rwlock_wrlock(&vud->vu_dispatch_rwlock);
> +
So this is assuming that fv_queue_cleanup_thread() is called with
vu_dispatch_rwlock already taken for writing, but there are no
clear evidence in the code why it should care for the locking at
all in the first place.
On the contrary, one of its two callers is a vhost-user callback,
in which we can reasonably have this assumption, while we can
have the opposite assumption for the other one in virtio_loop().
This makes me think that the drop/reacquire trick should only
be done in fv_queue_set_started(), instead of...
> pthread_mutex_destroy(&ourqi->vq_lock);
> close(ourqi->kill_fd);
> ourqi->kick_fd = -1;
> @@ -952,7 +961,11 @@ int virtio_loop(struct fuse_session *se)
> /*
> * Make sure all fv_queue_thread()s quit on exit, as we're about to
> * free virtio dev and fuse session, no one should access them anymore.
> + * Hold ->vu_dispatch_rwlock in write mode as fv_queue_cleanup_thread()
> + * assumes mutex is locked and unlocks/re-locks it.
> */
> +
> + pthread_rwlock_wrlock(&se->virtio_dev->vu_dispatch_rwlock);
... artificially introducing another critical section here.
The issue isn't even specific to vu_dispatch_rwlock actually :
fv_queue_cleanup_thread() shouldn't be called with any lock
held because it might sleep in pthread_join() and cause a
deadlock all the same. So I'd rather document that instead :
drop all locks before calling fv_queue_cleanup_thread().
Also, since pthread_rwlock_wrlock() can fail, I think we should
always check it's return value, at least with an assert() like
already done elsewhere.
> for (int i = 0; i < se->virtio_dev->nqueues; i++) {
> if (!se->virtio_dev->qi[i]) {
> continue;
> @@ -961,6 +974,7 @@ int virtio_loop(struct fuse_session *se)
> fuse_log(FUSE_LOG_INFO, "%s: Stopping queue %d thread\n", __func__, i);
> fv_queue_cleanup_thread(se->virtio_dev, i);
> }
> + pthread_rwlock_unlock(&se->virtio_dev->vu_dispatch_rwlock);
>
> fuse_log(FUSE_LOG_INFO, "%s: Exit\n", __func__);
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 1/6] virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
2021-01-26 15:56 ` [Virtio-fs] " Greg Kurz
@ 2021-01-26 18:33 ` Vivek Goyal
-1 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-26 18:33 UTC (permalink / raw)
To: Greg Kurz
Cc: mst, qemu-devel, dgilbert, virtio-fs, stefanha, marcandre.lureau
On Tue, Jan 26, 2021 at 04:56:00PM +0100, Greg Kurz wrote:
> On Mon, 25 Jan 2021 13:01:10 -0500
> Vivek Goyal <vgoyal@redhat.com> wrote:
>
> > When we are shutting down virtqueues, virtio_loop() receives a message
> > VHOST_USER_GET_VRING_BASE from master. We acquire ->vu_dispatch_rwlock
> > and get into the process of shutting down virtqueue. In one of the
> > final steps, we are waiting for fv_queue_thread() to exit/finish and
> > wait with ->vu_dispatch_rwlock held.
> >
> > But it is possible that fv_queue_thread() itself is waiting to get
> > ->vu_dispatch_rwlock (With --thread-pool=0 option). If requests
> > are being processed by fv_queue_worker(), then fv_queue_worker()
> > can wait for the ->vu_dispatch_rwlock, and fv_queue_thread() will
> > wait for fv_queue_worker() before thread pool can be stopped.
> >
> > IOW, if guest is shutdown uncleanly (some sort of emergency reboot),
> > it is possible that virtiofsd is processing a fs request and
> > qemu initiates device shutdown sequence. In that case there seem
> > to be two options. Either abort the existing request completely or
> > let existing request finish.
> >
> > This patch is taking second approach. That is drop the ->vu_dispatch_rwlock
> > temporarily so that fv_queue_thread() can finish and deadlock does not
> > happen.
> >
> > ->vu_dispatch_rwlock provides mutual exclusion between virtio_loop()
> > (handling vhost-user protocol messages) and fv_queue_thread() (handling
> > fuse filesystem requests). Rational seems to be that protocol message
> > might change queue memory mappings, so we don't want both to proceed
> > at the same time.
> >
> > In this case queue is shutting down, so I hope it is fine for fv_queue_thread() to send response back while virtio_loop() is still waiting (and not handling
>
> It looks this lacks a \n after "fine for"
Hi Greg,
Will fix.
>
> > any further vho-user protocol messages).
> >
> > IOW, assumption here is that while virto_loop is blocked processing
> > VHOST_USER_GET_VRING_BASE message, it is still ok to send back the
> > response on vq by fv_queue_thread().
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> > tools/virtiofsd/fuse_virtio.c | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> > index 9577eaa68d..6805d8ba01 100644
> > --- a/tools/virtiofsd/fuse_virtio.c
> > +++ b/tools/virtiofsd/fuse_virtio.c
> > @@ -813,11 +813,20 @@ static void fv_queue_cleanup_thread(struct fv_VuDev *vud, int qidx)
> > fuse_log(FUSE_LOG_ERR, "Eventfd_write for queue %d: %s\n",
> > qidx, strerror(errno));
> > }
> > +
> > + /*
> > + * Drop ->vu_dispath_rwlock and reacquire. We are about to wait for
> > + * for fv_queue_thread() and that might require ->vu_dispatch_rwlock
> > + * to finish.
> > + */
> > + pthread_rwlock_unlock(&vud->vu_dispatch_rwlock);
> > ret = pthread_join(ourqi->thread, NULL);
> > if (ret) {
> > fuse_log(FUSE_LOG_ERR, "%s: Failed to join thread idx %d err %d\n",
> > __func__, qidx, ret);
> > }
> > + pthread_rwlock_wrlock(&vud->vu_dispatch_rwlock);
> > +
>
> So this is assuming that fv_queue_cleanup_thread() is called with
> vu_dispatch_rwlock already taken for writing, but there are no
> clear evidence in the code why it should care for the locking at
> all in the first place.
>
> On the contrary, one of its two callers is a vhost-user callback,
> in which we can reasonably have this assumption, while we can
> have the opposite assumption for the other one in virtio_loop().
>
> This makes me think that the drop/reacquire trick should only
> be done in fv_queue_set_started(), instead of...
I think this sounds reasonable. I will drop lock/re-acquire in
fv_queue_set_started() around the call to fv_queue_cleanup_thread().
>
> > pthread_mutex_destroy(&ourqi->vq_lock);
> > close(ourqi->kill_fd);
> > ourqi->kick_fd = -1;
> > @@ -952,7 +961,11 @@ int virtio_loop(struct fuse_session *se)
> > /*
> > * Make sure all fv_queue_thread()s quit on exit, as we're about to
> > * free virtio dev and fuse session, no one should access them anymore.
> > + * Hold ->vu_dispatch_rwlock in write mode as fv_queue_cleanup_thread()
> > + * assumes mutex is locked and unlocks/re-locks it.
> > */
> > +
> > + pthread_rwlock_wrlock(&se->virtio_dev->vu_dispatch_rwlock);
>
>
> ... artificially introducing another critical section here.
>
> The issue isn't even specific to vu_dispatch_rwlock actually :
> fv_queue_cleanup_thread() shouldn't be called with any lock
> held because it might sleep in pthread_join() and cause a
> deadlock all the same. So I'd rather document that instead :
> drop all locks before calling fv_queue_cleanup_thread().
Sounds good. Will do.
>
> Also, since pthread_rwlock_wrlock() can fail, I think we should
> always check it's return value, at least with an assert() like
> already done elsewhere.
Will check return code of pthread_rwlock_wrlock() and probably use
assert().
Vivek
>
> > for (int i = 0; i < se->virtio_dev->nqueues; i++) {
> > if (!se->virtio_dev->qi[i]) {
> > continue;
> > @@ -961,6 +974,7 @@ int virtio_loop(struct fuse_session *se)
> > fuse_log(FUSE_LOG_INFO, "%s: Stopping queue %d thread\n", __func__, i);
> > fv_queue_cleanup_thread(se->virtio_dev, i);
> > }
> > + pthread_rwlock_unlock(&se->virtio_dev->vu_dispatch_rwlock);
> >
> > fuse_log(FUSE_LOG_INFO, "%s: Exit\n", __func__);
> >
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Virtio-fs] [PATCH 1/6] virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
@ 2021-01-26 18:33 ` Vivek Goyal
0 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-26 18:33 UTC (permalink / raw)
To: Greg Kurz; +Cc: mst, qemu-devel, virtio-fs, marcandre.lureau
On Tue, Jan 26, 2021 at 04:56:00PM +0100, Greg Kurz wrote:
> On Mon, 25 Jan 2021 13:01:10 -0500
> Vivek Goyal <vgoyal@redhat.com> wrote:
>
> > When we are shutting down virtqueues, virtio_loop() receives a message
> > VHOST_USER_GET_VRING_BASE from master. We acquire ->vu_dispatch_rwlock
> > and get into the process of shutting down virtqueue. In one of the
> > final steps, we are waiting for fv_queue_thread() to exit/finish and
> > wait with ->vu_dispatch_rwlock held.
> >
> > But it is possible that fv_queue_thread() itself is waiting to get
> > ->vu_dispatch_rwlock (With --thread-pool=0 option). If requests
> > are being processed by fv_queue_worker(), then fv_queue_worker()
> > can wait for the ->vu_dispatch_rwlock, and fv_queue_thread() will
> > wait for fv_queue_worker() before thread pool can be stopped.
> >
> > IOW, if guest is shutdown uncleanly (some sort of emergency reboot),
> > it is possible that virtiofsd is processing a fs request and
> > qemu initiates device shutdown sequence. In that case there seem
> > to be two options. Either abort the existing request completely or
> > let existing request finish.
> >
> > This patch is taking second approach. That is drop the ->vu_dispatch_rwlock
> > temporarily so that fv_queue_thread() can finish and deadlock does not
> > happen.
> >
> > ->vu_dispatch_rwlock provides mutual exclusion between virtio_loop()
> > (handling vhost-user protocol messages) and fv_queue_thread() (handling
> > fuse filesystem requests). Rational seems to be that protocol message
> > might change queue memory mappings, so we don't want both to proceed
> > at the same time.
> >
> > In this case queue is shutting down, so I hope it is fine for fv_queue_thread() to send response back while virtio_loop() is still waiting (and not handling
>
> It looks this lacks a \n after "fine for"
Hi Greg,
Will fix.
>
> > any further vho-user protocol messages).
> >
> > IOW, assumption here is that while virto_loop is blocked processing
> > VHOST_USER_GET_VRING_BASE message, it is still ok to send back the
> > response on vq by fv_queue_thread().
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> > tools/virtiofsd/fuse_virtio.c | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> > index 9577eaa68d..6805d8ba01 100644
> > --- a/tools/virtiofsd/fuse_virtio.c
> > +++ b/tools/virtiofsd/fuse_virtio.c
> > @@ -813,11 +813,20 @@ static void fv_queue_cleanup_thread(struct fv_VuDev *vud, int qidx)
> > fuse_log(FUSE_LOG_ERR, "Eventfd_write for queue %d: %s\n",
> > qidx, strerror(errno));
> > }
> > +
> > + /*
> > + * Drop ->vu_dispath_rwlock and reacquire. We are about to wait for
> > + * for fv_queue_thread() and that might require ->vu_dispatch_rwlock
> > + * to finish.
> > + */
> > + pthread_rwlock_unlock(&vud->vu_dispatch_rwlock);
> > ret = pthread_join(ourqi->thread, NULL);
> > if (ret) {
> > fuse_log(FUSE_LOG_ERR, "%s: Failed to join thread idx %d err %d\n",
> > __func__, qidx, ret);
> > }
> > + pthread_rwlock_wrlock(&vud->vu_dispatch_rwlock);
> > +
>
> So this is assuming that fv_queue_cleanup_thread() is called with
> vu_dispatch_rwlock already taken for writing, but there are no
> clear evidence in the code why it should care for the locking at
> all in the first place.
>
> On the contrary, one of its two callers is a vhost-user callback,
> in which we can reasonably have this assumption, while we can
> have the opposite assumption for the other one in virtio_loop().
>
> This makes me think that the drop/reacquire trick should only
> be done in fv_queue_set_started(), instead of...
I think this sounds reasonable. I will drop lock/re-acquire in
fv_queue_set_started() around the call to fv_queue_cleanup_thread().
>
> > pthread_mutex_destroy(&ourqi->vq_lock);
> > close(ourqi->kill_fd);
> > ourqi->kick_fd = -1;
> > @@ -952,7 +961,11 @@ int virtio_loop(struct fuse_session *se)
> > /*
> > * Make sure all fv_queue_thread()s quit on exit, as we're about to
> > * free virtio dev and fuse session, no one should access them anymore.
> > + * Hold ->vu_dispatch_rwlock in write mode as fv_queue_cleanup_thread()
> > + * assumes mutex is locked and unlocks/re-locks it.
> > */
> > +
> > + pthread_rwlock_wrlock(&se->virtio_dev->vu_dispatch_rwlock);
>
>
> ... artificially introducing another critical section here.
>
> The issue isn't even specific to vu_dispatch_rwlock actually :
> fv_queue_cleanup_thread() shouldn't be called with any lock
> held because it might sleep in pthread_join() and cause a
> deadlock all the same. So I'd rather document that instead :
> drop all locks before calling fv_queue_cleanup_thread().
Sounds good. Will do.
>
> Also, since pthread_rwlock_wrlock() can fail, I think we should
> always check it's return value, at least with an assert() like
> already done elsewhere.
Will check return code of pthread_rwlock_wrlock() and probably use
assert().
Vivek
>
> > for (int i = 0; i < se->virtio_dev->nqueues; i++) {
> > if (!se->virtio_dev->qi[i]) {
> > continue;
> > @@ -961,6 +974,7 @@ int virtio_loop(struct fuse_session *se)
> > fuse_log(FUSE_LOG_INFO, "%s: Stopping queue %d thread\n", __func__, i);
> > fv_queue_cleanup_thread(se->virtio_dev, i);
> > }
> > + pthread_rwlock_unlock(&se->virtio_dev->vu_dispatch_rwlock);
> >
> > fuse_log(FUSE_LOG_INFO, "%s: Exit\n", __func__);
> >
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 2/6] libvhost-user: Use slave_mutex in all slave messages
2021-01-25 18:01 ` [Virtio-fs] " Vivek Goyal
@ 2021-01-28 14:31 ` Greg Kurz
-1 siblings, 0 replies; 52+ messages in thread
From: Greg Kurz @ 2021-01-28 14:31 UTC (permalink / raw)
To: Vivek Goyal
Cc: mst, qemu-devel, dgilbert, virtio-fs, stefanha, marcandre.lureau
On Mon, 25 Jan 2021 13:01:11 -0500
Vivek Goyal <vgoyal@redhat.com> wrote:
> dev->slave_mutex needs to be taken when sending messages on slave_fd.
> Currently _vu_queue_notify() does not do that.
>
> Introduce a helper vu_message_slave_send_receive() which sends as well
> as receive response. Use this helper in all the paths which send
> message on slave_fd channel.
>
Does this fix any known bug ?
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
LGTM
Reviewed-by: Greg Kurz <groug@kaod.org>
> subprojects/libvhost-user/libvhost-user.c | 50 ++++++++++++-----------
> 1 file changed, 27 insertions(+), 23 deletions(-)
>
> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> index 4cf4aef63d..7a56c56dc8 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -403,7 +403,7 @@ vu_send_reply(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
> * Processes a reply on the slave channel.
> * Entered with slave_mutex held and releases it before exit.
> * Returns true on success.
> - * *payload is written on success
> + * *payload is written on success, if payload is not NULL.
> */
> static bool
> vu_process_message_reply(VuDev *dev, const VhostUserMsg *vmsg,
> @@ -427,7 +427,9 @@ vu_process_message_reply(VuDev *dev, const VhostUserMsg *vmsg,
> goto out;
> }
>
> - *payload = msg_reply.payload.u64;
> + if (payload) {
> + *payload = msg_reply.payload.u64;
> + }
> result = true;
>
> out:
> @@ -435,6 +437,25 @@ out:
> return result;
> }
>
> +/* Returns true on success, false otherwise */
> +static bool
> +vu_message_slave_send_receive(VuDev *dev, VhostUserMsg *vmsg, uint64_t *payload)
> +{
> + pthread_mutex_lock(&dev->slave_mutex);
> + if (!vu_message_write(dev, dev->slave_fd, vmsg)) {
> + pthread_mutex_unlock(&dev->slave_mutex);
> + return false;
> + }
> +
> + if ((vmsg->flags & VHOST_USER_NEED_REPLY_MASK) == 0) {
> + pthread_mutex_unlock(&dev->slave_mutex);
> + return true;
> + }
> +
> + /* Also unlocks the slave_mutex */
> + return vu_process_message_reply(dev, vmsg, payload);
> +}
> +
> /* Kick the log_call_fd if required. */
> static void
> vu_log_kick(VuDev *dev)
> @@ -1340,16 +1361,8 @@ bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq *vq, int fd,
> return false;
> }
>
> - pthread_mutex_lock(&dev->slave_mutex);
> - if (!vu_message_write(dev, dev->slave_fd, &vmsg)) {
> - pthread_mutex_unlock(&dev->slave_mutex);
> - return false;
> - }
> -
> - /* Also unlocks the slave_mutex */
> - res = vu_process_message_reply(dev, &vmsg, &payload);
> + res = vu_message_slave_send_receive(dev, &vmsg, &payload);
> res = res && (payload == 0);
> -
> return res;
> }
>
> @@ -2395,10 +2408,7 @@ static void _vu_queue_notify(VuDev *dev, VuVirtq *vq, bool sync)
> vmsg.flags |= VHOST_USER_NEED_REPLY_MASK;
> }
>
> - vu_message_write(dev, dev->slave_fd, &vmsg);
> - if (ack) {
> - vu_message_read_default(dev, dev->slave_fd, &vmsg);
> - }
> + vu_message_slave_send_receive(dev, &vmsg, NULL);
> return;
> }
>
> @@ -2942,17 +2952,11 @@ int64_t vu_fs_cache_request(VuDev *dev, VhostUserSlaveRequest req, int fd,
> return -EINVAL;
> }
>
> - pthread_mutex_lock(&dev->slave_mutex);
> - if (!vu_message_write(dev, dev->slave_fd, &vmsg)) {
> - pthread_mutex_unlock(&dev->slave_mutex);
> - return -EIO;
> - }
> -
> - /* Also unlocks the slave_mutex */
> - res = vu_process_message_reply(dev, &vmsg, &payload);
> + res = vu_message_slave_send_receive(dev, &vmsg, &payload);
> if (!res) {
> return -EIO;
> }
> +
> /*
> * Payload is delivered as uint64_t but is actually signed for
> * errors.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Virtio-fs] [PATCH 2/6] libvhost-user: Use slave_mutex in all slave messages
@ 2021-01-28 14:31 ` Greg Kurz
0 siblings, 0 replies; 52+ messages in thread
From: Greg Kurz @ 2021-01-28 14:31 UTC (permalink / raw)
To: Vivek Goyal; +Cc: mst, qemu-devel, virtio-fs, marcandre.lureau
On Mon, 25 Jan 2021 13:01:11 -0500
Vivek Goyal <vgoyal@redhat.com> wrote:
> dev->slave_mutex needs to be taken when sending messages on slave_fd.
> Currently _vu_queue_notify() does not do that.
>
> Introduce a helper vu_message_slave_send_receive() which sends as well
> as receive response. Use this helper in all the paths which send
> message on slave_fd channel.
>
Does this fix any known bug ?
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
LGTM
Reviewed-by: Greg Kurz <groug@kaod.org>
> subprojects/libvhost-user/libvhost-user.c | 50 ++++++++++++-----------
> 1 file changed, 27 insertions(+), 23 deletions(-)
>
> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> index 4cf4aef63d..7a56c56dc8 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -403,7 +403,7 @@ vu_send_reply(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
> * Processes a reply on the slave channel.
> * Entered with slave_mutex held and releases it before exit.
> * Returns true on success.
> - * *payload is written on success
> + * *payload is written on success, if payload is not NULL.
> */
> static bool
> vu_process_message_reply(VuDev *dev, const VhostUserMsg *vmsg,
> @@ -427,7 +427,9 @@ vu_process_message_reply(VuDev *dev, const VhostUserMsg *vmsg,
> goto out;
> }
>
> - *payload = msg_reply.payload.u64;
> + if (payload) {
> + *payload = msg_reply.payload.u64;
> + }
> result = true;
>
> out:
> @@ -435,6 +437,25 @@ out:
> return result;
> }
>
> +/* Returns true on success, false otherwise */
> +static bool
> +vu_message_slave_send_receive(VuDev *dev, VhostUserMsg *vmsg, uint64_t *payload)
> +{
> + pthread_mutex_lock(&dev->slave_mutex);
> + if (!vu_message_write(dev, dev->slave_fd, vmsg)) {
> + pthread_mutex_unlock(&dev->slave_mutex);
> + return false;
> + }
> +
> + if ((vmsg->flags & VHOST_USER_NEED_REPLY_MASK) == 0) {
> + pthread_mutex_unlock(&dev->slave_mutex);
> + return true;
> + }
> +
> + /* Also unlocks the slave_mutex */
> + return vu_process_message_reply(dev, vmsg, payload);
> +}
> +
> /* Kick the log_call_fd if required. */
> static void
> vu_log_kick(VuDev *dev)
> @@ -1340,16 +1361,8 @@ bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq *vq, int fd,
> return false;
> }
>
> - pthread_mutex_lock(&dev->slave_mutex);
> - if (!vu_message_write(dev, dev->slave_fd, &vmsg)) {
> - pthread_mutex_unlock(&dev->slave_mutex);
> - return false;
> - }
> -
> - /* Also unlocks the slave_mutex */
> - res = vu_process_message_reply(dev, &vmsg, &payload);
> + res = vu_message_slave_send_receive(dev, &vmsg, &payload);
> res = res && (payload == 0);
> -
> return res;
> }
>
> @@ -2395,10 +2408,7 @@ static void _vu_queue_notify(VuDev *dev, VuVirtq *vq, bool sync)
> vmsg.flags |= VHOST_USER_NEED_REPLY_MASK;
> }
>
> - vu_message_write(dev, dev->slave_fd, &vmsg);
> - if (ack) {
> - vu_message_read_default(dev, dev->slave_fd, &vmsg);
> - }
> + vu_message_slave_send_receive(dev, &vmsg, NULL);
> return;
> }
>
> @@ -2942,17 +2952,11 @@ int64_t vu_fs_cache_request(VuDev *dev, VhostUserSlaveRequest req, int fd,
> return -EINVAL;
> }
>
> - pthread_mutex_lock(&dev->slave_mutex);
> - if (!vu_message_write(dev, dev->slave_fd, &vmsg)) {
> - pthread_mutex_unlock(&dev->slave_mutex);
> - return -EIO;
> - }
> -
> - /* Also unlocks the slave_mutex */
> - res = vu_process_message_reply(dev, &vmsg, &payload);
> + res = vu_message_slave_send_receive(dev, &vmsg, &payload);
> if (!res) {
> return -EIO;
> }
> +
> /*
> * Payload is delivered as uint64_t but is actually signed for
> * errors.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 2/6] libvhost-user: Use slave_mutex in all slave messages
2021-01-28 14:31 ` [Virtio-fs] " Greg Kurz
@ 2021-01-28 14:48 ` Vivek Goyal
-1 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-28 14:48 UTC (permalink / raw)
To: Greg Kurz
Cc: mst, qemu-devel, dgilbert, virtio-fs, stefanha, marcandre.lureau
On Thu, Jan 28, 2021 at 03:31:23PM +0100, Greg Kurz wrote:
> On Mon, 25 Jan 2021 13:01:11 -0500
> Vivek Goyal <vgoyal@redhat.com> wrote:
>
> > dev->slave_mutex needs to be taken when sending messages on slave_fd.
> > Currently _vu_queue_notify() does not do that.
> >
> > Introduce a helper vu_message_slave_send_receive() which sends as well
> > as receive response. Use this helper in all the paths which send
> > message on slave_fd channel.
> >
>
> Does this fix any known bug ?
I am not aware of any bug. This fix is based on code inspection.
Also I wanted a central place/function to send messages on slave channel
so that I can check state of slave channel (open/close) and act
accordingly. Otherwise I will have to do the check at every place
which is trying to send/receive message on slave channel.
Vivek
>
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
>
> LGTM
>
> Reviewed-by: Greg Kurz <groug@kaod.org>
>
> > subprojects/libvhost-user/libvhost-user.c | 50 ++++++++++++-----------
> > 1 file changed, 27 insertions(+), 23 deletions(-)
> >
> > diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> > index 4cf4aef63d..7a56c56dc8 100644
> > --- a/subprojects/libvhost-user/libvhost-user.c
> > +++ b/subprojects/libvhost-user/libvhost-user.c
> > @@ -403,7 +403,7 @@ vu_send_reply(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
> > * Processes a reply on the slave channel.
> > * Entered with slave_mutex held and releases it before exit.
> > * Returns true on success.
> > - * *payload is written on success
> > + * *payload is written on success, if payload is not NULL.
> > */
> > static bool
> > vu_process_message_reply(VuDev *dev, const VhostUserMsg *vmsg,
> > @@ -427,7 +427,9 @@ vu_process_message_reply(VuDev *dev, const VhostUserMsg *vmsg,
> > goto out;
> > }
> >
> > - *payload = msg_reply.payload.u64;
> > + if (payload) {
> > + *payload = msg_reply.payload.u64;
> > + }
> > result = true;
> >
> > out:
> > @@ -435,6 +437,25 @@ out:
> > return result;
> > }
> >
> > +/* Returns true on success, false otherwise */
> > +static bool
> > +vu_message_slave_send_receive(VuDev *dev, VhostUserMsg *vmsg, uint64_t *payload)
> > +{
> > + pthread_mutex_lock(&dev->slave_mutex);
> > + if (!vu_message_write(dev, dev->slave_fd, vmsg)) {
> > + pthread_mutex_unlock(&dev->slave_mutex);
> > + return false;
> > + }
> > +
> > + if ((vmsg->flags & VHOST_USER_NEED_REPLY_MASK) == 0) {
> > + pthread_mutex_unlock(&dev->slave_mutex);
> > + return true;
> > + }
> > +
> > + /* Also unlocks the slave_mutex */
> > + return vu_process_message_reply(dev, vmsg, payload);
> > +}
> > +
> > /* Kick the log_call_fd if required. */
> > static void
> > vu_log_kick(VuDev *dev)
> > @@ -1340,16 +1361,8 @@ bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq *vq, int fd,
> > return false;
> > }
> >
> > - pthread_mutex_lock(&dev->slave_mutex);
> > - if (!vu_message_write(dev, dev->slave_fd, &vmsg)) {
> > - pthread_mutex_unlock(&dev->slave_mutex);
> > - return false;
> > - }
> > -
> > - /* Also unlocks the slave_mutex */
> > - res = vu_process_message_reply(dev, &vmsg, &payload);
> > + res = vu_message_slave_send_receive(dev, &vmsg, &payload);
> > res = res && (payload == 0);
> > -
> > return res;
> > }
> >
> > @@ -2395,10 +2408,7 @@ static void _vu_queue_notify(VuDev *dev, VuVirtq *vq, bool sync)
> > vmsg.flags |= VHOST_USER_NEED_REPLY_MASK;
> > }
> >
> > - vu_message_write(dev, dev->slave_fd, &vmsg);
> > - if (ack) {
> > - vu_message_read_default(dev, dev->slave_fd, &vmsg);
> > - }
> > + vu_message_slave_send_receive(dev, &vmsg, NULL);
> > return;
> > }
> >
> > @@ -2942,17 +2952,11 @@ int64_t vu_fs_cache_request(VuDev *dev, VhostUserSlaveRequest req, int fd,
> > return -EINVAL;
> > }
> >
> > - pthread_mutex_lock(&dev->slave_mutex);
> > - if (!vu_message_write(dev, dev->slave_fd, &vmsg)) {
> > - pthread_mutex_unlock(&dev->slave_mutex);
> > - return -EIO;
> > - }
> > -
> > - /* Also unlocks the slave_mutex */
> > - res = vu_process_message_reply(dev, &vmsg, &payload);
> > + res = vu_message_slave_send_receive(dev, &vmsg, &payload);
> > if (!res) {
> > return -EIO;
> > }
> > +
> > /*
> > * Payload is delivered as uint64_t but is actually signed for
> > * errors.
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Virtio-fs] [PATCH 2/6] libvhost-user: Use slave_mutex in all slave messages
@ 2021-01-28 14:48 ` Vivek Goyal
0 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-28 14:48 UTC (permalink / raw)
To: Greg Kurz; +Cc: mst, qemu-devel, virtio-fs, marcandre.lureau
On Thu, Jan 28, 2021 at 03:31:23PM +0100, Greg Kurz wrote:
> On Mon, 25 Jan 2021 13:01:11 -0500
> Vivek Goyal <vgoyal@redhat.com> wrote:
>
> > dev->slave_mutex needs to be taken when sending messages on slave_fd.
> > Currently _vu_queue_notify() does not do that.
> >
> > Introduce a helper vu_message_slave_send_receive() which sends as well
> > as receive response. Use this helper in all the paths which send
> > message on slave_fd channel.
> >
>
> Does this fix any known bug ?
I am not aware of any bug. This fix is based on code inspection.
Also I wanted a central place/function to send messages on slave channel
so that I can check state of slave channel (open/close) and act
accordingly. Otherwise I will have to do the check at every place
which is trying to send/receive message on slave channel.
Vivek
>
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
>
> LGTM
>
> Reviewed-by: Greg Kurz <groug@kaod.org>
>
> > subprojects/libvhost-user/libvhost-user.c | 50 ++++++++++++-----------
> > 1 file changed, 27 insertions(+), 23 deletions(-)
> >
> > diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> > index 4cf4aef63d..7a56c56dc8 100644
> > --- a/subprojects/libvhost-user/libvhost-user.c
> > +++ b/subprojects/libvhost-user/libvhost-user.c
> > @@ -403,7 +403,7 @@ vu_send_reply(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
> > * Processes a reply on the slave channel.
> > * Entered with slave_mutex held and releases it before exit.
> > * Returns true on success.
> > - * *payload is written on success
> > + * *payload is written on success, if payload is not NULL.
> > */
> > static bool
> > vu_process_message_reply(VuDev *dev, const VhostUserMsg *vmsg,
> > @@ -427,7 +427,9 @@ vu_process_message_reply(VuDev *dev, const VhostUserMsg *vmsg,
> > goto out;
> > }
> >
> > - *payload = msg_reply.payload.u64;
> > + if (payload) {
> > + *payload = msg_reply.payload.u64;
> > + }
> > result = true;
> >
> > out:
> > @@ -435,6 +437,25 @@ out:
> > return result;
> > }
> >
> > +/* Returns true on success, false otherwise */
> > +static bool
> > +vu_message_slave_send_receive(VuDev *dev, VhostUserMsg *vmsg, uint64_t *payload)
> > +{
> > + pthread_mutex_lock(&dev->slave_mutex);
> > + if (!vu_message_write(dev, dev->slave_fd, vmsg)) {
> > + pthread_mutex_unlock(&dev->slave_mutex);
> > + return false;
> > + }
> > +
> > + if ((vmsg->flags & VHOST_USER_NEED_REPLY_MASK) == 0) {
> > + pthread_mutex_unlock(&dev->slave_mutex);
> > + return true;
> > + }
> > +
> > + /* Also unlocks the slave_mutex */
> > + return vu_process_message_reply(dev, vmsg, payload);
> > +}
> > +
> > /* Kick the log_call_fd if required. */
> > static void
> > vu_log_kick(VuDev *dev)
> > @@ -1340,16 +1361,8 @@ bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq *vq, int fd,
> > return false;
> > }
> >
> > - pthread_mutex_lock(&dev->slave_mutex);
> > - if (!vu_message_write(dev, dev->slave_fd, &vmsg)) {
> > - pthread_mutex_unlock(&dev->slave_mutex);
> > - return false;
> > - }
> > -
> > - /* Also unlocks the slave_mutex */
> > - res = vu_process_message_reply(dev, &vmsg, &payload);
> > + res = vu_message_slave_send_receive(dev, &vmsg, &payload);
> > res = res && (payload == 0);
> > -
> > return res;
> > }
> >
> > @@ -2395,10 +2408,7 @@ static void _vu_queue_notify(VuDev *dev, VuVirtq *vq, bool sync)
> > vmsg.flags |= VHOST_USER_NEED_REPLY_MASK;
> > }
> >
> > - vu_message_write(dev, dev->slave_fd, &vmsg);
> > - if (ack) {
> > - vu_message_read_default(dev, dev->slave_fd, &vmsg);
> > - }
> > + vu_message_slave_send_receive(dev, &vmsg, NULL);
> > return;
> > }
> >
> > @@ -2942,17 +2952,11 @@ int64_t vu_fs_cache_request(VuDev *dev, VhostUserSlaveRequest req, int fd,
> > return -EINVAL;
> > }
> >
> > - pthread_mutex_lock(&dev->slave_mutex);
> > - if (!vu_message_write(dev, dev->slave_fd, &vmsg)) {
> > - pthread_mutex_unlock(&dev->slave_mutex);
> > - return -EIO;
> > - }
> > -
> > - /* Also unlocks the slave_mutex */
> > - res = vu_process_message_reply(dev, &vmsg, &payload);
> > + res = vu_message_slave_send_receive(dev, &vmsg, &payload);
> > if (!res) {
> > return -EIO;
> > }
> > +
> > /*
> > * Payload is delivered as uint64_t but is actually signed for
> > * errors.
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 2/6] libvhost-user: Use slave_mutex in all slave messages
2021-01-28 14:48 ` [Virtio-fs] " Vivek Goyal
@ 2021-01-28 15:06 ` Greg Kurz
-1 siblings, 0 replies; 52+ messages in thread
From: Greg Kurz @ 2021-01-28 15:06 UTC (permalink / raw)
To: Vivek Goyal
Cc: mst, qemu-devel, dgilbert, virtio-fs, stefanha, marcandre.lureau
On Thu, 28 Jan 2021 09:48:35 -0500
Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Jan 28, 2021 at 03:31:23PM +0100, Greg Kurz wrote:
> > On Mon, 25 Jan 2021 13:01:11 -0500
> > Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > > dev->slave_mutex needs to be taken when sending messages on slave_fd.
> > > Currently _vu_queue_notify() does not do that.
> > >
> > > Introduce a helper vu_message_slave_send_receive() which sends as well
> > > as receive response. Use this helper in all the paths which send
> > > message on slave_fd channel.
> > >
> >
> > Does this fix any known bug ?
>
> I am not aware of any bug. This fix is based on code inspection.
>
> Also I wanted a central place/function to send messages on slave channel
> so that I can check state of slave channel (open/close) and act
> accordingly. Otherwise I will have to do the check at every place
> which is trying to send/receive message on slave channel.
>
Makes sense. Thanks for the clarification.
> Vivek
>
> >
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> >
> > LGTM
> >
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> >
> > > subprojects/libvhost-user/libvhost-user.c | 50 ++++++++++++-----------
> > > 1 file changed, 27 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> > > index 4cf4aef63d..7a56c56dc8 100644
> > > --- a/subprojects/libvhost-user/libvhost-user.c
> > > +++ b/subprojects/libvhost-user/libvhost-user.c
> > > @@ -403,7 +403,7 @@ vu_send_reply(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
> > > * Processes a reply on the slave channel.
> > > * Entered with slave_mutex held and releases it before exit.
> > > * Returns true on success.
> > > - * *payload is written on success
> > > + * *payload is written on success, if payload is not NULL.
> > > */
> > > static bool
> > > vu_process_message_reply(VuDev *dev, const VhostUserMsg *vmsg,
> > > @@ -427,7 +427,9 @@ vu_process_message_reply(VuDev *dev, const VhostUserMsg *vmsg,
> > > goto out;
> > > }
> > >
> > > - *payload = msg_reply.payload.u64;
> > > + if (payload) {
> > > + *payload = msg_reply.payload.u64;
> > > + }
> > > result = true;
> > >
> > > out:
> > > @@ -435,6 +437,25 @@ out:
> > > return result;
> > > }
> > >
> > > +/* Returns true on success, false otherwise */
> > > +static bool
> > > +vu_message_slave_send_receive(VuDev *dev, VhostUserMsg *vmsg, uint64_t *payload)
> > > +{
> > > + pthread_mutex_lock(&dev->slave_mutex);
> > > + if (!vu_message_write(dev, dev->slave_fd, vmsg)) {
> > > + pthread_mutex_unlock(&dev->slave_mutex);
> > > + return false;
> > > + }
> > > +
> > > + if ((vmsg->flags & VHOST_USER_NEED_REPLY_MASK) == 0) {
> > > + pthread_mutex_unlock(&dev->slave_mutex);
> > > + return true;
> > > + }
> > > +
> > > + /* Also unlocks the slave_mutex */
> > > + return vu_process_message_reply(dev, vmsg, payload);
> > > +}
> > > +
> > > /* Kick the log_call_fd if required. */
> > > static void
> > > vu_log_kick(VuDev *dev)
> > > @@ -1340,16 +1361,8 @@ bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq *vq, int fd,
> > > return false;
> > > }
> > >
> > > - pthread_mutex_lock(&dev->slave_mutex);
> > > - if (!vu_message_write(dev, dev->slave_fd, &vmsg)) {
> > > - pthread_mutex_unlock(&dev->slave_mutex);
> > > - return false;
> > > - }
> > > -
> > > - /* Also unlocks the slave_mutex */
> > > - res = vu_process_message_reply(dev, &vmsg, &payload);
> > > + res = vu_message_slave_send_receive(dev, &vmsg, &payload);
> > > res = res && (payload == 0);
> > > -
> > > return res;
> > > }
> > >
> > > @@ -2395,10 +2408,7 @@ static void _vu_queue_notify(VuDev *dev, VuVirtq *vq, bool sync)
> > > vmsg.flags |= VHOST_USER_NEED_REPLY_MASK;
> > > }
> > >
> > > - vu_message_write(dev, dev->slave_fd, &vmsg);
> > > - if (ack) {
> > > - vu_message_read_default(dev, dev->slave_fd, &vmsg);
> > > - }
> > > + vu_message_slave_send_receive(dev, &vmsg, NULL);
> > > return;
> > > }
> > >
> > > @@ -2942,17 +2952,11 @@ int64_t vu_fs_cache_request(VuDev *dev, VhostUserSlaveRequest req, int fd,
> > > return -EINVAL;
> > > }
> > >
> > > - pthread_mutex_lock(&dev->slave_mutex);
> > > - if (!vu_message_write(dev, dev->slave_fd, &vmsg)) {
> > > - pthread_mutex_unlock(&dev->slave_mutex);
> > > - return -EIO;
> > > - }
> > > -
> > > - /* Also unlocks the slave_mutex */
> > > - res = vu_process_message_reply(dev, &vmsg, &payload);
> > > + res = vu_message_slave_send_receive(dev, &vmsg, &payload);
> > > if (!res) {
> > > return -EIO;
> > > }
> > > +
> > > /*
> > > * Payload is delivered as uint64_t but is actually signed for
> > > * errors.
> >
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Virtio-fs] [PATCH 2/6] libvhost-user: Use slave_mutex in all slave messages
@ 2021-01-28 15:06 ` Greg Kurz
0 siblings, 0 replies; 52+ messages in thread
From: Greg Kurz @ 2021-01-28 15:06 UTC (permalink / raw)
To: Vivek Goyal; +Cc: mst, qemu-devel, virtio-fs, marcandre.lureau
On Thu, 28 Jan 2021 09:48:35 -0500
Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Jan 28, 2021 at 03:31:23PM +0100, Greg Kurz wrote:
> > On Mon, 25 Jan 2021 13:01:11 -0500
> > Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > > dev->slave_mutex needs to be taken when sending messages on slave_fd.
> > > Currently _vu_queue_notify() does not do that.
> > >
> > > Introduce a helper vu_message_slave_send_receive() which sends as well
> > > as receive response. Use this helper in all the paths which send
> > > message on slave_fd channel.
> > >
> >
> > Does this fix any known bug ?
>
> I am not aware of any bug. This fix is based on code inspection.
>
> Also I wanted a central place/function to send messages on slave channel
> so that I can check state of slave channel (open/close) and act
> accordingly. Otherwise I will have to do the check at every place
> which is trying to send/receive message on slave channel.
>
Makes sense. Thanks for the clarification.
> Vivek
>
> >
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> >
> > LGTM
> >
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> >
> > > subprojects/libvhost-user/libvhost-user.c | 50 ++++++++++++-----------
> > > 1 file changed, 27 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> > > index 4cf4aef63d..7a56c56dc8 100644
> > > --- a/subprojects/libvhost-user/libvhost-user.c
> > > +++ b/subprojects/libvhost-user/libvhost-user.c
> > > @@ -403,7 +403,7 @@ vu_send_reply(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
> > > * Processes a reply on the slave channel.
> > > * Entered with slave_mutex held and releases it before exit.
> > > * Returns true on success.
> > > - * *payload is written on success
> > > + * *payload is written on success, if payload is not NULL.
> > > */
> > > static bool
> > > vu_process_message_reply(VuDev *dev, const VhostUserMsg *vmsg,
> > > @@ -427,7 +427,9 @@ vu_process_message_reply(VuDev *dev, const VhostUserMsg *vmsg,
> > > goto out;
> > > }
> > >
> > > - *payload = msg_reply.payload.u64;
> > > + if (payload) {
> > > + *payload = msg_reply.payload.u64;
> > > + }
> > > result = true;
> > >
> > > out:
> > > @@ -435,6 +437,25 @@ out:
> > > return result;
> > > }
> > >
> > > +/* Returns true on success, false otherwise */
> > > +static bool
> > > +vu_message_slave_send_receive(VuDev *dev, VhostUserMsg *vmsg, uint64_t *payload)
> > > +{
> > > + pthread_mutex_lock(&dev->slave_mutex);
> > > + if (!vu_message_write(dev, dev->slave_fd, vmsg)) {
> > > + pthread_mutex_unlock(&dev->slave_mutex);
> > > + return false;
> > > + }
> > > +
> > > + if ((vmsg->flags & VHOST_USER_NEED_REPLY_MASK) == 0) {
> > > + pthread_mutex_unlock(&dev->slave_mutex);
> > > + return true;
> > > + }
> > > +
> > > + /* Also unlocks the slave_mutex */
> > > + return vu_process_message_reply(dev, vmsg, payload);
> > > +}
> > > +
> > > /* Kick the log_call_fd if required. */
> > > static void
> > > vu_log_kick(VuDev *dev)
> > > @@ -1340,16 +1361,8 @@ bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq *vq, int fd,
> > > return false;
> > > }
> > >
> > > - pthread_mutex_lock(&dev->slave_mutex);
> > > - if (!vu_message_write(dev, dev->slave_fd, &vmsg)) {
> > > - pthread_mutex_unlock(&dev->slave_mutex);
> > > - return false;
> > > - }
> > > -
> > > - /* Also unlocks the slave_mutex */
> > > - res = vu_process_message_reply(dev, &vmsg, &payload);
> > > + res = vu_message_slave_send_receive(dev, &vmsg, &payload);
> > > res = res && (payload == 0);
> > > -
> > > return res;
> > > }
> > >
> > > @@ -2395,10 +2408,7 @@ static void _vu_queue_notify(VuDev *dev, VuVirtq *vq, bool sync)
> > > vmsg.flags |= VHOST_USER_NEED_REPLY_MASK;
> > > }
> > >
> > > - vu_message_write(dev, dev->slave_fd, &vmsg);
> > > - if (ack) {
> > > - vu_message_read_default(dev, dev->slave_fd, &vmsg);
> > > - }
> > > + vu_message_slave_send_receive(dev, &vmsg, NULL);
> > > return;
> > > }
> > >
> > > @@ -2942,17 +2952,11 @@ int64_t vu_fs_cache_request(VuDev *dev, VhostUserSlaveRequest req, int fd,
> > > return -EINVAL;
> > > }
> > >
> > > - pthread_mutex_lock(&dev->slave_mutex);
> > > - if (!vu_message_write(dev, dev->slave_fd, &vmsg)) {
> > > - pthread_mutex_unlock(&dev->slave_mutex);
> > > - return -EIO;
> > > - }
> > > -
> > > - /* Also unlocks the slave_mutex */
> > > - res = vu_process_message_reply(dev, &vmsg, &payload);
> > > + res = vu_message_slave_send_receive(dev, &vmsg, &payload);
> > > if (!res) {
> > > return -EIO;
> > > }
> > > +
> > > /*
> > > * Payload is delivered as uint64_t but is actually signed for
> > > * errors.
> >
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 4/6] qemu, vhost-user: Extend protocol to start/stop/flush slave channel
2021-01-25 18:01 ` [Virtio-fs] " Vivek Goyal
@ 2021-01-28 16:52 ` Stefan Hajnoczi
-1 siblings, 0 replies; 52+ messages in thread
From: Stefan Hajnoczi @ 2021-01-28 16:52 UTC (permalink / raw)
To: Vivek Goyal; +Cc: virtio-fs, marcandre.lureau, mst, qemu-devel, dgilbert
[-- Attachment #1: Type: text/plain, Size: 3085 bytes --]
On Mon, Jan 25, 2021 at 01:01:13PM -0500, Vivek Goyal wrote:
> Currently we don't have a mechanism to flush slave channel while shutting
> down vhost-user device and that can result a deadlock. Consider following
> scenario.
>
> 1. Slave gets a request from guest on virtqueue (say index 1, vq1), to map
> a portion of file in qemu address space.
>
> 2. Thread serving vq1 receives this request and sends a message to qemu on
> slave channel/fd and gets blocked in waiting for a response from qemu.
>
> 3. In the mean time, user does "echo b > /proc/sysrq-trigger" in guest. This
> leads to qemu reset and ultimately in main thread we end up in
> vhost_dev_stop() which is trying to shutdown virtqueues for the device.
>
> 4. Slave gets VHOST_USER_GET_VRING_BASE message to shutdown a virtqueue on
> unix socket being used for communication.
>
> 5. Slave tries to shutdown the thread serving vq1 and waits for it to
> terminate. But vq1 thread can't terminate because it is waiting for
> a response from qemu on slave_fd. And qemu is not processing slave_fd
> anymore as qemu is ressing (and not running main event loop anymore)
> and is waiting for a response to VHOST_USER_GET_VRING_BASE.
>
> So we have a deadlock. Qemu is waiting on slave to response to
> VHOST_USER_GET_VRING_BASE message and slave is waiting on qemu to
> respond to request it sent on slave_fd.
>
> I can reliably reproduce this race with virtio-fs.
>
> Hence here is the proposal to solve this problem. Enhance vhost-user
> protocol to properly shutdown slave_fd channel. And if there are pending
> requests, flush the channel completely before sending the request to
> shutdown virtqueues.
>
> New workflow to shutdown slave channel is.
>
> - Qemu sends VHOST_USER_STOP_SLAVE_CHANNEL request to slave. It waits
> for an reply from guest.
>
> - Then qemu sits in a tight loop waiting for
> VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE message from slave on slave_fd.
> And while waiting for this message, qemu continues to process requests
> on slave_fd to flush any pending requests. This will unblock threads
> in slave and allow slave to shutdown slave channel.
>
> - Once qemu gets VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE message, it knows
> no more requests will come on slave_fd and it can continue to shutdown
> virtqueues now.
>
> - Once device starts again, qemu will send VHOST_USER_START_SLAVE_CHANNEL
> message to slave to open the slave channel and receive requests.
>
> IOW, this allows for proper shutdown of slave channel, making sure
> no threads in slave are blocked on sending/receiving message. And
> this in-turn allows for shutting down of virtqueues, hence resolving
> the deadlock.
Is the new message necessary? How about letting QEMU handle slave fd
activity while waiting for virtqueues to stop instead?
In other words, QEMU should monitor both the UNIX domain socket and the
slave fd after it has sent VHOST_USER_GET_VRING_BASE and is awaiting the
response.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Virtio-fs] [PATCH 4/6] qemu, vhost-user: Extend protocol to start/stop/flush slave channel
@ 2021-01-28 16:52 ` Stefan Hajnoczi
0 siblings, 0 replies; 52+ messages in thread
From: Stefan Hajnoczi @ 2021-01-28 16:52 UTC (permalink / raw)
To: Vivek Goyal; +Cc: virtio-fs, marcandre.lureau, mst, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3085 bytes --]
On Mon, Jan 25, 2021 at 01:01:13PM -0500, Vivek Goyal wrote:
> Currently we don't have a mechanism to flush slave channel while shutting
> down vhost-user device and that can result a deadlock. Consider following
> scenario.
>
> 1. Slave gets a request from guest on virtqueue (say index 1, vq1), to map
> a portion of file in qemu address space.
>
> 2. Thread serving vq1 receives this request and sends a message to qemu on
> slave channel/fd and gets blocked in waiting for a response from qemu.
>
> 3. In the mean time, user does "echo b > /proc/sysrq-trigger" in guest. This
> leads to qemu reset and ultimately in main thread we end up in
> vhost_dev_stop() which is trying to shutdown virtqueues for the device.
>
> 4. Slave gets VHOST_USER_GET_VRING_BASE message to shutdown a virtqueue on
> unix socket being used for communication.
>
> 5. Slave tries to shutdown the thread serving vq1 and waits for it to
> terminate. But vq1 thread can't terminate because it is waiting for
> a response from qemu on slave_fd. And qemu is not processing slave_fd
> anymore as qemu is ressing (and not running main event loop anymore)
> and is waiting for a response to VHOST_USER_GET_VRING_BASE.
>
> So we have a deadlock. Qemu is waiting on slave to response to
> VHOST_USER_GET_VRING_BASE message and slave is waiting on qemu to
> respond to request it sent on slave_fd.
>
> I can reliably reproduce this race with virtio-fs.
>
> Hence here is the proposal to solve this problem. Enhance vhost-user
> protocol to properly shutdown slave_fd channel. And if there are pending
> requests, flush the channel completely before sending the request to
> shutdown virtqueues.
>
> New workflow to shutdown slave channel is.
>
> - Qemu sends VHOST_USER_STOP_SLAVE_CHANNEL request to slave. It waits
> for an reply from guest.
>
> - Then qemu sits in a tight loop waiting for
> VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE message from slave on slave_fd.
> And while waiting for this message, qemu continues to process requests
> on slave_fd to flush any pending requests. This will unblock threads
> in slave and allow slave to shutdown slave channel.
>
> - Once qemu gets VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE message, it knows
> no more requests will come on slave_fd and it can continue to shutdown
> virtqueues now.
>
> - Once device starts again, qemu will send VHOST_USER_START_SLAVE_CHANNEL
> message to slave to open the slave channel and receive requests.
>
> IOW, this allows for proper shutdown of slave channel, making sure
> no threads in slave are blocked on sending/receiving message. And
> this in-turn allows for shutting down of virtqueues, hence resolving
> the deadlock.
Is the new message necessary? How about letting QEMU handle slave fd
activity while waiting for virtqueues to stop instead?
In other words, QEMU should monitor both the UNIX domain socket and the
slave fd after it has sent VHOST_USER_GET_VRING_BASE and is awaiting the
response.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 3/6] vhost-user: Return error code from slave_read()
2021-01-25 18:01 ` [Virtio-fs] " Vivek Goyal
@ 2021-01-29 9:45 ` Greg Kurz
-1 siblings, 0 replies; 52+ messages in thread
From: Greg Kurz @ 2021-01-29 9:45 UTC (permalink / raw)
To: Vivek Goyal
Cc: mst, qemu-devel, dgilbert, virtio-fs, stefanha, marcandre.lureau
On Mon, 25 Jan 2021 13:01:12 -0500
Vivek Goyal <vgoyal@redhat.com> wrote:
> Right now slave_read() is called through main event loop and does not
> return error. In next few patches I want to call slave_read() from
> vhost device shutdown path as well and want to know if an error
> happened so that caller can give up and return error accordingly.
>
> Hence, create helper function do_slave_read(), which returns an
> integer. Success is 0 and negative number is error code. slave_read()
> calls do_slave_read() and ignores error code.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
> hw/virtio/vhost-user.c | 43 ++++++++++++++++++++++++++++++++++++------
> 1 file changed, 37 insertions(+), 6 deletions(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index d95dbc39e3..867cac034f 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1401,7 +1401,7 @@ static uint64_t vhost_user_slave_handle_vring_host_notifier(
> return false;
> }
>
> -static void slave_read(void *opaque)
> +static int do_slave_read(void *opaque)
> {
> struct vhost_dev *dev = opaque;
> struct vhost_user *u = dev->opaque;
> @@ -1432,13 +1432,22 @@ static void slave_read(void *opaque)
> size = recvmsg(u->slave_fd, &msgh, 0);
> } while (size < 0 && (errno == EINTR || errno == EAGAIN));
>
> - if (size != VHOST_USER_HDR_SIZE) {
> + if (size < 0) {
> + ret = -errno;
> error_report("Failed to read from slave.");
> goto err;
> }
>
> + if (size != VHOST_USER_HDR_SIZE) {
> + error_report("Failed to read %lu bytes from slave.",
> + VHOST_USER_HDR_SIZE);
Maybe also print size ?
And, question from a newbie : any idea why short reads are
considered as errors instead of retrying ? Same question
stands for the other locations where we check the numbers
of read/written bytes in this function.
> + ret = -EBADMSG;
> + goto err;
> + }
> +
> if (msgh.msg_flags & MSG_CTRUNC) {
> error_report("Truncated message.");
> + ret = -EBADMSG;
> goto err;
> }
>
> @@ -1456,6 +1465,7 @@ static void slave_read(void *opaque)
> error_report("Failed to read msg header."
> " Size %d exceeds the maximum %zu.", hdr.size,
> VHOST_USER_PAYLOAD_SIZE);
> + ret = -EBADMSG;
> goto err;
> }
>
> @@ -1464,8 +1474,15 @@ static void slave_read(void *opaque)
> size = read(u->slave_fd, &payload, hdr.size);
> } while (size < 0 && (errno == EINTR || errno == EAGAIN));
>
> - if (size != hdr.size) {
> + if (size == -1) {
Maybe make it (size < 0) for consistency with the error checking
added above ? And this seems to be the preferred way in the QEMU
tree :)
> error_report("Failed to read payload from slave.");
> + ret = errno;
ret = -errno;
And this should be done before error_report() to ensure errno
isn't cloberred.
> + goto err;
> + }
> +
> + if (size != hdr.size) {
> + error_report("Failed to read %d payload bytes from slave.", hdr.size);
> + ret = -EBADMSG;
> goto err;
> }
>
> @@ -1529,13 +1546,22 @@ static void slave_read(void *opaque)
> size = writev(u->slave_fd, iovec, ARRAY_SIZE(iovec));
> } while (size < 0 && (errno == EINTR || errno == EAGAIN));
>
> - if (size != VHOST_USER_HDR_SIZE + hdr.size) {
> + if (size == -1) {
size < 0
> error_report("Failed to send msg reply to slave.");
> + ret = -errno;
Move before error_report()
> + goto err;
> + }
> +
> + if (size != VHOST_USER_HDR_SIZE + hdr.size) {
> + error_report("Failed to send msg reply to slave. Wrote %d bytes"
> + " expected %lu bytes.", size,
> + VHOST_USER_HDR_SIZE + hdr.size);
> + ret = -EIO;
> goto err;
> }
> }
>
> - return;
> + return 0;
>
> err:
> qemu_set_fd_handler(u->slave_fd, NULL, NULL, NULL);
> @@ -1546,7 +1572,12 @@ err:
> close(fd[i]);
> }
> }
> - return;
> + return ret;
> +}
> +
> +static void slave_read(void *opaque)
> +{
> + do_slave_read(opaque);
> }
>
> static int vhost_setup_slave_channel(struct vhost_dev *dev)
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Virtio-fs] [PATCH 3/6] vhost-user: Return error code from slave_read()
@ 2021-01-29 9:45 ` Greg Kurz
0 siblings, 0 replies; 52+ messages in thread
From: Greg Kurz @ 2021-01-29 9:45 UTC (permalink / raw)
To: Vivek Goyal; +Cc: mst, qemu-devel, virtio-fs, marcandre.lureau
On Mon, 25 Jan 2021 13:01:12 -0500
Vivek Goyal <vgoyal@redhat.com> wrote:
> Right now slave_read() is called through main event loop and does not
> return error. In next few patches I want to call slave_read() from
> vhost device shutdown path as well and want to know if an error
> happened so that caller can give up and return error accordingly.
>
> Hence, create helper function do_slave_read(), which returns an
> integer. Success is 0 and negative number is error code. slave_read()
> calls do_slave_read() and ignores error code.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
> hw/virtio/vhost-user.c | 43 ++++++++++++++++++++++++++++++++++++------
> 1 file changed, 37 insertions(+), 6 deletions(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index d95dbc39e3..867cac034f 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1401,7 +1401,7 @@ static uint64_t vhost_user_slave_handle_vring_host_notifier(
> return false;
> }
>
> -static void slave_read(void *opaque)
> +static int do_slave_read(void *opaque)
> {
> struct vhost_dev *dev = opaque;
> struct vhost_user *u = dev->opaque;
> @@ -1432,13 +1432,22 @@ static void slave_read(void *opaque)
> size = recvmsg(u->slave_fd, &msgh, 0);
> } while (size < 0 && (errno == EINTR || errno == EAGAIN));
>
> - if (size != VHOST_USER_HDR_SIZE) {
> + if (size < 0) {
> + ret = -errno;
> error_report("Failed to read from slave.");
> goto err;
> }
>
> + if (size != VHOST_USER_HDR_SIZE) {
> + error_report("Failed to read %lu bytes from slave.",
> + VHOST_USER_HDR_SIZE);
Maybe also print size ?
And, question from a newbie : any idea why short reads are
considered as errors instead of retrying ? Same question
stands for the other locations where we check the numbers
of read/written bytes in this function.
> + ret = -EBADMSG;
> + goto err;
> + }
> +
> if (msgh.msg_flags & MSG_CTRUNC) {
> error_report("Truncated message.");
> + ret = -EBADMSG;
> goto err;
> }
>
> @@ -1456,6 +1465,7 @@ static void slave_read(void *opaque)
> error_report("Failed to read msg header."
> " Size %d exceeds the maximum %zu.", hdr.size,
> VHOST_USER_PAYLOAD_SIZE);
> + ret = -EBADMSG;
> goto err;
> }
>
> @@ -1464,8 +1474,15 @@ static void slave_read(void *opaque)
> size = read(u->slave_fd, &payload, hdr.size);
> } while (size < 0 && (errno == EINTR || errno == EAGAIN));
>
> - if (size != hdr.size) {
> + if (size == -1) {
Maybe make it (size < 0) for consistency with the error checking
added above ? And this seems to be the preferred way in the QEMU
tree :)
> error_report("Failed to read payload from slave.");
> + ret = errno;
ret = -errno;
And this should be done before error_report() to ensure errno
isn't cloberred.
> + goto err;
> + }
> +
> + if (size != hdr.size) {
> + error_report("Failed to read %d payload bytes from slave.", hdr.size);
> + ret = -EBADMSG;
> goto err;
> }
>
> @@ -1529,13 +1546,22 @@ static void slave_read(void *opaque)
> size = writev(u->slave_fd, iovec, ARRAY_SIZE(iovec));
> } while (size < 0 && (errno == EINTR || errno == EAGAIN));
>
> - if (size != VHOST_USER_HDR_SIZE + hdr.size) {
> + if (size == -1) {
size < 0
> error_report("Failed to send msg reply to slave.");
> + ret = -errno;
Move before error_report()
> + goto err;
> + }
> +
> + if (size != VHOST_USER_HDR_SIZE + hdr.size) {
> + error_report("Failed to send msg reply to slave. Wrote %d bytes"
> + " expected %lu bytes.", size,
> + VHOST_USER_HDR_SIZE + hdr.size);
> + ret = -EIO;
> goto err;
> }
> }
>
> - return;
> + return 0;
>
> err:
> qemu_set_fd_handler(u->slave_fd, NULL, NULL, NULL);
> @@ -1546,7 +1572,12 @@ err:
> close(fd[i]);
> }
> }
> - return;
> + return ret;
> +}
> +
> +static void slave_read(void *opaque)
> +{
> + do_slave_read(opaque);
> }
>
> static int vhost_setup_slave_channel(struct vhost_dev *dev)
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 1/6] virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
2021-01-26 18:33 ` [Virtio-fs] " Vivek Goyal
@ 2021-01-29 12:03 ` Greg Kurz
-1 siblings, 0 replies; 52+ messages in thread
From: Greg Kurz @ 2021-01-29 12:03 UTC (permalink / raw)
To: Vivek Goyal
Cc: mst, qemu-devel, dgilbert, virtio-fs, stefanha, marcandre.lureau
On Tue, 26 Jan 2021 13:33:36 -0500
Vivek Goyal <vgoyal@redhat.com> wrote:
[...]
> >
> > Also, since pthread_rwlock_wrlock() can fail, I think we should
> > always check it's return value, at least with an assert() like
> > already done elsewhere.
>
> Will check return code of pthread_rwlock_wrlock() and probably use
> assert().
>
It turns out that pthread_rwlock_rdlock() and pthread_rwlock_unlock() can
also fail for various reasons that would likely indicate a programming
error, but their return values are never checked anywhere.
I have a patch to address this globally in this file. Should I post it
now or you prefer this series goes first ?
> Vivek
>
> >
> > > for (int i = 0; i < se->virtio_dev->nqueues; i++) {
> > > if (!se->virtio_dev->qi[i]) {
> > > continue;
> > > @@ -961,6 +974,7 @@ int virtio_loop(struct fuse_session *se)
> > > fuse_log(FUSE_LOG_INFO, "%s: Stopping queue %d thread\n", __func__, i);
> > > fv_queue_cleanup_thread(se->virtio_dev, i);
> > > }
> > > + pthread_rwlock_unlock(&se->virtio_dev->vu_dispatch_rwlock);
> > >
> > > fuse_log(FUSE_LOG_INFO, "%s: Exit\n", __func__);
> > >
> >
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Virtio-fs] [PATCH 1/6] virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
@ 2021-01-29 12:03 ` Greg Kurz
0 siblings, 0 replies; 52+ messages in thread
From: Greg Kurz @ 2021-01-29 12:03 UTC (permalink / raw)
To: Vivek Goyal; +Cc: mst, qemu-devel, virtio-fs, marcandre.lureau
On Tue, 26 Jan 2021 13:33:36 -0500
Vivek Goyal <vgoyal@redhat.com> wrote:
[...]
> >
> > Also, since pthread_rwlock_wrlock() can fail, I think we should
> > always check it's return value, at least with an assert() like
> > already done elsewhere.
>
> Will check return code of pthread_rwlock_wrlock() and probably use
> assert().
>
It turns out that pthread_rwlock_rdlock() and pthread_rwlock_unlock() can
also fail for various reasons that would likely indicate a programming
error, but their return values are never checked anywhere.
I have a patch to address this globally in this file. Should I post it
now or you prefer this series goes first ?
> Vivek
>
> >
> > > for (int i = 0; i < se->virtio_dev->nqueues; i++) {
> > > if (!se->virtio_dev->qi[i]) {
> > > continue;
> > > @@ -961,6 +974,7 @@ int virtio_loop(struct fuse_session *se)
> > > fuse_log(FUSE_LOG_INFO, "%s: Stopping queue %d thread\n", __func__, i);
> > > fv_queue_cleanup_thread(se->virtio_dev, i);
> > > }
> > > + pthread_rwlock_unlock(&se->virtio_dev->vu_dispatch_rwlock);
> > >
> > > fuse_log(FUSE_LOG_INFO, "%s: Exit\n", __func__);
> > >
> >
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 4/6] qemu, vhost-user: Extend protocol to start/stop/flush slave channel
2021-01-28 16:52 ` [Virtio-fs] " Stefan Hajnoczi
@ 2021-01-29 14:16 ` Vivek Goyal
-1 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-29 14:16 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: virtio-fs, marcandre.lureau, mst, qemu-devel, dgilbert
On Thu, Jan 28, 2021 at 04:52:34PM +0000, Stefan Hajnoczi wrote:
> On Mon, Jan 25, 2021 at 01:01:13PM -0500, Vivek Goyal wrote:
> > Currently we don't have a mechanism to flush slave channel while shutting
> > down vhost-user device and that can result a deadlock. Consider following
> > scenario.
> >
> > 1. Slave gets a request from guest on virtqueue (say index 1, vq1), to map
> > a portion of file in qemu address space.
> >
> > 2. Thread serving vq1 receives this request and sends a message to qemu on
> > slave channel/fd and gets blocked in waiting for a response from qemu.
> >
> > 3. In the mean time, user does "echo b > /proc/sysrq-trigger" in guest. This
> > leads to qemu reset and ultimately in main thread we end up in
> > vhost_dev_stop() which is trying to shutdown virtqueues for the device.
> >
> > 4. Slave gets VHOST_USER_GET_VRING_BASE message to shutdown a virtqueue on
> > unix socket being used for communication.
> >
> > 5. Slave tries to shutdown the thread serving vq1 and waits for it to
> > terminate. But vq1 thread can't terminate because it is waiting for
> > a response from qemu on slave_fd. And qemu is not processing slave_fd
> > anymore as qemu is ressing (and not running main event loop anymore)
> > and is waiting for a response to VHOST_USER_GET_VRING_BASE.
> >
> > So we have a deadlock. Qemu is waiting on slave to response to
> > VHOST_USER_GET_VRING_BASE message and slave is waiting on qemu to
> > respond to request it sent on slave_fd.
> >
> > I can reliably reproduce this race with virtio-fs.
> >
> > Hence here is the proposal to solve this problem. Enhance vhost-user
> > protocol to properly shutdown slave_fd channel. And if there are pending
> > requests, flush the channel completely before sending the request to
> > shutdown virtqueues.
> >
> > New workflow to shutdown slave channel is.
> >
> > - Qemu sends VHOST_USER_STOP_SLAVE_CHANNEL request to slave. It waits
> > for an reply from guest.
> >
> > - Then qemu sits in a tight loop waiting for
> > VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE message from slave on slave_fd.
> > And while waiting for this message, qemu continues to process requests
> > on slave_fd to flush any pending requests. This will unblock threads
> > in slave and allow slave to shutdown slave channel.
> >
> > - Once qemu gets VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE message, it knows
> > no more requests will come on slave_fd and it can continue to shutdown
> > virtqueues now.
> >
> > - Once device starts again, qemu will send VHOST_USER_START_SLAVE_CHANNEL
> > message to slave to open the slave channel and receive requests.
> >
> > IOW, this allows for proper shutdown of slave channel, making sure
> > no threads in slave are blocked on sending/receiving message. And
> > this in-turn allows for shutting down of virtqueues, hence resolving
> > the deadlock.
>
> Is the new message necessary?
Hi Stefan,
It probably is not necessary but it feels like a cleaner and simpler
solution.
There slave is a separate channel from virtqueues. And if device is
stopping, we want to make sure there are no pending requests in
slave channel. It is possible that virtqueue shutodwn is successful
but other entity could still be sending messages on slave channel. In
that case, shall we say device shutdown complete and stop polling
slave channel?
IOW, the way we have explicit protocol messages to shutdown each
vring, it makes sense to have explicit protocol message to shutdown
slave channel as well. Right now there is no mechanism to do that.
IMHO, introducing an explicit mechanism in protocol to shutdown
slave channel feels better as compared to say keep on serving
slave channel in parallel while waiting for virtuqueues to shutdown
and stop serving slave as soon as last virtuqueue is stopped (there
could still be pending slave message right after last virtuqueue
got shutdown).
> How about letting QEMU handle slave fd
> activity while waiting for virtqueues to stop instead?
>
> In other words, QEMU should monitor both the UNIX domain socket and the
> slave fd after it has sent VHOST_USER_GET_VRING_BASE and is awaiting the
> response.
I can give it a try. If there is a strong preference for this solution.
I guess I can patch vhost_user_get_vring_base() to poll and serve both
unix domain socket and slave fd.
But at this point of time I do think that adding a mechanism to shutodwn
slave channel (the way we have mechanism to shutdown vring), sounds
better from design point of view.
Vivek
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Virtio-fs] [PATCH 4/6] qemu, vhost-user: Extend protocol to start/stop/flush slave channel
@ 2021-01-29 14:16 ` Vivek Goyal
0 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-29 14:16 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: virtio-fs, marcandre.lureau, mst, qemu-devel
On Thu, Jan 28, 2021 at 04:52:34PM +0000, Stefan Hajnoczi wrote:
> On Mon, Jan 25, 2021 at 01:01:13PM -0500, Vivek Goyal wrote:
> > Currently we don't have a mechanism to flush slave channel while shutting
> > down vhost-user device and that can result a deadlock. Consider following
> > scenario.
> >
> > 1. Slave gets a request from guest on virtqueue (say index 1, vq1), to map
> > a portion of file in qemu address space.
> >
> > 2. Thread serving vq1 receives this request and sends a message to qemu on
> > slave channel/fd and gets blocked in waiting for a response from qemu.
> >
> > 3. In the mean time, user does "echo b > /proc/sysrq-trigger" in guest. This
> > leads to qemu reset and ultimately in main thread we end up in
> > vhost_dev_stop() which is trying to shutdown virtqueues for the device.
> >
> > 4. Slave gets VHOST_USER_GET_VRING_BASE message to shutdown a virtqueue on
> > unix socket being used for communication.
> >
> > 5. Slave tries to shutdown the thread serving vq1 and waits for it to
> > terminate. But vq1 thread can't terminate because it is waiting for
> > a response from qemu on slave_fd. And qemu is not processing slave_fd
> > anymore as qemu is ressing (and not running main event loop anymore)
> > and is waiting for a response to VHOST_USER_GET_VRING_BASE.
> >
> > So we have a deadlock. Qemu is waiting on slave to response to
> > VHOST_USER_GET_VRING_BASE message and slave is waiting on qemu to
> > respond to request it sent on slave_fd.
> >
> > I can reliably reproduce this race with virtio-fs.
> >
> > Hence here is the proposal to solve this problem. Enhance vhost-user
> > protocol to properly shutdown slave_fd channel. And if there are pending
> > requests, flush the channel completely before sending the request to
> > shutdown virtqueues.
> >
> > New workflow to shutdown slave channel is.
> >
> > - Qemu sends VHOST_USER_STOP_SLAVE_CHANNEL request to slave. It waits
> > for an reply from guest.
> >
> > - Then qemu sits in a tight loop waiting for
> > VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE message from slave on slave_fd.
> > And while waiting for this message, qemu continues to process requests
> > on slave_fd to flush any pending requests. This will unblock threads
> > in slave and allow slave to shutdown slave channel.
> >
> > - Once qemu gets VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE message, it knows
> > no more requests will come on slave_fd and it can continue to shutdown
> > virtqueues now.
> >
> > - Once device starts again, qemu will send VHOST_USER_START_SLAVE_CHANNEL
> > message to slave to open the slave channel and receive requests.
> >
> > IOW, this allows for proper shutdown of slave channel, making sure
> > no threads in slave are blocked on sending/receiving message. And
> > this in-turn allows for shutting down of virtqueues, hence resolving
> > the deadlock.
>
> Is the new message necessary?
Hi Stefan,
It probably is not necessary but it feels like a cleaner and simpler
solution.
There slave is a separate channel from virtqueues. And if device is
stopping, we want to make sure there are no pending requests in
slave channel. It is possible that virtqueue shutodwn is successful
but other entity could still be sending messages on slave channel. In
that case, shall we say device shutdown complete and stop polling
slave channel?
IOW, the way we have explicit protocol messages to shutdown each
vring, it makes sense to have explicit protocol message to shutdown
slave channel as well. Right now there is no mechanism to do that.
IMHO, introducing an explicit mechanism in protocol to shutdown
slave channel feels better as compared to say keep on serving
slave channel in parallel while waiting for virtuqueues to shutdown
and stop serving slave as soon as last virtuqueue is stopped (there
could still be pending slave message right after last virtuqueue
got shutdown).
> How about letting QEMU handle slave fd
> activity while waiting for virtqueues to stop instead?
>
> In other words, QEMU should monitor both the UNIX domain socket and the
> slave fd after it has sent VHOST_USER_GET_VRING_BASE and is awaiting the
> response.
I can give it a try. If there is a strong preference for this solution.
I guess I can patch vhost_user_get_vring_base() to poll and serve both
unix domain socket and slave fd.
But at this point of time I do think that adding a mechanism to shutodwn
slave channel (the way we have mechanism to shutdown vring), sounds
better from design point of view.
Vivek
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 3/6] vhost-user: Return error code from slave_read()
2021-01-29 9:45 ` [Virtio-fs] " Greg Kurz
@ 2021-01-29 15:02 ` Vivek Goyal
-1 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-29 15:02 UTC (permalink / raw)
To: Greg Kurz
Cc: mst, qemu-devel, dgilbert, virtio-fs, stefanha, marcandre.lureau
On Fri, Jan 29, 2021 at 10:45:07AM +0100, Greg Kurz wrote:
> On Mon, 25 Jan 2021 13:01:12 -0500
> Vivek Goyal <vgoyal@redhat.com> wrote:
>
> > Right now slave_read() is called through main event loop and does not
> > return error. In next few patches I want to call slave_read() from
> > vhost device shutdown path as well and want to know if an error
> > happened so that caller can give up and return error accordingly.
> >
> > Hence, create helper function do_slave_read(), which returns an
> > integer. Success is 0 and negative number is error code. slave_read()
> > calls do_slave_read() and ignores error code.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> > hw/virtio/vhost-user.c | 43 ++++++++++++++++++++++++++++++++++++------
> > 1 file changed, 37 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index d95dbc39e3..867cac034f 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -1401,7 +1401,7 @@ static uint64_t vhost_user_slave_handle_vring_host_notifier(
> > return false;
> > }
> >
> > -static void slave_read(void *opaque)
> > +static int do_slave_read(void *opaque)
> > {
> > struct vhost_dev *dev = opaque;
> > struct vhost_user *u = dev->opaque;
> > @@ -1432,13 +1432,22 @@ static void slave_read(void *opaque)
> > size = recvmsg(u->slave_fd, &msgh, 0);
> > } while (size < 0 && (errno == EINTR || errno == EAGAIN));
> >
> > - if (size != VHOST_USER_HDR_SIZE) {
> > + if (size < 0) {
> > + ret = -errno;
> > error_report("Failed to read from slave.");
> > goto err;
> > }
> >
> > + if (size != VHOST_USER_HDR_SIZE) {
> > + error_report("Failed to read %lu bytes from slave.",
> > + VHOST_USER_HDR_SIZE);
>
> Maybe also print size ?
Sounds good. That way it will be clear how many bytes we were expecting
and how many did we get.
>
> And, question from a newbie : any idea why short reads are
> considered as errors instead of retrying ? Same question
> stands for the other locations where we check the numbers
> of read/written bytes in this function.
I had same question when I was modifying the code. Atleast recvmsg()
man page does not say anything about read number of bytes can less
than number of bytes requested. But read() man page does say that
fewer bytes can be returned.
So maybe something to improve upon down the line.
>
> > + ret = -EBADMSG;
> > + goto err;
> > + }
> > +
> > if (msgh.msg_flags & MSG_CTRUNC) {
> > error_report("Truncated message.");
> > + ret = -EBADMSG;
> > goto err;
> > }
> >
> > @@ -1456,6 +1465,7 @@ static void slave_read(void *opaque)
> > error_report("Failed to read msg header."
> > " Size %d exceeds the maximum %zu.", hdr.size,
> > VHOST_USER_PAYLOAD_SIZE);
> > + ret = -EBADMSG;
> > goto err;
> > }
> >
> > @@ -1464,8 +1474,15 @@ static void slave_read(void *opaque)
> > size = read(u->slave_fd, &payload, hdr.size);
> > } while (size < 0 && (errno == EINTR || errno == EAGAIN));
> >
> > - if (size != hdr.size) {
> > + if (size == -1) {
>
> Maybe make it (size < 0) for consistency with the error checking
> added above ? And this seems to be the preferred way in the QEMU
> tree :)
Ok, will do. Don't have any strong preferene. read() man page says
it returns -1 in case of error, so checked for that.
>
> > error_report("Failed to read payload from slave.");
> > + ret = errno;
>
> ret = -errno;
>
> And this should be done before error_report() to ensure errno
> isn't cloberred.
Aha.. good catch. Will fix it.
>
> > + goto err;
> > + }
> > +
> > + if (size != hdr.size) {
> > + error_report("Failed to read %d payload bytes from slave.", hdr.size);
> > + ret = -EBADMSG;
> > goto err;
> > }
> >
> > @@ -1529,13 +1546,22 @@ static void slave_read(void *opaque)
> > size = writev(u->slave_fd, iovec, ARRAY_SIZE(iovec));
> > } while (size < 0 && (errno == EINTR || errno == EAGAIN));
> >
> > - if (size != VHOST_USER_HDR_SIZE + hdr.size) {
> > + if (size == -1) {
>
> size < 0
>
> > error_report("Failed to send msg reply to slave.");
> > + ret = -errno;
>
> Move before error_report()
Will do.
Vivek
>
> > + goto err;
> > + }
> > +
> > + if (size != VHOST_USER_HDR_SIZE + hdr.size) {
> > + error_report("Failed to send msg reply to slave. Wrote %d bytes"
> > + " expected %lu bytes.", size,
> > + VHOST_USER_HDR_SIZE + hdr.size);
> > + ret = -EIO;
> > goto err;
> > }
> > }
> >
> > - return;
> > + return 0;
> >
> > err:
> > qemu_set_fd_handler(u->slave_fd, NULL, NULL, NULL);
> > @@ -1546,7 +1572,12 @@ err:
> > close(fd[i]);
> > }
> > }
> > - return;
> > + return ret;
> > +}
> > +
> > +static void slave_read(void *opaque)
> > +{
> > + do_slave_read(opaque);
> > }
> >
> > static int vhost_setup_slave_channel(struct vhost_dev *dev)
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Virtio-fs] [PATCH 3/6] vhost-user: Return error code from slave_read()
@ 2021-01-29 15:02 ` Vivek Goyal
0 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-29 15:02 UTC (permalink / raw)
To: Greg Kurz; +Cc: mst, qemu-devel, virtio-fs, marcandre.lureau
On Fri, Jan 29, 2021 at 10:45:07AM +0100, Greg Kurz wrote:
> On Mon, 25 Jan 2021 13:01:12 -0500
> Vivek Goyal <vgoyal@redhat.com> wrote:
>
> > Right now slave_read() is called through main event loop and does not
> > return error. In next few patches I want to call slave_read() from
> > vhost device shutdown path as well and want to know if an error
> > happened so that caller can give up and return error accordingly.
> >
> > Hence, create helper function do_slave_read(), which returns an
> > integer. Success is 0 and negative number is error code. slave_read()
> > calls do_slave_read() and ignores error code.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> > hw/virtio/vhost-user.c | 43 ++++++++++++++++++++++++++++++++++++------
> > 1 file changed, 37 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index d95dbc39e3..867cac034f 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -1401,7 +1401,7 @@ static uint64_t vhost_user_slave_handle_vring_host_notifier(
> > return false;
> > }
> >
> > -static void slave_read(void *opaque)
> > +static int do_slave_read(void *opaque)
> > {
> > struct vhost_dev *dev = opaque;
> > struct vhost_user *u = dev->opaque;
> > @@ -1432,13 +1432,22 @@ static void slave_read(void *opaque)
> > size = recvmsg(u->slave_fd, &msgh, 0);
> > } while (size < 0 && (errno == EINTR || errno == EAGAIN));
> >
> > - if (size != VHOST_USER_HDR_SIZE) {
> > + if (size < 0) {
> > + ret = -errno;
> > error_report("Failed to read from slave.");
> > goto err;
> > }
> >
> > + if (size != VHOST_USER_HDR_SIZE) {
> > + error_report("Failed to read %lu bytes from slave.",
> > + VHOST_USER_HDR_SIZE);
>
> Maybe also print size ?
Sounds good. That way it will be clear how many bytes we were expecting
and how many did we get.
>
> And, question from a newbie : any idea why short reads are
> considered as errors instead of retrying ? Same question
> stands for the other locations where we check the numbers
> of read/written bytes in this function.
I had same question when I was modifying the code. Atleast recvmsg()
man page does not say anything about read number of bytes can less
than number of bytes requested. But read() man page does say that
fewer bytes can be returned.
So maybe something to improve upon down the line.
>
> > + ret = -EBADMSG;
> > + goto err;
> > + }
> > +
> > if (msgh.msg_flags & MSG_CTRUNC) {
> > error_report("Truncated message.");
> > + ret = -EBADMSG;
> > goto err;
> > }
> >
> > @@ -1456,6 +1465,7 @@ static void slave_read(void *opaque)
> > error_report("Failed to read msg header."
> > " Size %d exceeds the maximum %zu.", hdr.size,
> > VHOST_USER_PAYLOAD_SIZE);
> > + ret = -EBADMSG;
> > goto err;
> > }
> >
> > @@ -1464,8 +1474,15 @@ static void slave_read(void *opaque)
> > size = read(u->slave_fd, &payload, hdr.size);
> > } while (size < 0 && (errno == EINTR || errno == EAGAIN));
> >
> > - if (size != hdr.size) {
> > + if (size == -1) {
>
> Maybe make it (size < 0) for consistency with the error checking
> added above ? And this seems to be the preferred way in the QEMU
> tree :)
Ok, will do. Don't have any strong preferene. read() man page says
it returns -1 in case of error, so checked for that.
>
> > error_report("Failed to read payload from slave.");
> > + ret = errno;
>
> ret = -errno;
>
> And this should be done before error_report() to ensure errno
> isn't cloberred.
Aha.. good catch. Will fix it.
>
> > + goto err;
> > + }
> > +
> > + if (size != hdr.size) {
> > + error_report("Failed to read %d payload bytes from slave.", hdr.size);
> > + ret = -EBADMSG;
> > goto err;
> > }
> >
> > @@ -1529,13 +1546,22 @@ static void slave_read(void *opaque)
> > size = writev(u->slave_fd, iovec, ARRAY_SIZE(iovec));
> > } while (size < 0 && (errno == EINTR || errno == EAGAIN));
> >
> > - if (size != VHOST_USER_HDR_SIZE + hdr.size) {
> > + if (size == -1) {
>
> size < 0
>
> > error_report("Failed to send msg reply to slave.");
> > + ret = -errno;
>
> Move before error_report()
Will do.
Vivek
>
> > + goto err;
> > + }
> > +
> > + if (size != VHOST_USER_HDR_SIZE + hdr.size) {
> > + error_report("Failed to send msg reply to slave. Wrote %d bytes"
> > + " expected %lu bytes.", size,
> > + VHOST_USER_HDR_SIZE + hdr.size);
> > + ret = -EIO;
> > goto err;
> > }
> > }
> >
> > - return;
> > + return 0;
> >
> > err:
> > qemu_set_fd_handler(u->slave_fd, NULL, NULL, NULL);
> > @@ -1546,7 +1572,12 @@ err:
> > close(fd[i]);
> > }
> > }
> > - return;
> > + return ret;
> > +}
> > +
> > +static void slave_read(void *opaque)
> > +{
> > + do_slave_read(opaque);
> > }
> >
> > static int vhost_setup_slave_channel(struct vhost_dev *dev)
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 1/6] virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
2021-01-29 12:03 ` [Virtio-fs] " Greg Kurz
@ 2021-01-29 15:04 ` Vivek Goyal
-1 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-29 15:04 UTC (permalink / raw)
To: Greg Kurz
Cc: mst, qemu-devel, dgilbert, virtio-fs, stefanha, marcandre.lureau
On Fri, Jan 29, 2021 at 01:03:09PM +0100, Greg Kurz wrote:
> On Tue, 26 Jan 2021 13:33:36 -0500
> Vivek Goyal <vgoyal@redhat.com> wrote:
>
> [...]
>
> > >
> > > Also, since pthread_rwlock_wrlock() can fail, I think we should
> > > always check it's return value, at least with an assert() like
> > > already done elsewhere.
> >
> > Will check return code of pthread_rwlock_wrlock() and probably use
> > assert().
> >
>
> It turns out that pthread_rwlock_rdlock() and pthread_rwlock_unlock() can
> also fail for various reasons that would likely indicate a programming
> error, but their return values are never checked anywhere.
>
> I have a patch to address this globally in this file. Should I post it
> now or you prefer this series goes first ?
Please go ahead and post your patch. Your patch can go first and I can
rebase my patches on top of yours.
Vivek
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Virtio-fs] [PATCH 1/6] virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
@ 2021-01-29 15:04 ` Vivek Goyal
0 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-29 15:04 UTC (permalink / raw)
To: Greg Kurz; +Cc: mst, qemu-devel, virtio-fs, marcandre.lureau
On Fri, Jan 29, 2021 at 01:03:09PM +0100, Greg Kurz wrote:
> On Tue, 26 Jan 2021 13:33:36 -0500
> Vivek Goyal <vgoyal@redhat.com> wrote:
>
> [...]
>
> > >
> > > Also, since pthread_rwlock_wrlock() can fail, I think we should
> > > always check it's return value, at least with an assert() like
> > > already done elsewhere.
> >
> > Will check return code of pthread_rwlock_wrlock() and probably use
> > assert().
> >
>
> It turns out that pthread_rwlock_rdlock() and pthread_rwlock_unlock() can
> also fail for various reasons that would likely indicate a programming
> error, but their return values are never checked anywhere.
>
> I have a patch to address this globally in this file. Should I post it
> now or you prefer this series goes first ?
Please go ahead and post your patch. Your patch can go first and I can
rebase my patches on top of yours.
Vivek
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 4/6] qemu, vhost-user: Extend protocol to start/stop/flush slave channel
2021-01-29 14:16 ` [Virtio-fs] " Vivek Goyal
@ 2021-01-29 15:11 ` Vivek Goyal
-1 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-29 15:11 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: virtio-fs, marcandre.lureau, mst, qemu-devel, dgilbert
On Fri, Jan 29, 2021 at 09:16:00AM -0500, Vivek Goyal wrote:
> On Thu, Jan 28, 2021 at 04:52:34PM +0000, Stefan Hajnoczi wrote:
> > On Mon, Jan 25, 2021 at 01:01:13PM -0500, Vivek Goyal wrote:
> > > Currently we don't have a mechanism to flush slave channel while shutting
> > > down vhost-user device and that can result a deadlock. Consider following
> > > scenario.
> > >
> > > 1. Slave gets a request from guest on virtqueue (say index 1, vq1), to map
> > > a portion of file in qemu address space.
> > >
> > > 2. Thread serving vq1 receives this request and sends a message to qemu on
> > > slave channel/fd and gets blocked in waiting for a response from qemu.
> > >
> > > 3. In the mean time, user does "echo b > /proc/sysrq-trigger" in guest. This
> > > leads to qemu reset and ultimately in main thread we end up in
> > > vhost_dev_stop() which is trying to shutdown virtqueues for the device.
> > >
> > > 4. Slave gets VHOST_USER_GET_VRING_BASE message to shutdown a virtqueue on
> > > unix socket being used for communication.
> > >
> > > 5. Slave tries to shutdown the thread serving vq1 and waits for it to
> > > terminate. But vq1 thread can't terminate because it is waiting for
> > > a response from qemu on slave_fd. And qemu is not processing slave_fd
> > > anymore as qemu is ressing (and not running main event loop anymore)
> > > and is waiting for a response to VHOST_USER_GET_VRING_BASE.
> > >
> > > So we have a deadlock. Qemu is waiting on slave to response to
> > > VHOST_USER_GET_VRING_BASE message and slave is waiting on qemu to
> > > respond to request it sent on slave_fd.
> > >
> > > I can reliably reproduce this race with virtio-fs.
> > >
> > > Hence here is the proposal to solve this problem. Enhance vhost-user
> > > protocol to properly shutdown slave_fd channel. And if there are pending
> > > requests, flush the channel completely before sending the request to
> > > shutdown virtqueues.
> > >
> > > New workflow to shutdown slave channel is.
> > >
> > > - Qemu sends VHOST_USER_STOP_SLAVE_CHANNEL request to slave. It waits
> > > for an reply from guest.
> > >
> > > - Then qemu sits in a tight loop waiting for
> > > VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE message from slave on slave_fd.
> > > And while waiting for this message, qemu continues to process requests
> > > on slave_fd to flush any pending requests. This will unblock threads
> > > in slave and allow slave to shutdown slave channel.
> > >
> > > - Once qemu gets VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE message, it knows
> > > no more requests will come on slave_fd and it can continue to shutdown
> > > virtqueues now.
> > >
> > > - Once device starts again, qemu will send VHOST_USER_START_SLAVE_CHANNEL
> > > message to slave to open the slave channel and receive requests.
> > >
> > > IOW, this allows for proper shutdown of slave channel, making sure
> > > no threads in slave are blocked on sending/receiving message. And
> > > this in-turn allows for shutting down of virtqueues, hence resolving
> > > the deadlock.
> >
> > Is the new message necessary?
>
> Hi Stefan,
>
> It probably is not necessary but it feels like a cleaner and simpler
> solution.
>
> There slave is a separate channel from virtqueues. And if device is
> stopping, we want to make sure there are no pending requests in
> slave channel. It is possible that virtqueue shutodwn is successful
> but other entity could still be sending messages on slave channel. In
> that case, shall we say device shutdown complete and stop polling
> slave channel?
>
> IOW, the way we have explicit protocol messages to shutdown each
> vring, it makes sense to have explicit protocol message to shutdown
> slave channel as well. Right now there is no mechanism to do that.
>
Thinking more from "slave" point of view. It might have a separate thread
to send messages on slave channel. It has no idea when to stop sending
messages on slave channel, as there is no mechanism to shutdown slave.
Only method seems to be that master will close slave fd and that
will close connection.
That means if a device is being stopped (and not removed), then it
is possible virtqueues got stopped but a thread in slave is blocked
on slave channel (recvmsg()) and expecting a response back.
That's why I think enhancing protocol to explicitly shutdown slave
channel makes sense. Instead of assuming that stopping a virtuqueue
should automatically mean slave channel is stopped.
Vivek
> IMHO, introducing an explicit mechanism in protocol to shutdown
> slave channel feels better as compared to say keep on serving
> slave channel in parallel while waiting for virtuqueues to shutdown
> and stop serving slave as soon as last virtuqueue is stopped (there
> could still be pending slave message right after last virtuqueue
> got shutdown).
>
>
> > How about letting QEMU handle slave fd
> > activity while waiting for virtqueues to stop instead?
> >
> > In other words, QEMU should monitor both the UNIX domain socket and the
> > slave fd after it has sent VHOST_USER_GET_VRING_BASE and is awaiting the
> > response.
>
> I can give it a try. If there is a strong preference for this solution.
> I guess I can patch vhost_user_get_vring_base() to poll and serve both
> unix domain socket and slave fd.
>
> But at this point of time I do think that adding a mechanism to shutodwn
> slave channel (the way we have mechanism to shutdown vring), sounds
> better from design point of view.
>
> Vivek
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Virtio-fs] [PATCH 4/6] qemu, vhost-user: Extend protocol to start/stop/flush slave channel
@ 2021-01-29 15:11 ` Vivek Goyal
0 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-01-29 15:11 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: virtio-fs, marcandre.lureau, mst, qemu-devel
On Fri, Jan 29, 2021 at 09:16:00AM -0500, Vivek Goyal wrote:
> On Thu, Jan 28, 2021 at 04:52:34PM +0000, Stefan Hajnoczi wrote:
> > On Mon, Jan 25, 2021 at 01:01:13PM -0500, Vivek Goyal wrote:
> > > Currently we don't have a mechanism to flush slave channel while shutting
> > > down vhost-user device and that can result a deadlock. Consider following
> > > scenario.
> > >
> > > 1. Slave gets a request from guest on virtqueue (say index 1, vq1), to map
> > > a portion of file in qemu address space.
> > >
> > > 2. Thread serving vq1 receives this request and sends a message to qemu on
> > > slave channel/fd and gets blocked in waiting for a response from qemu.
> > >
> > > 3. In the mean time, user does "echo b > /proc/sysrq-trigger" in guest. This
> > > leads to qemu reset and ultimately in main thread we end up in
> > > vhost_dev_stop() which is trying to shutdown virtqueues for the device.
> > >
> > > 4. Slave gets VHOST_USER_GET_VRING_BASE message to shutdown a virtqueue on
> > > unix socket being used for communication.
> > >
> > > 5. Slave tries to shutdown the thread serving vq1 and waits for it to
> > > terminate. But vq1 thread can't terminate because it is waiting for
> > > a response from qemu on slave_fd. And qemu is not processing slave_fd
> > > anymore as qemu is ressing (and not running main event loop anymore)
> > > and is waiting for a response to VHOST_USER_GET_VRING_BASE.
> > >
> > > So we have a deadlock. Qemu is waiting on slave to response to
> > > VHOST_USER_GET_VRING_BASE message and slave is waiting on qemu to
> > > respond to request it sent on slave_fd.
> > >
> > > I can reliably reproduce this race with virtio-fs.
> > >
> > > Hence here is the proposal to solve this problem. Enhance vhost-user
> > > protocol to properly shutdown slave_fd channel. And if there are pending
> > > requests, flush the channel completely before sending the request to
> > > shutdown virtqueues.
> > >
> > > New workflow to shutdown slave channel is.
> > >
> > > - Qemu sends VHOST_USER_STOP_SLAVE_CHANNEL request to slave. It waits
> > > for an reply from guest.
> > >
> > > - Then qemu sits in a tight loop waiting for
> > > VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE message from slave on slave_fd.
> > > And while waiting for this message, qemu continues to process requests
> > > on slave_fd to flush any pending requests. This will unblock threads
> > > in slave and allow slave to shutdown slave channel.
> > >
> > > - Once qemu gets VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE message, it knows
> > > no more requests will come on slave_fd and it can continue to shutdown
> > > virtqueues now.
> > >
> > > - Once device starts again, qemu will send VHOST_USER_START_SLAVE_CHANNEL
> > > message to slave to open the slave channel and receive requests.
> > >
> > > IOW, this allows for proper shutdown of slave channel, making sure
> > > no threads in slave are blocked on sending/receiving message. And
> > > this in-turn allows for shutting down of virtqueues, hence resolving
> > > the deadlock.
> >
> > Is the new message necessary?
>
> Hi Stefan,
>
> It probably is not necessary but it feels like a cleaner and simpler
> solution.
>
> There slave is a separate channel from virtqueues. And if device is
> stopping, we want to make sure there are no pending requests in
> slave channel. It is possible that virtqueue shutodwn is successful
> but other entity could still be sending messages on slave channel. In
> that case, shall we say device shutdown complete and stop polling
> slave channel?
>
> IOW, the way we have explicit protocol messages to shutdown each
> vring, it makes sense to have explicit protocol message to shutdown
> slave channel as well. Right now there is no mechanism to do that.
>
Thinking more from "slave" point of view. It might have a separate thread
to send messages on slave channel. It has no idea when to stop sending
messages on slave channel, as there is no mechanism to shutdown slave.
Only method seems to be that master will close slave fd and that
will close connection.
That means if a device is being stopped (and not removed), then it
is possible virtqueues got stopped but a thread in slave is blocked
on slave channel (recvmsg()) and expecting a response back.
That's why I think enhancing protocol to explicitly shutdown slave
channel makes sense. Instead of assuming that stopping a virtuqueue
should automatically mean slave channel is stopped.
Vivek
> IMHO, introducing an explicit mechanism in protocol to shutdown
> slave channel feels better as compared to say keep on serving
> slave channel in parallel while waiting for virtuqueues to shutdown
> and stop serving slave as soon as last virtuqueue is stopped (there
> could still be pending slave message right after last virtuqueue
> got shutdown).
>
>
> > How about letting QEMU handle slave fd
> > activity while waiting for virtqueues to stop instead?
> >
> > In other words, QEMU should monitor both the UNIX domain socket and the
> > slave fd after it has sent VHOST_USER_GET_VRING_BASE and is awaiting the
> > response.
>
> I can give it a try. If there is a strong preference for this solution.
> I guess I can patch vhost_user_get_vring_base() to poll and serve both
> unix domain socket and slave fd.
>
> But at this point of time I do think that adding a mechanism to shutodwn
> slave channel (the way we have mechanism to shutdown vring), sounds
> better from design point of view.
>
> Vivek
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 4/6] qemu, vhost-user: Extend protocol to start/stop/flush slave channel
2021-01-29 15:11 ` [Virtio-fs] " Vivek Goyal
@ 2021-02-08 17:41 ` Stefan Hajnoczi
-1 siblings, 0 replies; 52+ messages in thread
From: Stefan Hajnoczi @ 2021-02-08 17:41 UTC (permalink / raw)
To: Vivek Goyal; +Cc: virtio-fs, marcandre.lureau, mst, qemu-devel, dgilbert
[-- Attachment #1: Type: text/plain, Size: 5475 bytes --]
On Fri, Jan 29, 2021 at 10:11:55AM -0500, Vivek Goyal wrote:
> On Fri, Jan 29, 2021 at 09:16:00AM -0500, Vivek Goyal wrote:
> > On Thu, Jan 28, 2021 at 04:52:34PM +0000, Stefan Hajnoczi wrote:
> > > On Mon, Jan 25, 2021 at 01:01:13PM -0500, Vivek Goyal wrote:
> > > > Currently we don't have a mechanism to flush slave channel while shutting
> > > > down vhost-user device and that can result a deadlock. Consider following
> > > > scenario.
> > > >
> > > > 1. Slave gets a request from guest on virtqueue (say index 1, vq1), to map
> > > > a portion of file in qemu address space.
> > > >
> > > > 2. Thread serving vq1 receives this request and sends a message to qemu on
> > > > slave channel/fd and gets blocked in waiting for a response from qemu.
> > > >
> > > > 3. In the mean time, user does "echo b > /proc/sysrq-trigger" in guest. This
> > > > leads to qemu reset and ultimately in main thread we end up in
> > > > vhost_dev_stop() which is trying to shutdown virtqueues for the device.
> > > >
> > > > 4. Slave gets VHOST_USER_GET_VRING_BASE message to shutdown a virtqueue on
> > > > unix socket being used for communication.
> > > >
> > > > 5. Slave tries to shutdown the thread serving vq1 and waits for it to
> > > > terminate. But vq1 thread can't terminate because it is waiting for
> > > > a response from qemu on slave_fd. And qemu is not processing slave_fd
> > > > anymore as qemu is ressing (and not running main event loop anymore)
> > > > and is waiting for a response to VHOST_USER_GET_VRING_BASE.
> > > >
> > > > So we have a deadlock. Qemu is waiting on slave to response to
> > > > VHOST_USER_GET_VRING_BASE message and slave is waiting on qemu to
> > > > respond to request it sent on slave_fd.
> > > >
> > > > I can reliably reproduce this race with virtio-fs.
> > > >
> > > > Hence here is the proposal to solve this problem. Enhance vhost-user
> > > > protocol to properly shutdown slave_fd channel. And if there are pending
> > > > requests, flush the channel completely before sending the request to
> > > > shutdown virtqueues.
> > > >
> > > > New workflow to shutdown slave channel is.
> > > >
> > > > - Qemu sends VHOST_USER_STOP_SLAVE_CHANNEL request to slave. It waits
> > > > for an reply from guest.
> > > >
> > > > - Then qemu sits in a tight loop waiting for
> > > > VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE message from slave on slave_fd.
> > > > And while waiting for this message, qemu continues to process requests
> > > > on slave_fd to flush any pending requests. This will unblock threads
> > > > in slave and allow slave to shutdown slave channel.
> > > >
> > > > - Once qemu gets VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE message, it knows
> > > > no more requests will come on slave_fd and it can continue to shutdown
> > > > virtqueues now.
> > > >
> > > > - Once device starts again, qemu will send VHOST_USER_START_SLAVE_CHANNEL
> > > > message to slave to open the slave channel and receive requests.
> > > >
> > > > IOW, this allows for proper shutdown of slave channel, making sure
> > > > no threads in slave are blocked on sending/receiving message. And
> > > > this in-turn allows for shutting down of virtqueues, hence resolving
> > > > the deadlock.
> > >
> > > Is the new message necessary?
> >
> > Hi Stefan,
> >
> > It probably is not necessary but it feels like a cleaner and simpler
> > solution.
> >
> > There slave is a separate channel from virtqueues. And if device is
> > stopping, we want to make sure there are no pending requests in
> > slave channel. It is possible that virtqueue shutodwn is successful
> > but other entity could still be sending messages on slave channel. In
> > that case, shall we say device shutdown complete and stop polling
> > slave channel?
> >
> > IOW, the way we have explicit protocol messages to shutdown each
> > vring, it makes sense to have explicit protocol message to shutdown
> > slave channel as well. Right now there is no mechanism to do that.
> >
>
> Thinking more from "slave" point of view. It might have a separate thread
> to send messages on slave channel. It has no idea when to stop sending
> messages on slave channel, as there is no mechanism to shutdown slave.
> Only method seems to be that master will close slave fd and that
> will close connection.
>
> That means if a device is being stopped (and not removed), then it
> is possible virtqueues got stopped but a thread in slave is blocked
> on slave channel (recvmsg()) and expecting a response back.
>
> That's why I think enhancing protocol to explicitly shutdown slave
> channel makes sense. Instead of assuming that stopping a virtuqueue
> should automatically mean slave channel is stopped.
This deadlock can occur during any vhost-user protocol message, not just
shutdown:
When QEMU's vhost-user master implementation sends a vhost-user protocol
message, vhost_user_read() does a "blocking" read during which slave_fd
is not monitored by QEMU.
So the problem is more general and is not fully solved by an explicit
shutdown message. If the slave needs slave channel reply in order to
process a vhost-user protocol message then a deadlock can occur.
This can be solved by monitoring both the vhost-user fd and slave_fd in
vhost_user_read(). I don't think there is a need to shutdown the slave
channel BTW.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Virtio-fs] [PATCH 4/6] qemu, vhost-user: Extend protocol to start/stop/flush slave channel
@ 2021-02-08 17:41 ` Stefan Hajnoczi
0 siblings, 0 replies; 52+ messages in thread
From: Stefan Hajnoczi @ 2021-02-08 17:41 UTC (permalink / raw)
To: Vivek Goyal; +Cc: virtio-fs, marcandre.lureau, mst, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5475 bytes --]
On Fri, Jan 29, 2021 at 10:11:55AM -0500, Vivek Goyal wrote:
> On Fri, Jan 29, 2021 at 09:16:00AM -0500, Vivek Goyal wrote:
> > On Thu, Jan 28, 2021 at 04:52:34PM +0000, Stefan Hajnoczi wrote:
> > > On Mon, Jan 25, 2021 at 01:01:13PM -0500, Vivek Goyal wrote:
> > > > Currently we don't have a mechanism to flush slave channel while shutting
> > > > down vhost-user device and that can result a deadlock. Consider following
> > > > scenario.
> > > >
> > > > 1. Slave gets a request from guest on virtqueue (say index 1, vq1), to map
> > > > a portion of file in qemu address space.
> > > >
> > > > 2. Thread serving vq1 receives this request and sends a message to qemu on
> > > > slave channel/fd and gets blocked in waiting for a response from qemu.
> > > >
> > > > 3. In the mean time, user does "echo b > /proc/sysrq-trigger" in guest. This
> > > > leads to qemu reset and ultimately in main thread we end up in
> > > > vhost_dev_stop() which is trying to shutdown virtqueues for the device.
> > > >
> > > > 4. Slave gets VHOST_USER_GET_VRING_BASE message to shutdown a virtqueue on
> > > > unix socket being used for communication.
> > > >
> > > > 5. Slave tries to shutdown the thread serving vq1 and waits for it to
> > > > terminate. But vq1 thread can't terminate because it is waiting for
> > > > a response from qemu on slave_fd. And qemu is not processing slave_fd
> > > > anymore as qemu is ressing (and not running main event loop anymore)
> > > > and is waiting for a response to VHOST_USER_GET_VRING_BASE.
> > > >
> > > > So we have a deadlock. Qemu is waiting on slave to response to
> > > > VHOST_USER_GET_VRING_BASE message and slave is waiting on qemu to
> > > > respond to request it sent on slave_fd.
> > > >
> > > > I can reliably reproduce this race with virtio-fs.
> > > >
> > > > Hence here is the proposal to solve this problem. Enhance vhost-user
> > > > protocol to properly shutdown slave_fd channel. And if there are pending
> > > > requests, flush the channel completely before sending the request to
> > > > shutdown virtqueues.
> > > >
> > > > New workflow to shutdown slave channel is.
> > > >
> > > > - Qemu sends VHOST_USER_STOP_SLAVE_CHANNEL request to slave. It waits
> > > > for an reply from guest.
> > > >
> > > > - Then qemu sits in a tight loop waiting for
> > > > VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE message from slave on slave_fd.
> > > > And while waiting for this message, qemu continues to process requests
> > > > on slave_fd to flush any pending requests. This will unblock threads
> > > > in slave and allow slave to shutdown slave channel.
> > > >
> > > > - Once qemu gets VHOST_USER_SLAVE_STOP_CHANNEL_COMPLETE message, it knows
> > > > no more requests will come on slave_fd and it can continue to shutdown
> > > > virtqueues now.
> > > >
> > > > - Once device starts again, qemu will send VHOST_USER_START_SLAVE_CHANNEL
> > > > message to slave to open the slave channel and receive requests.
> > > >
> > > > IOW, this allows for proper shutdown of slave channel, making sure
> > > > no threads in slave are blocked on sending/receiving message. And
> > > > this in-turn allows for shutting down of virtqueues, hence resolving
> > > > the deadlock.
> > >
> > > Is the new message necessary?
> >
> > Hi Stefan,
> >
> > It probably is not necessary but it feels like a cleaner and simpler
> > solution.
> >
> > There slave is a separate channel from virtqueues. And if device is
> > stopping, we want to make sure there are no pending requests in
> > slave channel. It is possible that virtqueue shutodwn is successful
> > but other entity could still be sending messages on slave channel. In
> > that case, shall we say device shutdown complete and stop polling
> > slave channel?
> >
> > IOW, the way we have explicit protocol messages to shutdown each
> > vring, it makes sense to have explicit protocol message to shutdown
> > slave channel as well. Right now there is no mechanism to do that.
> >
>
> Thinking more from "slave" point of view. It might have a separate thread
> to send messages on slave channel. It has no idea when to stop sending
> messages on slave channel, as there is no mechanism to shutdown slave.
> Only method seems to be that master will close slave fd and that
> will close connection.
>
> That means if a device is being stopped (and not removed), then it
> is possible virtqueues got stopped but a thread in slave is blocked
> on slave channel (recvmsg()) and expecting a response back.
>
> That's why I think enhancing protocol to explicitly shutdown slave
> channel makes sense. Instead of assuming that stopping a virtuqueue
> should automatically mean slave channel is stopped.
This deadlock can occur during any vhost-user protocol message, not just
shutdown:
When QEMU's vhost-user master implementation sends a vhost-user protocol
message, vhost_user_read() does a "blocking" read during which slave_fd
is not monitored by QEMU.
So the problem is more general and is not fully solved by an explicit
shutdown message. If the slave needs slave channel reply in order to
process a vhost-user protocol message then a deadlock can occur.
This can be solved by monitoring both the vhost-user fd and slave_fd in
vhost_user_read(). I don't think there is a need to shutdown the slave
channel BTW.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [RFC PATCH 0/6] vhost-user: Shutdown/Flush slave channel properly
2021-01-25 18:01 ` [Virtio-fs] " Vivek Goyal
@ 2021-02-10 21:39 ` Michael S. Tsirkin
-1 siblings, 0 replies; 52+ messages in thread
From: Michael S. Tsirkin @ 2021-02-10 21:39 UTC (permalink / raw)
To: Vivek Goyal; +Cc: virtio-fs, marcandre.lureau, qemu-devel, stefanha, dgilbert
On Mon, Jan 25, 2021 at 01:01:09PM -0500, Vivek Goyal wrote:
> Hi,
>
> We are working on DAX support in virtiofs and have some patches out of
> the tree hosted here.
>
> https://gitlab.com/virtio-fs/qemu/-/commits/virtio-fs-dev
>
> These patches have not been proposed for merge yet, becasue David
> Gilbert noticed that we can run into a deadlock during an emergency
> reboot of guest kernel. (echo b > /proc/sysrq-trigger).
>
> I have provided details of deadlock in 4th path of the series with
> subject "qemu, vhost-user: Extend protocol to start/stop/flush slave
> channel".
>
> Basic problem seems to be that we don't have a proper mechanism to
> shutdown slave channel when vhost-user device is stopping. This means
> there might be pending messages in slave channel and slave is blocked
> and waiting for response.
>
> This is an RFC patch series to enhance vhost-user protocol to
> properly shutdown/flush slave channel and avoid the deadlock. Though
> we faced the issue in the context of virtiofs, any vhost-user
> device using slave channel can potentially run into issues and
> can benefit from these patches.
>
> Any feedback is welcome. Currently patches are based on out of
> tree code but after I get some feedback, I can only take pieces
> which are relevant to upstream and post separately.
>
> Thanks
> Vivek
No comments so far - do you plan to post a non-RFC patchset?
> Vivek Goyal (6):
> virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
> libvhost-user: Use slave_mutex in all slave messages
> vhost-user: Return error code from slave_read()
> qemu, vhost-user: Extend protocol to start/stop/flush slave channel
> libvhost-user: Add support to start/stop/flush slave channel
> virtiofsd: Opt in for slave start/stop/shutdown functionality
>
> hw/virtio/vhost-user.c | 151 +++++++++++++++++++++-
> subprojects/libvhost-user/libvhost-user.c | 147 +++++++++++++++++----
> subprojects/libvhost-user/libvhost-user.h | 8 +-
> tools/virtiofsd/fuse_virtio.c | 20 +++
> 4 files changed, 294 insertions(+), 32 deletions(-)
>
> --
> 2.25.4
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Virtio-fs] [RFC PATCH 0/6] vhost-user: Shutdown/Flush slave channel properly
@ 2021-02-10 21:39 ` Michael S. Tsirkin
0 siblings, 0 replies; 52+ messages in thread
From: Michael S. Tsirkin @ 2021-02-10 21:39 UTC (permalink / raw)
To: Vivek Goyal; +Cc: virtio-fs, marcandre.lureau, qemu-devel
On Mon, Jan 25, 2021 at 01:01:09PM -0500, Vivek Goyal wrote:
> Hi,
>
> We are working on DAX support in virtiofs and have some patches out of
> the tree hosted here.
>
> https://gitlab.com/virtio-fs/qemu/-/commits/virtio-fs-dev
>
> These patches have not been proposed for merge yet, becasue David
> Gilbert noticed that we can run into a deadlock during an emergency
> reboot of guest kernel. (echo b > /proc/sysrq-trigger).
>
> I have provided details of deadlock in 4th path of the series with
> subject "qemu, vhost-user: Extend protocol to start/stop/flush slave
> channel".
>
> Basic problem seems to be that we don't have a proper mechanism to
> shutdown slave channel when vhost-user device is stopping. This means
> there might be pending messages in slave channel and slave is blocked
> and waiting for response.
>
> This is an RFC patch series to enhance vhost-user protocol to
> properly shutdown/flush slave channel and avoid the deadlock. Though
> we faced the issue in the context of virtiofs, any vhost-user
> device using slave channel can potentially run into issues and
> can benefit from these patches.
>
> Any feedback is welcome. Currently patches are based on out of
> tree code but after I get some feedback, I can only take pieces
> which are relevant to upstream and post separately.
>
> Thanks
> Vivek
No comments so far - do you plan to post a non-RFC patchset?
> Vivek Goyal (6):
> virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
> libvhost-user: Use slave_mutex in all slave messages
> vhost-user: Return error code from slave_read()
> qemu, vhost-user: Extend protocol to start/stop/flush slave channel
> libvhost-user: Add support to start/stop/flush slave channel
> virtiofsd: Opt in for slave start/stop/shutdown functionality
>
> hw/virtio/vhost-user.c | 151 +++++++++++++++++++++-
> subprojects/libvhost-user/libvhost-user.c | 147 +++++++++++++++++----
> subprojects/libvhost-user/libvhost-user.h | 8 +-
> tools/virtiofsd/fuse_virtio.c | 20 +++
> 4 files changed, 294 insertions(+), 32 deletions(-)
>
> --
> 2.25.4
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [RFC PATCH 0/6] vhost-user: Shutdown/Flush slave channel properly
2021-02-10 21:39 ` [Virtio-fs] " Michael S. Tsirkin
@ 2021-02-10 22:15 ` Vivek Goyal
-1 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-02-10 22:15 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtio-fs, marcandre.lureau, qemu-devel, stefanha, dgilbert
On Wed, Feb 10, 2021 at 04:39:06PM -0500, Michael S. Tsirkin wrote:
> On Mon, Jan 25, 2021 at 01:01:09PM -0500, Vivek Goyal wrote:
> > Hi,
> >
> > We are working on DAX support in virtiofs and have some patches out of
> > the tree hosted here.
> >
> > https://gitlab.com/virtio-fs/qemu/-/commits/virtio-fs-dev
> >
> > These patches have not been proposed for merge yet, becasue David
> > Gilbert noticed that we can run into a deadlock during an emergency
> > reboot of guest kernel. (echo b > /proc/sysrq-trigger).
> >
> > I have provided details of deadlock in 4th path of the series with
> > subject "qemu, vhost-user: Extend protocol to start/stop/flush slave
> > channel".
> >
> > Basic problem seems to be that we don't have a proper mechanism to
> > shutdown slave channel when vhost-user device is stopping. This means
> > there might be pending messages in slave channel and slave is blocked
> > and waiting for response.
> >
> > This is an RFC patch series to enhance vhost-user protocol to
> > properly shutdown/flush slave channel and avoid the deadlock. Though
> > we faced the issue in the context of virtiofs, any vhost-user
> > device using slave channel can potentially run into issues and
> > can benefit from these patches.
> >
> > Any feedback is welcome. Currently patches are based on out of
> > tree code but after I get some feedback, I can only take pieces
> > which are relevant to upstream and post separately.
> >
> > Thanks
> > Vivek
>
> No comments so far - do you plan to post a non-RFC patchset?
Yes. Stefan wants me to poll both unix fd and slave fd in device
shutdown path and serve both of these in parallel, instead of
adding a new slave channel shutdown message. I am planning to give
it a try and post new patches.
Vivek
>
>
> > Vivek Goyal (6):
> > virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
> > libvhost-user: Use slave_mutex in all slave messages
> > vhost-user: Return error code from slave_read()
> > qemu, vhost-user: Extend protocol to start/stop/flush slave channel
> > libvhost-user: Add support to start/stop/flush slave channel
> > virtiofsd: Opt in for slave start/stop/shutdown functionality
> >
> > hw/virtio/vhost-user.c | 151 +++++++++++++++++++++-
> > subprojects/libvhost-user/libvhost-user.c | 147 +++++++++++++++++----
> > subprojects/libvhost-user/libvhost-user.h | 8 +-
> > tools/virtiofsd/fuse_virtio.c | 20 +++
> > 4 files changed, 294 insertions(+), 32 deletions(-)
> >
> > --
> > 2.25.4
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Virtio-fs] [RFC PATCH 0/6] vhost-user: Shutdown/Flush slave channel properly
@ 2021-02-10 22:15 ` Vivek Goyal
0 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-02-10 22:15 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtio-fs, marcandre.lureau, qemu-devel
On Wed, Feb 10, 2021 at 04:39:06PM -0500, Michael S. Tsirkin wrote:
> On Mon, Jan 25, 2021 at 01:01:09PM -0500, Vivek Goyal wrote:
> > Hi,
> >
> > We are working on DAX support in virtiofs and have some patches out of
> > the tree hosted here.
> >
> > https://gitlab.com/virtio-fs/qemu/-/commits/virtio-fs-dev
> >
> > These patches have not been proposed for merge yet, becasue David
> > Gilbert noticed that we can run into a deadlock during an emergency
> > reboot of guest kernel. (echo b > /proc/sysrq-trigger).
> >
> > I have provided details of deadlock in 4th path of the series with
> > subject "qemu, vhost-user: Extend protocol to start/stop/flush slave
> > channel".
> >
> > Basic problem seems to be that we don't have a proper mechanism to
> > shutdown slave channel when vhost-user device is stopping. This means
> > there might be pending messages in slave channel and slave is blocked
> > and waiting for response.
> >
> > This is an RFC patch series to enhance vhost-user protocol to
> > properly shutdown/flush slave channel and avoid the deadlock. Though
> > we faced the issue in the context of virtiofs, any vhost-user
> > device using slave channel can potentially run into issues and
> > can benefit from these patches.
> >
> > Any feedback is welcome. Currently patches are based on out of
> > tree code but after I get some feedback, I can only take pieces
> > which are relevant to upstream and post separately.
> >
> > Thanks
> > Vivek
>
> No comments so far - do you plan to post a non-RFC patchset?
Yes. Stefan wants me to poll both unix fd and slave fd in device
shutdown path and serve both of these in parallel, instead of
adding a new slave channel shutdown message. I am planning to give
it a try and post new patches.
Vivek
>
>
> > Vivek Goyal (6):
> > virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
> > libvhost-user: Use slave_mutex in all slave messages
> > vhost-user: Return error code from slave_read()
> > qemu, vhost-user: Extend protocol to start/stop/flush slave channel
> > libvhost-user: Add support to start/stop/flush slave channel
> > virtiofsd: Opt in for slave start/stop/shutdown functionality
> >
> > hw/virtio/vhost-user.c | 151 +++++++++++++++++++++-
> > subprojects/libvhost-user/libvhost-user.c | 147 +++++++++++++++++----
> > subprojects/libvhost-user/libvhost-user.h | 8 +-
> > tools/virtiofsd/fuse_virtio.c | 20 +++
> > 4 files changed, 294 insertions(+), 32 deletions(-)
> >
> > --
> > 2.25.4
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [RFC PATCH 0/6] vhost-user: Shutdown/Flush slave channel properly
2021-01-25 18:01 ` [Virtio-fs] " Vivek Goyal
@ 2021-02-23 14:14 ` Michael S. Tsirkin
-1 siblings, 0 replies; 52+ messages in thread
From: Michael S. Tsirkin @ 2021-02-23 14:14 UTC (permalink / raw)
To: Vivek Goyal; +Cc: virtio-fs, marcandre.lureau, qemu-devel, stefanha, dgilbert
On Mon, Jan 25, 2021 at 01:01:09PM -0500, Vivek Goyal wrote:
> Hi,
>
> We are working on DAX support in virtiofs and have some patches out of
> the tree hosted here.
>
> https://gitlab.com/virtio-fs/qemu/-/commits/virtio-fs-dev
any plans to post a non RFC version?
> These patches have not been proposed for merge yet, becasue David
> Gilbert noticed that we can run into a deadlock during an emergency
> reboot of guest kernel. (echo b > /proc/sysrq-trigger).
>
> I have provided details of deadlock in 4th path of the series with
> subject "qemu, vhost-user: Extend protocol to start/stop/flush slave
> channel".
>
> Basic problem seems to be that we don't have a proper mechanism to
> shutdown slave channel when vhost-user device is stopping. This means
> there might be pending messages in slave channel and slave is blocked
> and waiting for response.
>
> This is an RFC patch series to enhance vhost-user protocol to
> properly shutdown/flush slave channel and avoid the deadlock. Though
> we faced the issue in the context of virtiofs, any vhost-user
> device using slave channel can potentially run into issues and
> can benefit from these patches.
>
> Any feedback is welcome. Currently patches are based on out of
> tree code but after I get some feedback, I can only take pieces
> which are relevant to upstream and post separately.
>
> Thanks
> Vivek
>
> Vivek Goyal (6):
> virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
> libvhost-user: Use slave_mutex in all slave messages
> vhost-user: Return error code from slave_read()
> qemu, vhost-user: Extend protocol to start/stop/flush slave channel
> libvhost-user: Add support to start/stop/flush slave channel
> virtiofsd: Opt in for slave start/stop/shutdown functionality
>
> hw/virtio/vhost-user.c | 151 +++++++++++++++++++++-
> subprojects/libvhost-user/libvhost-user.c | 147 +++++++++++++++++----
> subprojects/libvhost-user/libvhost-user.h | 8 +-
> tools/virtiofsd/fuse_virtio.c | 20 +++
> 4 files changed, 294 insertions(+), 32 deletions(-)
>
> --
> 2.25.4
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Virtio-fs] [RFC PATCH 0/6] vhost-user: Shutdown/Flush slave channel properly
@ 2021-02-23 14:14 ` Michael S. Tsirkin
0 siblings, 0 replies; 52+ messages in thread
From: Michael S. Tsirkin @ 2021-02-23 14:14 UTC (permalink / raw)
To: Vivek Goyal; +Cc: virtio-fs, marcandre.lureau, qemu-devel
On Mon, Jan 25, 2021 at 01:01:09PM -0500, Vivek Goyal wrote:
> Hi,
>
> We are working on DAX support in virtiofs and have some patches out of
> the tree hosted here.
>
> https://gitlab.com/virtio-fs/qemu/-/commits/virtio-fs-dev
any plans to post a non RFC version?
> These patches have not been proposed for merge yet, becasue David
> Gilbert noticed that we can run into a deadlock during an emergency
> reboot of guest kernel. (echo b > /proc/sysrq-trigger).
>
> I have provided details of deadlock in 4th path of the series with
> subject "qemu, vhost-user: Extend protocol to start/stop/flush slave
> channel".
>
> Basic problem seems to be that we don't have a proper mechanism to
> shutdown slave channel when vhost-user device is stopping. This means
> there might be pending messages in slave channel and slave is blocked
> and waiting for response.
>
> This is an RFC patch series to enhance vhost-user protocol to
> properly shutdown/flush slave channel and avoid the deadlock. Though
> we faced the issue in the context of virtiofs, any vhost-user
> device using slave channel can potentially run into issues and
> can benefit from these patches.
>
> Any feedback is welcome. Currently patches are based on out of
> tree code but after I get some feedback, I can only take pieces
> which are relevant to upstream and post separately.
>
> Thanks
> Vivek
>
> Vivek Goyal (6):
> virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
> libvhost-user: Use slave_mutex in all slave messages
> vhost-user: Return error code from slave_read()
> qemu, vhost-user: Extend protocol to start/stop/flush slave channel
> libvhost-user: Add support to start/stop/flush slave channel
> virtiofsd: Opt in for slave start/stop/shutdown functionality
>
> hw/virtio/vhost-user.c | 151 +++++++++++++++++++++-
> subprojects/libvhost-user/libvhost-user.c | 147 +++++++++++++++++----
> subprojects/libvhost-user/libvhost-user.h | 8 +-
> tools/virtiofsd/fuse_virtio.c | 20 +++
> 4 files changed, 294 insertions(+), 32 deletions(-)
>
> --
> 2.25.4
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [RFC PATCH 0/6] vhost-user: Shutdown/Flush slave channel properly
2021-02-23 14:14 ` [Virtio-fs] " Michael S. Tsirkin
@ 2021-02-23 14:23 ` Vivek Goyal
-1 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-02-23 14:23 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Greg Kurz, qemu-devel, virtio-fs, stefanha, marcandre.lureau, dgilbert
On Tue, Feb 23, 2021 at 09:14:16AM -0500, Michael S. Tsirkin wrote:
> On Mon, Jan 25, 2021 at 01:01:09PM -0500, Vivek Goyal wrote:
> > Hi,
> >
> > We are working on DAX support in virtiofs and have some patches out of
> > the tree hosted here.
> >
> > https://gitlab.com/virtio-fs/qemu/-/commits/virtio-fs-dev
>
> any plans to post a non RFC version?
We want to post a non-RFC version. But review comments have not been
taken care of yet.
Stefan says don't extend vhost-user protocl. Instead, modify
vhost_user_read() so that it polls both u->user->chr (unix domain socket)
as well as u->slave_fd. IOW, keep on servicing slave fd request while we
are waiting for vhost user message response.
Have not been able to figure out how to do that given unix domain
socket details are abstracted behind char device interface.
CCing Greg, He might have ideas on how do that.
Vivek
>
> > These patches have not been proposed for merge yet, becasue David
> > Gilbert noticed that we can run into a deadlock during an emergency
> > reboot of guest kernel. (echo b > /proc/sysrq-trigger).
> >
> > I have provided details of deadlock in 4th path of the series with
> > subject "qemu, vhost-user: Extend protocol to start/stop/flush slave
> > channel".
> >
> > Basic problem seems to be that we don't have a proper mechanism to
> > shutdown slave channel when vhost-user device is stopping. This means
> > there might be pending messages in slave channel and slave is blocked
> > and waiting for response.
> >
> > This is an RFC patch series to enhance vhost-user protocol to
> > properly shutdown/flush slave channel and avoid the deadlock. Though
> > we faced the issue in the context of virtiofs, any vhost-user
> > device using slave channel can potentially run into issues and
> > can benefit from these patches.
> >
> > Any feedback is welcome. Currently patches are based on out of
> > tree code but after I get some feedback, I can only take pieces
> > which are relevant to upstream and post separately.
> >
> > Thanks
> > Vivek
> >
> > Vivek Goyal (6):
> > virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
> > libvhost-user: Use slave_mutex in all slave messages
> > vhost-user: Return error code from slave_read()
> > qemu, vhost-user: Extend protocol to start/stop/flush slave channel
> > libvhost-user: Add support to start/stop/flush slave channel
> > virtiofsd: Opt in for slave start/stop/shutdown functionality
> >
> > hw/virtio/vhost-user.c | 151 +++++++++++++++++++++-
> > subprojects/libvhost-user/libvhost-user.c | 147 +++++++++++++++++----
> > subprojects/libvhost-user/libvhost-user.h | 8 +-
> > tools/virtiofsd/fuse_virtio.c | 20 +++
> > 4 files changed, 294 insertions(+), 32 deletions(-)
> >
> > --
> > 2.25.4
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Virtio-fs] [RFC PATCH 0/6] vhost-user: Shutdown/Flush slave channel properly
@ 2021-02-23 14:23 ` Vivek Goyal
0 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-02-23 14:23 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, virtio-fs, marcandre.lureau
On Tue, Feb 23, 2021 at 09:14:16AM -0500, Michael S. Tsirkin wrote:
> On Mon, Jan 25, 2021 at 01:01:09PM -0500, Vivek Goyal wrote:
> > Hi,
> >
> > We are working on DAX support in virtiofs and have some patches out of
> > the tree hosted here.
> >
> > https://gitlab.com/virtio-fs/qemu/-/commits/virtio-fs-dev
>
> any plans to post a non RFC version?
We want to post a non-RFC version. But review comments have not been
taken care of yet.
Stefan says don't extend vhost-user protocl. Instead, modify
vhost_user_read() so that it polls both u->user->chr (unix domain socket)
as well as u->slave_fd. IOW, keep on servicing slave fd request while we
are waiting for vhost user message response.
Have not been able to figure out how to do that given unix domain
socket details are abstracted behind char device interface.
CCing Greg, He might have ideas on how do that.
Vivek
>
> > These patches have not been proposed for merge yet, becasue David
> > Gilbert noticed that we can run into a deadlock during an emergency
> > reboot of guest kernel. (echo b > /proc/sysrq-trigger).
> >
> > I have provided details of deadlock in 4th path of the series with
> > subject "qemu, vhost-user: Extend protocol to start/stop/flush slave
> > channel".
> >
> > Basic problem seems to be that we don't have a proper mechanism to
> > shutdown slave channel when vhost-user device is stopping. This means
> > there might be pending messages in slave channel and slave is blocked
> > and waiting for response.
> >
> > This is an RFC patch series to enhance vhost-user protocol to
> > properly shutdown/flush slave channel and avoid the deadlock. Though
> > we faced the issue in the context of virtiofs, any vhost-user
> > device using slave channel can potentially run into issues and
> > can benefit from these patches.
> >
> > Any feedback is welcome. Currently patches are based on out of
> > tree code but after I get some feedback, I can only take pieces
> > which are relevant to upstream and post separately.
> >
> > Thanks
> > Vivek
> >
> > Vivek Goyal (6):
> > virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
> > libvhost-user: Use slave_mutex in all slave messages
> > vhost-user: Return error code from slave_read()
> > qemu, vhost-user: Extend protocol to start/stop/flush slave channel
> > libvhost-user: Add support to start/stop/flush slave channel
> > virtiofsd: Opt in for slave start/stop/shutdown functionality
> >
> > hw/virtio/vhost-user.c | 151 +++++++++++++++++++++-
> > subprojects/libvhost-user/libvhost-user.c | 147 +++++++++++++++++----
> > subprojects/libvhost-user/libvhost-user.h | 8 +-
> > tools/virtiofsd/fuse_virtio.c | 20 +++
> > 4 files changed, 294 insertions(+), 32 deletions(-)
> >
> > --
> > 2.25.4
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [RFC PATCH 0/6] vhost-user: Shutdown/Flush slave channel properly
2021-01-25 18:01 ` [Virtio-fs] " Vivek Goyal
@ 2021-03-14 22:21 ` Michael S. Tsirkin
-1 siblings, 0 replies; 52+ messages in thread
From: Michael S. Tsirkin @ 2021-03-14 22:21 UTC (permalink / raw)
To: Vivek Goyal; +Cc: virtio-fs, marcandre.lureau, qemu-devel, stefanha, dgilbert
On Mon, Jan 25, 2021 at 01:01:09PM -0500, Vivek Goyal wrote:
> Hi,
>
> We are working on DAX support in virtiofs and have some patches out of
> the tree hosted here.
>
> https://gitlab.com/virtio-fs/qemu/-/commits/virtio-fs-dev
ping anyone wants to pick this up and post a non-rfc version?
> These patches have not been proposed for merge yet, becasue David
> Gilbert noticed that we can run into a deadlock during an emergency
> reboot of guest kernel. (echo b > /proc/sysrq-trigger).
>
> I have provided details of deadlock in 4th path of the series with
> subject "qemu, vhost-user: Extend protocol to start/stop/flush slave
> channel".
>
> Basic problem seems to be that we don't have a proper mechanism to
> shutdown slave channel when vhost-user device is stopping. This means
> there might be pending messages in slave channel and slave is blocked
> and waiting for response.
>
> This is an RFC patch series to enhance vhost-user protocol to
> properly shutdown/flush slave channel and avoid the deadlock. Though
> we faced the issue in the context of virtiofs, any vhost-user
> device using slave channel can potentially run into issues and
> can benefit from these patches.
>
> Any feedback is welcome. Currently patches are based on out of
> tree code but after I get some feedback, I can only take pieces
> which are relevant to upstream and post separately.
>
> Thanks
> Vivek
>
> Vivek Goyal (6):
> virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
> libvhost-user: Use slave_mutex in all slave messages
> vhost-user: Return error code from slave_read()
> qemu, vhost-user: Extend protocol to start/stop/flush slave channel
> libvhost-user: Add support to start/stop/flush slave channel
> virtiofsd: Opt in for slave start/stop/shutdown functionality
>
> hw/virtio/vhost-user.c | 151 +++++++++++++++++++++-
> subprojects/libvhost-user/libvhost-user.c | 147 +++++++++++++++++----
> subprojects/libvhost-user/libvhost-user.h | 8 +-
> tools/virtiofsd/fuse_virtio.c | 20 +++
> 4 files changed, 294 insertions(+), 32 deletions(-)
>
> --
> 2.25.4
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Virtio-fs] [RFC PATCH 0/6] vhost-user: Shutdown/Flush slave channel properly
@ 2021-03-14 22:21 ` Michael S. Tsirkin
0 siblings, 0 replies; 52+ messages in thread
From: Michael S. Tsirkin @ 2021-03-14 22:21 UTC (permalink / raw)
To: Vivek Goyal; +Cc: virtio-fs, marcandre.lureau, qemu-devel
On Mon, Jan 25, 2021 at 01:01:09PM -0500, Vivek Goyal wrote:
> Hi,
>
> We are working on DAX support in virtiofs and have some patches out of
> the tree hosted here.
>
> https://gitlab.com/virtio-fs/qemu/-/commits/virtio-fs-dev
ping anyone wants to pick this up and post a non-rfc version?
> These patches have not been proposed for merge yet, becasue David
> Gilbert noticed that we can run into a deadlock during an emergency
> reboot of guest kernel. (echo b > /proc/sysrq-trigger).
>
> I have provided details of deadlock in 4th path of the series with
> subject "qemu, vhost-user: Extend protocol to start/stop/flush slave
> channel".
>
> Basic problem seems to be that we don't have a proper mechanism to
> shutdown slave channel when vhost-user device is stopping. This means
> there might be pending messages in slave channel and slave is blocked
> and waiting for response.
>
> This is an RFC patch series to enhance vhost-user protocol to
> properly shutdown/flush slave channel and avoid the deadlock. Though
> we faced the issue in the context of virtiofs, any vhost-user
> device using slave channel can potentially run into issues and
> can benefit from these patches.
>
> Any feedback is welcome. Currently patches are based on out of
> tree code but after I get some feedback, I can only take pieces
> which are relevant to upstream and post separately.
>
> Thanks
> Vivek
>
> Vivek Goyal (6):
> virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
> libvhost-user: Use slave_mutex in all slave messages
> vhost-user: Return error code from slave_read()
> qemu, vhost-user: Extend protocol to start/stop/flush slave channel
> libvhost-user: Add support to start/stop/flush slave channel
> virtiofsd: Opt in for slave start/stop/shutdown functionality
>
> hw/virtio/vhost-user.c | 151 +++++++++++++++++++++-
> subprojects/libvhost-user/libvhost-user.c | 147 +++++++++++++++++----
> subprojects/libvhost-user/libvhost-user.h | 8 +-
> tools/virtiofsd/fuse_virtio.c | 20 +++
> 4 files changed, 294 insertions(+), 32 deletions(-)
>
> --
> 2.25.4
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [RFC PATCH 0/6] vhost-user: Shutdown/Flush slave channel properly
2021-03-14 22:21 ` [Virtio-fs] " Michael S. Tsirkin
@ 2021-03-14 22:26 ` Vivek Goyal
-1 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-03-14 22:26 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Greg Kurz, qemu-devel, virtio-fs, stefanha, marcandre.lureau, dgilbert
On Sun, Mar 14, 2021 at 06:21:04PM -0400, Michael S. Tsirkin wrote:
> On Mon, Jan 25, 2021 at 01:01:09PM -0500, Vivek Goyal wrote:
> > Hi,
> >
> > We are working on DAX support in virtiofs and have some patches out of
> > the tree hosted here.
> >
> > https://gitlab.com/virtio-fs/qemu/-/commits/virtio-fs-dev
>
> ping anyone wants to pick this up and post a non-rfc version?
Hi Michael,
Greg has picked this work and has alredy posted V2 of patches here.
https://lore.kernel.org/qemu-devel/20210312092212.782255-8-groug@kaod.org/T/
Please have a look.
Thanks
Vivek
>
> > These patches have not been proposed for merge yet, becasue David
> > Gilbert noticed that we can run into a deadlock during an emergency
> > reboot of guest kernel. (echo b > /proc/sysrq-trigger).
> >
> > I have provided details of deadlock in 4th path of the series with
> > subject "qemu, vhost-user: Extend protocol to start/stop/flush slave
> > channel".
> >
> > Basic problem seems to be that we don't have a proper mechanism to
> > shutdown slave channel when vhost-user device is stopping. This means
> > there might be pending messages in slave channel and slave is blocked
> > and waiting for response.
> >
> > This is an RFC patch series to enhance vhost-user protocol to
> > properly shutdown/flush slave channel and avoid the deadlock. Though
> > we faced the issue in the context of virtiofs, any vhost-user
> > device using slave channel can potentially run into issues and
> > can benefit from these patches.
> >
> > Any feedback is welcome. Currently patches are based on out of
> > tree code but after I get some feedback, I can only take pieces
> > which are relevant to upstream and post separately.
> >
> > Thanks
> > Vivek
> >
> > Vivek Goyal (6):
> > virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
> > libvhost-user: Use slave_mutex in all slave messages
> > vhost-user: Return error code from slave_read()
> > qemu, vhost-user: Extend protocol to start/stop/flush slave channel
> > libvhost-user: Add support to start/stop/flush slave channel
> > virtiofsd: Opt in for slave start/stop/shutdown functionality
> >
> > hw/virtio/vhost-user.c | 151 +++++++++++++++++++++-
> > subprojects/libvhost-user/libvhost-user.c | 147 +++++++++++++++++----
> > subprojects/libvhost-user/libvhost-user.h | 8 +-
> > tools/virtiofsd/fuse_virtio.c | 20 +++
> > 4 files changed, 294 insertions(+), 32 deletions(-)
> >
> > --
> > 2.25.4
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Virtio-fs] [RFC PATCH 0/6] vhost-user: Shutdown/Flush slave channel properly
@ 2021-03-14 22:26 ` Vivek Goyal
0 siblings, 0 replies; 52+ messages in thread
From: Vivek Goyal @ 2021-03-14 22:26 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, virtio-fs, marcandre.lureau
On Sun, Mar 14, 2021 at 06:21:04PM -0400, Michael S. Tsirkin wrote:
> On Mon, Jan 25, 2021 at 01:01:09PM -0500, Vivek Goyal wrote:
> > Hi,
> >
> > We are working on DAX support in virtiofs and have some patches out of
> > the tree hosted here.
> >
> > https://gitlab.com/virtio-fs/qemu/-/commits/virtio-fs-dev
>
> ping anyone wants to pick this up and post a non-rfc version?
Hi Michael,
Greg has picked this work and has alredy posted V2 of patches here.
https://lore.kernel.org/qemu-devel/20210312092212.782255-8-groug@kaod.org/T/
Please have a look.
Thanks
Vivek
>
> > These patches have not been proposed for merge yet, becasue David
> > Gilbert noticed that we can run into a deadlock during an emergency
> > reboot of guest kernel. (echo b > /proc/sysrq-trigger).
> >
> > I have provided details of deadlock in 4th path of the series with
> > subject "qemu, vhost-user: Extend protocol to start/stop/flush slave
> > channel".
> >
> > Basic problem seems to be that we don't have a proper mechanism to
> > shutdown slave channel when vhost-user device is stopping. This means
> > there might be pending messages in slave channel and slave is blocked
> > and waiting for response.
> >
> > This is an RFC patch series to enhance vhost-user protocol to
> > properly shutdown/flush slave channel and avoid the deadlock. Though
> > we faced the issue in the context of virtiofs, any vhost-user
> > device using slave channel can potentially run into issues and
> > can benefit from these patches.
> >
> > Any feedback is welcome. Currently patches are based on out of
> > tree code but after I get some feedback, I can only take pieces
> > which are relevant to upstream and post separately.
> >
> > Thanks
> > Vivek
> >
> > Vivek Goyal (6):
> > virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
> > libvhost-user: Use slave_mutex in all slave messages
> > vhost-user: Return error code from slave_read()
> > qemu, vhost-user: Extend protocol to start/stop/flush slave channel
> > libvhost-user: Add support to start/stop/flush slave channel
> > virtiofsd: Opt in for slave start/stop/shutdown functionality
> >
> > hw/virtio/vhost-user.c | 151 +++++++++++++++++++++-
> > subprojects/libvhost-user/libvhost-user.c | 147 +++++++++++++++++----
> > subprojects/libvhost-user/libvhost-user.h | 8 +-
> > tools/virtiofsd/fuse_virtio.c | 20 +++
> > 4 files changed, 294 insertions(+), 32 deletions(-)
> >
> > --
> > 2.25.4
>
^ permalink raw reply [flat|nested] 52+ messages in thread
end of thread, other threads:[~2021-03-14 22:27 UTC | newest]
Thread overview: 52+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-25 18:01 [RFC PATCH 0/6] vhost-user: Shutdown/Flush slave channel properly Vivek Goyal
2021-01-25 18:01 ` [Virtio-fs] " Vivek Goyal
2021-01-25 18:01 ` [PATCH 1/6] virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit Vivek Goyal
2021-01-25 18:01 ` [Virtio-fs] " Vivek Goyal
2021-01-26 15:56 ` Greg Kurz
2021-01-26 15:56 ` [Virtio-fs] " Greg Kurz
2021-01-26 18:33 ` Vivek Goyal
2021-01-26 18:33 ` [Virtio-fs] " Vivek Goyal
2021-01-29 12:03 ` Greg Kurz
2021-01-29 12:03 ` [Virtio-fs] " Greg Kurz
2021-01-29 15:04 ` Vivek Goyal
2021-01-29 15:04 ` [Virtio-fs] " Vivek Goyal
2021-01-25 18:01 ` [PATCH 2/6] libvhost-user: Use slave_mutex in all slave messages Vivek Goyal
2021-01-25 18:01 ` [Virtio-fs] " Vivek Goyal
2021-01-28 14:31 ` Greg Kurz
2021-01-28 14:31 ` [Virtio-fs] " Greg Kurz
2021-01-28 14:48 ` Vivek Goyal
2021-01-28 14:48 ` [Virtio-fs] " Vivek Goyal
2021-01-28 15:06 ` Greg Kurz
2021-01-28 15:06 ` [Virtio-fs] " Greg Kurz
2021-01-25 18:01 ` [PATCH 3/6] vhost-user: Return error code from slave_read() Vivek Goyal
2021-01-25 18:01 ` [Virtio-fs] " Vivek Goyal
2021-01-29 9:45 ` Greg Kurz
2021-01-29 9:45 ` [Virtio-fs] " Greg Kurz
2021-01-29 15:02 ` Vivek Goyal
2021-01-29 15:02 ` [Virtio-fs] " Vivek Goyal
2021-01-25 18:01 ` [PATCH 4/6] qemu, vhost-user: Extend protocol to start/stop/flush slave channel Vivek Goyal
2021-01-25 18:01 ` [Virtio-fs] " Vivek Goyal
2021-01-28 16:52 ` Stefan Hajnoczi
2021-01-28 16:52 ` [Virtio-fs] " Stefan Hajnoczi
2021-01-29 14:16 ` Vivek Goyal
2021-01-29 14:16 ` [Virtio-fs] " Vivek Goyal
2021-01-29 15:11 ` Vivek Goyal
2021-01-29 15:11 ` [Virtio-fs] " Vivek Goyal
2021-02-08 17:41 ` Stefan Hajnoczi
2021-02-08 17:41 ` [Virtio-fs] " Stefan Hajnoczi
2021-01-25 18:01 ` [PATCH 5/6] libvhost-user: Add support " Vivek Goyal
2021-01-25 18:01 ` [Virtio-fs] " Vivek Goyal
2021-01-25 18:01 ` [PATCH 6/6] virtiofsd: Opt in for slave start/stop/shutdown functionality Vivek Goyal
2021-01-25 18:01 ` [Virtio-fs] " Vivek Goyal
2021-02-10 21:39 ` [RFC PATCH 0/6] vhost-user: Shutdown/Flush slave channel properly Michael S. Tsirkin
2021-02-10 21:39 ` [Virtio-fs] " Michael S. Tsirkin
2021-02-10 22:15 ` Vivek Goyal
2021-02-10 22:15 ` [Virtio-fs] " Vivek Goyal
2021-02-23 14:14 ` Michael S. Tsirkin
2021-02-23 14:14 ` [Virtio-fs] " Michael S. Tsirkin
2021-02-23 14:23 ` Vivek Goyal
2021-02-23 14:23 ` [Virtio-fs] " Vivek Goyal
2021-03-14 22:21 ` Michael S. Tsirkin
2021-03-14 22:21 ` [Virtio-fs] " Michael S. Tsirkin
2021-03-14 22:26 ` Vivek Goyal
2021-03-14 22:26 ` [Virtio-fs] " Vivek Goyal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.