Front + Back merging #8
Conversation
Introduce notion of "K2 I/O prio" which get's set during request insertion. K2 I/O prio is derived from the request ioprio, if set, or from current's ioprio if not. During merging compare against K2 ioprio. Use it also for book-keeping, since "current" can change between insertion and deletion (dispatch)
|
As it turns out, request->ioprio is invalid almost all the time … That led to crashes, since in that case the ioprio is derived from "current". However, "current" is different between insertion (process) and dispatch (kworker). Yikes. Fixed it by saving "K2 I/O prio" in request structures (they have fields we can use). Front and back merges are now functional; at least within a single I/O prio (used default/none). the .request_merge callback puzzles me. I thought I knew for what it was used, but it seems request merging works fine with out it (again, at least with a single worker). |
|
Regarding the ioprio: I would prefer to see the ioprio being derived from the task_struct's I/O context in the first place and only falling back to the nice value if the I/O context contains invalid prio settings (which I never observed). This allows to separate a process' I/O priority from its CPU scheduling settings. For instance, in our benchmark suite, we only boost the ioprio via ioprio_set and leave the nice values untouched. We do need the request_merge callback (as you have implemented it). Indeed, you won't be able to observe it doing something than only using a single worker. Still, there are corner cases, when request_merge is invoked: For instance consider the following situation: Process A inserts a request and thereby initializes last_merge. Then, a second proc. B (e.g. with a lower ioprio than A) inserts a request but is unable to set last_merge since the request of A mentioned above is not yet dispatched. Now, a third proc. C with the same ioprio as B inserts an other request suitable for front merging with the last request of B. Fast-path merging will fail in that case, so the request_merge function is needed. Note, that there are similar situations related to backmerging when the scheduler queues contain multiple requests with the same hash key but different ioprios. |
Deal with multiple requests that have the same start sector (and possibly different I/O prios)
Umm … that requires current->alloc_lock to be held, or get_io_context(), which is more light weight but an internal API. Either way, I don't see that make much sense, since:
Funny. That is always the case for me: Update: After browsing github a bit more I think that whole I/O priority stuff wasn't properly implemented in 4.15. I think we need at least 5.0.
Is there an (easy) way to make functional tests for that? I'd like to have more than staring at the code, scratching your head and saying "Mhm, seems to be ok" … :/ |
First consider the bio/request's ioprio. If invalid, fall back to the I/O priority of the io_context, if does not exist or the priority is invalid, fall back to deriving the I/O priority from the task's nice value
Do not Oops, if the idx is out of range. Instead clamp it to idle priority and issue a rate-limited warning. Same goes for k2_queue. Only Oops if data direction is neither read nor write. There are ways to avoid Oopsing in those cases too, but data direction should be correct.
|
Ok, please send me a notification when you think that your PR is finished, I will then merge it and afterwards port it to a kernel of the 5.x series. I am sorry, but a can not think of an easy way to reproduce such situations. Maybe try setting up a RT proc. that performs heavy I/O and two low-prio processes that merge their requests, all doing I/O at the same LBA and hope you see an example of the request_merge function being invoked, but I guess this is highly non-deterministic... |
|
Ok, I re-enabled .request_merge callback as you suggested. In simple tests front and back-merging seem to work. Do we want to run a couple of benchmarks before merging? |
next/former_request are still missing, but this is the (untested) code I have so far, so you can have a look.