| Age | Commit message (Collapse) | Author |
|---|
|
The gofer client's LRU cache has a default limit of 1000 dentries. Any attempt
to cache more dentries than that will make the LRU cache evict and destroy the
least recently used dentry. However, the eviction is expensive because it
requires holding fs.renameMu for writing - which in turn creates a lot of
contention. All filesystem operations that involve path traversal require
fs.renameMu for reading atleast.
Therefore, it is in our best interest to keep the cache small and clean.
When a dentry is inserted in the dentry tree, it grabs a ref on its parent for
its entire lifetime. Hence the parent is longer evictable (because refs > 0).
This change additionally calls checkCachingLocked on directories that have been
added to so that they can be removed from the LRU cache if needed.
This change implies that the LRU cache will only contain the leaves from the
filesystem tree which significantly reduces the LRU cache size and consequently
reduces the number of expensive LRU cache evictions.
> Why are opened dentries not removed from LRU cache?
When a file description is open(2)-ed, the file description holds a ref on its
dentry for its entire lifetime. However, calling checkCachingLocked() on opened
dentries actually ends up hurting performance. Applications usually open file
descriptors for a short duration. So upon close(2), the dentry is reinserted
into the cache anyway. So the precautionary work done in removing the opened
dentry from the cache went for waste as it did not really reduce an eviction.
Local benchmarking has shown that this change improves performance by 3-4%.
Across 6 runs, without this change it took 296.127 seconds to build runsc while
with this change it took only 285.136 seconds.
PiperOrigin-RevId: 369510494
|
|
PiperOrigin-RevId: 369505182
|
|
Remote revalidating requires to update file size on every write
on a file opened with O_APPEND. If host FD exists, it can be
used to update the size and skip round trip to the gofer. With
this change, O_APPEND writes with remote revalidating is almost
as fast as exclusive mode:
BM_Append
VFS1 60.7us
VFS2 56.8us
VFS2 exclusive 14.2us
This change 15.8us
Updates #1792
PiperOrigin-RevId: 369486801
|
|
Fixes #2926, #674
PiperOrigin-RevId: 369457123
|
|
This is done for IPv4, UDP and TCP headers.
This also changes the packet checkers used in tests to error on
zero-checksum, not sure why it was allowed before.
And while I'm here, make comments' case consistent.
RELNOTES: n/a
Fixes #5049
PiperOrigin-RevId: 369383862
|
|
Currently the verity action is a global variable, which causes the same
action for all verity mounts, and is overwritten for each new verity
mount. Changed it to a member of verity fs.
PiperOrigin-RevId: 369348522
|
|
In the previous spot, there was a roughly 50% chance that leak checking would
actually run. Move it to the waitContainer() call on the root container, where
it is guaranteed to run before the sandbox process is terminated. Add it to
runsc/cli/main.go as well for good measure, in case the sandbox exit path does
not involve waitContainer().
PiperOrigin-RevId: 369329796
|
|
This change replaces individual private members in tcp.endpoint with a single
private TCPEndpointState member.
Some internal substructures within endpoint (receiver, sender) have been broken
into a public substructure (which is then copied into the TCPEndpointState
returned from completeState()) alongside other private fields.
Fixes #4466
PiperOrigin-RevId: 369329514
|
|
While using remote-validation, the vast majority of time spent during
FS operations is re-walking the path to check for modifications and
then closing the file given that in most cases it has not been
modified externally.
This change introduces a new 9P message called MultiGetAttr which bulks
query attributes of several files in one shot. The returned attributes are
then used to update cached dentries before they are walked. File attributes
are updated for files that still exist. Dentries that have been deleted are
removed from the cache. And negative cache entries are removed if a new
file/directory was created externally. Similarly, synthetic dentries are
replaced if a file/directory is created externally.
The bulk update needs to be carefull not to follow symlinks, cross mount
points, because the gofer doesn't know how to resolve symlinks and where
mounts points are located. It also doesn't walk to the parent ("..") to
avoid deadlocks.
Here are the results:
Workload VFS1 VFS2 Change
bazel action 115s 70s 28.8s
Stat/100 11,043us 7,623us 974us
Updates #1638
PiperOrigin-RevId: 369325957
|
|
- Added delay to increase the RTT: In DSACK tests with RACK enabled and low
RTT, TLP can be detected before sending ACK and the tests flake. Increasing
the RTT will ensure that TLP does not happen before the ACK is sent.
- Fix TestRACKOnePacketTailLoss: The ACK does not contain DSACK, which means
either the original or retransmission (probe) was lost and SACKRecovery count
must be incremented.
Before: http://sponge2/c9bd51de-f72f-481c-a7f3-e782e7524883
After: http://sponge2/1307a796-103a-4a45-b699-e8d239220ed1
PiperOrigin-RevId: 369305720
|
|
There is a loop that fills a byte array with zero-s. Let's use copy() instead
of setting elements one by one.
The new implementation is two time faster than the previous one and it is more
than 10x faster with the race detector.
Reported-by: syzbot+5f57d988a5f929af5a91@syzkaller.appspotmail.com
PiperOrigin-RevId: 369283919
|
|
This fixes a race that occurs while the endpoint is being unregistered
and the transport demuxer attempts to match the incoming packet to any
endpoint. The race specifically occurs when the unregistration (and
deletion of the endpoint) occurs, after a successful endpointsByNIC
lookup and before the endpoints map is further looked up with ingress
NICID of the packet.
The fix is to notify the caller of lookup-with-NICID failure, so that
the logic falls through to handling unknown destination packets.
For TCP this can mean replying back with RST.
The syscall test in this CL catches this race as the ACK completing the
handshake could get silently dropped on a listener close, causing no
RST sent to the peer and timing out the poll waiting for POLLHUP.
Fixes #5850
PiperOrigin-RevId: 369023779
|
|
Runsc build benchmark's mutex profile shows that we are wasting roughly 25-30
seconds waiting for filesystem.renameMu to get unlocked. Earlier
checkCachingLocked required the renameMu to be locked for writing. This is a
filesystem wide lock which puts all other filesystem operations on hold and
hence is really expensive. Something to note is that all path resolution
operations hold renameMu for reading.
With this change, we allow to check for caching without even holding renameMu.
This change introduces more fine grained locks (fs.cacheMu and dentry.cachingMu)
which protect the cache (removing the requirement to hold renameMu for writing
to modify the cache) and synchronize concurrent dentry caching attempts on a per
dentry basis. We still require to hold renameMu for writing while destroying
dentries and evicting from the cache but this still significantly reduces the
write locking critical section.
Local benchmarking showed that this improved runsc build benchmark time by 4-5%.
Across 6 runs, without this change it took 310.9025 seconds to build runsc
while with this change it took 296.127 seconds.
Runsc build benchmark's mutex profile: https://gvisor.dev/profile/gvisor-buildkite/78a3f968-36ca-4944-93f7-77a8792d56b4/28a1d260-790b-4a9e-94da-a4daede08ee3/tmp/profile/ptrace/BenchmarkBuildRunsc/page_cache.clean/filesystem.bindfs/benchmarks/runsc/mutex.pprof/flamegraph
PiperOrigin-RevId: 368958136
|
|
Add a coverage-report flag that will cause the sandbox to generate a coverage
report (with suffix .cov) in the debug log directory upon exiting. For the
report to be generated, runsc must have been built with the following Bazel
flags: `--collect_code_coverage --instrumentation_filter=...`.
With coverage reports, we should be able to aggregate results across all tests
to surface code coverage statistics for the project as a whole.
The report is simply a text file with each line representing a covered block
as `file:start_line.start_col,end_line.end_col`. Note that this is similar to
the format of coverage reports generated with `go test -coverprofile`,
although we omit the count and number of statements, which are not useful for
us.
Some simple ways of getting coverage reports:
bazel test <some_test> --collect_code_coverage \
--instrumentation_filter=//pkg/...
bazel build //runsc --collect_code_coverage \
--instrumentation_filter=//pkg/...
runsc -coverage-report=dir/ <other_flags> do ...
PiperOrigin-RevId: 368952911
|
|
gohacks.Memmove() takes in the number of bytes to move. The current generated
code passes len(src) and len(dst) as the number of bytes to move.
However, the marshal.Marshallable interface allows passing in larger buffers.
The stated precondition is that the buffer should be "at least" SizeBytes()
in length but it is allowed to be larger.
This change now correctly calls Memmove with the argument for the number of
bytes to move as type.SizeBytes(). This was caught when I made lisafs use the
Unsafe marshalling API and got a lot of memory violations.
PiperOrigin-RevId: 368952642
|
|
Also count failed TCP port allocations
PiperOrigin-RevId: 368939619
|
|
PiperOrigin-RevId: 368938936
|
|
PiperOrigin-RevId: 368919557
|
|
PiperOrigin-RevId: 368919504
|
|
Building nogo targets takes a very long time. This change extracts it into its
own BuildKite job.
This change also additionally speeds up other targets that were using the bazel
flag --test_tag_filters. Without --build_tag_filters, the filter is not
applied while building the specified targets and so we might end up building
targets that are not actually tested.
PiperOrigin-RevId: 368918211
|
|
PiperOrigin-RevId: 368779532
|
|
Fields allow counter metrics to have multiple tabular values.
At most one field is supported at the moment.
PiperOrigin-RevId: 368767040
|
|
PiperOrigin-RevId: 368749894
|
|
Reduce the ephemeral port range, which decreases the calls to makeEP.
PiperOrigin-RevId: 368748379
|
|
Otherwise ConnectedEndpoint.sndbuf will be restored as 0 and writes
to the socket will fail with EAGAIN.
PiperOrigin-RevId: 368746660
|
|
Thanks ianlewis@ for discovering the bug/fix!
PiperOrigin-RevId: 368740744
|
|
This was semi-automated -- there are many addresses that were not replaced.
Future commits should clean those up.
Parse4 and Parse6 were given their own package because //pkg/test can introduce
dependency cycles, as it depends transitively on //pkg/tcpip and some other
netstack packages.
PiperOrigin-RevId: 368726528
|
|
Go 1.17 is adding a new register-based calling convention [1] ("ABIInternal"),
which used is when calling between Go functions. Assembly functions are still
written using the old ABI ("ABI0"). That is, they still accept arguments on the
stack, and pass arguments to other functions on the stack. The call rules look
approximately like this:
1. Direct call from Go function to Go function: compiler emits direct
ABIInternal call.
2. Indirect call from Go function to Go function: compiler emits indirect
ABIInternal call.
3. Direct call from Go function to assembly function: compiler emits direct
ABI0 call.
4. Indirect call from Go function to assembly function: compiler emits indirect
ABIInternal call to ABI conversion wrapper function.
5. Direct or indirect call from assembly function to assembly function:
assembly/linker emits call to original ABI0 function.
6. Direct or indirect call from assembly function to Go function:
assembly/linker emits ABI0 call to ABI conversion wrapper function.
Case 4 is the interesting one here. Since the compiler can't know the ABI of an
indirect call, all indirect calls are made with ABIInternal. In order to
support indirect ABI0 assembly function calls, a wrapper is generated that
translates ABIInternal arguments to ABI0 arguments, calls the target function,
and then converts results back.
When the address of an ABI0 function is taken from Go code, it evaluates to the
address of this wrapper function rather than the target function so that later
indirect calls will work as expected.
This is normally fine, but gVisor does more than just call some of the assembly
functions we take the address of: either noting the start and end address for
future reference from a signal handler (safecopy), or copying the function text
to a new mapping (platforms).
Both of these fail with wrappers enabled (currently, this is Go tip with
GOEXPERIMENT=regabiwrappers) because these operations end up operating on the
wrapper instead of the target function.
We work around this issue by taking advantage of case 5: references to assembly
symbols from other assembly functions resolve directly to the desired target
symbol. Thus, rather than using reflect to get the address of a Go reference to
the functions, we create assembly stubs that return the address of the
function. This approach works just as well on current versions of Go, so the
change can be made immediately and doesn't require any build tags.
[1] https://go.googlesource.com/go/+/refs/heads/master/src/cmd/compile/abi-internal.md
PiperOrigin-RevId: 368505655
|
|
PiperOrigin-RevId: 368495641
|
|
PiperOrigin-RevId: 368470656
|
|
Netstack is supposed to be somewhat independent of the rest of gVisor, and
others should be able to use it without pulling in excessive dependencies.
Currently, there is no way to fight dependency creep besides careful code
review.
This change introduces a test rule `netstack_deps_check` that ensures the target
only relies on gVisor targets and a short allowlist of external dependencies.
Users who add a dependency will see an error and have to manually update the
allowlist.
The set of packages to test comes from //runsc, as it uses packages we would
expect users to commonly rely on. It was generated via:
$ find ./runsc -name BUILD | xargs grep tcpip | awk '{print $2}' | sort | uniq
(Note: We considered giving //pkg/tcpip it's own go.mod, but this breaks go
tooling.)
PiperOrigin-RevId: 368456711
|
|
We do not currently run random save tests.
PiperOrigin-RevId: 368309921
|
|
Fix a race where the ACK completing the handshake can be dropped by
a closing listener without RST to the peer. The listener close would
reset the accepted queue and that causes the connecting endpoint
in SYNRCVD state to drop the ACK thinking the queue if filled up.
PiperOrigin-RevId: 368165509
|
|
It's a common pattern in test code to reinterpret_cast<sockaddr*> from
sockaddr_* structs. Make AsSockAddr() for them so code looks better.
Note: Why not a wrapper type for `sockaddr_storage` and etc?
It's also a common need to have a local in-out variable of socklen_t.
Creating a wrapper type may however lead to this wrong code:
Wrapper addr;
socklen_t addrlen = sizeof(addr);
where sizeof(Wrapper) may not equal to sizeof(sockaddr_storage).
PiperOrigin-RevId: 368126229
|
|
PiperOrigin-RevId: 368121539
|
|
Some FileDescriptions in verity fs were opened but DecRef() were missing
after used. This could result in a ref leak.
PiperOrigin-RevId: 368096759
|
|
Reported-by: syzbot+a6ef0f95a2c9e7da26f3@syzkaller.appspotmail.com
Reported-by: syzbot+2eaf8a9f115edec468fe@syzkaller.appspotmail.com
PiperOrigin-RevId: 368093861
|
|
Use MarshalUnsafe for packed types as it is faster than MarshalBytes.
PiperOrigin-RevId: 368076368
|
|
Holding this lock can cause the user's callback to deadlock if it
attempts to inspect the accept queue.
PiperOrigin-RevId: 368068334
|
|
Fixes #5817
PiperOrigin-RevId: 368060056
|
|
Some other cleanup while I'm here:
- Remove unused arguments
- Handle some unhandled errors
- Remove redundant casts
- Remove redundant parens
- Avoid shadowing `hash` package name
PiperOrigin-RevId: 367816161
|
|
Use a linked list with cached length and capacity. The current channel
is already composed with a mutex and condition variable, and is never
used for its channel-like properties. Channels also require eager
allocation equal to their capacity, which a linked list does not.
PiperOrigin-RevId: 367766626
|
|
The current SNAT implementation has several limitations:
- SNAT source port has to be specified. It is not optional.
- SNAT source port range is not supported.
- SNAT for UDP is a one-way translation. No response packets
are handled (because conntrack doesn't support UDP currently).
- SNAT and REDIRECT can't work on the same connection.
Fixes #5489
PiperOrigin-RevId: 367750325
|
|
If the parent is not enabled in verity stepLocked(), failure to find
the child dentry could just mean an incorrect path.
PiperOrigin-RevId: 367733412
|
|
PiperOrigin-RevId: 367730917
|
|
Move maxListenBacklog check to the caller of endpoint Listen so that it
is applicable to Unix domain sockets as well.
This was changed in cl/366935921.
Reported-by: syzbot+a35ae7cdfdde0c41cf7a@syzkaller.appspotmail.com
PiperOrigin-RevId: 367728052
|
|
To match the V4 variant.
PiperOrigin-RevId: 367691981
|
|
Both code paths perform this check; extract it and remove the comment
that suggests it is unique to one of the paths.
PiperOrigin-RevId: 367666160
|
|
Both callers of this function still drop this error on the floor, but
progress is progress.
Updates #4690.
PiperOrigin-RevId: 367604788
|
|
Set root dentry and root hash in verity fs before we verify the root
directory if a root hash is provided. These are used during
verification.
PiperOrigin-RevId: 367547346
|
