Summary:
`-Wextra-semi` or `-Wextra-semi-stmt`
If the code compiles, this is safe to land.
Reviewed By: palmje
Differential Revision: D51995015
fbshipit-source-id: c92e3c06ae30b98afdcb204c52db8ea3a4e28729
Summary:
Release notes: https://blog.rust-lang.org/2023/10/05/Rust-1.73.0.html
This release is coupled with an update of the `anyhow` and `thiserror` crates because the unstable standard library API for backtraces has changed.
Fbcode changes:
- `feature(default_free_fn)` deleted (D50300881)
- `noop_method_call` lint becomes warn-by-default (D50486032, D50516201)
- stronger `invalid_reference_casting` detection (D50488164)
- `feature(unix_chown)` has been stabilized
- `feature(provide_any)` and `std::provider` deleted
- `clippy::unwrap_or_else_default` renamed to `clippy::unwrap_or_default`
- type inference ambiguities (D51780425)
- `nu-command` build error (D51779062)
Reviewed By: AndreasBackx, shayne-fletcher
Differential Revision: D50294321
fbshipit-source-id: 0fac87f6ba072ad029f9ce41ce94ed813e855b20
Summary:
This removes the last unsafe QueuedImmediateExecutor from the code (excluding
tests).
In the tests, a global CPU executor had to be used instead of a ManualExecutor
due to the FuseChannel code relying on threads to read from the FUSE
filedescriptor, the timing of which would make tests flaky.
Reviewed By: jdelliot
Differential Revision: D51302394
fbshipit-source-id: f520bf4d50ce8228a2470e21b74b4a713ab7e710
Summary: This was followup code review from the initial implementation, to improve readability. There is no behavior change here, just moving/renaming some things.
Reviewed By: kmancini
Differential Revision: D51375399
fbshipit-source-id: ebfde4a3bbb67f1efb421c7e385430626b00d212
Summary:
I'm planning on re-using this thread pool in both PrjFS and FUSE, let's thus
move it to a more generic location.
Reviewed By: genevievehelsel
Differential Revision: D51302395
fbshipit-source-id: 39a503fd31a7b7464fc0a38f8d4152e33808293f
Summary:
The blob loading code used to rely on an unsafe detached future that could lead
to deadlocks. To fix this, the code needs to be reworked quite significantly
to allow the loading future to be chained and added to a proper executor by the
calling context.
Simply chaining the future is unfortunately not sufficient: it prevents the
loading context from being interrupted by a truncation, this can be solved by
using a `folly::collect` with both the promise and the load future. In the case
where the load is interrupted, the promise will be set and the collect will
early complete. Since `folly::collect` is only early completing when one of the
future fails, some boilerplate code had to be added to handle the
`BrokenPromise` case.
However the solution above exposes another very subtle issue: if the returned
future is dropped, the loading will never complete but the inode will stay in
its loading state. Thus future loads will hang waiting for a promise that will
never be set. Solving this can be done by detecting when the loading future is
dropped and simply resetting the loading state.
Reviewed By: jdelliot
Differential Revision: D50415722
fbshipit-source-id: 33833b744ca0bcacfc7bc8c53a463dd99a327889
Summary: Somehow forgot to add the target for this test file...
Reviewed By: jdelliot
Differential Revision: D51375040
fbshipit-source-id: 76d153cdd599218f3e2bb86a650422a767b324e6
Summary:
X-link: https://github.com/facebookincubator/velox/pull/7542
ties all of the pieces together. The bulk of the net-new logic is in `OverlayFile`, with the LMDB stuff being ported from other implementations or just delegating calls to other classes.
Reviewed By: kmancini
Differential Revision: D46914322
fbshipit-source-id: 3434b71c92ece6b94a3c08828df286b04152d50f
Summary:
This is mostly a copy of `SqliteDatabase`, `SqliteConnection`, and `SqliteTreeStore` (with a name change to the more generic `StoreInterface`)
`LMDBStoreInterface` implements two different types of functions, those mapping to `FileContentStore/InodeCatalog`, and those mapping to `OverlayFile`
Reviewed By: kmancini
Differential Revision: D47001908
fbshipit-source-id: 2f1ba34d2900f910e8c6c49a2fc4e298f5a7561d
Summary: On D50793884 there was a mistake where trying to resolve symlink cycles would make EdenFS time out due to `resolveSymlinkPathImpl` being stuck on the cycle. This was caused by not using the `remainingRecursionDepth` parameter.
Reviewed By: xavierd
Differential Revision: D51160001
fbshipit-source-id: 269713ec96db2820a99ededbc0667439ba98b2f1
Summary:
I'm trying to better understand this whole code and will be starting by merging
loadChild and loadChildLocked, who seemingly do the exact same thing but
slightly differently. First step will be to use ImmediateFuture since most of
the callers already were converting the returned Future to an ImmediateFuture.
Reviewed By: kmancini
Differential Revision: D50902991
fbshipit-source-id: e88cfbe25341b4b1320945e464870aaef2132bff
Summary:
These were the last ones and could be converted to an ImmediateFuture without
too much trouble.
Reviewed By: kmancini
Differential Revision: D50720494
fbshipit-source-id: eb97946ad07aaa7a3b90f5b6bf0b7f6ea6122f7b
Summary:
There was an issue on EdenFS when trying to determine the type of a symlink: if the target path contained symlinks in its path EdenFS incorrectly would try to internally read the unresolved path, which it was incapable of.
For instance, if the directory structure looked somewhat like this:
```
├── a -> b/y
├── b -> x
└── x
└── y
└── z
```
trying to show the contents of `a` would fail as EdenFS would mark that symlink as a file instead of a directory symlink.
That is fixed by this diff by adding a method for resolving the target before internally trying to read the target of a symlink.
Reviewed By: xavierd
Differential Revision: D50793884
fbshipit-source-id: 920a0b4ee6f4f1b9a9fd1143608f2d42b5ca2299
Summary:
makeFutureWith is strictly equivalent to a try/catch but requires less code to
handle exceptions, use it.
Reviewed By: genevievehelsel
Differential Revision: D50902990
fbshipit-source-id: 8d887e2c0efd97244763b27c2b3ec2b20145e97e
Summary: Adds some helper functions that will be used for resolving symlinks on D50793884
Reviewed By: xavierd
Differential Revision: D50888103
fbshipit-source-id: dadf02e4635cc86ef9470c4f925386711d42d0e6
Summary:
Eden builds allow for shadow declarations to hide members, parameters, etc.
This can lead to subtle bugs and frustrations. Lets fix that. This first update enables shadow warngings for most problematic types of shadows. With errors on by default, this will block local and CI builds.
Reviewed By: xavierd
Differential Revision: D50434604
fbshipit-source-id: 976bd2e86c620f1f0e62e19867c81840fee645c9
Summary:
Moves populateOnDiskChildrenState out of the Windows FSCK implementation and
into a separate library target for re-use in the PrjFS background scrubber.
In doing so, this renames newly exported interfaces. We also add a paramter to
optionally use FIND_FIRST_EX_ON_DISK_ENTRIES_ONLY to be usable while the
virtualization provider is running.
Reviewed By: genevievehelsel
Differential Revision: D50670155
fbshipit-source-id: b8d339da15fe7f3a741168357c7481258c261b66
Summary: There was a bug on Windows symlinks where EdenFS would error out if the target symlink would not exist and the target path was inside of the EdenFS mount. The cause was not handling the errors from `getTreeOrTreeEntry`
Reviewed By: genevievehelsel
Differential Revision: D50610932
fbshipit-source-id: 17333a6afe7ab577d21bc9d59883c8be7b412571
Summary:
This removes the remaining QueuedImmediateExecutor from checkout, most of the
computation will now run on the Thrift executor which isn't an inline executor
and thus doesn't suffer from deadlocks.
Most of this diff are in tests are these need to be taught to use the
ImmediateFuture instead of the folly::Future.
Reviewed By: genevievehelsel
Differential Revision: D50347460
fbshipit-source-id: 79fcfd2494371c1a9422692862356162b64e11a1
Summary:
The checkout code is a heavy user of `QueuedImmediateExecutor` due to relying
on `folly::Future`, the immediate executor is used to convert `ImmediateFuture`
onto `folly::Future`. Unfortunately, this transformation is unsafe and leads to
deadlocks.
The full migration of checkout to `ImmediateFuture` is done in the next diff,
and as a result still relies on `QueuedImmediateExecutor` in `EdenMount`.
As part of this diff, the CheckoutAction is greatly simplified, from relying on
refcounts and `Promise`, the code now merely makes use of future chaining, thus
making it easier to read.
Reviewed By: genevievehelsel
Differential Revision: D50347461
fbshipit-source-id: 8159f523f6fbe4414b99ecb924ba0446f7edeb6e
Summary:
In D50227150, the QueuedImmediateExecutor was removed from the
VirtualInodeLoader to eliminate a potential deadlock, this however force the
caller to pass down an executor. Instead, a slightly different approach can be
taken: let's simply chain the futures and return an
`ImmediateFuture<vector<T>>` instead of returning a `vector<SemiFuture<T>>`.
The outcome will be similar: the futures will run on the Thrift executor, but
is now done implicitely in this change.
Reviewed By: genevievehelsel
Differential Revision: D50347463
fbshipit-source-id: cf7023e9eee53e14955b852b44c1902b348dab61
Summary: Various Thrift calls need the path of the VirtualInode, let's pass it down.
Reviewed By: genevievehelsel
Differential Revision: D50281108
fbshipit-source-id: 49744227b223492ce0354f43cabe1508a33341ca
Summary:
This is just some general cleanups as the QueuedImmediateExecutor users are
dwindling down.
Reviewed By: genevievehelsel
Differential Revision: D50347465
fbshipit-source-id: 30891010664cd20234427c264f687007647fd257
Summary:
Using QueuedImmediateExecutor can lead to deadlocks as seen in D50199539, let's
instead make use of an actual executor that the caller passes. When called in
the context of a Thrift call, the Thrift executor is passed in as the rest of
the Thrift handling is already done in that executor.
Reviewed By: genevievehelsel
Differential Revision: D50227150
fbshipit-source-id: 042617afa1e05fe602807caab7b6419e18ba6848
Summary:
When enabled, the configuration added in D47099273 causes error conflicts
reported by EdenFS during update to leave the repository in an "unfinished
update" state. The original intent of this configuration was to deal with
network errors, but this behavior also applies to errors caused by EdenFS being
unable to remove a locked placeholder file.
This adds integration tests to ensure that a successfully retried update can
bring the repository to the desired end state. These tests reveal that that
resuming update to deal with a failed placeholder invalidation doesn't yet
work, so some assertions are currently commented out with a TODO.
Reviewed By: genevievehelsel
Differential Revision: D50250253
fbshipit-source-id: b88e47d91819167f5f079c676b4026f3e02f7606
Summary: Before all the EdenFS symlink changes on Windows, replacing a "fake" symlink with an actual symlink would make that file show up as modified. This diff reverts that behavior.
Reviewed By: kmancini
Differential Revision: D49755711
fbshipit-source-id: 03ea79e9af59264a2c1303240494a862d140f86f
The internal and external repositories are out of sync. This Pull Request attempts to brings them back in sync by patching the GitHub repository. Please carefully review this patch. You must disable ShipIt for your project in order to merge this pull request. DO NOT IMPORT this pull request. Instead, merge it directly on GitHub using the MERGE BUTTON. Re-enable ShipIt after merging.
Summary: As title, this diff fully separate GlobNode and GlobTree, GlobTree only does Glob on tree and do not depend on inode. GlobNode and GlobTree both use shared utilities to minimize code duplication
Reviewed By: kmancini
Differential Revision: D49998533
fbshipit-source-id: 6323cacd58aa68678ee348620956a967e719b7aa
Summary: This unifies the naming convention between `FileContentStore` and `InodeCatalog`
Reviewed By: kmancini
Differential Revision: D48584876
fbshipit-source-id: 316f1d1e49228036338a0ae4371b7d129357f884
Summary: `FileContentStore` is too generic of a name with the introduction of another implementation (`LMDBFileContentStore`), so rename it to `FsFileContentStore`
Reviewed By: kmancini
Differential Revision: D48581932
fbshipit-source-id: e3bc57733399c095f62a38839305f2d37c3e8a47
Summary: In preparation for `LMDBFileContentStore`, this diff extends the `IFileContentStore` class to hold either a `folly::File` or an `InodeNumber`. This logic follows from D48569910
Reviewed By: kmancini
Differential Revision: D48570175
fbshipit-source-id: 9b046aff027896a77729bc84f4c96c683b7d5107
Summary:
In preparation for `LMDBFileContentStore`, this diff extends the OverlayFile class to hold either a `folly::File` or an `InodeNumber`.
`FsFileContentStore` returns a `folly::File` which wraps an open `fd` to the raw file on disk. This allows for direct manipulation of the data without a trip through the `FsFileContentStore`. It also acts as a way to refcount any open IO requests to the underlying overlay storage (see: D17973664)
`LMDBFileContentStore` doesn't have the ability to return a file descriptor for direct manipulation, so instead it will return its key and operations to the `OverlayFile` will query `LMDBFileContentStore` on demand using the given `InodeNumber`.
In theory, `LMDBFileContentStore` doesn't need the concept of an `OverlayFile` since it doesn't need to refcount open file handles, but adapting the class to work with the `LMDBFileContentStore` model is cleaner than working around `OverlayFile`/`OverlayFileAccess` class.
The following images depict the two different architectures
{F1075774500}
{F1075774523}
Reviewed By: kmancini
Differential Revision: D48569910
fbshipit-source-id: 2f171ede0c4ac168ca01bb1c65dc9d401edc4681
Summary:
See why on previous diff
Now with this diff GlobTree is independent of inode
Reviewed By: kmancini
Differential Revision: D49933175
fbshipit-source-id: 1551a2b7e054df5df88ac37fbf0bf45f91e34548
Summary:
Split the code dealing with checkout entries absent from TreeInode state into a
separate function to make processCheckoutEntry slightly more comprehensible.
Reviewed By: xavierd
Differential Revision: D49961307
fbshipit-source-id: 8744f9f595f39402689773e85d285c9d628f6a9f
Summary:
anyhow-1.0.73 [uses][1] the new `Error::provide` API. This API is
available starting in Rust 1.73. This means that if you want backtraces,
you need one:
- anyhow-1.0.72 & Rust 1.72
- anyhow-1.0.73 & Rust 1.73
Since we're still on 1.72, pin the version to avoid accidentally losing
backtraces.
There are no real changes between anyhow-1.0.71 and anyhow-1.0.72. But
update to anyhow-1.0.72 so that we're right up until the point where we
get backtrace support.
[1]: https://github.com/dtolnay/anyhow/pull/319
Reviewed By: shayne-fletcher
Differential Revision: D49970958
fbshipit-source-id: 1193611e6d16bc840e63b689e932ea3d33562152
Summary:
This is a stack to refactor GlobNode out of inode directory as much as possible.
I'm working on fine grained watchman subscription and i need to use GlobNode for globbing, which will have fs files depend on inode files. According to Katie, we want inode to be the top level.
Since the api im using won't require inode, the plan is to move the part that doesn't require inode into utils and have the inode glob search remain in inode
To split inode and tree implementation in this file, we need to make `evaluateImpl` and `evaluateRecursiveComponentImpl` stop referring to inodes, which is very hard without rewriting the methods because both methods do double recursion inside.
This code is hacky as it reuses runtime error for lines that we know will never reach during compile time. But I think this file is already using this hacky way multiple times https://www.internalfb.com/code/fbsource/[6cf48e74661705c6aefc18ebaecd7e229d71cb99]/fbcode/eden/fs/inodes/GlobNode.cpp?lines=122
I think with modern c++ we can probably get away this exception with constraints. But I'm not confident to do big refactor on this file for now
Reviewed By: kmancini
Differential Revision: D49900357
fbshipit-source-id: a769137238da78aba1da2720f9f1fc07f7e3ad6e
Summary: In order to pass the `requestInfo` map to deeper layers, thread the Thrift layer `ObjectFetchContext` to `DiffContext` and `StatsFetchContext`. This is not consumed anywhere yet, but will passed to the `sapling_backing_store` in follow up diffs. This is part of a broader effort to thread a common client identifier from scm clients (`sapling` and `edenfs` to Mononoke)
Reviewed By: jdelliot
Differential Revision: D49716042
fbshipit-source-id: b5df9754058e1f1585954ff6c25bbafa9aa820c1
Summary: In order to pass the `requestInfo` map to deeper layers, thread the Thrift layer `ObjectFetchContext` to `CheckoutContext`. This is not consumed anywhere yet, but will passed to the `sapling_backing_store` in follow up diffs. This is part of a broader effort to thread a common client identifier from scm clients (`sapling` and `edenfs` to Mononoke)
Reviewed By: jdelliot
Differential Revision: D49716030
fbshipit-source-id: 7b079e554711ec610dd919243c7e459b2737fa48
Summary: Some users reported seeing files that were supposed to be symlinks when having symlinks disabled. This was caused by the initial mode for those "symlinks" being symlinks; with symlinks disabled this symlink mode was supposed to be ignored and the file should have been treated as a regular file.
Reviewed By: kmancini
Differential Revision: D49610768
fbshipit-source-id: 73b78cc1b5f278ac2dd871bf9454fec63c723979
Summary:
Currently EdenFS is swallowing errors opening the local store. None of the
mounts will be mounted, but EdenFS will report that start was successful.
Previously, errors opening the local store would cause startup to fail.
D38190050 accidentally caused this behavior.
We want to go back to EdenFS more explictly failing to start because this is
clearer to users, and EdenFS will not be able to work with out it's local store.
I discovered a potential cause of graceful restart leaving behind stale mounts
while putting the start failure behavior back:
If EdenFS fails in startup sometime between receiveing takeover data and
creating setting up a mount object, that mount will be left stale. We probably
don't want that, but fixing that is more complicated, and it's also very old
behavior (happened before D38190050), so I'm going to delay the fix on that.
Reviewed By: MichaelCuevas
Differential Revision: D48813074
fbshipit-source-id: 3c129c48b423afb2b0224f1eec1031c26c8968c6
Summary: Before this change there was a bug where trying to read symlinks with POSIX-style paths (e.g., `/foo/bar`) would fail. This applied both to reading symlinks directory or trying to list the contents of their parent directories.
Reviewed By: genevievehelsel
Differential Revision: D49601495
fbshipit-source-id: 403d06c580d676d61fb6bd8233e5ac45b072c2df
Summary:
We would like to know how effective our various caching entities are. Here we are adding counters in Overlay to track hits and misses when loading an overlay directory.
File operations in the Overaly are only used when the overlay checker is being ran - as such not adding counters for those.
Reviewed By: MichaelCuevas
Differential Revision: D49552392
fbshipit-source-id: 1d2e699b4b9c5a1bad3d47ef1e7945a5f7f37999
Summary: We would like to know how effective our various caching entities are. Here we are adding counters in EdenStats to track lookup hits and misses in InodeMetadataTable.
Reviewed By: MichaelCuevas
Differential Revision: D49548923
fbshipit-source-id: bd48b71213ecb28d6f0316a7d69955b80aefdb37
Summary: To aid in build times, switched to using forward declarations to remove an depedenciny on EdenStats.h in IndoeMap.h.
Reviewed By: chadaustin
Differential Revision: D49546029
fbshipit-source-id: 1306d28465527e7d6109d413f7e9f4ecb2e7e74e
