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 On Friday, July 10, 2020 at 11:32:01 AM UTC-4 Fotis Xenakis wrote: > 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 <[email protected]> > --- > 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 > > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/4f927c83-2c61-4fd5-8820-3ebd5cf86434n%40googlegroups.com.
