Skip to content

graphdriver/copy: faster copy of hard links#46812

Merged
thaJeztah merged 1 commit intomoby:masterfrom
robmry:46810-vfs_faster_copy_of_hard_links
Nov 14, 2023
Merged

graphdriver/copy: faster copy of hard links#46812
thaJeztah merged 1 commit intomoby:masterfrom
robmry:46810-vfs_faster_copy_of_hard_links

Conversation

@robmry
Copy link
Contributor

@robmry robmry commented Nov 13, 2023

Fixes #46810

  • What I did

Speed up vfs's copy of a BusyBox filesystem (which consists mainly of hard links to a single binary), making moby's integration tests run more quickly and more reliably in a dev container.

  • How I did it

The DirCopy() function in "graphdriver/copy/copy.go" has a special case for skip file-attribute copying when making a hard link to an already-copied file, if "copyMode == Hardlink". Do the same for copies of hard-links in the source filesystem.

  • How to verify it

Running the integration tests, as described in #46810 ...

Without the change (and with the printfs described above), in this case all the tests finished just within their 5s timeouts in ~88s:

INFO: Testing against a local daemon
=== RUN   TestBridgeICC
=== RUN   TestBridgeICC/IPv4_non-internal_network
CLI Post call took 4.730112544s
CLI Post call took 4.702928253s
=== RUN   TestBridgeICC/IPv4_internal_network
CLI Post call took 4.674024294s
CLI Post call took 4.757430418s
=== RUN   TestBridgeICC/IPv6_ULA_on_non-internal_network
CLI Post call took 4.743797003s
CLI Post call took 4.734315211s
=== RUN   TestBridgeICC/IPv6_ULA_on_internal_network
CLI Post call took 4.754429169s
CLI Post call took 4.753247336s
=== RUN   TestBridgeICC/IPv6_link-local_address_on_non-internal_network
CLI Post call took 4.897648835s
CLI Post call took 4.752835585s
=== RUN   TestBridgeICC/IPv6_link-local_address_on_internal_network
CLI Post call took 5.149620378s
CLI Post call took 4.732314336s
=== RUN   TestBridgeICC/IPv6_non-internal_network_with_SLAAC_LL_address
CLI Post call took 4.896428502s
CLI Post call took 4.747824461s
=== RUN   TestBridgeICC/IPv6_internal_network_with_SLAAC_LL_address
CLI Post call took 5.117011001s
CLI Post call took 4.738395294s
--- PASS: TestBridgeICC (88.47s)
    --- PASS: TestBridgeICC/IPv4_non-internal_network (10.49s)
    --- PASS: TestBridgeICC/IPv4_internal_network (10.44s)
    --- PASS: TestBridgeICC/IPv6_ULA_on_non-internal_network (10.60s)
    --- PASS: TestBridgeICC/IPv6_ULA_on_internal_network (10.53s)
    --- PASS: TestBridgeICC/IPv6_link-local_address_on_non-internal_network (10.76s)
    --- PASS: TestBridgeICC/IPv6_link-local_address_on_internal_network (10.91s)
    --- PASS: TestBridgeICC/IPv6_non-internal_network_with_SLAAC_LL_address (10.74s)
    --- PASS: TestBridgeICC/IPv6_internal_network_with_SLAAC_LL_address (10.93s)
PASS

DONE 9 tests in 88.516s

With the change, tests completed in ~22s:

INFO: Testing against a local daemon
=== RUN   TestBridgeICC
=== RUN   TestBridgeICC/IPv4_non-internal_network
CLI Post call took 414.793708ms
CLI Post call took 412.339542ms
=== RUN   TestBridgeICC/IPv4_internal_network
CLI Post call took 413.349417ms
CLI Post call took 415.359375ms
=== RUN   TestBridgeICC/IPv6_ULA_on_non-internal_network
CLI Post call took 433.486417ms
CLI Post call took 410.262041ms
=== RUN   TestBridgeICC/IPv6_ULA_on_internal_network
CLI Post call took 430.581ms
CLI Post call took 412.697042ms
=== RUN   TestBridgeICC/IPv6_link-local_address_on_non-internal_network
CLI Post call took 466.356583ms
CLI Post call took 413.003459ms
=== RUN   TestBridgeICC/IPv6_link-local_address_on_internal_network
CLI Post call took 449.755042ms
CLI Post call took 407.237459ms
=== RUN   TestBridgeICC/IPv6_non-internal_network_with_SLAAC_LL_address
CLI Post call took 446.003292ms
CLI Post call took 430.060083ms
=== RUN   TestBridgeICC/IPv6_internal_network_with_SLAAC_LL_address
CLI Post call took 431.701709ms
CLI Post call took 403.693125ms
--- PASS: TestBridgeICC (21.49s)
    --- PASS: TestBridgeICC/IPv4_non-internal_network (1.91s)
    --- PASS: TestBridgeICC/IPv4_internal_network (1.84s)
    --- PASS: TestBridgeICC/IPv6_ULA_on_non-internal_network (1.91s)
    --- PASS: TestBridgeICC/IPv6_ULA_on_internal_network (1.82s)
    --- PASS: TestBridgeICC/IPv6_link-local_address_on_non-internal_network (1.96s)
    --- PASS: TestBridgeICC/IPv6_link-local_address_on_internal_network (1.86s)
    --- PASS: TestBridgeICC/IPv6_non-internal_network_with_SLAAC_LL_address (3.10s)
    --- PASS: TestBridgeICC/IPv6_internal_network_with_SLAAC_LL_address (4.09s)
