Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gui: Enable large blocks by default, add to config dialog #5405

Merged
merged 3 commits into from Feb 2, 2019

Conversation

Projects
None yet
6 participants
@calmh
Copy link
Member

commented Dec 25, 2018

Purpose

It's been ~9 months since we introduced large blocks support; the odds of a cluster being mixed between 0.14.45- and 0.14.56+ should be quite small. This enables it for newly created folders.

Also moves a couple of </div> that were out of balance and reindents accordingly, so try a white space insensitive diff when reviewing...

screenshot 2018-12-25 at 14 39 26

gui: Enable large blocks by default, add to config dialog
Also moves a couple of </div> that were out of balance and reindents
accordingly, so try a white space insensitive diff when reviewing...
@Catfriend1

This comment has been minimized.

Copy link

commented Dec 26, 2018

I've already read multiple threads on the forum about enabling large blocks causing trouble. Assuming I've got synchthing versions >=0.14.48 in place (most are 0.14.54's) and I don't clear the database but post-enable large blocks on all nodes and all existing folders:

  • will files get rehashed automatically?
  • will files get transferred even if they were the same already?
  • are problems expected that are already known?
    So in summary I'd like to know if it can be enabled later after a folder had been created in the past or if it should only get enabled for folders newly created.
@wweich

This comment has been minimized.

Copy link
Member

commented Dec 26, 2018

The large blocks setting enables variable block size. Files won't get rehashed, so files which would get higher block sizes are kept in DB with small block sizes.
You can enable it for existing folders. Only new / changed files will get variable block sizes. But if it doesn't get enabled on all nodes at the same time, and there are changes to a file on two devices (one with large blocks, one without) you probably end up transferring the whole file, as not a single block matches.

@calmh

This comment has been minimized.

Copy link
Member Author

commented Dec 26, 2018

Which problems are these?

@Catfriend1

This comment has been minimized.

Copy link

commented Dec 26, 2018

Thanks for the explanation. I'm currently thinking about enabling it on my nodes and making large blocks also default after a user notice question on android telling that this setting if turned on needs to be consistent across all nodes.
Related: https://forum.syncthing.net/t/one-sided-large-blocks-cause-huge-sync-conflicts/11705/10

@imsodin

This comment has been minimized.

Copy link
Member

commented Dec 30, 2018

In my opinion we could just enable it for all folders, also existing once. If we want to be cautious, do it with the usual banner in the web UI where the user needs to confirm or deny switching and with a warning about the incompatibility with <0.14.45. However I'd say this is unlikely enough to just enable it without notice. Just my 5 cents, I don't care much either way (I do like the feature though :) ).

@Catfriend1

This comment has been minimized.

Copy link

commented Dec 30, 2018

I basically agree with @imsodin
Trying out large Blocks the last days, I noticed the already mentioned aspect about files getting retransferred and/or conflicts getting created for files bigger than 256 MByte when one side gets a database reset (I did this on one device to resolve other problems remaining in the database from the far past) and found the creation of those conflicts "duplicates" of the same original file a little bit nasty to clean up. Would there be any way to implement a "hint" for a syncthing instance, that starts a fresh database, to notify other nodes about that so they handle block sizes correctly without creating conflicts?
I guess the following happened:

  • Folder F on Device A and B was in sync without large blocks.
  • I've enabled large blocks on both devices simultaneously. > Folder still in sync.
  • For another misbehaving folder G, I reset the database on device B and kept the folder F config on B including the large blocks setting.
  • Rehash on B took place, large blocks were put into the database (folder F).
  • Device A and B exchanged index for folder F.
  • Device B got "small legacy blocks" from A for folder F while it had (itself) large blocks from the rehash.
  • Sync-conflict-XXXXX files got created and needed to be cleant up manually.
@wweich

This comment has been minimized.

Copy link
Member

commented Dec 30, 2018

Syncthing could be smart enough to see that the block size is "above normal" and do a rehash itself before creating the conflict.

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

so wazzup wit dis?

@calmh

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

I'm awaiting either acceptance or actionable review comments. I don't think just enabling it everywhere is the right thing at this point.

@imsodin

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

What's the intention of adding it to the folder edit dialog? As I remember the only reason for making it configurable is that it isn't backward compatible - other than that there is no reason not to enable it in general. Or more general question: What would be the next steps to "streamline" large blocks?

@calmh

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

My thinking behind adding it to the dialog was that since it's a backwards-incompatible thing there should be an easy opt out. It also makes it a little bit visible that there is something new that differs.

With default-on most new folders would get it set. The trouble with flipping it on everywhere is that there'll be issues on existing folders shared between v1.0.1 and v1.0.2 (if 1.0.2 flips it on automatically).
Especially things like NAS:es (and Android sometimes) tend to version lag quite a while, leaving even people who want the latest with mixed version clusters.

Screwing up their existing stuff is unkind. We want people to run with automatic upgrades. Rewarding that by breaking their setup is counterproductive.

After a while with default-on it's easier to set it to always on, and remove the option completely. Certainly some will anyway be taken by surprise, but it'll be fewer.

@imsodin

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

Thanks for the explanation!

I'd add a help symbol/link to https://docs.syncthing.net/advanced/folder-uselargeblocks.html, potentially together with some sort of pointer about recommending to activate and compatibility.

Also on the screenshot "Full Rescan Interval" is bold, while it shouldn't be (subentry of "Scanning"), but I can't see the reason in the html (maybe the screenshot is outdated?).

