[PATCH] virtio-fs: fix lost wakeup in driver

[Fotis Xenakis]

Previously, virtio::fs::fuse_request used a waitqueue for waiting until
the request was processed by the device. After the request was
dispatched, the calling thread would wait on that waitqueue, woken by
the driver's request callback thread, when the request was processed.
This left room for a lost wakeup, where the request would be processed
before the interested thread would wait() on it (happened during testing
with fio). This is now fixed, using a waiter instead of a waitqueue.

This also includes a few, cosmetic mostly, improvements.

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>
---
drivers/virtio-fs.cc | 83 +++++++++++-----------------------
drivers/virtio-fs.hh | 28 +++++++++---
fs/virtiofs/virtiofs_vfsops.cc | 6 ++-
3 files changed, 52 insertions(+), 65 deletions(-)

diff --git a/drivers/virtio-fs.cc b/drivers/virtio-fs.cc
index e0e090bc..00f862a1 100644
--- a/drivers/virtio-fs.cc
+++ b/drivers/virtio-fs.cc
@@ -5,68 +5,44 @@
* BSD license as described in the LICENSE file in the top-level directory.
*/

-#include <sys/cdefs.h>
-
-#include "drivers/virtio.hh"
-#include "drivers/virtio-fs.hh"
-#include <osv/interrupt.hh>
-
-#include <osv/mempool.hh>
-#include <osv/mmu.hh>
-
#include <string>
-#include <string.h>
-#include <map>
-#include <errno.h>
-#include <osv/debug.h>
-
-#include <osv/sched.hh>
-#include "osv/trace.hh"
-#include "osv/aligned_new.hh"

+#include <osv/debug.h>
#include <osv/device.h>
+#include <osv/interrupt.hh>
+#include <osv/mmio.hh>
+#include <osv/msi.hh>
+#include <osv/sched.hh>

-using namespace memory;
+#include "drivers/pci-device.hh"
+#include "drivers/virtio.hh"
+#include "drivers/virtio-fs.hh"
+#include "drivers/virtio-vring.hh"
+#include "fs/virtiofs/fuse_kernel.h"

using fuse_request = virtio::fs::fuse_request;

