From: Greg Kurz <groug@kaod.org> To: qemu-devel@nongnu.org Cc: virtio-fs@redhat.com, Greg Kurz <groug@kaod.org>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Vivek Goyal <vgoyal@redhat.com> Subject: [PATCH v2] virtiofsd: vu_dispatch locking should never fail Date: Wed, 3 Feb 2021 19:24:34 +0100 [thread overview] Message-ID: <20210203182434.93870-1-groug@kaod.org> (raw) pthread_rwlock_rdlock() and pthread_rwlock_wrlock() can fail if a deadlock condition is detected or the current thread already owns the lock. They can also fail, like pthread_rwlock_unlock(), if the mutex wasn't properly initialized. None of these are ever expected to happen with fv_VuDev::vu_dispatch_rwlock. Some users already check the return value and assert, some others don't. Introduce rdlock/wrlock/unlock wrappers that just do the former and use them everywhere for improved consistency and robustness. This is just cleanup. It doesn't fix any actual issue. Signed-off-by: Greg Kurz <groug@kaod.org> --- v2: - open-code helpers instead of defining them with a macro (Vivek, Stefan) - fixed rd/wr typo in fv_queue_thread() (Stefan) - make it clear in the changelog this is just cleanup (Stefan) tools/virtiofsd/fuse_virtio.c | 49 +++++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c index ddcefee4272f..523ee64fb7ae 100644 --- a/tools/virtiofsd/fuse_virtio.c +++ b/tools/virtiofsd/fuse_virtio.c @@ -187,6 +187,31 @@ static void copy_iov(struct iovec *src_iov, int src_count, } } +/* + * pthread_rwlock_rdlock() and pthread_rwlock_wrlock can fail if + * a deadlock condition is detected or the current thread already + * owns the lock. They can also fail, like pthread_rwlock_unlock(), + * if the mutex wasn't properly initialized. None of these are ever + * expected to happen. + */ +static void vu_dispatch_rdlock(struct fv_VuDev *vud) +{ + int ret = pthread_rwlock_rdlock(&vud->vu_dispatch_rwlock); + assert(ret == 0); +} + +static void vu_dispatch_wrlock(struct fv_VuDev *vud) +{ + int ret = pthread_rwlock_wrlock(&vud->vu_dispatch_rwlock); + assert(ret == 0); +} + +static void vu_dispatch_unlock(struct fv_VuDev *vud) +{ + int ret = pthread_rwlock_unlock(&vud->vu_dispatch_rwlock); + assert(ret == 0); +} + /* * Called back by ll whenever it wants to send a reply/message back * The 1st element of the iov starts with the fuse_out_header @@ -240,12 +265,12 @@ int virtio_send_msg(struct fuse_session *se, struct fuse_chan *ch, copy_iov(iov, count, in_sg, in_num, tosend_len); - pthread_rwlock_rdlock(&qi->virtio_dev->vu_dispatch_rwlock); + vu_dispatch_rdlock(qi->virtio_dev); pthread_mutex_lock(&qi->vq_lock); vu_queue_push(dev, q, elem, tosend_len); vu_queue_notify(dev, q); pthread_mutex_unlock(&qi->vq_lock); - pthread_rwlock_unlock(&qi->virtio_dev->vu_dispatch_rwlock); + vu_dispatch_unlock(qi->virtio_dev); req->reply_sent = true; @@ -403,12 +428,12 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch, ret = 0; - pthread_rwlock_rdlock(&qi->virtio_dev->vu_dispatch_rwlock); + vu_dispatch_rdlock(qi->virtio_dev); pthread_mutex_lock(&qi->vq_lock); vu_queue_push(dev, q, elem, tosend_len); vu_queue_notify(dev, q); pthread_mutex_unlock(&qi->vq_lock); - pthread_rwlock_unlock(&qi->virtio_dev->vu_dispatch_rwlock); + vu_dispatch_unlock(qi->virtio_dev); err: if (ret == 0) { @@ -558,12 +583,12 @@ out: fuse_log(FUSE_LOG_DEBUG, "%s: elem %d no reply sent\n", __func__, elem->index); - pthread_rwlock_rdlock(&qi->virtio_dev->vu_dispatch_rwlock); + vu_dispatch_rdlock(qi->virtio_dev); pthread_mutex_lock(&qi->vq_lock); vu_queue_push(dev, q, elem, 0); vu_queue_notify(dev, q); pthread_mutex_unlock(&qi->vq_lock); - pthread_rwlock_unlock(&qi->virtio_dev->vu_dispatch_rwlock); + vu_dispatch_unlock(qi->virtio_dev); } pthread_mutex_destroy(&req->ch.lock); @@ -596,7 +621,6 @@ static void *fv_queue_thread(void *opaque) qi->qidx, qi->kick_fd); while (1) { struct pollfd pf[2]; - int ret; pf[0].fd = qi->kick_fd; pf[0].events = POLLIN; @@ -645,8 +669,7 @@ static void *fv_queue_thread(void *opaque) break; } /* Mutual exclusion with virtio_loop() */ - ret = pthread_rwlock_rdlock(&qi->virtio_dev->vu_dispatch_rwlock); - assert(ret == 0); /* there is no possible error case */ + vu_dispatch_rdlock(qi->virtio_dev); pthread_mutex_lock(&qi->vq_lock); /* out is from guest, in is too guest */ unsigned int in_bytes, out_bytes; @@ -672,7 +695,7 @@ static void *fv_queue_thread(void *opaque) } pthread_mutex_unlock(&qi->vq_lock); - pthread_rwlock_unlock(&qi->virtio_dev->vu_dispatch_rwlock); + vu_dispatch_unlock(qi->virtio_dev); /* Process all the requests. */ if (!se->thread_pool_size && req_list != NULL) { @@ -799,7 +822,6 @@ int virtio_loop(struct fuse_session *se) while (!fuse_session_exited(se)) { struct pollfd pf[1]; bool ok; - int ret; pf[0].fd = se->vu_socketfd; pf[0].events = POLLIN; pf[0].revents = 0; @@ -825,12 +847,11 @@ int virtio_loop(struct fuse_session *se) assert(pf[0].revents & POLLIN); fuse_log(FUSE_LOG_DEBUG, "%s: Got VU event\n", __func__); /* Mutual exclusion with fv_queue_thread() */ - ret = pthread_rwlock_wrlock(&se->virtio_dev->vu_dispatch_rwlock); - assert(ret == 0); /* there is no possible error case */ + vu_dispatch_wrlock(se->virtio_dev); ok = vu_dispatch(&se->virtio_dev->dev); - pthread_rwlock_unlock(&se->virtio_dev->vu_dispatch_rwlock); + vu_dispatch_unlock(se->virtio_dev); if (!ok) { fuse_log(FUSE_LOG_ERR, "%s: vu_dispatch failed\n", __func__); -- 2.26.2
WARNING: multiple messages have this Message-ID (diff)
From: Greg Kurz <groug@kaod.org> To: qemu-devel@nongnu.org Cc: virtio-fs@redhat.com, Vivek Goyal <vgoyal@redhat.com> Subject: [Virtio-fs] [PATCH v2] virtiofsd: vu_dispatch locking should never fail Date: Wed, 3 Feb 2021 19:24:34 +0100 [thread overview] Message-ID: <20210203182434.93870-1-groug@kaod.org> (raw) pthread_rwlock_rdlock() and pthread_rwlock_wrlock() can fail if a deadlock condition is detected or the current thread already owns the lock. They can also fail, like pthread_rwlock_unlock(), if the mutex wasn't properly initialized. None of these are ever expected to happen with fv_VuDev::vu_dispatch_rwlock. Some users already check the return value and assert, some others don't. Introduce rdlock/wrlock/unlock wrappers that just do the former and use them everywhere for improved consistency and robustness. This is just cleanup. It doesn't fix any actual issue. Signed-off-by: Greg Kurz <groug@kaod.org> --- v2: - open-code helpers instead of defining them with a macro (Vivek, Stefan) - fixed rd/wr typo in fv_queue_thread() (Stefan) - make it clear in the changelog this is just cleanup (Stefan) tools/virtiofsd/fuse_virtio.c | 49 +++++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c index ddcefee4272f..523ee64fb7ae 100644 --- a/tools/virtiofsd/fuse_virtio.c +++ b/tools/virtiofsd/fuse_virtio.c @@ -187,6 +187,31 @@ static void copy_iov(struct iovec *src_iov, int src_count, } } +/* + * pthread_rwlock_rdlock() and pthread_rwlock_wrlock can fail if + * a deadlock condition is detected or the current thread already + * owns the lock. They can also fail, like pthread_rwlock_unlock(), + * if the mutex wasn't properly initialized. None of these are ever + * expected to happen. + */ +static void vu_dispatch_rdlock(struct fv_VuDev *vud) +{ + int ret = pthread_rwlock_rdlock(&vud->vu_dispatch_rwlock); + assert(ret == 0); +} + +static void vu_dispatch_wrlock(struct fv_VuDev *vud) +{ + int ret = pthread_rwlock_wrlock(&vud->vu_dispatch_rwlock); + assert(ret == 0); +} + +static void vu_dispatch_unlock(struct fv_VuDev *vud) +{ + int ret = pthread_rwlock_unlock(&vud->vu_dispatch_rwlock); + assert(ret == 0); +} + /* * Called back by ll whenever it wants to send a reply/message back * The 1st element of the iov starts with the fuse_out_header @@ -240,12 +265,12 @@ int virtio_send_msg(struct fuse_session *se, struct fuse_chan *ch, copy_iov(iov, count, in_sg, in_num, tosend_len); - pthread_rwlock_rdlock(&qi->virtio_dev->vu_dispatch_rwlock); + vu_dispatch_rdlock(qi->virtio_dev); pthread_mutex_lock(&qi->vq_lock); vu_queue_push(dev, q, elem, tosend_len); vu_queue_notify(dev, q); pthread_mutex_unlock(&qi->vq_lock); - pthread_rwlock_unlock(&qi->virtio_dev->vu_dispatch_rwlock); + vu_dispatch_unlock(qi->virtio_dev); req->reply_sent = true; @@ -403,12 +428,12 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch, ret = 0; - pthread_rwlock_rdlock(&qi->virtio_dev->vu_dispatch_rwlock); + vu_dispatch_rdlock(qi->virtio_dev); pthread_mutex_lock(&qi->vq_lock); vu_queue_push(dev, q, elem, tosend_len); vu_queue_notify(dev, q); pthread_mutex_unlock(&qi->vq_lock); - pthread_rwlock_unlock(&qi->virtio_dev->vu_dispatch_rwlock); + vu_dispatch_unlock(qi->virtio_dev); err: if (ret == 0) { @@ -558,12 +583,12 @@ out: fuse_log(FUSE_LOG_DEBUG, "%s: elem %d no reply sent\n", __func__, elem->index); - pthread_rwlock_rdlock(&qi->virtio_dev->vu_dispatch_rwlock); + vu_dispatch_rdlock(qi->virtio_dev); pthread_mutex_lock(&qi->vq_lock); vu_queue_push(dev, q, elem, 0); vu_queue_notify(dev, q); pthread_mutex_unlock(&qi->vq_lock); - pthread_rwlock_unlock(&qi->virtio_dev->vu_dispatch_rwlock); + vu_dispatch_unlock(qi->virtio_dev); } pthread_mutex_destroy(&req->ch.lock); @@ -596,7 +621,6 @@ static void *fv_queue_thread(void *opaque) qi->qidx, qi->kick_fd); while (1) { struct pollfd pf[2]; - int ret; pf[0].fd = qi->kick_fd; pf[0].events = POLLIN; @@ -645,8 +669,7 @@ static void *fv_queue_thread(void *opaque) break; } /* Mutual exclusion with virtio_loop() */ - ret = pthread_rwlock_rdlock(&qi->virtio_dev->vu_dispatch_rwlock); - assert(ret == 0); /* there is no possible error case */ + vu_dispatch_rdlock(qi->virtio_dev); pthread_mutex_lock(&qi->vq_lock); /* out is from guest, in is too guest */ unsigned int in_bytes, out_bytes; @@ -672,7 +695,7 @@ static void *fv_queue_thread(void *opaque) } pthread_mutex_unlock(&qi->vq_lock); - pthread_rwlock_unlock(&qi->virtio_dev->vu_dispatch_rwlock); + vu_dispatch_unlock(qi->virtio_dev); /* Process all the requests. */ if (!se->thread_pool_size && req_list != NULL) { @@ -799,7 +822,6 @@ int virtio_loop(struct fuse_session *se) while (!fuse_session_exited(se)) { struct pollfd pf[1]; bool ok; - int ret; pf[0].fd = se->vu_socketfd; pf[0].events = POLLIN; pf[0].revents = 0; @@ -825,12 +847,11 @@ int virtio_loop(struct fuse_session *se) assert(pf[0].revents & POLLIN); fuse_log(FUSE_LOG_DEBUG, "%s: Got VU event\n", __func__); /* Mutual exclusion with fv_queue_thread() */ - ret = pthread_rwlock_wrlock(&se->virtio_dev->vu_dispatch_rwlock); - assert(ret == 0); /* there is no possible error case */ + vu_dispatch_wrlock(se->virtio_dev); ok = vu_dispatch(&se->virtio_dev->dev); - pthread_rwlock_unlock(&se->virtio_dev->vu_dispatch_rwlock); + vu_dispatch_unlock(se->virtio_dev); if (!ok) { fuse_log(FUSE_LOG_ERR, "%s: vu_dispatch failed\n", __func__); -- 2.26.2
next reply other threads:[~2021-02-03 18:26 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-02-03 18:24 Greg Kurz [this message] 2021-02-03 18:24 ` [Virtio-fs] [PATCH v2] virtiofsd: vu_dispatch locking should never fail Greg Kurz 2021-02-03 20:23 ` Vivek Goyal 2021-02-03 20:23 ` [Virtio-fs] " Vivek Goyal 2021-02-04 9:37 ` Stefan Hajnoczi 2021-02-04 9:37 ` [Virtio-fs] " Stefan Hajnoczi 2021-02-16 11:24 ` Dr. David Alan Gilbert 2021-02-16 11:24 ` [Virtio-fs] " Dr. David Alan Gilbert
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210203182434.93870-1-groug@kaod.org \ --to=groug@kaod.org \ --cc=dgilbert@redhat.com \ --cc=qemu-devel@nongnu.org \ --cc=stefanha@redhat.com \ --cc=vgoyal@redhat.com \ --cc=virtio-fs@redhat.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.