@AudriusButkevicius
Copy link
Member

left a comment

Gooooooo

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

I say, cut a 2.0.0 and break everyone. We were rolling in that 0.x stuff way too long, time to roll forward.

@calmh

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

Yeah but you say that for every change ever and it still ain't happenin'

@imsodin I'll check it out & sort it

@alex2108

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

I think the problem with sync conflicts that @Catfriend1 mentioned should be somehow handled, otherwise there will be a lot of complaints about conflicts after merging this, resetting database or syncing files with rsync and adding the folder afterwards is not that uncommon I think.

Another thought: could this even happen when the folder always had large blocks on all devices? Hysteresis could lead to mismatch of the block size.

@calmh

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

This is specifically for new folders, so I don’t see that being a common failure case.

@Catfriend1

This comment has been minimized.

Copy link

commented Jan 15, 2019

What will happen if a user shares an existing and already scanned folder from node A to B while A hasn't large blocks enabled and B sees this a new folder in the config and automatically turns large blocks on?

@calmh

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2019

You'll get a mismatch in the settings.

What we could do, maybe, is make large blocks "infectious". Send a boolean in the initial negotiation, if we see it we enable large blocks regardless. It doesn't help much with earlier versions, but it would help in future conversions.

Also keep in mind that the effect of mismatching settings is just a less efficient sync and some wasted bandwidth. Horror stories about sync conflicts are about special case scenarios like setting up a new cluster with devices that already have matching data but mismatching settings.

@Catfriend1

This comment has been minimized.

Copy link

commented Jan 18, 2019

I've also thought of proposing the infectious approach when nodes see each other next time connecting those days. If possible, that might help a lot during migration and can be dropped later in a future version.

@calmh

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2019

While convenient though, that would make it extremely frustrating and almost impossible to turn off large blocks, should one want to do that.

@Catfriend1

This comment has been minimized.

Copy link

commented Jan 18, 2019

You're right on this. Is large blocks considered ready for wide production use? From my reading I think it is fine.

calmh added some commits Feb 2, 2019

Merge branch 'master' into largeb
* master: (61 commits)
  build: Remove outdated&non-functional setup command (fixes #5454) (#5455)
  cmd/syncthing, lib/config: Update default config creation (#5492)
  golangci: Add config file
  cmd/syncthing: Correct strings.HasPrefix args order (#5498)
  all: Revert the underscore sillyness
  all: A few more interesting linter fixes (#5502)
  all: Even more boring linter fixes (#5501)
  all: Bunch of more linter fixes (#5500)
  all: Fix some linter errors (#5499)
  lib/db: Fix race in NamespacedKV (#5496)
  docker: Build outside GOPATH (fixes #5495)
  cmd/syncthing: Pass SIGTERM on in monitor (fixes #5493) (#5494)
  lib/model: In tests disable watching for changes by default (fixes #5246) (#5485)
  gui, man, authors: Update docs, translations, and contributors
  test, lib/rc: Integration test fixes and polish (#5488)
  mod: Update dependencies and tidy (fixes #5311) (#5486)
  lib/config: Add omitempty to DeprecatedMinHomeDiskFreePct (fixes #5482) (#5484)
  all: Copy owner/group from parent (fixes #5445) (#5479)
  vendor: rm -rf vendor (#5478)
  lib/model: Improve errors while pulling (#5474)
  ...
Merge branch 'master' into largeb
* master:
  cmd/syncthing: Fixup previous commit

@calmh calmh merged commit c75cfcf into syncthing:master Feb 2, 2019

7 checks passed

Build (Linux, Cross) (Syncthing) TeamCity build finished
Details
Build (Mac) (Syncthing) TeamCity build finished
Details
Build (Source) (Syncthing) TeamCity build finished
Details
Build (Windows) (Syncthing) TeamCity build finished
Details
Check Correctness (Syncthing) TeamCity build finished
Details
Check Old Go Version (Syncthing) TeamCity build finished
Details
GolangCI No issues found!
Details

@calmh calmh deleted the calmh:largeb branch Feb 2, 2019

@Catfriend1

This comment has been minimized.

Copy link

commented Feb 2, 2019

Should we enable large blocks for new folders on Android also?
What will happen if a user shares a folder from Syncthing 1.0.0 (noLargeBlocks) to an Android instance running Syncthing 1.0.1 and we suggest enabling the blocks, is there any way for the other side on Android to know that a shared folder has largeBlocks enabled or disabled to propose the right setting state to the user when adding?

@calmh calmh added this to the v1.1.0 milestone Feb 5, 2019

calmh added a commit to calmh/syncthing that referenced this pull request Feb 12, 2019

Merge branch 'master' into ldap3
* master:
  Fix builds on Windows without CGO (syncthing#5518)
  lib/db, lib/model: Remove dead code (syncthing#5517)
  gui: Add missing translation string (fixes syncthing#5515)
  lib/model: Don't use LocalDeviceID as normal id in tests (syncthing#5512)
  gui, man, authors: Update docs, translations, and contributors
  lib/model: Improve TestIssue5063 (syncthing#5509)
  lib/model: Add progressEmitter to supervisor (model) (syncthing#5510)
  lib/model: Fail test instead of panic due to closing channel twice (syncthing#5508)
  lib/model: Helperize test os and remove error return value (syncthing#5507)
  gui: Enable large blocks by default, add to config dialog (syncthing#5405)

@calmh calmh added enhancement and removed enhancement labels Mar 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.