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

lib/model, lib/protocol: Fix file comparisons (fixes #8594) #8603

Merged
merged 10 commits into from
Oct 16, 2022

Conversation

calmh
Copy link
Member

@calmh calmh commented Oct 13, 2022

Currently there's a couple of problems with file comparisons introduced in the xattr+ownership work.

  1. The comparisons for when xattrs and ownership differ were logically wrong. I made it so that two files are considered equal if one has nil platform data for a given platform, and the other one has data for that platform. The intention was that when a file comes from Windows and we add Linux data locally, it shouldn't cause a resync everywhere. But that's wrong, because the added Linux data might be of interest to other Linux hosts...
  2. There's a comparison for "equivalence" before we insert a fileinfo in the database. If it's "equivalent" to the global version we use that instead. That meant that the inode change time we added was forgotten, because the global version generally has a zero inode change time (from the network) and this was considered equivalent to the one we just scanned. It also meant we didn't store xattr-only changes if there wasn't already xattr data present, as per number one above.

This attempts to fix that, by making the comparisons stricter. The effect will be more changes and more syncs, but the changes to apply should be minimal.

This very likely also fixes #8604 and fixes #8590.

Previous debug input didn't really give enough info to show what was
happening, while it also printed full block lists which are enormously
verbose. Now it consistently prints 1. what it sees on disk, 2. what it
got from CurrentFile (without blocks), 3. the action taken on that file.
* debug:
  wip
  wip
  lib/scanner: More sensible debug output
* main:
  lib/scanner: More sensible debug output (syncthing#8596)
  gui: Allow automatic device ID selection on WebKit browsers (ref syncthing#8544) (syncthing#8597)
  gui, man, authors: Update docs, translations, and contributors
@calmh calmh marked this pull request as ready for review October 14, 2022 11:59
@calmh calmh changed the title lib/model, lib/protocol: Fix file comparisons (fixes #xxx) lib/model, lib/protocol: Fix file comparisons (fixes #8594) Oct 14, 2022
@calmh calmh requested a review from imsodin October 14, 2022 12:01
@imsodin imsodin linked an issue Oct 16, 2022 that may be closed by this pull request
Copy link
Member

@imsodin imsodin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generally stricter ownership/xattr checking is clear enough. The flag for the strict inode change time is a bit harder to reason about. Also they are independent, right? Maybe split them up in two PRs.

For the change time: The strict flag is only needed in a code branch for a receive-only optimisation, to not create an insignificant local change that will be resolved by the subsequent pull. I don't think this had any other intended purpose nor can I see one now. I'd rather move those conditions behind a receive-only flag than adding the strict inode change time flag that's only used there.

@imsodin
Copy link
Member

imsodin commented Oct 16, 2022

In maybe a bit clearer (and fewer) words:
For the equivalence check the current behaviour seems correct for recv-only folders (and recv-enc), and it's only intended to solve aproblem for receive-only folders. Thus I propose to keep the current check but move it behind a folder.Type in [recv-only, recv-end] condition.

@calmh
Copy link
Member Author

calmh commented Oct 16, 2022

I implemented what I think you mean, in hiding the equivalence check behind a condition on the folder type, and removing the strict checking mode which is no longer required.

@calmh calmh merged commit d3f5063 into syncthing:main Oct 16, 2022
@calmh calmh deleted the handlexattrsownership branch October 16, 2022 15:04
@calmh calmh added this to the v1.22.1 milestone Oct 17, 2022
calmh added a commit to imsodin/syncthing that referenced this pull request Nov 8, 2022
* main: (36 commits)
  lib/protocol: Ignore inode time when xattr&ownership is ignored (fixes syncthing#8654) (syncthing#8655)
  lib/fs: Try to remove read only Windows files (fixes syncthing#3744) (syncthing#8650)
  gui: Add copy to clipboard, share by email, and share by SMS buttons to device IDs (fixes syncthing#2771, ref syncthing#3868) (syncthing#7984)
  gui, man, authors: Update docs, translations, and contributors
  build: Add GitHub actions build for Windows (syncthing#8627)
  gui: Fix connection type icon width (fixes syncthing#8592) (syncthing#8644)
  gui: Adjust connection type icon size scaling and alignment (syncthing#8645)
  docker: Use healthcheck endpoint (syncthing#8640)
  lib/connections: Use adaptive write size for rate limited connections (fixes syncthing#8630) (syncthing#8631)
  gui: Mark devices that haven't connected for a long time (fixes syncthing#7703) (syncthing#8530)
  gui: Fix rescan interval when add encrypted folder with watch for changes enabled (fixes syncthing#8570) (syncthing#8571)
  gui: Always show Out of Sync Items for remote devices (syncthing#8632)
  lib/fs: Let xattr test avoid non-test attributes (fixes syncthing#8601) (syncthing#8628)
  build: Add GitHub actions build for Windows
  gui, man, authors: Update docs, translations, and contributors
  gui: Display folder and device count number (syncthing#8615)
  gui, man, authors: Update docs, translations, and contributors
  lib/model, lib/protocol: Fix file comparisons (fixes syncthing#8594) (syncthing#8603)
  lib/scanner: More sensible debug output (syncthing#8596)
  gui: Allow automatic device ID selection on WebKit browsers (ref syncthing#8544) (syncthing#8597)
  ...
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Oct 16, 2023
@syncthing syncthing locked and limited conversation to collaborators Oct 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
3 participants