lib/config: Sanity checks on MaxConcurrentWrites (ref #7064) #7069
Conversation
calmh
left a comment
There was a problem hiding this comment.
I think the 2 needs to be named somewhere, and we should skip the Infof. Also we have in some cases done this type of clamping in a method on the options type, I wonder if it might not be cleaner to have all such logic centralized there...
|
It's a mixture at present: Sometimes we replace 0 with the default value in config and othertimes we leave 0 and use a default value outside of config. The former has the advantage that everything is in one place and the stored config value is the same as the effective one, while the latter keeps the stored value the same as what the user specified, making it obvious if it's the default value and preventing possible confusion when the stored value is different from the specified one. I think the former is better and thus moved this change to the config. |
|
The upper limit sounds kind of low though, especially considering superfast storage like NVME RAID or similar. I am no storage expert by any means, but even my very slow SD card has been able to withstand unlimited |
|
Withstand != Perform Efficiently There is no such things as infinite bandwidth storage to support infinite concurrent requests, and at the point your requests start queueing up, your efficiency in things like read ahead drops to 0 |
|
I am just saying that, as Simon also mentioned in the first post, it would be good to have some kind of a technical reason to back up limiting the setting at |
|
That's not what I meant: I invited anyone that has concrete arguments/requirements for a different upper limit to state them. I did not mean to open a discussion on it. This is about preventing the worst in case of misconfiguration (or messing up default values). If anyone does think we should debate parallel io, then please don't do that on this PR. Good to merge? |
|
I really have no opinion on what amount of concurrent I/O people would or could run with, and see no reason to limit it for that reason. But in practice the amount of concurrent I/O will also be the lower bound on the number of running threads and those are neither free nor unlimited, so an upper limit makes sense. I doubt that a lack of more concurrent I/O will ever be the performance bottleneck. |
* main: lib/folder: Clear pull errors when nothing is needed anymore (syncthing#7093) lib/api: Fix debug endpoints (ref syncthing#7001) (syncthing#7092) gui, man, authors: Update docs, translations, and contributors lib/config: Sanity checks on MaxConcurrentWrites (ref syncthing#7064) (syncthing#7069) lib/ur: Fix panics in failure-reporting (fixes syncthing#7090) (syncthing#7091) lib/ur: Fix panics in failure-reporting (fixes syncthing#7090) (syncthing#7091) build: Update dependencies (syncthing#7088) lib: Remove USE_BADGER experiment (syncthing#7089) build: Update notify (fixes syncthing#7063) (syncthing#7080) lib/api: Fix /rest/config path and add methods to cors (ref syncthing#7001) (syncthing#7081) lib/api: Allow OPTIONS method in CORS preflight request handling (ref syncthing#7017) (syncthing#7079) gui: Fix another undefined variable access (fixes syncthing#7077) (syncthing#7078) lib/config: Check for "msdos" when detecting FAT FS in Android (syncthing#7072) gui, man, authors: Update docs, translations, and contributors
* main: (42 commits) gui: Initialise sharing when accepting new device (fixes syncthing#7113) (syncthing#7114) gui: Initialise sharing when accepting new device (fixes syncthing#7113) (syncthing#7114) lib/model: Prevent test deadlock (syncthing#7110) lib/api, lib/db: Add file debug endpoint (syncthing#7095) gui: Add advance config port mapping to gui (fixes syncthing#4824) (syncthing#7017) lib/tlsutil: Add O and OU to generated certificates (fixes syncthing#7108) (syncthing#7109) all: Add untrusted folders behind feature flag (ref syncthing#62) (syncthing#7055) build: Update notify (fixes syncthing#5360) (syncthing#7106) lib/model: Fix locking when resending cluster-configs (syncthing#7107) gui: Remove superfluous translate in previous (ref syncthing#7102) gui: Add warning when JavaScript is disabled in Web browser (fixes syncthing#7099) (syncthing#7102) lib/model: Add done chan to track folder-lifetime (fixes syncthing#6664) (syncthing#7094) lib/model: Send indexes for newly shared folder (fixes syncthing#7098) (syncthing#7100) lib/folder: Clear pull errors when nothing is needed anymore (syncthing#7093) lib/api: Fix debug endpoints (ref syncthing#7001) (syncthing#7092) gui, man, authors: Update docs, translations, and contributors lib/config: Sanity checks on MaxConcurrentWrites (ref syncthing#7064) (syncthing#7069) lib/ur: Fix panics in failure-reporting (fixes syncthing#7090) (syncthing#7091) lib/ur: Fix panics in failure-reporting (fixes syncthing#7090) (syncthing#7091) build: Update dependencies (syncthing#7088) ...
This changes the meaning of
MaxConcurrentWrites <= 0to mean default and additional puts an upper limit on the config value too. I guess the latter is going to be controversial, as it's clearly user error. My reasoning is that I'd rather check for that than have to explain the error to a user. And the limit of 64 is entirely randomly chosen, I do not know anything about concurrent IO, so if there is a technical reason to change that or drop it alltogether, please let me know.