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

all: Add copy-on-write filesystem support (fixes #4271) #6746

Merged
merged 51 commits into from
Jun 18, 2020

Conversation

AudriusButkevicius
Copy link
Member

@AudriusButkevicius AudriusButkevicius commented Jun 15, 2020

I can't merge into my own branch and have a PR here, so opening the full thing.
Only the last commit is interesting here.

@AudriusButkevicius AudriusButkevicius changed the title Use copy range all: Add copy-on-write filesystem support (fixes #4271) Jun 16, 2020
lib/fs/basicfs_copy_range.go Outdated Show resolved Hide resolved
lib/fs/basicfs_copy_range.go Outdated Show resolved Hide resolved
lib/fs/basicfs_copy_range.go Outdated Show resolved Hide resolved
lib/fs/basicfs_copy_range_copyfilerange.go Outdated Show resolved Hide resolved
lib/fs/basicfs_copy_range_ioctl.go Outdated Show resolved Hide resolved
lib/fs/basicfs_copy_range_ioctl.go Outdated Show resolved Hide resolved
lib/fs/basicfs_copy_range_sendfile.go Outdated Show resolved Hide resolved
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.

Just a few "organisational" nits.

lib/fs/basicfs_copy_range.go Outdated Show resolved Hide resolved
lib/fs/basicfs_copy_range_allwithfallback.go Outdated Show resolved Hide resolved
@AudriusButkevicius
Copy link
Member Author

We gud?

@calmh
Copy link
Member

calmh commented Jun 18, 2020

Write a docs page on roughly what the different configs do?

@calmh calmh merged commit 4812fd3 into syncthing:main Jun 18, 2020
@calmh calmh added this to the v1.8.0 milestone Jun 18, 2020
@AudriusButkevicius
Copy link
Member Author

Also, when we add this in the change log, we should mention that this is an option you need to enable.

@AudriusButkevicius AudriusButkevicius deleted the use_copy_range branch June 18, 2020 07:02
@Atera
Copy link

Atera commented Jun 18, 2020

I volunteered to test in the other PR. Would love to give this a spin. If the documentation's not done yet, could you briefly state what I need to tweak to enable the changes? I add a 'copyRangeMethod' to the folder options and set it to 'copy_file_range'? I seem to see options standard, ioctl, copy_file_range, sendfile, and all, but I'm not sure I'm looking in the right place or what these behaviours are.

@AudriusButkevicius
Copy link
Member Author

If you start a master build you'll see new option in folder config after the first save, change that option to one of the mechanisms as appropriate.

calmh added a commit to Shidory/syncthing that referenced this pull request Jun 23, 2020
* main: (218 commits)
  gui, man, authors: Update docs, translations, and contributors
  lib/protocol: Use sha256.Sum256 in NewDeviceID (syncthing#6775)
  lib/scanner: Fix Validate docs (syncthing#6776)
  build: Set CGO_ENABLED=0 by default, except on specific platforms (syncthing#6768)
  gui, lib/api: Add more version info to about dialog (syncthing#6773)
  lib/model: Use right db snap when scanning recvonly folder (syncthing#6769)
  build: Change version strings to not have plus in them (ref syncthing#6758) (syncthing#6760)
  lib/model: Don't ignore stat failure in performFinish (syncthing#6766)
  lib/fs: Add support for Windows duplicate extents (syncthing#6764)
  lib/model: Don't stay scanning forever on fail (syncthing#6761)
  gui, lib/ignore: Handle editing ignores with error (fixes syncthing#5425) (syncthing#6757)
  lib: Print nicely rounded durations (syncthing#6756)
  all: Add copy-on-write filesystem support (fixes syncthing#4271) (syncthing#6746)
  lib/rand: Various minor fixes (syncthing#6752)
  lib/db: Improve error message on meta inconsistency (syncthing#6751)
  lib/db: Use explicit byte type for type prefixes (syncthing#6754)
  gui, man, authors: Update docs, translations, and contributors
  lib/model: Check dir before deletion when pulling (syncthing#6741)
  build: Update integration tests and add to build.go (syncthing#6744)
  cmd/stdiscosrv: Don't abuse wrong header (syncthing#6749)
  ...
@calmh
Copy link
Member

calmh commented Jul 14, 2020

Any docs on this available? I would like to point to a page with some guidance so that we can get some testing early in the 1.8.0 RC cycle as I expect we might need iterations on this...

@AudriusButkevicius
Copy link
Member Author

Yes, it's in the folder section.

@calmh
Copy link
Member

calmh commented Jul 14, 2020

Right... Mind if I copy/move it to a separate advanced section, to make it easier to link & refer to?

@AudriusButkevicius
Copy link
Member Author

Sure, whatevs.

@mksully22
Copy link

This pull request (commit 4812fd3) fails compile on ppc64le (compiling on Alpine).
A git bisect was used to narrow down the commit that was responsible. The Error reported:

--- FAIL: TestCopyRange (2.73s)
    --- FAIL: TestCopyRange/tmp (2.73s)
        --- FAIL: TestCopyRange/tmp/ioctl (0.34s)
            --- FAIL: TestCopyRange/tmp/ioctl/append_to_end (0.02s)
                filesystem_copy_range_test.go:332: inappropriate ioctl for device
            --- FAIL: TestCopyRange/tmp/ioctl/append_to_end_offsets_at_start (0.03s)
                filesystem_copy_range_test.go:332: inappropriate ioctl for device
            --- FAIL: TestCopyRange/tmp/ioctl/overwrite_part_of_destination_region (0.02s)
                filesystem_copy_range_test.go:332: inappropriate ioctl for device
            --- FAIL: TestCopyRange/tmp/ioctl/overwrite_all_of_destination (0.03s)
                filesystem_copy_range_test.go:332: inappropriate ioctl for device
            --- FAIL: TestCopyRange/tmp/ioctl/overwrite_part_of_destination (0.02s)
                filesystem_copy_range_test.go:332: inappropriate ioctl for device
            --- FAIL: TestCopyRange/tmp/ioctl/destination_gets_expanded_as_it_is_being_written_to (0.02s)
                filesystem_copy_range_test.go:332: inappropriate ioctl for device
            --- FAIL: TestCopyRange/tmp/ioctl/unaligned_source_offset_unaligned_size (0.01s)
                filesystem_copy_range_test.go:332: inappropriate ioctl for device
            --- FAIL: TestCopyRange/tmp/ioctl/unaligned_source_offset_aligned_size (0.03s)
                filesystem_copy_range_test.go:332: inappropriate ioctl for device
            --- FAIL: TestCopyRange/tmp/ioctl/unaligned_destination_offset_unaligned_size (0.03s)
                filesystem_copy_range_test.go:332: inappropriate ioctl for device
            --- FAIL: TestCopyRange/tmp/ioctl/unaligned_destination_offset_aligned_size (0.03s)
                filesystem_copy_range_test.go:332: inappropriate ioctl for device
            --- FAIL: TestCopyRange/tmp/ioctl/aligned_offsets_unaligned_size (0.03s)
                filesystem_copy_range_test.go:332: inappropriate ioctl for device
            --- FAIL: TestCopyRange/tmp/ioctl/last_block (0.01s)
                filesystem_copy_range_test.go:332: inappropriate ioctl for device
            --- FAIL: TestCopyRange/tmp/ioctl/whole_file_copy_block_aligned (0.01s)
                filesystem_copy_range_test.go:332: inappropriate ioctl for device
            --- FAIL: TestCopyRange/tmp/ioctl/whole_file_copy_not_block_aligned (0.01s)
                filesystem_copy_range_test.go:332: inappropriate ioctl for device
FAIL
FAIL    github.com/syncthing/syncthing/lib/fs   3.214s

@AudriusButkevicius
Copy link
Member Author

There was a follow up PR in 1.8.0 that probably fixes this.

@mksully22
Copy link

Thank you Audrius. I just re-attempted to build the latest (main branch) with git log showing the latest commit being:
'''
commit fd8bea6 (HEAD -> main, origin/main, origin/HEAD)
Author: Simon Frei [email protected]
Date: Mon Aug 10 18:48:37 2020 +0200

lib/osutil: Preserve perms in AtomicWriter (fixes #tbd) (#6885)

'''

It also fails in the same way at this level. Is the PR you are thinking of uncommitted? If so can you give me a link to it? I'd be happy to give it a try.

@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 Jun 18, 2021
@syncthing syncthing locked and limited conversation to collaborators Jun 18, 2021
@AudriusButkevicius
Copy link
Member Author

For people finding this ticket in the future.
This is now implemented as an optional configuration, but the default behaviour remains the same, copy data into application memory.

You can find the docs for the feature here: https://docs.syncthing.net/advanced/folder-copyrangemethod.html

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
Development

Successfully merging this pull request may close these issues.

6 participants