From d91c4b010b6e01c1e544e4ffe3ed8138a56db88a Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Mon, 27 Jan 2020 17:31:17 +0100 Subject: [PATCH] lib/config, lib/model: Limit concurrent pulls (fixes #5914) (#6290) Adds a new folder state "Waiting to Sync" in the same vein as the existing "Waiting to Scan". This vastly improves performances in the rare cases when there are lots and lots of folders operating. --- gui/default/index.html | 6 ++- .../syncthing/core/syncthingController.js | 2 +- lib/config/config.go | 2 +- lib/config/config_test.go | 28 ++++++++++++- lib/config/migrations.go | 8 ++++ lib/config/optionsconfiguration.go | 25 ++++++++++- lib/config/testdata/v10.xml | 12 ------ lib/config/testdata/v11.xml | 13 ------ lib/config/testdata/v12.xml | 14 ------- lib/config/testdata/v13.xml | 14 ------- lib/config/testdata/v14.xml | 14 ------- lib/config/testdata/v15.xml | 14 ------- lib/config/testdata/v16.xml | 14 ------- lib/config/testdata/v17.xml | 15 ------- lib/config/testdata/v18.xml | 15 ------- lib/config/testdata/v19.xml | 15 ------- lib/config/testdata/v20.xml | 15 ------- lib/config/testdata/v21.xml | 15 ------- lib/config/testdata/v23.xml | 16 ------- lib/config/testdata/v24.xml | 16 ------- lib/config/testdata/v25.xml | 16 ------- lib/config/testdata/v26.xml | 16 ------- lib/config/testdata/v27.xml | 16 ------- lib/config/testdata/v28.xml | 16 ------- lib/config/testdata/v29.xml | 16 ------- lib/config/testdata/v6.xml | 12 ------ lib/config/testdata/v7.xml | 12 ------ lib/config/testdata/v8.xml | 12 ------ lib/config/testdata/v9.xml | 12 ------ lib/model/folder.go | 42 ++++++++++++++++--- lib/model/folder_sendrecv.go | 19 --------- lib/model/folderstate.go | 3 ++ lib/model/model.go | 4 +- test/folders.sh | 11 +++++ 34 files changed, 118 insertions(+), 362 deletions(-) delete mode 100644 lib/config/testdata/v10.xml delete mode 100644 lib/config/testdata/v11.xml delete mode 100644 lib/config/testdata/v12.xml delete mode 100644 lib/config/testdata/v13.xml delete mode 100644 lib/config/testdata/v14.xml delete mode 100644 lib/config/testdata/v15.xml delete mode 100644 lib/config/testdata/v16.xml delete mode 100644 lib/config/testdata/v17.xml delete mode 100644 lib/config/testdata/v18.xml delete mode 100644 lib/config/testdata/v19.xml delete mode 100644 lib/config/testdata/v20.xml delete mode 100644 lib/config/testdata/v21.xml delete mode 100644 lib/config/testdata/v23.xml delete mode 100644 lib/config/testdata/v24.xml delete mode 100644 lib/config/testdata/v25.xml delete mode 100644 lib/config/testdata/v26.xml delete mode 100644 lib/config/testdata/v27.xml delete mode 100644 lib/config/testdata/v28.xml delete mode 100644 lib/config/testdata/v29.xml delete mode 100644 lib/config/testdata/v6.xml delete mode 100644 lib/config/testdata/v7.xml delete mode 100644 lib/config/testdata/v8.xml delete mode 100644 lib/config/testdata/v9.xml create mode 100755 test/folders.sh diff --git a/gui/default/index.html b/gui/default/index.html index e2dbe1742..a8dfc83e5 100644 --- a/gui/default/index.html +++ b/gui/default/index.html @@ -313,7 +313,7 @@ - + @@ -324,6 +324,10 @@ + + + + diff --git a/gui/default/syncthing/core/syncthingController.js b/gui/default/syncthing/core/syncthingController.js index c1ed4332d..86dadf7aa 100755 --- a/gui/default/syncthing/core/syncthingController.js +++ b/gui/default/syncthing/core/syncthingController.js @@ -837,7 +837,7 @@ angular.module('syncthing.core') if (status === 'stopped' || status === 'outofsync' || status === 'error' || status === 'faileditems') { return 'danger'; } - if (status === 'unshared' || status === 'scan-waiting') { + if (status === 'unshared' || status === 'scan-waiting' || status === 'sync-waiting') { return 'warning'; } diff --git a/lib/config/config.go b/lib/config/config.go index 85dadd913..e18045209 100644 --- a/lib/config/config.go +++ b/lib/config/config.go @@ -31,7 +31,7 @@ import ( const ( OldestHandledVersion = 10 - CurrentVersion = 29 + CurrentVersion = 30 MaxRescanIntervalS = 365 * 24 * 60 * 60 ) diff --git a/lib/config/config_test.go b/lib/config/config_test.go index 1a424d73b..9382701aa 100644 --- a/lib/config/config_test.go +++ b/lib/config/config_test.go @@ -86,8 +86,13 @@ func TestDefaultValues(t *testing.T) { func TestDeviceConfig(t *testing.T) { for i := OldestHandledVersion; i <= CurrentVersion; i++ { + cfgFile := fmt.Sprintf("testdata/v%d.xml", i) + if _, err := os.Stat(cfgFile); os.IsNotExist(err) { + continue + } + os.RemoveAll(filepath.Join("testdata", DefaultMarkerName)) - wr, err := load(fmt.Sprintf("testdata/v%d.xml", i), device1) + wr, err := load(cfgFile, device1) if err != nil { t.Fatal(err) } @@ -1127,6 +1132,27 @@ func TestRemoveDeviceWithEmptyID(t *testing.T) { } } +func TestMaxConcurrentFolders(t *testing.T) { + cases := []struct { + input int + output int + }{ + {input: -42, output: 0}, + {input: -1, output: 0}, + {input: 0, output: runtime.GOMAXPROCS(-1)}, + {input: 1, output: 1}, + {input: 42, output: 42}, + } + + for _, tc := range cases { + opts := OptionsConfiguration{RawMaxFolderConcurrency: tc.input} + res := opts.MaxFolderConcurrency() + if res != tc.output { + t.Errorf("Wrong MaxFolderConcurrency, %d => %d, expected %d", tc.input, res, tc.output) + } + } +} + // defaultConfigAsMap returns a valid default config as a JSON-decoded // map[string]interface{}. This is useful to override random elements and // re-encode into JSON. diff --git a/lib/config/migrations.go b/lib/config/migrations.go index 1bcbd90c8..53964210e 100644 --- a/lib/config/migrations.go +++ b/lib/config/migrations.go @@ -25,6 +25,7 @@ import ( // update the config version. The order of migrations doesn't matter here, // put the newest on top for readability. var migrations = migrationSet{ + {30, migrateToConfigV30}, {29, migrateToConfigV29}, {28, migrateToConfigV28}, {27, migrateToConfigV27}, @@ -84,6 +85,13 @@ func (m migration) apply(cfg *Configuration) { cfg.Version = m.targetVersion } +func migrateToConfigV30(cfg *Configuration) { + // The "max concurrent scans" option is now spelled "max folder concurrency" + // to be more general. + cfg.Options.RawMaxFolderConcurrency = cfg.Options.DeprecatedMaxConcurrentScans + cfg.Options.DeprecatedMaxConcurrentScans = 0 +} + func migrateToConfigV29(cfg *Configuration) { // The new crash reporting option should follow the state of global // discovery / usage reporting, and we should display an appropriate diff --git a/lib/config/optionsconfiguration.go b/lib/config/optionsconfiguration.go index f5b0b8ad2..4d3f08f32 100644 --- a/lib/config/optionsconfiguration.go +++ b/lib/config/optionsconfiguration.go @@ -8,6 +8,7 @@ package config import ( "fmt" + "runtime" "github.com/syncthing/syncthing/lib/rand" "github.com/syncthing/syncthing/lib/util" @@ -52,7 +53,7 @@ type OptionsConfiguration struct { TrafficClass int `xml:"trafficClass" json:"trafficClass"` DefaultFolderPath string `xml:"defaultFolderPath" json:"defaultFolderPath" default:"~"` SetLowPriority bool `xml:"setLowPriority" json:"setLowPriority" default:"true"` - MaxConcurrentScans int `xml:"maxConcurrentScans" json:"maxConcurrentScans"` + RawMaxFolderConcurrency int `xml:"maxFolderConcurrency" json:"maxFolderConcurrency"` CRURL string `xml:"crashReportingURL" json:"crURL" default:"https://crash.syncthing.net/newcrash"` // crash reporting URL CREnabled bool `xml:"crashReportingEnabled" json:"crashReportingEnabled" default:"true" restart:"true"` StunKeepaliveStartS int `xml:"stunKeepaliveStartS" json:"stunKeepaliveStartS" default:"180"` // 0 for off @@ -66,6 +67,7 @@ type OptionsConfiguration struct { DeprecatedUPnPTimeoutS int `xml:"upnpTimeoutSeconds,omitempty" json:"-"` DeprecatedRelayServers []string `xml:"relayServer,omitempty" json:"-"` DeprecatedMinHomeDiskFreePct float64 `xml:"minHomeDiskFreePct,omitempty" json:"-"` + DeprecatedMaxConcurrentScans int `xml:"maxConcurrentScans,omitempty" json:"-"` } func (opts OptionsConfiguration) Copy() OptionsConfiguration { @@ -152,3 +154,24 @@ func (opts OptionsConfiguration) GlobalDiscoveryServers() []string { } return util.UniqueTrimmedStrings(servers) } + +func (opts OptionsConfiguration) MaxFolderConcurrency() int { + // If a value is set, trust that. + if opts.RawMaxFolderConcurrency > 0 { + return opts.RawMaxFolderConcurrency + } + if opts.RawMaxFolderConcurrency < 0 { + // -1 etc means unlimited, which in the implementation means zero + return 0 + } + // Otherwise default to the number of CPU cores in the system as a rough + // approximation of system powerfullness. + if n := runtime.GOMAXPROCS(-1); n > 0 { + return n + } + // We should never get here to begin with, but since we're here let's + // use some sort of reasonable compromise between the old "no limit" and + // getting nothing done... (Median number of folders out there at time + // of writing is two, 95-percentile at 12 folders.) + return 4 // https://xkcd.com/221/ +} diff --git a/lib/config/testdata/v10.xml b/lib/config/testdata/v10.xml deleted file mode 100644 index 08a377c50..000000000 --- a/lib/config/testdata/v10.xml +++ /dev/null @@ -1,12 +0,0 @@ - - - - - - -
a
- - -
b
- - diff --git a/lib/config/testdata/v11.xml b/lib/config/testdata/v11.xml deleted file mode 100644 index d6d60999c..000000000 --- a/lib/config/testdata/v11.xml +++ /dev/null @@ -1,13 +0,0 @@ - - - - - 1 - - -
a
- - -
b
- - diff --git a/lib/config/testdata/v12.xml b/lib/config/testdata/v12.xml deleted file mode 100644 index 794b4d382..000000000 --- a/lib/config/testdata/v12.xml +++ /dev/null @@ -1,14 +0,0 @@ - - - - - 1 - -1 - - -
tcp://a
- - -
tcp://b
- - diff --git a/lib/config/testdata/v13.xml b/lib/config/testdata/v13.xml deleted file mode 100644 index 1fec6e1bd..000000000 --- a/lib/config/testdata/v13.xml +++ /dev/null @@ -1,14 +0,0 @@ - - - - - 1 - -1 - - -
tcp://a
- - -
tcp://b
- - diff --git a/lib/config/testdata/v14.xml b/lib/config/testdata/v14.xml deleted file mode 100644 index 31cca4ae0..000000000 --- a/lib/config/testdata/v14.xml +++ /dev/null @@ -1,14 +0,0 @@ - - - - - 1 - -1 - - -
tcp://a
- - -
tcp://b
- - diff --git a/lib/config/testdata/v15.xml b/lib/config/testdata/v15.xml deleted file mode 100644 index a102f2be9..000000000 --- a/lib/config/testdata/v15.xml +++ /dev/null @@ -1,14 +0,0 @@ - - - - - 1 - -1 - - -
tcp://a
- - -
tcp://b
- - diff --git a/lib/config/testdata/v16.xml b/lib/config/testdata/v16.xml deleted file mode 100644 index 4c18e0d10..000000000 --- a/lib/config/testdata/v16.xml +++ /dev/null @@ -1,14 +0,0 @@ - - - - - 1 - -1 - - -
tcp://a
- - -
tcp://b
- - diff --git a/lib/config/testdata/v17.xml b/lib/config/testdata/v17.xml deleted file mode 100644 index 52e29a2b1..000000000 --- a/lib/config/testdata/v17.xml +++ /dev/null @@ -1,15 +0,0 @@ - - - - - 1 - -1 - true - - -
tcp://a
- - -
tcp://b
- - diff --git a/lib/config/testdata/v18.xml b/lib/config/testdata/v18.xml deleted file mode 100644 index 4dc56e66e..000000000 --- a/lib/config/testdata/v18.xml +++ /dev/null @@ -1,15 +0,0 @@ - - - - - 1 - -1 - true - - -
tcp://a
- - -
tcp://b
- - diff --git a/lib/config/testdata/v19.xml b/lib/config/testdata/v19.xml deleted file mode 100644 index 29d17601c..000000000 --- a/lib/config/testdata/v19.xml +++ /dev/null @@ -1,15 +0,0 @@ - - - - - 1 - -1 - true - - -
tcp://a
- - -
tcp://b
- - diff --git a/lib/config/testdata/v20.xml b/lib/config/testdata/v20.xml deleted file mode 100644 index 22eca7b5e..000000000 --- a/lib/config/testdata/v20.xml +++ /dev/null @@ -1,15 +0,0 @@ - - - - - 1 - -1 - true - - -
tcp://a
- - -
tcp://b
- - diff --git a/lib/config/testdata/v21.xml b/lib/config/testdata/v21.xml deleted file mode 100644 index 354a77da9..000000000 --- a/lib/config/testdata/v21.xml +++ /dev/null @@ -1,15 +0,0 @@ - - - - - 1 - -1 - true - - -
tcp://a
- - -
tcp://b
- - diff --git a/lib/config/testdata/v23.xml b/lib/config/testdata/v23.xml deleted file mode 100644 index 2a577fc50..000000000 --- a/lib/config/testdata/v23.xml +++ /dev/null @@ -1,16 +0,0 @@ - - - basic - - - 1 - -1 - true - - -
tcp://a
- - -
tcp://b
- - diff --git a/lib/config/testdata/v24.xml b/lib/config/testdata/v24.xml deleted file mode 100644 index 2a577fc50..000000000 --- a/lib/config/testdata/v24.xml +++ /dev/null @@ -1,16 +0,0 @@ - - - basic - - - 1 - -1 - true - - -
tcp://a
- - -
tcp://b
- - diff --git a/lib/config/testdata/v25.xml b/lib/config/testdata/v25.xml deleted file mode 100644 index 9c47f99e8..000000000 --- a/lib/config/testdata/v25.xml +++ /dev/null @@ -1,16 +0,0 @@ - - - basic - - - 1 - -1 - true - - -
tcp://a
- - -
tcp://b
- - diff --git a/lib/config/testdata/v26.xml b/lib/config/testdata/v26.xml deleted file mode 100644 index 4aae7bfd3..000000000 --- a/lib/config/testdata/v26.xml +++ /dev/null @@ -1,16 +0,0 @@ - - - basic - - - 1 - -1 - true - - -
tcp://a
- - -
tcp://b
- - diff --git a/lib/config/testdata/v27.xml b/lib/config/testdata/v27.xml deleted file mode 100644 index 643cb7c5d..000000000 --- a/lib/config/testdata/v27.xml +++ /dev/null @@ -1,16 +0,0 @@ - - - basic - - - 1 - -1 - true - - -
tcp://a
- - -
tcp://b
- - diff --git a/lib/config/testdata/v28.xml b/lib/config/testdata/v28.xml deleted file mode 100644 index 231cd882c..000000000 --- a/lib/config/testdata/v28.xml +++ /dev/null @@ -1,16 +0,0 @@ - - - basic - - - 1 - -1 - true - - -
tcp://a
- - -
tcp://b
- - diff --git a/lib/config/testdata/v29.xml b/lib/config/testdata/v29.xml deleted file mode 100644 index 231cd882c..000000000 --- a/lib/config/testdata/v29.xml +++ /dev/null @@ -1,16 +0,0 @@ - - - basic - - - 1 - -1 - true - - -
tcp://a
- - -
tcp://b
- - diff --git a/lib/config/testdata/v6.xml b/lib/config/testdata/v6.xml deleted file mode 100644 index 245a48681..000000000 --- a/lib/config/testdata/v6.xml +++ /dev/null @@ -1,12 +0,0 @@ - - - - - - -
a
- - -
b
- - diff --git a/lib/config/testdata/v7.xml b/lib/config/testdata/v7.xml deleted file mode 100644 index b637e864e..000000000 --- a/lib/config/testdata/v7.xml +++ /dev/null @@ -1,12 +0,0 @@ - - - - - - -
a
- - -
b
- - diff --git a/lib/config/testdata/v8.xml b/lib/config/testdata/v8.xml deleted file mode 100644 index 0df863cc3..000000000 --- a/lib/config/testdata/v8.xml +++ /dev/null @@ -1,12 +0,0 @@ - - - - - - -
a
- - -
b
- - diff --git a/lib/config/testdata/v9.xml b/lib/config/testdata/v9.xml deleted file mode 100644 index f8ff9a565..000000000 --- a/lib/config/testdata/v9.xml +++ /dev/null @@ -1,12 +0,0 @@ - - - - - - -
a
- - -
b
- - diff --git a/lib/model/folder.go b/lib/model/folder.go index 045e55bbb..5fe3cf4bc 100644 --- a/lib/model/folder.go +++ b/lib/model/folder.go @@ -33,8 +33,9 @@ import ( "github.com/thejerf/suture" ) -// scanLimiter limits the number of concurrent scans. A limit of zero means no limit. -var scanLimiter = newByteSemaphore(0) +// folderIOLimiter limits the number of concurrent I/O heavy operations, +// such as scans and pulls. A limit of zero means no limit. +var folderIOLimiter = newByteSemaphore(0) type folder struct { suture.Service @@ -130,7 +131,7 @@ func (f *folder) serve(ctx context.Context) { pull := func() { startTime := time.Now() - if f.puller.pull() { + if f.pull() { // We're good. Don't schedule another pull and reset // the pause interval. pause = f.basePause() @@ -164,7 +165,7 @@ func (f *folder) serve(ctx context.Context) { case <-initialCompleted: // Initial scan has completed, we should do a pull initialCompleted = nil // never hit this case again - if !f.puller.pull() { + if !f.pull() { // Pulling failed, try again later. pullFailTimer.Reset(pause) } @@ -279,6 +280,35 @@ func (f *folder) getHealthError() error { return nil } +func (f *folder) pull() bool { + select { + case <-f.initialScanFinished: + default: + // Once the initial scan finished, a pull will be scheduled + return true + } + + // If there is nothing to do, don't even enter sync-waiting state. + abort := true + snap := f.fset.Snapshot() + snap.WithNeed(protocol.LocalDeviceID, func(intf db.FileIntf) bool { + abort = false + return false + }) + snap.Release() + if abort { + return true + } + + f.setState(FolderSyncWaiting) + defer f.setState(FolderIdle) + + folderIOLimiter.take(1) + defer folderIOLimiter.give(1) + + return f.puller.pull() +} + func (f *folder) scanSubdirs(subDirs []string) error { if err := f.getHealthError(); err != nil { // If there is a health error we set it as the folder error. We do not @@ -312,8 +342,8 @@ func (f *folder) scanSubdirs(subDirs []string) error { f.setError(nil) f.setState(FolderScanWaiting) - scanLimiter.take(1) - defer scanLimiter.give(1) + folderIOLimiter.take(1) + defer folderIOLimiter.give(1) for i := range subDirs { sub := osutil.NativeFilename(subDirs[i]) diff --git a/lib/model/folder_sendrecv.go b/lib/model/folder_sendrecv.go index 7f1d60465..1920acf11 100644 --- a/lib/model/folder_sendrecv.go +++ b/lib/model/folder_sendrecv.go @@ -140,25 +140,6 @@ func newSendReceiveFolder(model *model, fset *db.FileSet, ignores *ignore.Matche // pull returns true if it manages to get all needed items from peers, i.e. get // the device in sync with the global state. func (f *sendReceiveFolder) pull() bool { - select { - case <-f.initialScanFinished: - default: - // Once the initial scan finished, a pull will be scheduled - return true - } - - // If there is nothing to do, don't even enter pulling state. - abort := true - snap := f.fset.Snapshot() - snap.WithNeed(protocol.LocalDeviceID, func(intf db.FileIntf) bool { - abort = false - return false - }) - snap.Release() - if abort { - return true - } - if err := f.CheckHealth(); err != nil { l.Debugln("Skipping pull of", f.Description(), "due to folder error:", err) return false diff --git a/lib/model/folderstate.go b/lib/model/folderstate.go index d1a4c54c6..6a273029d 100644 --- a/lib/model/folderstate.go +++ b/lib/model/folderstate.go @@ -19,6 +19,7 @@ const ( FolderIdle folderState = iota FolderScanning FolderScanWaiting + FolderSyncWaiting FolderSyncPreparing FolderSyncing FolderError @@ -32,6 +33,8 @@ func (s folderState) String() string { return "scanning" case FolderScanWaiting: return "scan-waiting" + case FolderSyncWaiting: + return "sync-waiting" case FolderSyncPreparing: return "sync-preparing" case FolderSyncing: diff --git a/lib/model/model.go b/lib/model/model.go index d4ebbafca..dcffba06f 100644 --- a/lib/model/model.go +++ b/lib/model/model.go @@ -208,7 +208,7 @@ func NewModel(cfg config.Wrapper, id protocol.DeviceID, clientName, clientVersio m.deviceStatRefs[devID] = stats.NewDeviceStatisticsReference(m.db, devID.String()) } m.Add(m.progressEmitter) - scanLimiter.setCapacity(cfg.Options().MaxConcurrentScans) + folderIOLimiter.setCapacity(cfg.Options().MaxFolderConcurrency()) return m } @@ -2483,7 +2483,7 @@ func (m *model) CommitConfiguration(from, to config.Configuration) bool { } m.fmut.Unlock() - scanLimiter.setCapacity(to.Options.MaxConcurrentScans) + folderIOLimiter.setCapacity(to.Options.MaxFolderConcurrency()) // Some options don't require restart as those components handle it fine // by themselves. Compare the options structs containing only the diff --git a/test/folders.sh b/test/folders.sh new file mode 100755 index 000000000..52cfb62a8 --- /dev/null +++ b/test/folders.sh @@ -0,0 +1,11 @@ +#!/bin/bash + +for ((id=0;id<200;id++)); do +cat < + fake + + + +EOT +done \ No newline at end of file