namespace virtio {

-// Wait for the request to be marked as completed.
-void fs::fuse_request::wait()
-{
- WITH_LOCK(req_mutex) {
- req_wait.wait(req_mutex);
- }
-}
-
-// Mark the request as completed.
-void fs::fuse_request::done()
-{
- WITH_LOCK(req_mutex) {
- req_wait.wake_one(req_mutex);
- }
-}
-
-static void fuse_req_enqueue_input(vring* queue, fuse_request* req)
+static void fuse_req_enqueue_input(vring& queue, fuse_request& req)
{
// Header goes first
- queue->add_out_sg(&req->in_header, sizeof(struct fuse_in_header));
+ queue.add_out_sg(&req.in_header, sizeof(struct fuse_in_header));

// Add fuse in arguments as out sg
- if (req->input_args_size > 0) {
- queue->add_out_sg(req->input_args_data, req->input_args_size);
+ if (req.input_args_size > 0) {
+ queue.add_out_sg(req.input_args_data, req.input_args_size);
}
}

-static void fuse_req_enqueue_output(vring* queue, fuse_request* req)
+static void fuse_req_enqueue_output(vring& queue, fuse_request& req)
{
// Header goes first
- queue->add_in_sg(&req->out_header, sizeof(struct fuse_out_header));
+ queue.add_in_sg(&req.out_header, sizeof(struct fuse_out_header));

// Add fuse out arguments as in sg
- if (req->output_args_size > 0) {
- queue->add_in_sg(req->output_args_data, req->output_args_size);
+ if (req.output_args_size > 0) {
+ queue.add_in_sg(req.output_args_data, req.output_args_size);
}
}

@@ -194,7 +170,7 @@ void fs::req_done()
auto* queue = get_virt_queue(VQ_REQUEST);

while (true) {
- virtio_driver::wait_for_queue(queue, &vring::used_ring_not_empty);
+ wait_for_queue(queue, &vring::used_ring_not_empty);

fuse_request* req;
u32 len;
@@ -210,26 +186,21 @@ void fs::req_done()
}
}

-int fs::make_request(fuse_request* req)
+int fs::make_request(fuse_request& req)
{
- // The lock is here for parallel requests protection
- WITH_LOCK(_lock) {
- if (!req) {
- return EIO;
- }
-
- auto* queue = get_virt_queue(VQ_REQUEST);
+ auto* queue = get_virt_queue(VQ_REQUEST);

+ WITH_LOCK(_lock) {
queue->init_sg();

- fuse_req_enqueue_input(queue, req);
- fuse_req_enqueue_output(queue, req);
+ fuse_req_enqueue_input(*queue, req);
+ fuse_req_enqueue_output(*queue, req);

- queue->add_buf_wait(req);
+ queue->add_buf_wait(&req);
queue->kick();
-
- return 0;
}
+
+ return 0;
}

hw_driver* fs::probe(hw_device* dev)
diff --git a/drivers/virtio-fs.hh b/drivers/virtio-fs.hh
index 88a16939..0505a9b0 100644
--- a/drivers/virtio-fs.hh
+++ b/drivers/virtio-fs.hh
@@ -10,9 +10,13 @@

#include <functional>

+#include <osv/mmio.hh>
#include <osv/mutex.h>
-#include <osv/waitqueue.hh>
+#include <osv/sched.hh>
+#include <osv/wait_record.hh>

+#include "drivers/device.hh"
+#include "drivers/driver.hh"
#include "drivers/virtio.hh"
#include "drivers/virtio-device.hh"
#include "fs/virtiofs/fuse_kernel.h"
@@ -26,6 +30,8 @@ enum {

class fs : public virtio_driver {
public:
+ // A fuse_request encapsulates a low-level virtio-fs device request. This is
+ // a single-use object.
struct fuse_request {
struct fuse_in_header in_header;
struct fuse_out_header out_header;
@@ -36,12 +42,19 @@ public:
void* output_args_data;
size_t output_args_size;

- void wait();
- void done();
+ // Constructs a fuse_request, determining that wait() on it will be
+ // called by @t.
+ fuse_request(sched::thread* t): processed{t} {}
+
+ // Waits for the request to be marked as completed. Should only be
+ // called once, from the thread specified in the constructor.
+ void wait() { processed.wait(); }
+ // Marks the request as completed. Should only be called once.
+ void done() { processed.wake(); }

private:
- mutex_t req_mutex;
- waitqueue req_wait;
+ // Signifies whether the request has been processed by the device
+ waiter processed;
};

struct fs_config {
@@ -67,7 +80,7 @@ public:
virtual std::string get_name() const { return _driver_name; }
void read_config();

- int make_request(fuse_request*);
+ int make_request(fuse_request&);
dax_window* get_dax() {
return (_dax.addr != mmio_nullptr) ? &_dax : nullptr;
}
@@ -94,8 +107,9 @@ private:
// maintains the virtio instance number for multiple drives
static int _instance;
int _id;
+
// This mutex protects parallel make_request invocations
- mutex _lock;
+ mutex _lock; // TODO: Maintain one mutex per virtqueue
};

}
diff --git a/fs/virtiofs/virtiofs_vfsops.cc b/fs/virtiofs/virtiofs_vfsops.cc
index b60a766b..be4a32e6 100644
--- a/fs/virtiofs/virtiofs_vfsops.cc
+++ b/fs/virtiofs/virtiofs_vfsops.cc
@@ -15,6 +15,7 @@
#include <osv/debug.h>
#include <osv/device.h>
#include <osv/mutex.h>
+#include <osv/sched.hh>

#include "drivers/virtio-fs.hh"
#include "virtiofs.hh"
@@ -35,7 +36,8 @@ int fuse_req_send_and_receive_reply(virtio::fs* drv, uint32_t opcode,
uint64_t nodeid, void* input_args_data, size_t input_args_size,
void* output_args_data, size_t output_args_size)
{
- std::unique_ptr<fuse_request> req {new (std::nothrow) fuse_request()};
+ std::unique_ptr<fuse_request> req {
+ new (std::nothrow) fuse_request(sched::thread::current())};
if (!req) {
return ENOMEM;
}
@@ -52,7 +54,7 @@ int fuse_req_send_and_receive_reply(virtio::fs* drv, uint32_t opcode,
req->output_args_size = output_args_size;

assert(drv);
- drv->make_request(req.get());
+ drv->make_request(*req);
req->wait();

int error = -req->out_header.error;
--
2.27.0

[Waldek Kozaczuk]

Fotis,

Would you mind splitting this patch into two - one fixing formatting and another one addressing the real issue? I will make it much easier to review.

Thanks for the patch,

Waldek

[Fotis Xenakis]

This patch series fixes a lost wakeup bug in the driver.

Changes since v1:
- Better split into separate commits.

Fotis Xenakis (2):
virtio-fs: minor driver code improvements
virtio-fs: fix lost wakeup in driver

drivers/virtio-fs.cc | 83 +++++++++++-----------------------
drivers/virtio-fs.hh | 28 +++++++++---
fs/virtiofs/virtiofs_vfsops.cc | 6 ++-
3 files changed, 52 insertions(+), 65 deletions(-)

--
2.27.0

[Fotis Xenakis]

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>
---

drivers/virtio-fs.cc | 67 ++++++++++++++--------------------
drivers/virtio-fs.hh | 8 +++-
fs/virtiofs/virtiofs_vfsops.cc | 2 +-
3 files changed, 34 insertions(+), 43 deletions(-)

diff --git a/drivers/virtio-fs.cc b/drivers/virtio-fs.cc
index e0e090bc..b1b622e4 100644
--- a/drivers/virtio-fs.cc
+++ b/drivers/virtio-fs.cc
@@ -5,28 +5,20 @@

@@ -48,25 +40,25 @@ void fs::fuse_request::done()

}
}

-static void fuse_req_enqueue_input(vring* queue, fuse_request* req)
+static void fuse_req_enqueue_input(vring& queue, fuse_request& req)
{
// Header goes first
- queue->add_out_sg(&req->in_header, sizeof(struct fuse_in_header));
+ queue.add_out_sg(&req.in_header, sizeof(struct fuse_in_header));

// Add fuse in arguments as out sg
- if (req->input_args_size > 0) {
- queue->add_out_sg(req->input_args_data, req->input_args_size);
+ if (req.input_args_size > 0) {
+ queue.add_out_sg(req.input_args_data, req.input_args_size);
}
}

-static void fuse_req_enqueue_output(vring* queue, fuse_request* req)
+static void fuse_req_enqueue_output(vring& queue, fuse_request& req)
{
// Header goes first
- queue->add_in_sg(&req->out_header, sizeof(struct fuse_out_header));
+ queue.add_in_sg(&req.out_header, sizeof(struct fuse_out_header));

// Add fuse out arguments as in sg
- if (req->output_args_size > 0) {
- queue->add_in_sg(req->output_args_data, req->output_args_size);
+ if (req.output_args_size > 0) {
+ queue.add_in_sg(req.output_args_data, req.output_args_size);
}
}

@@ -194,7 +186,7 @@ void fs::req_done()

auto* queue = get_virt_queue(VQ_REQUEST);

while (true) {
- virtio_driver::wait_for_queue(queue, &vring::used_ring_not_empty);
+ wait_for_queue(queue, &vring::used_ring_not_empty);

fuse_request* req;
u32 len;

@@ -210,26 +202,21 @@ void fs::req_done()

index 88a16939..c7a71b17 100644
--- a/drivers/virtio-fs.hh
+++ b/drivers/virtio-fs.hh
@@ -10,9 +10,12 @@

#include <functional>

+#include <osv/mmio.hh>
#include <osv/mutex.h>

#include <osv/waitqueue.hh>

+#include "drivers/device.hh"
+#include "drivers/driver.hh"
#include "drivers/virtio.hh"
#include "drivers/virtio-device.hh"
#include "fs/virtiofs/fuse_kernel.h"

@@ -67,7 +70,7 @@ public:

virtual std::string get_name() const { return _driver_name; }
void read_config();

- int make_request(fuse_request*);
+ int make_request(fuse_request&);
dax_window* get_dax() {
return (_dax.addr != mmio_nullptr) ? &_dax : nullptr;
}

@@ -94,8 +97,9 @@ private:

// maintains the virtio instance number for multiple drives
static int _instance;
int _id;
+
// This mutex protects parallel make_request invocations
- mutex _lock;
+ mutex _lock; // TODO: Maintain one mutex per virtqueue
};

}
diff --git a/fs/virtiofs/virtiofs_vfsops.cc b/fs/virtiofs/virtiofs_vfsops.cc

index b60a766b..3dc575c4 100644
--- a/fs/virtiofs/virtiofs_vfsops.cc
+++ b/fs/virtiofs/virtiofs_vfsops.cc
@@ -52,7 +52,7 @@ int fuse_req_send_and_receive_reply(virtio::fs* drv, uint32_t opcode,

[Fotis Xenakis]

Previously, virtio::fs::fuse_request used a waitqueue for waiting until
the request was processed by the device. After the request was
dispatched, the calling thread would wait on that waitqueue, woken by
the driver's request callback thread, when the request was processed.
This left room for a lost wakeup, where the request would be processed
before the interested thread would wait() on it (happened during testing
with fio). This is now fixed, using a waiter instead of a waitqueue.

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>
---

drivers/virtio-fs.cc | 16 ----------------
drivers/virtio-fs.hh | 20 +++++++++++++++-----
fs/virtiofs/virtiofs_vfsops.cc | 4 +++-
3 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/drivers/virtio-fs.cc b/drivers/virtio-fs.cc
index b1b622e4..00f862a1 100644
--- a/drivers/virtio-fs.cc
+++ b/drivers/virtio-fs.cc
@@ -24,22 +24,6 @@ using fuse_request = virtio::fs::fuse_request;

namespace virtio {

-// Wait for the request to be marked as completed.
-void fs::fuse_request::wait()
-{
- WITH_LOCK(req_mutex) {
- req_wait.wait(req_mutex);
- }
-}
-
-// Mark the request as completed.
-void fs::fuse_request::done()
-{
- WITH_LOCK(req_mutex) {
- req_wait.wake_one(req_mutex);
- }
-}
-

static void fuse_req_enqueue_input(vring& queue, fuse_request& req)
{
// Header goes first

diff --git a/drivers/virtio-fs.hh b/drivers/virtio-fs.hh
index c7a71b17..0505a9b0 100644
--- a/drivers/virtio-fs.hh
+++ b/drivers/virtio-fs.hh
@@ -12,7 +12,8 @@

#include <osv/mmio.hh>
#include <osv/mutex.h>
-#include <osv/waitqueue.hh>
+#include <osv/sched.hh>
+#include <osv/wait_record.hh>

#include "drivers/device.hh"
#include "drivers/driver.hh"
@@ -29,6 +30,8 @@ enum {

class fs : public virtio_driver {
public:
+ // A fuse_request encapsulates a low-level virtio-fs device request. This is
+ // a single-use object.
struct fuse_request {
struct fuse_in_header in_header;
struct fuse_out_header out_header;

@@ -39,12 +42,19 @@ public:

void* output_args_data;
size_t output_args_size;

- void wait();
- void done();
+ // Constructs a fuse_request, determining that wait() on it will be
+ // called by @t.
+ fuse_request(sched::thread* t): processed{t} {}
+
+ // Waits for the request to be marked as completed. Should only be
+ // called once, from the thread specified in the constructor.
+ void wait() { processed.wait(); }
+ // Marks the request as completed. Should only be called once.
+ void done() { processed.wake(); }

private:
- mutex_t req_mutex;
- waitqueue req_wait;
+ // Signifies whether the request has been processed by the device
+ waiter processed;
};

struct fs_config {

diff --git a/fs/virtiofs/virtiofs_vfsops.cc b/fs/virtiofs/virtiofs_vfsops.cc
index 3dc575c4..be4a32e6 100644
--- a/fs/virtiofs/virtiofs_vfsops.cc
+++ b/fs/virtiofs/virtiofs_vfsops.cc

@@ -15,6 +15,7 @@
#include <osv/debug.h>
#include <osv/device.h>
#include <osv/mutex.h>
+#include <osv/sched.hh>

#include "drivers/virtio-fs.hh"
#include "virtiofs.hh"
@@ -35,7 +36,8 @@ int fuse_req_send_and_receive_reply(virtio::fs* drv, uint32_t opcode,
uint64_t nodeid, void* input_args_data, size_t input_args_size,
void* output_args_data, size_t output_args_size)
{
- std::unique_ptr<fuse_request> req {new (std::nothrow) fuse_request()};
+ std::unique_ptr<fuse_request> req {
+ new (std::nothrow) fuse_request(sched::thread::current())};
if (!req) {
return ENOMEM;
}

--
2.27.0

[Commit Bot]

From: Fotis Xenakis <fo...@windowslive.com>
Committer: Waldemar Kozaczuk <jwkoz...@gmail.com>
Branch: master

virtio-fs: minor driver code improvements

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>
Message-Id: <AM0PR03MB629203A23A...@AM0PR03MB6292.eurprd03.prod.outlook.com>

---
diff --git a/drivers/virtio-fs.cc b/drivers/virtio-fs.cc

[Commit Bot]

From: Fotis Xenakis <fo...@windowslive.com>
Committer: Waldemar Kozaczuk <jwkoz...@gmail.com>
Branch: master

virtio-fs: fix lost wakeup in driver

Previously, virtio::fs::fuse_request used a waitqueue for waiting until
the request was processed by the device. After the request was
dispatched, the calling thread would wait on that waitqueue, woken by
the driver's request callback thread, when the request was processed.
This left room for a lost wakeup, where the request would be processed
before the interested thread would wait() on it (happened during testing
with fio). This is now fixed, using a waiter instead of a waitqueue.

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>

Message-Id: <AM0PR03MB62922E4EBB...@AM0PR03MB6292.eurprd03.prod.outlook.com>

---
diff --git a/drivers/virtio-fs.cc b/drivers/virtio-fs.cc

--- a/drivers/virtio-fs.cc
+++ b/drivers/virtio-fs.cc
@@ -24,22 +24,6 @@ using fuse_request = virtio::fs::fuse_request;

namespace virtio {

-// Wait for the request to be marked as completed.
-void fs::fuse_request::wait()
-{
- WITH_LOCK(req_mutex) {
- req_wait.wait(req_mutex);
- }
-}
-
-// Mark the request as completed.
-void fs::fuse_request::done()
-{
- WITH_LOCK(req_mutex) {
- req_wait.wake_one(req_mutex);
- }
-}
-
static void fuse_req_enqueue_input(vring& queue, fuse_request& req)
{
// Header goes first
diff --git a/drivers/virtio-fs.hh b/drivers/virtio-fs.hh