From ed8ff1ff9c4c0f847ffc2ab4624bd999539a0890 Mon Sep 17 00:00:00 2001 From: dzwdz Date: Thu, 11 Jul 2024 21:43:50 +0200 Subject: kernel: start cleaning up VfsRequest * I'm being more strict about the linked list state to hopefully ensure I'm not leaking any references. * vfsreq_create was renamed to vfsreq_dispatchcopy as that name feels more clear. It copies its argument, and dispatches it. * Requests for user backends are now handled more like requests for kernel backends - there's an accept() function that accepts a request. --- src/kernel/vfs/request.c | 127 +++++++++++++++++++++++++++++++---------------- 1 file changed, 83 insertions(+), 44 deletions(-) (limited to 'src/kernel/vfs/request.c') diff --git a/src/kernel/vfs/request.c b/src/kernel/vfs/request.c index 8fe0114..4489f5a 100644 --- a/src/kernel/vfs/request.c +++ b/src/kernel/vfs/request.c @@ -6,45 +6,75 @@ #include #include -static void vfs_backend_user_accept(VfsReq *req); +/** Checks if the request flags don't contradict each other. + * While this could be done by caller of vfsreq_dispatchcopy, as currently + * there's only one of them per operation, doing this here means that any + * potential future callers can't forget those checks. + * Doing this so late is kinda inefficient but I don't really care. */ +static bool +vfsreq_isvalid(VfsReq *r) +{ + /* Notable omissions: + * - Simplifying the path for open(). Already done when resolving the mount. + * - Checking the handle permissions. We don't have access to it. + * */ + if (r->type == VFSOP_OPEN && !(r->flags & OPEN_WRITE) && (r->flags & OPEN_CREATE)) { + return false; + } + + // XXX if i add a handle field to vfs_request, check ->readable ->writeable here + return true; +} -void vfsreq_create(VfsReq req_) { +void +vfsreq_dispatchcopy(VfsReq tmpl) +{ VfsReq *req; - if (req_.caller) { - proc_setstate(req_.caller, PS_WAITS4FS); - if (!req_.caller->reqslot) - req_.caller->reqslot = kmalloc(sizeof *req, "reqslot"); - req = req_.caller->reqslot; + VfsBackend *backend; + + /* allocate memory for the request and move it there */ + if (tmpl.caller) { + // TODO: ensure we aren't overriding an existing request + // TODO: wouldn't it be better to just optimize malloc? + // see commit afa28cfc7069f7649632f7b33daac47ee56d3819 + proc_setstate(tmpl.caller, PS_WAITS4FS); + if (!tmpl.caller->reqslot) + tmpl.caller->reqslot = kmalloc(sizeof *req, "reqslot"); + req = tmpl.caller->reqslot; /* (re)using a single allocation for all request a process makes */ } else { req = kmalloc(sizeof *req, "reqanon"); } - memcpy(req, &req_, sizeof *req); - if (req->backend) { - assert(req->backend->usehcnt); - req->backend->usehcnt++; + + memcpy(req, &tmpl, sizeof *req); + backend = req->backend; + if (backend) { + assert(backend->usehcnt > 0); + backend->usehcnt++; } - if (req->type == VFSOP_OPEN && !(req->flags & OPEN_WRITE) && (req->flags & OPEN_CREATE)) { + /* Check request validity. Doing this after the memcpy means that it + * doesn't need to be special cased in vfsreq_finish. */ + if (!vfsreq_isvalid(req)) { vfsreq_finish_short(req, -EINVAL); return; } - - // TODO if i add a handle field to vfs_request, check ->readable ->writeable here - - if (req->backend && req->backend->provhcnt) { - VfsReq **iter = &req->backend->queue; - while (*iter != NULL) // find free spot in queue - iter = &(*iter)->queue_next; - *iter = req; - vfs_backend_tryaccept(req->backend); - } else { - vfsreq_finish_short(req, -1); + assert(req->queue_next == NULL); + assert(req->postqueue_next == NULL); + + if (backend == NULL) { + /* null mount - probably never had a real backend */ + vfsreq_finish_short(req, -ENOENT); + } else if (backend->is_user) { + vfsback_useraccept(req); + } else { /* kernel backend */ + assert(backend->kern.accept); + backend->kern.accept(req); } } -void vfsreq_finish(VfsReq *req, char __user *stored, long ret, - int flags, Proc *handler) +void +vfsreq_finish(VfsReq *req, char __user *stored, long ret, int flags, Proc *handler) { if (req->type == VFSOP_OPEN && ret >= 0) { Handle *h; @@ -88,6 +118,9 @@ void vfsreq_finish(VfsReq *req, char __user *stored, long ret, vfsback_userdown(req->backend); } + assert(req->queue_next == NULL); + assert(req->postqueue_next == NULL); + if (req->caller) { assert(req->caller->state == PS_WAITS4FS); regs_savereturn(&req->caller->regs, ret); @@ -97,27 +130,33 @@ void vfsreq_finish(VfsReq *req, char __user *stored, long ret, } } -void vfs_backend_tryaccept(VfsBackend *backend) { - VfsReq *req = backend->queue; - if (!req) return; - if (backend->is_user && !backend->user.handler) return; - - backend->queue = req->queue_next; - if (backend->is_user) { - vfs_backend_user_accept(req); - } else { - assert(backend->kern.accept); - backend->kern.accept(req); - } -} - -static void vfs_backend_user_accept(VfsReq *req) { +void +vfsback_useraccept(VfsReq *req) +{ + VfsBackend *backend; Proc *handler; struct ufs_request res = {0}; int len; - assert(req && req->backend && req->backend->user.handler); - handler = req->backend->user.handler; + assert(req != NULL); + backend = req->backend; + assert(backend); + assert(backend->is_user); + if (backend->provhcnt == 0) { + vfsreq_finish_short(req, -EPIPE); + return; + } else if (backend->user.handler == NULL) { + /* queue the request up */ + // TODO use postqueue + VfsReq **it = &backend->queue; + while (*it != NULL) { /* find a free spot in queue */ + it = &(*it)->queue_next; + } + *it = req; + return; + } + + handler = backend->user.handler; assert(handler->state == PS_WAITS4REQUEST); // the pcpy calls aren't present in all kernel backends @@ -158,7 +197,6 @@ static void vfs_backend_user_accept(VfsReq *req) { proc_setstate(handler, PS_RUNNING); regs_savereturn(&handler->regs, hid); req->backend->user.handler = NULL; - return; } static void @@ -204,7 +242,8 @@ vfsback_provdown(VfsBackend *b) VfsReq *q = b->queue; while (q) { VfsReq *q2 = q->queue_next; - vfsreq_finish_short(q, -1); + q->queue_next = NULL; + vfsreq_finish_short(q, -EPIPE); q = q2; } b->queue = NULL; -- cgit v1.2.3