PASS

DONE 9 tests in 22.711s

Unit tests are still happy...

The other timed-out integration tests listed in the original description also pass.
  • Description for the changelog

graphdriver/copy: faster copy of hard links

@robmry
graphdriver/copy: faster copy of hard links
a3fdad8 
The DirCopy() function in "graphdriver/copy/copy.go" has a special case for
skip file-attribute copying when making a hard link to an already-copied
file, if "copyMode == Hardlink". Do the same for copies of hard-links in
the source filesystem.

Significantly speeds up vfs's copy of a BusyBox filesystem (which
consists mainly of hard links to a single binary), making moby's
integration tests run more quickly and more reliably in a dev container.

Fixes moby#46810

Signed-off-by: Rob Murray <rob.murray@docker.com>
thaJeztah
thaJeztah approved these changes Nov 13, 2023
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah added the kind/bugfix PR's that fix bugs label Nov 13, 2023
@thaJeztah thaJeztah added this to the 25.0.0 milestone Nov 13, 2023
@thaJeztah
Copy link
Member

thaJeztah commented Nov 13, 2023

Some failure on Windows; looks like it already was restarted once (but haven't checked if it was the same failure); let me post it here in case it's related;

== Failed
=== FAIL: github.com/docker/docker/integration/container TestWindowsDevices/process/class/5B45201D-F2F2-4F3B-85BB-30FF1F953599_mounted (1.49s)
    devices_windows_test.go:107: assertion failed: error is not nil: Error response from daemon: failed to create task for container: failed to create shim task: hcs::CreateComputeSystem 979f82ae3a58a34a26c3c3116f0ccdb5558cb4ff8d7f2494def2165b81129da0: The parameter is incorrect.: unknown

@thaJeztah
Copy link
Member

thaJeztah commented Nov 14, 2023

Looks like that was a glitch; CI is happy now 👍

@thaJeztah thaJeztah merged commit e6ae462 into moby:master Nov 14, 2023
@thaJeztah
Copy link
Member

thaJeztah commented Nov 14, 2023

Out of curiousity; had a quick check why this was missing; The ishardlink was originally added in bc5503f (#13067); but looks like the later optimization made in b467f8b (#35537) did not take that into account.

Looking at this code, I'm now actually wondering if we should have that variable at all, or if we instead could just return early (which is what the ishardlink var does if it's set); perhaps that would make the code less brittle (keeping things together).

@robmry
Copy link
Contributor Author

robmry commented Nov 14, 2023

Looking at this code, I'm now actually wondering if we should have that variable at all, or if we instead could just return early (which is what the ishardlink var does if it's set); perhaps that would make the code less brittle (keeping things together).

Minimal change seemed like the best plan for my first commit (!) ... but factoring the "switch" out into a function that returns something to say whether to copy the metadata would make the intention hard to miss.

(There might also be scope for speeding up regular-file copying by doing some of the i/o bound work in parallel on a pool of worker goroutines. But, speeding up the BusyBox-based integration tests seemed like a quick-win worth having, and I wasn't sure whether there was any driver to try to go further.)

@robmry robmry deleted the 46810-vfs_faster_copy_of_hard_links branch December 4, 2023 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees

No one assigned

Labels

area/performance area/storage Image Storage kind/bugfix PR's that fix bugs status/2-code-review

Projects

None yet

Milestone

25.0.0

Development

Successfully merging this pull request may close these issues.

Integration test timeouts

3 participants

@robmry @thaJeztah @cpuguy83