Conversation
6b0d99e to
bad67b3
Compare
Atemu
left a comment
There was a problem hiding this comment.
This is something I think NixOS needs.
The diff LGTM and is very high quality I might add.
Could you go into detail what testing you have done on this? A NixOS test would also be amazing but not required.
One thing I was a little unsure of was that we build bfq as a module and don't load it by default but the kernel appears to just load the module when bfq is asked for.
Thank you!
I have been using this module as part of my Nix files with the following configuration since mid of last year without any issues: Testing this in a virtualised environment could be tricky, but I’ll think of something. I suppose that providing a basic configuration and verifying that the generated udev rules are correct would be a good start, but it might be nice to check if the settings are actually applied to block devices. |
|
I modified the module to reject values containing a line break, because these are not permitted within udev rules. I also added a test to check a reasonable module configuration results in a syntactically and semantically correct udev rule set as verified by |
Atemu
left a comment
There was a problem hiding this comment.
I didn't give it very thorough testing because the code looks very good to me and the NixOS test gives me confidence that it does work as expected.
The basic functionality works and there isn't anything obviously wrong with it. Should any issues crop up, we can fix them later.
Amazing work, thank you :)
| udevValue = types.addCheck types.nonEmptyStr (x: builtins.match "[^\n\r]*" x != null) // { | ||
| name = "udevValue"; | ||
| description = "udev rule value"; | ||
| descriptionClass = "noun"; | ||
| }; |
There was a problem hiding this comment.
👌
This could perhaps be made available as a type or something as this should be something that is generally useful in other modules/user config.
There was a problem hiding this comment.
Do you think it might be useful to have a generic framework for representing udev rules in Nix?
There was a problem hiding this comment.
That sounds like a good idea to me, though not necessarily a very high priority.
|
Successfully created backport PR for |
|
Ah one minor thing I missed: This should have a release note! I consider this module to be useful to many people, so they should be made aware that it exists now. |
|
Ah, one thing I noticed in actual deployment is that this affects loop devices which probably shouldn't have an iosched? It's easy enough to disable using the provided options but perhaps the module should handle that case somehow; either through default config or through documentation. |
I’ll prepare a PR. |
Description of changes
This module introduces the following options to set I/O schedulers for block devices and generates appropriate udev rules:
hardware.block.defaultScheduler: Default scheduler for all block devices.hardware.block.defaultSchedulerRotational: Default scheduler for rotational drives, such as hard drives.hardware.block.scheduler: Schedulers for specific block devices.This solves #57577. My personal motivation is that I often run workloads that do not perform well with the default scheduler and I would like to have a way to override it without writing individual udev rules for simple cases.
hardware.block.scheduleris an attribute set assigning schedulers by device name pattern. Using an attrset limits its use to non‐overlapping patterns. Overlapping patterns could be supported by instead using a list of attrsets, but I believe that users requiring these are served better by producing their own custom udev rules.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usageTested basic functionality of all binary files (usually in./result/bin/)Add a 👍 reaction to pull requests you find important.