Summary: Store tree and file timestamps in the InodeTable so they persist across runs.
Reviewed By: simpkins
Differential Revision: D6891479
fbshipit-source-id: 1c9e6266375aceeaf293a81e73cf7f5334dbc32d
Summary:
This is not at all clear from cppreference.com, but per
https://www.youtube.com/watch?v=dTeKf5Oek2c, it sounds to me like
recommended practice is to either:
`using namespace std::chrono_literals` (or string_literals or
whatever) to pull in a focused set of literals.
or
`using namespace std::literals` to pull in all standard literals.
or
`using namespace std` to pull in everything.
`using namespace std::literals::chrono_literals` is unnecessarily
verbose.
Adopt those standards in Eden.
Reviewed By: simpkins
Differential Revision: D8060944
fbshipit-source-id: 4d9dd4329698b7ff5e5c81b5b28780ca4d81a2a1
Summary:
This allows multiple inodes to update their metadata simultaneously
without contending on InodeTable's locks.
Reviewed By: simpkins
Differential Revision: D7885245
fbshipit-source-id: cc8ab6cd90b7424beec314a115852e08b64026ae
Summary:
Update the folly::Init code to define a `--logging` command line flag, and call
`folly::initLoggingOrDie()` with the value of this command line during
initialization.
This is similar to the existing code that initializes the glog library.
(Programs can use both glog and folly logging together in the same program, and
I expect that many programs will do so as parts get converted to folly::logging
and parts remain using glog.)
Reviewed By: yfeldblum
Differential Revision: D7827344
fbshipit-source-id: 8aa239fbad43bc0b551cbe40cad7b92fa97fcdde
Summary:
While running the secfs filesystem validation tests against Eden, I
discovered a test that caused the eden process to abort. I bisected
and found that D7451330 regressed renaming a directory onto an empty
one. This fixes that case.
Reviewed By: simpkins
Differential Revision: D7945727
fbshipit-source-id: 592ede1b391528c02cd12b2b6ebbf3733fe8f503
Summary:
I got tired of typing PathComponentPiece{"..."} in tests so here are
some operator literals.
Reviewed By: simpkins
Differential Revision: D7956732
fbshipit-source-id: 85d9f3fd725853a54da9e70fc659bd7eb9e0862c
Summary: This old Overlay code is no longer necessary.
Reviewed By: simpkins
Differential Revision: D7903912
fbshipit-source-id: 4a39d6ce7d1f6f81eb13715f2d5d17b22c10d413
Summary:
A persistent (but notably non-durable) mapping from inode
number to a fixed-size record stored in a memory-mapped file. The two
primary goals here are:
1. efficiently (and lazily) reify timestamps for inodes that aren't in the overlay
2. allow the kernel's page cache to drop pages under memory pressure
Reviewed By: simpkins
Differential Revision: D6877361
fbshipit-source-id: a4366b12e21e2bf483c83069cd93ef150829b2ac
Summary:
Make it clear (especially for the upcoming InodeMetadata struct) which
operations with EdenTimestamp and InodeTimestamps will never throw.
Reviewed By: simpkins
Differential Revision: D7920219
fbshipit-source-id: 5917da51b8128455893a1480def6f2c1c8de13d4
Summary:
Per yfeldblum's comment in D7886046, we can use folly::unit instead
of folly::Unit{}. We weren't using folly::unit anywhere, so this diff
replaces folly::Unit{} elsewhere in the Eden code.
Reviewed By: yfeldblum
Differential Revision: D7913462
fbshipit-source-id: fa6ab44ceb406d38713e0f4649224a74e6e51abd
Summary:
Historically, we have seen a number of messages like the following in the Eden
logs:
```
Journal for .hg/blackbox.log holds invalid Created, Created sequence
```
Apparently we were getting these invalid sequences because we were not always
recording a "rename" correctly. The "rename" constructor for a `JournalDelta`
assumed that the source path should be included in the list of "removed" files
while the destination path should be included in the list of "created" files.
However, that is not accurate if the destination path already existed before
the user ran `mv`.
Fortunately, we already check whether the destination file exists in
`TreeInode::doRename()`, so it is straightforward to determine whether the
action is a "rename" (destination does not exist) or an "replace" (destination
already exists) and then classify the destination path accordingly.
As demonstrated by the new test introduced in this commit
(`JournalUpdateTest::moveFileReplace`), in the old implementation,
a file that was removed after it was overwritten would not show up as
removed in the merged `JournalDelta`. Because Watchman relies on
`JournalDelta::merge()` via the Thrift method `getFilesChangedSince()`,
this would cause Watchman to report such a file as still existing even
though it was removed.
This definitely caused bugs in Nuclide. It is likely that other tools that rely
on Watchman in Eden (such as Buck) may have also done incorrect things
because of this bug, so this could explain past reported issues.
Reviewed By: simpkins
Differential Revision: D7888249
fbshipit-source-id: 3e57963f27c5421a6175d1a759db8d9597ed76f3
Summary:
When comparing two source control blob hashes, identical hashes can be assumed
to mean that the file contents are equal. However, differing hashes does not
necessarily mean that the file contents differ. In particular, mercurial
hashes history metadata in addition to the file contents when computing the
blob hash.
This updates Eden to always compare the file contents when the source control
blob hashes differ, rather than assuming that the file contents are different.
Reviewed By: wez
Differential Revision: D7825900
fbshipit-source-id: e611124a66cdd5c44589f20d1d4665a603286530
Summary:
Promote the folly logging code out of the experimental subdirectory.
We have been using this for several months in a few projects and are pretty
happy with it so far.
After moving it out of the experimental/ subdirectory I plan to update
folly::Init() to automatically support configuring it via a `--logging` command
line flag (similar to the initialization it already does today for glog).
Reviewed By: yfeldblum, chadaustin
Differential Revision: D7755455
fbshipit-source-id: 052db34c97f7516728f7cbb1a5ad959def2f6efb
Summary:
To avoid bugs similar to the ones fixed in D7781691, mark a bunch of
folly::Future<folly::Unit> functions with FOLLY_NODISCARD.
Reviewed By: simpkins
Differential Revision: D7782224
fbshipit-source-id: 23ba42aa63011cc33e5a6e18d5bc6d00403a78d3
Summary:
Disallow any kind of mutation operation inside of the .eden directory. We had some
code in place to prevent some of this already, but errors (including EPERM) weren't
passed out from unlink and rename out to FUSE.
Reviewed By: simpkins
Differential Revision: D7781691
fbshipit-source-id: aaecf13779eca75d6ee8765fc8bb3727ce9341de
Summary: The InodeTable work will homogenize mode_t storage, access, and modification across file and tree inodes. In preparation, have InodeBase keep track of the initial mode bits instead of a dtype_t.
Reviewed By: simpkins
Differential Revision: D7031924
fbshipit-source-id: f2e6e4467cecfc0ca06ad998cce0af18a99cc251
Summary: Also print the inode path if the assertions in EXPECT_FILE_INODE fail.
Reviewed By: simpkins
Differential Revision: D7035517
fbshipit-source-id: 6c50acb588d1c985c7e7a1586c06f04a657698f9
Summary:
Now that Eden writes tree data into the overlay even when not
materialized, checkouts can be slow as Eden actively tries to forget
allocated inode data for potentially large trees when
kPreciseInodeMemory is false. This pushes the bulk of that garbage
collection work onto a background thread, making checkouts fast again.
Reviewed By: simpkins
Differential Revision: D7683120
fbshipit-source-id: 2a22e4612a4438b3ed2527670343c49dfcd902bc
Summary: This gets rid of those pesky #ifdefs in FileInode.cpp and TreeInode.cpp.
Reviewed By: wez
Differential Revision: D7735914
fbshipit-source-id: d810461984e21f72670f43ca2d1b4f5aacbf376e
Summary:
This updates the code to store ServerState using a shared_ptr rather than
having it be an inlined member variable of EdenServer.
Previously EdenMount objects contained a raw pointer to the ServerState, with
the reasoning that the EdenServer object should outlive the EdenMount objects.
It turns out that this is not quite true in practice--EdenMount::destroy() will
normally be called before the EdenServer is destroyed, but this may not
actually destroy the EdenMount object immediately.
This fixes a race condition in the FuseTest.initMount() test that could cause
this test to occasionally fail when run on a heavily loaded system.
Reviewed By: chadaustin
Differential Revision: D7720509
fbshipit-source-id: 056ff5985587c8d8c32c11d17ba637ebd7598677
Summary:
I was worried we might leak data in the overlay in this particular
sequence of operations, so here's a test that shows we don't.
Reviewed By: simpkins
Differential Revision: D7663419
fbshipit-source-id: b4b0d08f863fe7b23501e9e4943d18ba0b746e42
Summary:
When we start storing permission changes and other inode
metadata keyed on inode number, we will need to remember inode numbers
before they're accessed to handle the case where the Eden process dies
before the inode is unloaded. Otherwise, data could be left in an
inconsistent state and some writes could be missed.
This change depends on saving (and forgetting) directories being
cheap.
Reviewed By: simpkins
Differential Revision: D7598094
fbshipit-source-id: 9ab127b30a9c09ab62aa08b50c13b3eaa40be60d
Summary:
Split the modifyConflict() test into 3 separate test functions. This test
runs the same checks in 54 separate variations (3 path names, 3 checkout types,
with 6 inode load options).
For each variation we set up a new test mount, creating an overlay and
LocalStore on disk. Simply setting up and tearing down the test mounts can be
expensive if the temporary directory is served from spinning disk. This causes
this test to occasionally time out when run on our continuous build hosts.
Splitting the test up into 3 separate test functions will effectively give it
3x longer to run, since each test function will have a separate timeout. This
will also enable these test functions to run in parallel (although that may not
actually help with performance if they are bottlenecked on disk I/O).
Reviewed By: chadaustin
Differential Revision: D7665409
fbshipit-source-id: 4bddd68e75f38b1b6cc2d57512a5b52855f3bade
Summary:
fdatasync() has a substantial cost and is unnecessary given
our durability guarantees (and the fact that most filesystems on Linux
try to avoid data loss in this common write + rename situation anyway)
Reviewed By: simpkins
Differential Revision: D7641131
fbshipit-source-id: d041e7090dc05a4d4400f86cad9501aa8a6988a9
Summary:
Drop the log level of the "no dir data for inode" message from DBG2 to DBG3 so
we do not have it enabled by default. (Our default log settings are
eden=DBG2.)
Reviewed By: chadaustin
Differential Revision: D7620918
fbshipit-source-id: 25cf30af08054f3ba879bd57f3841b1035e0c75e
Summary:
I'm about to change the code paths through which directories
are saved to and loaded from persistent storage. To help, consolidate
those operations.
Reviewed By: simpkins
Differential Revision: D7597978
fbshipit-source-id: 9cd20027746b0a372a97b7f5daf07c1b54e6b9ce
Summary:
Add a document that begins to sketch out the various transitions that
our inode data structures make so we can reason about their
correctness
Reviewed By: simpkins
Differential Revision: D7434693
fbshipit-source-id: 5478c108c338ccdadc22e864b077195c8be7d1b7
Summary:
Once a mount point has been unmounted we no longer need to care about
outstanding FUSE reference counts--we can treat them as if they are all zero.
This updates EdenMount to tell the InodeMap when the mount point is unloaded,
and changes InodeMap::unloadInode() to make use of this information when
deciding if it needs to remember the inode information.
Previously InodeMap would save information for inodes with outstanding FUSE
reference counts. Writing all of this state to the overlay could take a
non-trivial amount of time.
Reviewed By: chadaustin
Differential Revision: D7555998
fbshipit-source-id: 0896f867ce850ab3e61c262776d536de003685ff
Summary:
Remove the EDEN_HAS_COMMON_STATS checks now that the common/stats stubs have
the required APIs needed by Eden.
Reviewed By: wez
Differential Revision: D7479593
fbshipit-source-id: cc3db50288bfea7aefd6c91391ab800628b7978f
Summary:
Rename `getRefcount()` to `debugGetFuseRefcount()` to make it clear that this
returns the FUSE reference count rather than the pointer refcount, and that
the result should not be used for anything other than debugging or diagnostic
purposes.
There is already a `getFuseRefcount()` method to check the FUSE refcount while
the inode is being unloaded. The only time it is safe to check the FUSE
refcount for programmatic purposes, and `getFuseRefcount()` has a `DCHECK()` to
help ensure that this is the only time it is called.
(`getFuseRefcount()` also predates `getRefcount()`, which was added for the
`eden debug inode` command in D5277870.)
Reviewed By: chadaustin
Differential Revision: D7555997
fbshipit-source-id: e66f2b3d2b9eb14f8f8878d78978851929198a31
Summary:
Change EdenMount to destroy the Overlay object as soon as the `shutdown()`
operation completes. Previously the Overlay would not get destroyed until
EdenMount::destroy() was called.
During graceful restart we transfer mount information to the new process after
`shutdown()` completes, but potentially before `destroy()` has finished. This
previously resulted in a race condition where the new process could start
opening the overlay before the old process had released the lock. This change
should fix that race condition.
Reviewed By: chadaustin
Differential Revision: D7543295
fbshipit-source-id: 82ae33fe6bb0aa9f0a3b010fdd1d350c63442420
Summary: Each Overlay object should be owned only by its EdenMount.
Reviewed By: chadaustin
Differential Revision: D7543294
fbshipit-source-id: 6db40fea31ce298c61d047cba9165887e32926a1
Summary:
Change getScmStatus() so that callers must explicitly specify the commit to
diff against. This should help avoid race conditions around commit or checkout
operations where the parent commit has just changed and eden returns status
information against a commit that wasn't what the client was expecting.
This should still maintain backwards compatibility with older clients that do
not send this parameter yet: we will simply receive the hash as an empty string
in this case, and we still provide the old behavior in this case.
Reviewed By: wez
Differential Revision: D7512338
fbshipit-source-id: 1fb4645dda13b9108c66c2daaa802ea3445ac5f2
Summary:
Now that D7451330 writes saved inode numbers into the overlay, we
need to make sure they get deleted when checking out between trees.
Reviewed By: simpkins
Differential Revision: D7327942
fbshipit-source-id: 9593c7abe9d2e424b9ca1d9c5a5ab8b285867e6e
Summary:
Always remember the inode numbers of children when unloading
a tree. This fixes a bug where, if a tree is unloaded during a build,
child inode numbers would be regenerated, causing the same header to
be compiled once. gcc and clang use inode numbers to decide whether
to #include a file that uses #pragma once, so they must remain
consistent while a mount is up.
This is perhaps a bit controversial because it saves _every_ tree,
unconditionally, to the overlay. I will either stack another diff
that ensures these overlay trees are deleted when checking out to a
new tree or make that change in place here, depending on how urgently
this fix is needed.
Reviewed By: simpkins
Differential Revision: D7451330
fbshipit-source-id: 1c3bc0d55327924c88d1a559d2557cfc158991f2
Summary: Add test demonstrating inode numbers are not consistent across takeover.
Reviewed By: simpkins
Differential Revision: D7300867
fbshipit-source-id: 1e142c28ac00b569b96b5a1867bc8632771c2be9
Summary:
This cleans up the two createOverlayFile() implementations as well as
saveOverlayDir() to all use the same logic for writing out overlay files. This
new logic is more efficient than some of the old code, as we only open the file
once.
Additionally, this also changes the Overlay code to use openat() and related
APIs when accessing files in the overlay, so that we can specify paths relative
the top-level overlay directory. This means that all pathnames are guaranteed
to fit in a small path length known at compile time, and we can avoid ever
allocating path names on the heap.
One potential downside of using openat() is that this functionality may not be
available on Mac OS X, so we will likely still need to provide alternate
implementations that do use heap-allocated absolute paths for Mac support.
Reviewed By: chadaustin
Differential Revision: D7411499
fbshipit-source-id: dd76395130dda4c2b9403cce04f4201f6def0d10
Summary: Add a test case to confirm the behavior of Overlay::getFilePath().
Reviewed By: chadaustin
Differential Revision: D7411498
fbshipit-source-id: f859f29e252c1c296eb236113ad2a8b0a892db37
Summary:
Most of the logic in TreeInode::create(), symlink() and mknod() was very
similar. This adds a TreeInode::createImpl() helper method that these three
functions can share.
This also updates TreeInode::create() to throw EEXIST if the file in question
does exist, rather than using EDEN_BUG(). I believe it is possible (but
unlikely) for this to occur in practice since inode invalidation operations
triggered by a checkout are now processed asynchronously.
Reviewed By: chadaustin
Differential Revision: D7411500
fbshipit-source-id: 85d97995139eee6bff96381561fc28e76d7a2b7c
Summary:
Update all Overlay APIs to accept InodeNumbers and compute the path to the
overlay file internally, rather than requiring the caller to pass in the path
to the file inside the overlay.
Reviewed By: chadaustin
Differential Revision: D7411497
fbshipit-source-id: d057babb0b1b8d6c3b9adf1a21b84eed363dffa1
Summary: After the changes in D7052470 and D7346263, this bit is no longer necessary.
Reviewed By: simpkins
Differential Revision: D7401540
fbshipit-source-id: 04f3ae0376625d4e9584f31932f46774eb5caba4
Summary:
This adds a new LockedState helper class in FileInode to also keep track of
whether or not we are currently holding an refcount on the open file/blob. The
LockedState object automatically decrements this refcount on destruction.
Alternatively, this refcount can be transferred to a new EdenFileHandle object
when unlocking the state.
Previously most of the internal FileInode code simply used EdenFileHandle
objects to manage this refcount. However, this is prone to deadlock, since you
have to ensure that EdenFileHandle objects are never destroyed while the state
is already locked. This new LockedState API makes it harder to have code paths
that may accidentally destroy an EdenFileHandle object while holding the state
lock.
Reviewed By: chadaustin
Differential Revision: D7407423
fbshipit-source-id: 610fcda3220a9f49b734910b7a13e8d68a81a779
Summary:
Update InodeMap::onInodeUnreferenced() to take a pointer to a non-const
InodeBase. This allows the methods it calls, including
InodeBase::updateOverlayHeader() to be non-const.
Using non-const InodeBase objects seems to make sense here since the inode is
in the process of being destroyed.
In a future diff I plan to update FileInode::updateOverlayHeader() to share the
same code as normal file methods to ensure that the overlay file is open. This
modifies the FileInode state's open refcount, so it is useful to have this
method be non-const for that purpose.
Reviewed By: chadaustin
Differential Revision: D7407424
fbshipit-source-id: 541656c7b9b283c5e5650445de5bbdbaae3fc57f
Summary:
Update FuseChannel to send all invalidation requests in a separate thread.
This eliminates a deadlock that could previously occur during checkout
operations. The invalidation requests would block until they could acquire the
kernel's inode lock on the inode in question. However, the inode lock may
already be held by another thread attempting to perform an unlink() or rename()
call. These FUSE unlink or rename operations would be blocked waiting on
Eden's mount point rename lock, which was acquired by the checkout operation.
Checkout operations now let the invalidations complete asynchronously, but we
wait for all invalidation operations to complete before indicating to our
caller that the checkout has succeeded.
Reviewed By: chadaustin, wez
Differential Revision: D7404971
fbshipit-source-id: 6fa20c00d054e210eb0258d247d083010557f210
Summary:
Decouple inode number assignment from materialization status.
The idea is that we will always assign entries an inode number and
track whether an entry is materialized otherwise. This is necessary
to give consistent inode values across remounts.
Reviewed By: simpkins
Differential Revision: D7052470
fbshipit-source-id: 80d3f2a2938463198a3132182537e6223c79d509
Summary:
Verify Eden handles looking up an inode by number after graceful
restart and checkout.
Reviewed By: simpkins
Differential Revision: D7346263
fbshipit-source-id: 876b4837708da9ac31f72c06e7defc797fe126f3
Summary: Small refactoring prior to a fix in a later diff.
Reviewed By: simpkins
Differential Revision: D7358218
fbshipit-source-id: 198160df1aa0dc65b917c5a96c2998c3675b03fe
Summary: Now we can write unit tests for takeover. :)
Reviewed By: simpkins
Differential Revision: D7345419
fbshipit-source-id: 05741dab65016c59109bf190c83be2f676a6141d
Summary:
This replaces the `ensureDataLoaded()`, `materializeForWrite()`, and
`materializeAndTruncate()` methods in `FileInode` with new safer alternatives:
`runWhileDataLoaded()`, `runWhileMaterialized()`, and `truncateAndRun()`
These new methods take a function to run, and run it while holding the state
lock with the FileInode guaranteed to be in the desired state. They also
require the caller move in the state lock as an argument, to help ensure that
the caller gets the locking behavior correct.
The old methods required that the caller not already hold the state lock while
calling them. This pre-condition was violated in `FileInode::write()` and
possibly other places. They also required releasing the lock between they
confirmed the file state and when the caller's function ran, requiring
additional lock and unlock operations, and also making the code harder to
reason about since other threads could change the state during the time when
the lock was released.
Reviewed By: chadaustin
Differential Revision: D7363180
fbshipit-source-id: d8e667d0bc7006c519252a8d0682af97517997eb
Summary:
The changes in this diff comments out unused parameters. All changes are automated using clang-tidy.
This will allow us to enable `-Wunused-parameter` as error.
Reviewed By: simpkins
Differential Revision: D7371610
fbshipit-source-id: 0134e2f0b916313d690c073a46d747c52399a226
Summary:
FileInode::read() did not handle being called in the BLOB_LOADING or NOT_LOADED
states. This method is only called from EdenFileHandle::read(), which does no
checking that it is fully loaded first.
This fixes read() to always call ensureDataLoaded() first.
Reviewed By: chadaustin
Differential Revision: D7359878
fbshipit-source-id: f5a1c8a28db3267da3180b67f970430e3ea291da
Summary:
While debugging a takeover crash (task #25590050) we wanted to help confirm
that inode objects weren't ever getting copied somehow. That turned out not to
be the issue, but it still seems worth explicitly deleting the copy and move
constructors and assignment operators for InodeBase.
Reviewed By: wez
Differential Revision: D7342050
fbshipit-source-id: ae07214072bfb8353584a07066aae2dc1adf2797
Summary:
Update the getScmStatusBetweenRevisions() thrift call to use the new
diffCommits() function that diffs source control Tree objects without creating
TreeInode objects to perform the diff.
This addresses two bugs:
- Each call to EdenMount::diffRevisions() constructed a new root inode
associated with the mount, and this would never get destroyed. It was not
destroyed at the end of the diffRevisions() call since inodes are normally
not destroyed immediately when they are unreferenced. It was not destroyed
during EdenMount::shutdown() since EdenMount didn't have any references to
these additional root inode structures and their children.
- EdenMount::diffRevisions() incorrectly swapped ADDED and REMOVED statuses in
the result.
Reviewed By: wez
Differential Revision: D7341609
fbshipit-source-id: 16e755a0ff685f51c977c3b27d6af96908f33494
Summary:
Add a function for diffing two source control commits without needing to
instantiate TreeInode objects.
Reviewed By: wez
Differential Revision: D7341604
fbshipit-source-id: 557eef87faa2785ab96d51b09569a46f892a71f6
Summary:
Fix the code to generate exceptions based on an errno error using
std::generic_category rather than std::system_category.
Reviewed By: yfeldblum
Differential Revision: D7329997
fbshipit-source-id: 3fe257bbbc7a631c801f31120592c8bdbc25c8bf
Summary:
Add a new utils/SystemError.h header with helper functions to check if a
`std::system_error` contains an errno value.
Most of the code in Eden previously only checked for `std::system_category`
when looking for errno values. `std::generic_category` is the correct category
to use for errno exceptions, but folly/Exception.h incorrectly throws them as
`std::system_category` today. This change makes Eden treat either error type
as errno values for now.
Reviewed By: yfeldblum
Differential Revision: D7329999
fbshipit-source-id: 67a3c3ea10371c53a2e34236b7575deac4cbd53a
Summary:
This helps distinguish `facebook::eden::FileHandle` from
`facebook::eden::fusell::FileHandle`, and will make it possible to eliminate
the separate `facebook::eden::fusell` namespace in the future.
Reviewed By: chadaustin
Differential Revision: D7314456
fbshipit-source-id: 0869427766fe666d119a59c7df1c97825a8ad36e
Summary:
This test exercises code under eden/fs/fuse, so put it with that directory's
tests.
Reviewed By: chadaustin
Differential Revision: D7314457
fbshipit-source-id: b664aaa3086b2e65f7ae8b76ae71878f39fd9d17
Summary:
There's some code that uses Entry::hasInodeNumber to decide whether an
inode is in the process of being loaded. This change is a
prerequisite for D7052470.
Reviewed By: simpkins
Differential Revision: D7311182
fbshipit-source-id: e04a89c814950e6ce33e73b963b3ffb0f0e353b8
Summary:
Remove the `FuseChannel::stealFuseDevice()` method, and instead change the
session completion future to return the FUSE device FD if it is still valid.
This ensures we only extract the FUSE device when it is still valid: if an
unmount event is being processed by a FUSE worker thread simultaneously with a
call to `FuseChannel::takeoverStop()` the session completion future will return
an empty `folly::File` since the FUSE device has been shut down.
This also helps clarify the synchronization semantics around modification to
the `fuseDevice_` member variable.
Reviewed By: wez
Differential Revision: D7300847
fbshipit-source-id: 02ced8fc9d24e7cd526d911782949d0bfbf0e5a7
Summary:
The FuseChannel destructor previously could be invoked while there are still
outstanding requests, which would cause problems when responses were generated
for these requests.
This makes the FuseChannel destructor private, requiring users to always
allocate FuseChannel objects on the heap and destroy them using a destroy()
method. The destroy() method still blocks waiting for the FUSE worker threads
to exit, but if there are still outstanding FUSE requests it defers the
deletion of the FuseChannel until they are complete.
The EdenMount class currently couldn't ever destroy FuseChannel objects with
outstanding requests, since it waits for the completion future to fire before
invoking the destructor. Fixing this behavior is mostly for correctness of the
FuseChannel API and for the benefit of our unit tests.
Reviewed By: chadaustin
Differential Revision: D7297829
fbshipit-source-id: 643d1332be84a1f25ee30ba2a2ea3515806f00ef
Summary:
Make ServerState also store the clock as a member variable. This moves more
server-wide state into the ServerState class, and allows us to eliminate
another argument to the EdenMount constructor.
Reviewed By: chadaustin
Differential Revision: D7297832
fbshipit-source-id: 2a063d67752f46686987390b3faefb304455568a
Summary:
Move the EdenCPUThreadPool from EdenServer to ServerState.
This ensures that all EdenMount objects always have a thread pool. Previously
most unit tests were creating an EdenMount without a thread pool set, but the
FileInode code and TreeInode code could end up wanting to use the thread pool
even if no FuseChannel is running.
Reviewed By: chadaustin
Differential Revision: D7297828
fbshipit-source-id: 9861b3439cd091afe31fa13f449aa14656983318
Summary:
Remove the `FuseChannel::getSessionCompleteFuture()` method and instead have
`initialize()` and `initializeFromTakeover()` return the `Future` that can be
used to wait on session completion.
This makes it explicit from an API perspective that this session completion
future will only be invoked if initialization is successful. It also
eliminates the possibility of anyone calling getSessionCompleteFuture() more
than once.
I also changed the session completion future to use a folly::SemiFuture rather
than a folly::Future, to make it explicit that callers generally should not
execute their callback inline in the same thread where the SemiFuture is
fulfilled.
Reviewed By: chadaustin
Differential Revision: D7297835
fbshipit-source-id: 3a1157951f0738f1692833ed5e875c3e9c6d6d69
Summary: Testing convenience. The NO_CHECK constructor is no longer necessary now that simpkins made the original constructor constexpr.
Reviewed By: simpkins
Differential Revision: D7295539
fbshipit-source-id: 717f5a7a8d3d9790966fad441da7b3308495e2c3
Summary: Cleaning up the takeover initialization code for EdenMount and InodeMap.
Reviewed By: simpkins
Differential Revision: D7294419
fbshipit-source-id: 58506f04259bb1017e6cd2e80e40e5820de6200f
Summary:
Instead of having a rule that save() must be called after
InodeMap::shutdown, just have InodeMap::shutdown return the
SerializedInodeMap if it's desired.
Reviewed By: simpkins
Differential Revision: D7292773
fbshipit-source-id: 2ba35fc729e19af122fe5d6c5a3287ad6b8af946
Summary:
This changes FuseChannel to fulfill the session complete future immediately in
the thread where it finishes. This may occur inside a FUSE worker thread.
It is now up to the caller to use `via()` if they want their callback to run in
a separate thread.
This simplifies the FuseChannel code and also addresses a crash that occurs in
the unit tests sometimes: If the FuseChannel destructor is invoked without
explicitly invoking the FuseChannel, it would schedule the session complete
promise to be fulfilled in a separate EventBase thread. However, the promise
may then have already been destroyed by the time it can run in the EventBase
thread.
There are still additional synchronization issues to fix in the FuseChannel
tests, but this resolves one problem.
Reviewed By: chadaustin
Differential Revision: D7282001
fbshipit-source-id: be64d3ef6a0e664ed7a2cf93a549acc140184095
Summary:
Empty .gitignore files should be processed normally, and treated as no new
ignore rules. The logic in D6659654 had accidentally introduced a bug where we
would completely skip processing any directories that contained empty
.gitignore files.
Reviewed By: wez
Differential Revision: D7261276
fbshipit-source-id: 033e199f15d7763bff5f9a080a226c50e481aa9d
Summary:
- Update FuseChannel::initializate() to not require an Executor. Rather than
performing initialization using the supplied Executor, it simply starts one
worker thread first and performs initialization there.
- Change the API semantics slightly so that the session complete future is
invoked only if initialization succeeds. Previously the session complete
future could be invoked if initialization failed as well.
- Replace the activeThreads counter with a stoppedThreads counter to determine
when the session complete future should be invoked. The previous
activeThreads behavior was somewhat racy, since the activeThreads counter was
incremented inside each worker thread, but most places that checked it did
not wait to ensure that all worker threads had successfully incremented it
yet.
Reviewed By: chadaustin
Differential Revision: D7243910
fbshipit-source-id: 93b3465509bd9bf6fa90ea097e70dac3193172f9
Summary: It's a little surprising to me the clang build passed without these explicit template instantiations!
Reviewed By: simpkins
Differential Revision: D7253536
fbshipit-source-id: 2f48d5571777f4e978b6947183eefb03158d5014
Summary:
Split up the FuseChannel and EdenMount APIs for starting a brand new FUSE
channel vs taking over an existing channel.
These operations are somewhat different as initializing a new FUSE channel
requires performing a negotiation with the kernel that will not complete
immediately. Therefore these APIs return a Future object. Taking over an
existing FUSE channel is always an immediate operation, and therefor does not
need to return a Future object.
Reviewed By: chadaustin
Differential Revision: D7241997
fbshipit-source-id: 22780f2df76b2d554ab2a4043b6425fa7a4e9c94
Summary:
Ensure that we ignore SIGPIPE in unit tests. The
`FuseChannel::requestSessionExit()` function sends SIGPIPE to its worker
threads, and expects this to wake up the worker threads if they are stuck
inside a read() call.
In normal program builds folly/Subprocess.cpp causes SIGPIPE to be ignored on
startup. However, Subprocess.cpp is not necessarily linked into all of our
unit tests. I haven't tracked down exactly why, but SIGPIPE in the unit tests
seems to kill the entire process only in opt-ubsan builds.
In the future we probably should clean up how requestSessionExit() triggers its
worker threads to exit: the current code is still potentially racy, since the
worker threads could receive the SIGPIPE just after checking `sessionFinished_`
and before calling `read()`, in which case they may go back to sleep rather
than exiting. However for now this should be a simple fix to get the tests
passing in all build modes.
Reviewed By: wez
Differential Revision: D7224370
fbshipit-source-id: a56fe3331fc5aa6a49ccbe6b0678b479a44ffc07
Summary:
To make it clearer to me why all the calls to newPtrLocked were safe, and to
eliminate some duplication, I captured the newPtrLocked call patterns into
member functions on LoadedInode and TreeInode::Entry.
Reviewed By: simpkins
Differential Revision: D7207542
fbshipit-source-id: 25de77e72c0898be43b3fbdddab835d64101755e
Summary:
To make it clearer why newPtrLocked is safe during the Inode class
construction process, add a takeOwnership function (better name
suggestions welcome) that converts from the newly-instantiated
unique_ptr to a managed InodePtr.
Reviewed By: simpkins
Differential Revision: D7204818
fbshipit-source-id: 8a40189588442490120623da86195c6fc99c9c51
Summary:
This diff is mostly preparation. It's submitted separately to
remove mechanical refactoring noise from the following diff, where
every entry is assigned an inode number upon construction.
Reviewed By: simpkins
Differential Revision: D7116832
fbshipit-source-id: 2943c45340a9a751eb52bf13e19d233d829494c0
Summary:
If the root TreeInode wants to allocate inode numbers, the inode
allocator must be initialized first. But complete InodeMap
initialization requires the root TreeInode. So split this into two
parts.
Also, I changed the inode allocator to a single atomic increment instead
of a lock acquisiton.
Finally, the extra assertions in this diff uncovered what looks like a
bug in the takeover logic where nextInodeNumber_ could end up being
smaller than the value in the takeover data, since the max inode
number from the overlay was assigned after loading from takeover data.
Reviewed By: simpkins
Differential Revision: D7107706
fbshipit-source-id: ec43cc81c11d709261598739c622609b372433a2
Summary:
I spent way too long trying to figure out why my refactorings were
causing invariant errors inside InodeMap. It turns out that we
initialize the root TreeInode before InodeMap::initialize is called,
which I suspect resulted in duplicate inode numbers being handed out.
Reviewed By: simpkins
Differential Revision: D7106302
fbshipit-source-id: b459734fb96bfbb6b4b27a1d23de8b6406d30ca4
Summary:
This adds a FakeFuse class to simulate a FUSE connection to the kernel, but
that is actually communicating with our userspace test harness code. This also
adds a FakePrivHelper class which implements the PrivHelper interface but
returns FakeFuse connections rather than real FUSE connections to the kernel.
This will enable us to write unit tests that exercise more of the FUSE and
EdenMount logic, and will also enable us to test error conditions and ordering
behaviors that are difficult to reliably reproduce with a real FUSE mount.
This also includes some very basic tests using this new code. The code in
fuse/test/FuseChannelTest.cpp creates a FuseChannel using a FakeFuse object,
and the code in inodes/test/FuseTest.cpp creates a full EdenMount object using
FakePrivHelper and FakeFuse. The tests are pretty similar for now, and only
exercise the FUSE initialization code. I will expand these tests in subsequent
diffs.
Reviewed By: wez
Differential Revision: D7050826
fbshipit-source-id: 4f82375d65664ca3a5a228a577caa4d1d47454fe
Summary:
I'm seeing test failures that I have not yet understood and I
thought they might be caused by an implicit conversion from
fusell::InodeNumber to bool. Well, they're not, but this is how I
discovered that. I'm not sure I want to land this change, but I'm
going to leave it around until I figure out what's happening with my
other diffs.
Reviewed By: simpkins
Differential Revision: D7077635
fbshipit-source-id: 50bf67026d2d0da0220c4709e3db24d841960f4b
Summary:
I am working on a stack of diffs that changes how we allocate inode
numbers to tree entries. I was hitting test failures I could not
understand, so in the process of trying to understand the flows
through InodeMap, I found newChildLoadStarted to be redundant with
shouldLoadChild.
Note: Today, allocateInodeNumber() acquires the InodeMap's lock, but
in a later diff, inode numbers will be assigned en masse during
TreeInode construction.
Reviewed By: simpkins
Differential Revision: D7059719
fbshipit-source-id: 624b861040d585d2cae41d7ec2aae7d528ff8336
Summary:
Previously we returned an ENOENT error in response to a FUSE lookup() call for
a name that does not exist. However, this does not allow FUSE to cache the
result, so we will continue to receive lookup() calls for this path in the
future.
This changes EdenDispatcher to return a successful response with an inode
number of 0 instead. This tells the kernel that the name does not exist, but
allows the kernel to cache this negative lookup result (for as long as
specified in the entry_valid field in the response).
Reviewed By: wez
Differential Revision: D7076811
fbshipit-source-id: a2b9977e58d6b6eecb584699b9d93b5ad29ad5ad
Summary:
Update the TreeInode code to always update the FUSE cache whenever we add a new
entry to the directory.
Up to now we have been fine without this since the kernel never cached negative
lookup responses for us. In order to turn on FUSE caching of negative lookup()
responses we need to invalidate the cache whenever we create a new entry.
- Have create(), symlink(), mknod() and mkdir() invalidate the FUSE entry cache
if they are not being invoked from a FUSE request. These are almost always
invoked from a FUSE request, but flushing the cache is the correct thing to
do if we add code in the future that can trigger these from a thrift call or
via some other non-FUSE code path.
- Change the checkout code to invalidate the FUSE cache whenever it creates new
children entries.
Reviewed By: wez
Differential Revision: D7076812
fbshipit-source-id: d6489995b415260e84b3701c49713b0ef514f85d
Summary:
This removes the TARGETS files from the eden github repository. The
open source buck build has been failing for several months, since buck
removed support for the thrift_library() rule.
I will potentially take a stab at adding CMake build support for Eden
at some point in the future.
Reviewed By: chadaustin
Differential Revision: D6893233
fbshipit-source-id: e6023094a807cf481ac49998c6f21b213be6c288
Summary: Per code review comments on D6983198, this simplifies the way we check if mode bits have changed in a meaningful-to-source-control way.
Reviewed By: simpkins
Differential Revision: D7015339
fbshipit-source-id: 548ead337fbea1c1dcb72b880921671e9b6188ac
Summary:
mode_t isn't really part of a TreeEntry and I also wanted to see all
the places where we convert an entry type from source control into
mode bits.
Reviewed By: simpkins
Differential Revision: D6983198
fbshipit-source-id: ce1d0976f5fc5130c34a8c93c07a4e26a7cdaf71
Summary:
This is the type of a tree entry, which may be another tree, so
FileType is not an accurate name.
Reviewed By: simpkins
Differential Revision: D6981168
fbshipit-source-id: 997eb8a27f599310ed678ce221c8083722db8bff
Summary:
While we're changing how timestamps are stored, we might as well
implement this small-ish efficiency. Instead of storing timestamps as
a timespec (16 bytes), store them as 64-bit nanoseconds with a range
slightly larger than what ext4 supports. Assuming a million inodes,
this saves 24 MB.
This diff introduces the EdenTimestamp type. The next diff will start
using it.
Reviewed By: simpkins
Differential Revision: D6957659
fbshipit-source-id: 4551af6f5b8c1ff610ba88795f69e7d69d7f605d
Summary:
I want to rename FileType to TreeEntryType so I removed this one first
and replaced all of its uses with an isTree() method.
Reviewed By: simpkins
Differential Revision: D6980501
fbshipit-source-id: 105b8c599585e63efd44043e761db40e2824e77e
Summary:
Our Model TreeEntry code was a bit too general - in reality, both git
and hg only support a handful of specific tree entries: regular files,
executable files, symlinks, and trees. (git also supports
submodules.) This diff delays the expansion of a TreeEntry's type
into a full mode_t.
Reviewed By: simpkins
Differential Revision: D6980003
fbshipit-source-id: 73729208000668078a180b728d7e0bb9169c6f3c
Summary: I should have caught this before...
Reviewed By: wez
Differential Revision: D6995395
fbshipit-source-id: 08efd0aacb051f2b1754930b311e3a42afc3c538
Summary:
Another small refactoring that I don't want mixed into my
bigger diffs.
Reviewed By: wez
Differential Revision: D6927482
fbshipit-source-id: 28cc60dfdffb50921a5ef9cca4e2814b90d3b701
Summary:
There were places we were acquiring a lock unnecessarily. In
addition, I'm looking at reducing the number of places where we store
the full mode_t to see if we can get away with dirtype_t or something
similar.
Reviewed By: wez
Differential Revision: D6972140
fbshipit-source-id: bb29a4473f3056e39596600d22e67374ca484735
Summary:
Update FileInode so that getattr() and setattr() both return a reasonable value
in st_blocks.
Previously we always returned 0 in st_blocks, which caused applications like
`du` to always report files as using 0 space in Eden mounts. Now we compute
st_blocks based on st_size, so that `du` will report reasonable estimates for
when scanning the size of subdirectories inside an Eden mount.
Reviewed By: chadaustin
Differential Revision: D6932098
fbshipit-source-id: bd29e46821176e510f420e6e2b6ce480b80d50ff
Summary:
Move all of the privhelper functionality into a PrivHelper class. The
ServerState object now stores the PrivHelper object to use, rather than having
a global singleton.
This will make it easier to stub out the PrivHelper functionality during unit
tests.
Reviewed By: wez
Differential Revision: D6929862
fbshipit-source-id: e3edcb0a03ba9afdf34554cb961fd74557cdd6e3
Summary:
Update EdenMount::diff() to use the UserInfo object stored in the shared
ServerState rather than calling UserInfo::lookup() on each diff operation.
Reviewed By: wez
Differential Revision: D6929865
fbshipit-source-id: a68ab1fa9eb345b59972e67c3aac258b4dbcdab5
Summary:
Add a new ServerState class to store process-wide state that is shared across
multiple mounts. Up until now we have passed around the shared state data as
separate variables.
This is intentionally separate from the existing EdenServer class to allow unit
tests to create EdenMount objects without an EdenServer object.
Reviewed By: wez
Differential Revision: D6929861
fbshipit-source-id: 5f22efb6d79dfd70031be1dc37f494c2ad8af902
Summary:
We no longer need this and I believe that this was contributing
to this source of flakiness in our CI; this stack trace triggers when
we get an aborted or short read from the kernel:
```
*** Aborted at 1517827659 (Unix time, try 'date -d 1517827659') ***
*** Signal 11 (SIGSEGV) (0x2d0) received by PID 153706 (pthread TID 0x7f63ea37d700) (linux TID 160573) (maybe from PID 720, UID 0) (code: address not mapped to object), stack trace: ***
@ 0000000002abfa2d folly::symbolizer::(anonymous namespace)::signalHandler(int, siginfo_t*, void*)
./folly/experimental/symbolizer/SignalHandler.cpp:413
@ 00007f646e96dacf (unknown)
@ 00007f646e96aa20 __pthread_kill
@ 0000000001873748 facebook::eden::fusell::FuseChannel::requestSessionExit()
./eden/fs/fuse/FuseChannel.cpp:483
@ 000000000187d2f7 facebook::eden::fusell::FuseChannel::processSession()
./eden/fs/fuse/FuseChannel.cpp:656
@ 000000000187dab9 facebook::eden::fusell::FuseChannel::fuseWorkerThread(unsigned long)
./eden/fs/fuse/FuseChannel.cpp:493
@ 00007f646f267170 execute_native_thread_routine
@ 00007f646e9637a8 start_thread
@ 00007f646e047a7c __clone
```
my theory is that we're allowing shutdownImpl to free things out from under other
threads before they've all seen this signal and wound down fully. This is slightly
speculative in that I haven't managed to reproduce this stack trace on my devserver.
We don't really need this additional signal any longer.
Reviewed By: simpkins
Differential Revision: D6907734
fbshipit-source-id: 0f0138b631a7201fc9a4a1c93c2cde846e869cbd
Summary: Small refactoring I should have done with the previous diff.
Reviewed By: simpkins
Differential Revision: D6927152
fbshipit-source-id: 1dcda01134c3d63c62169c5728dba24ca0eebd68
Summary:
Update `TreeInode::diff()` to check if its hash matches the source control tree
it is being compared to, and return early if they are identical.
I'm surprised that I forgot to include this initially when implementing
`TreeInode::diff()`
This makes `hg status` faster when a large number of unmodified directories
have been loaded.
Reviewed By: chadaustin
Differential Revision: D6890615
fbshipit-source-id: 561630d0220b4875dbf3678161cdb41a8aa4fc82
Summary:
This re-orders some of the code in `TreeInode::diff()` slightly. This should
not affect the behavior of the code.
This moves the `isIgnored` check inside the main `contents_.wlock()` block.
This reduces the number of places where we grab the lock, and will help keep
things simple for an upcoming diff where I need to add some more checks in this
code with the lock held.
This also changes `inodeFuture` to use the new `Future::makeEmpty()`
constructor rather than having to use an `Optional<Future>`
Reviewed By: chadaustin
Differential Revision: D6890616
fbshipit-source-id: 354bbf6a6be6d356fd23e6c0fb6b534679bbe0bb
Summary: Small things I've needed in later diffs.
Reviewed By: wez
Differential Revision: D6877755
fbshipit-source-id: c9002eb0b92dbd8fe9c4f636d2ca79b25cde331f
Summary:
While working on timestamp storage, the fact that
InodeTimestamps was a member of InodeBase kept getting in the way.
Make it its own type.
Reviewed By: simpkins
Differential Revision: D6862835
fbshipit-source-id: 91d8984764f0586b9fa52e961eb5606a530e0416
Summary: This API seems like it should be const, as it does not modify the clock.
Reviewed By: chadaustin, zhupanov
Differential Revision: D6869719
fbshipit-source-id: c8bf4ccab34538b59e6baeedd0b0ff88b328236e
Summary:
This diff moves the mount-time initialization handling
out of the main loop. This rationale for this is:
* We don't (and shouldn't!) need to process FUSE_INIT for takeover
processing, and this structure allows us to make stronger assertions
about our state.
* we can avoid spinning up multiple threads in the (rare!) case that
the FUSE_INIT fails
* It is now a little harder for exceptions during initialization to
escape our notice.
In rearranging this stuff, I found a race condition in the worker thread
shutdown; we could erroneously emit a completion event before all of
the threads had been torn down and this resulted in sporadic integration
test failures hitting the assertion for the number of joined threads
in the destructor.
Reviewed By: simpkins
Differential Revision: D6766330
fbshipit-source-id: 32afb5a7c739c75aebfdb0a8f896eec5f41ad33f
Summary:
Dir's contents were represented as a vector of 64-bit
pointers to 48-byte structs. This change removes that layer of
indirection, reducing memory usage and slightly pessimizing insertion.
The diff is mostly mechanical outside of the TreeInode.h changes and
calls to emplace..
I'll run memory tests tomorrow, though it's a gamble as to whether
private bytes will show a difference. I may need to shrink the Entry
struct too.
Reviewed By: wez
Differential Revision: D6804957
fbshipit-source-id: b126656dbc7951565e74b6401adde6353e809056
Summary:
Tiny thing I noticed when reading code. Keep the entry name
as a StringPiece rather than bouncing through char*.
Reviewed By: zhupanov
Differential Revision: D6820080
fbshipit-source-id: 884e55f74094f44012efbe44b86d8e5903300967
Summary:
isSameAs calls getSha1 which was failing on symlinks. The
original concern was that asking for the SHA-1 of a symlink is
ambiguous: do you want the hash of the symlink or the target? But we
already check for whether you are requesting the SHA-1 of a symlink in
EdenServiceHandler, so it's redundant and incorrect to check in
FileInode too.
Reviewed By: simpkins
Differential Revision: D6847489
fbshipit-source-id: 13966da06bcde75c5c568e09fef14e735de47cfb
Summary:
It's interesting to see the total number of loaded files
vs. trees when the loaded inode count is high.
Reviewed By: wez
Differential Revision: D6765874
fbshipit-source-id: 178b30184428bd5cf5e005eb475e4f5a1476c385
Summary:
I'm not sure what was wrong with the old code, but I
simplified and clarified all of the time math and now `eden debug
unload` behaves as I'd expect it should.
Reviewed By: simpkins
Differential Revision: D6764962
fbshipit-source-id: 3ed359d4ab4652e95d1538a0982c24185999351c
Summary:
rdev doesn't add any value yet. We can add it back if we want
to implement support.
Reviewed By: simpkins
Differential Revision: D6792346
fbshipit-source-id: ce16317074f1daa456737c55804da8fb7f2b7a94
Summary:
This slightly improves the code in EdenMount by having
just a single call to the FuseChannel constructor with an optional
`fuse_init_out` instance that we can use to determine if we are
performing a takeover or not.
Reviewed By: simpkins
Differential Revision: D6766328
fbshipit-source-id: ece140a1572e2934a3e35bfe25b83af910346c18
Summary: Minor stylistic changes that were done during constification but factored out.
Reviewed By: chadaustin
Differential Revision: D6774976
fbshipit-source-id: d18cd339153cf16ff69be0de5f3eb019a4baa1a0
Summary:
The value we get is of enum class, so it is guaranteed to be in the valid set.
Omitting default we get compiler support if we ever expand the set of values.
Throw becomes obviated, because it becomes unreachable.
Reviewed By: simpkins
Differential Revision: D6771224
fbshipit-source-id: 039f0545d20649193b3fb74981ee7f027f31fd8e
Summary: Replaced memset used to 0-init structs with POD fields with = {} init.
Reviewed By: simpkins
Differential Revision: D6770868
fbshipit-source-id: 190205a277e3c73711813bc515b860b107d41f83
Summary:
This is moving some files around in preparation for
moving TakeoverData to using thrift for its serialization
Reviewed By: simpkins
Differential Revision: D6733405
fbshipit-source-id: 235ba237546f8ef606de8445db45683ce38a2d2c
Summary:
This implements a TODO/FATAL that is important for
graceful restarts to be useful in my "acid test" scenario,
which is to perform a graceful restart while buck build is
running.
Reviewed By: simpkins
Differential Revision: D6700189
fbshipit-source-id: dec1b818ebc9e907841bc127ee08c953b59d6487
Summary:
Previously, `EdenMount::create` would implicitly call
`EdenMount::initialize` which would load the root inode and the `.eden` magical
directory. That's fine for the fresh mount case, but in the case of the
graceful restart we need to take the opportunity to apply the `InodeMap`
from the old process before we start muddying its state.
This diff breaks out the `initialize` method from the `create` method and
makes the mount code call it after potentially loading the `InodeMap` from
the takeover data.
In addition, this diff removes the the `root->loadMaterializedChildren()`
call from the mount initialization code. It is no longer required to do
this eagerly and it makes things simpler and our memory profile a little
smaller to defer this (I haven't measured how much that impacts things).
Reviewed By: simpkins
Differential Revision: D6691182
fbshipit-source-id: 52033a6d64105b658314a919f69dbfcd4eea242b
Summary:
I'm so-so on a bit of the implementation here, but it works!
I had to change the `takeoverPromise` from the `pair<fuseDevice, connInfo>`
to a new helper struct because we now have three distinct pieces of data
to pass out of EdenMount to build up the overall TakeoverData.
The key change in this diff is that we have to release all of the file handles
we're maintaining in the `FileHandleMap` prior to shutting down the `InodeMap`,
otherwise the `InodeMap` will never complete (it waits for all inodes to be
unreferenced, and that cannot happen while there are open file handles). I've
made the `FileHandleMap` serialization and clearing contingent on performing a
takeover shutdown because that feels like the safest thing to do wrt. not
losing any pending writes.
Reviewed By: simpkins
Differential Revision: D6672437
fbshipit-source-id: 7b1f0f8e7ff09dbed850c7737383ecdf1e5ff0c7
Summary:
This is the key portion that makes the graceful restart
function. This diff connects almost all of the moving pieces together;
it informs the priv helper about the takeover mount and transfers
the InodeMap information into the new generation of the eden server.
It doesn't yet load the fileHandleMap (will tackle that in a follow up diff)
Reviewed By: simpkins
Differential Revision: D6670903
fbshipit-source-id: 1770d99eb1477440a6c1deed83b0da55b9c1bbe4
Summary:
this isn't how we really want to do this long term, it's
just the most expedient short term implementation.
This diff provides an implementation of the `InodeMap::save()` which
was previously a stub method; the new implementation returns a thrift
structure that encompasses the unloaded inodes in the map, and adds
a corresponding load() method that performs the reverse transformation.
The struct is serialized into the Takeover data.
This diff doesn't hook up the real serialized data to EdenServer; that will happen
in a follow-on diff.
The way that we actually want to handle this longer term is to store the
`numFuseReferences` field into the overlay file on disk, but to do so we
will need to add a mountGeneration field alongside it and ensure that we
always write out the correct information at the correct times. In addition,
we'd need to add equivalent data to TreeInode::Entry and add accessors that
safely return the correct values in the correct situations.
In the interest of getting something working, I've just dumped this code in
here.
I've also placed the thrift structure for this in `fuse/handlemap.thrift`;
this is a slight layering violation but one that feels "OK" in light of
the imminent refactor of the Takeover data struct to be its own thrift
struct anyway.
Reviewed By: simpkins
Differential Revision: D6670904
fbshipit-source-id: 11a0918954c741935c587e46fcb0e38849010de1
Summary:
This adds some plumbing to thread the fuse device descriptor and
negotiated capabilities through to the takeover code.
I initially wanted to just make the
unmount future yield the device descriptor, but since that uses
`SharedPromise` it is not compatible with a move-only type like
`folly:File`, so I added an optional promise to deal with just that.
I'm also populating the takeover mount information (path, bind mounts)
for each mount point.
Reviewed By: simpkins
Differential Revision: D6494509
fbshipit-source-id: a90684292dc1d8e06ce2c0721eadd8d393377f33
Summary: A prior shelve contained this fix but never got landed.
Reviewed By: wez
Differential Revision: D6676206
fbshipit-source-id: b8c733be663ff56e1a0625f09ec505891d430084
Summary:
We'll need a way to twiddle the refcount by more than
increment at a time for graceful restart. This mirrors the
same pattern we use to forget() by an arbitrary number.
Reviewed By: chadaustin
Differential Revision: D6668809
fbshipit-source-id: 6e5dc33b5e40f98f01293c89152bfe1e0879f572
Summary: We forgot to remove this when we moved to be fully inode based in the overlay
Reviewed By: chadaustin
Differential Revision: D6668810
fbshipit-source-id: b79af85a4bbbcefd9227ad69bb8d57b5274cdaed
Summary:
It is no longer correct to assert that state->file is set if O_TRUNC happened
before blob import from hg finished. It surprises me we never saw a crash
because of that. Also, the O_TRUNC path after blob import finishes can never
complete a future, so don't try.
Reviewed By: wez
Differential Revision: D6656699
fbshipit-source-id: 5e245fc46185714e5f5d81c2680835a3497747ff
Summary:
Today, if a file is ever opened for read, each FileInode keeps a copy
of the data as long as the FileInode is around. This results in
excessive memory consumption under common mistakes like repo-wide grep
or `hg revert .`.
I will audit all of the state machine transitions and blob accesses
before landing this diff.
Reviewed By: wez
Differential Revision: D6598957
fbshipit-source-id: 1eb4aeb08057ce993a29a86d298e153675fee4a1
Summary:
This came up when I was auditing the rules about when it's
safe to read from a FileInode. read() must only be called while openCount > 0.
Reviewed By: wez
Differential Revision: D6604898
fbshipit-source-id: 829ddc335bd58201c2b456ee544cdc6253ebf66c
Summary:
In a follow-on diff, the constraint will be that state->blob
will only be guaranteed valid after ensureDataLoaded() while a
FileHandle is alive. Thus, ensureDataLoaded() must return a
FileHandle.
Reviewed By: wez
Differential Revision: D6586237
fbshipit-source-id: ccc269d322b8c725c93145df5de2add9a2b90207
Summary: This is annoying because it isn't actually shadowing and because this isn't picked up in our default mode mode, or in sandcastle.
Reviewed By: chadaustin
Differential Revision: D6656548
fbshipit-source-id: e624cb563d6396c1ab6b93eae14651c16a8c0cd3
Summary:
Now that we have full control over the fuse session,
moving the threads here makes a lot of sense and makes things
easier from an overall state management perspective.
FuseChannel now provides a future that make it easier for EdenMount to wait
until all the threads and outstanding requests have completed.
There is also a future to determine when the first of the threads
has exited which is used to detect an error condition. We could
go a bit further with this and have the error condition propagate
out from our dispatcher, but that's a bit further away from my
goal of making the graceful restart stuff work.
I've removed the request counting stuff from Dispatcher as we
can now use the definitive signals from FuseChannel in its place.
Reviewed By: chadaustin
Differential Revision: D6580247
fbshipit-source-id: 6bcc3b8b531d59a3fdd0ca6fd09410ad64f8221a
Summary:
This serves a few purposes:
1. We can avoid some conditional code inside eden if we know that
we have a specific fuse_kernel.h header implementation.
2. We don't have to figure out a way to propagate the kernel
capabilities through the graceful restart process.
3. libfuse3 removed the channel/session hooks that we've been
using thus far to interject ourselves for mounting and
graceful restarting, so we were already effectively the
walking dead here.
4. We're now able to take advtange of the latest aspects of
the fuse kernel interface without being tied to the implementation
of libfuse2 or libfuse3. We're interested in the readdirplus
functionality and will look at enabling that in a future diff.
This may make some things slightly harder for the more immediate
macOS port but I belive that we're in a much better place overall.
This diff is relatively mechanical and sadly is (unavoidably) large.
The main aspects of this diff are:
1. The `fuse_ino_t` type was provided by libfuse so we needed to
replace it with our own definition. This has decent penetration
throughout the codebase.
2. The confusing `fuse_file_info` type that was multi-purpose and
had fields that were sometimes *in* parameters and sometimes *out*
parameters has been removed and replaced with a simpler *flags*
parameter that corresponds to the `open(2)` flags parameter.
The *out* portions are subsumed by existing file handle metadata
methods.
3. The fuse parameters returned from variations of the `LOOKUP` opcode
now return the fuse kernel type for this directly. I suspect
that we may need to introduce a compatibility type when we revisit
the macOS port, but this at least makes this diff slightly simpler.
You'll notice that some field and symbol name prefixes vary as
a result of this.
4. Similarly for `setattr`, libfuse separated the kernel data into
two parameters that were a little awkward to use; we're now just
passing the kernel data through and this, IMO, makes the interface
slightly more understandable.
5. The bulk of the code from `Dispatcher.cpp` that shimmed the
libfuse callbacks into the C++ virtual methods has been removed
and replaced by a `switch` statement based dispatcher in
`FuseChannel`. I'm not married to this being `switch` based
and may revise this to be driven by an `unordered_map` of
opcode -> dispatcher method defined in `FuseChannel`. Regardless,
`Dispatcher.cpp` is now much slimmer and should be easier to
replace by rolling it together into `EdenDispatcher.cpp` in
the future should we desire to do so.
6. This diff disables dispatching `poll` and `ioctl` calls. We
didn't make use of them and their interfaces are a bit fiddly.
7. `INTERRUPT` is also disabled here. I will re-enable it in
a follow-up diff where I can also revise how we track outstanding
requests for graceful shutdown.
8. I've imported `fuse_kernel.h` from libfuse. This is included
under the permissive 2-clause BSD license that it allows for
exactly this integration purpose.
Reviewed By: simpkins
Differential Revision: D6576472
fbshipit-source-id: 7cb088af5e06fe27bf22a1bed295c18c17d8006c
Summary:
Easy to overlook this; the issue is that we need to explicitly
do something about the error case when we're stitching together Promises
by hand, otherwise we will silently drop exceptions.
Flat manifest imports are failing in `RestartTestHg` in master
at the moment. That error was silently being swallowed and the test would
hang until it timed out.
This is an uglyish hack to explicitly propagate the error condition so that
that test will error out.
This diff doesn't fix the source of the manifest import issue; that is addressed
in the next diff (turned out to be that the `--takeover` flag was not being
passed correctly)
Reviewed By: bolinfest
Differential Revision: D6627973
fbshipit-source-id: b7093890f543618a11682e939f8802f1309831d4
Summary:
This is a codemod to change from using @/ to // in basic cases.
- TARGETS files with lines starting with @/ (but excluding @/third-party:
- autodeps lines in source and TARGETS files ( (dep|manual)=@/ ), excluding @/third-party
- Targets in string macros
The only thing left of the old format should be @/third-party:foo:bar
drop-conflicts
Reviewed By: ttsugriy
Differential Revision: D6605465
fbshipit-source-id: ae50de2e1edb3f97c0b839d4021f38d77b7ab64c
Summary:
Update the logic for dry-run checkouts to exit early whenever we hit a
directory that is unmodified from the original source control state. In this
case we can skip this entire subdirectory since we know there cannot be any
conflicts.
This should make dry-run checkout operations much faster in cases where there
are lots of changes between the source and destination source control trees,
but the working directory state has relatively few modified files.
Reviewed By: bolinfest
Differential Revision: D6605282
fbshipit-source-id: b5423749f3d47b10ed8d599ffaa0667c72fbaec2
Summary:
Previously the checkout code always called `JournalDiffCallback::performDiff()`
to get the list of unclean files before starting the checkout operation. After
the checkout completed it called `JournalDiffCallback::performDiff()` to get
the unclean files after the checkout operation.
Making both diff calls should be unnecessary: the checkout code can only modify
files that were unclean before the checkout (in the case of a `FORCEe` checkout)
or files that were modified between the source and destination source control
trees. Therefore the `performDiff()` call after checkout completes should be
unnecessary, and can be removed.
This diff also eliminates the initial `performDiff()` call for `DRY_RUN`
checkouts. We do not add a journal entry at all for `DRY_RUN` operations, so
we can skip this diff computation.
Reviewed By: wez
Differential Revision: D6455969
fbshipit-source-id: 20e0ac0d16d1fde844c1d6165a96611bfb370597
Summary:
I almost added a copy in a later diff and realized it would
have let me, but done the wrong thing.
Reviewed By: wez
Differential Revision: D6585810
fbshipit-source-id: 15295d04b06df397113be6080e1f2f6b8a473745
Summary: Added type identification capability to InodeBase and its descendants FileInode and TreeInode.
Reviewed By: simpkins
Differential Revision: D6564902
fbshipit-source-id: ce9300102d6d6d1c42616eb1e32042f21f6e6cce
Summary:
Add EdenCPUThreadPool and UnboundedQueueThreadPool types to make it clearer
that it's always okay for prefetch, deferred diff entry, and hg import to
shuttle work back to the main thread pool.
This diff changes no behavior - it just makes some invariants explicit.
Reviewed By: wez, simpkins
Differential Revision: D6504117
fbshipit-source-id: 3400ad55c00b3719ecba31807fd992442f622cd9
Summary:
Added to Eden capability to incorporate default user and general system level gitignore files.
NOTE: Work in progress, sending the review out to calibrate/ensure I am on right track.
Reviewed By: simpkins
Differential Revision: D6482863
fbshipit-source-id: 9834ca1a577a9599a1f8cb2243dca4e714866be8
Summary:
There's no technical reason to block an open() request until the data
load / materialization returns. This change returns immediately from
open() and then waits if necessary in a subsequent write() call.
Reviewed By: wez
Differential Revision: D6391486
fbshipit-source-id: 862f87e3c3a0d760bacb0f8ca7acc479037fec2f
Summary:
Follow-up to comments in D6466209. All access to the clock goes
through the Clock interface, making time deterministic in unit tests.
Reviewed By: simpkins
Differential Revision: D6477973
fbshipit-source-id: 24e51bdb52d0d079b34d91598d2e787d361f2525
Summary: Follow-up from D6366189. First use of the new FakeClock!
Reviewed By: simpkins
Differential Revision: D6466209
fbshipit-source-id: 4d4d8a9a83df2bee11149e7a0cbddaaf734d0e04
Summary:
Introduce a Clock seam. This will allow us to write tests around
ctime, mtime, and atime logic.
Reviewed By: wez
Differential Revision: D6392543
fbshipit-source-id: 1721d76d2364b135b4ef5c078ef60f7f8526259e
Summary:
open() called materializeInParent unconditionally, and setattr never
called it, implying it was possible to truncate a file without
materializing the parent. This change makes sure to precisely call
materializeInParent whenever the state transitions to materialized.
Reviewed By: wez
Differential Revision: D6389794
fbshipit-source-id: 1e740e133a83d5090a6b9801154b7eaeccb07f22
Summary:
To make the materialization code paths a bit clearer, this decouples
materialization from a blob and truncation. It also caches opened
files if openCount > 0 in both the truncation and materialization from
blob paths.
Reviewed By: wez
Differential Revision: D6388318
fbshipit-source-id: c95a85f5bdaa405130f2f7260143592cdc90d45e
Summary:
Replace the initLoggingGlogStyle() function with a more generic initLogging()
function that accepts a log config string to be parsed with parseLogConfig().
Reviewed By: bolinfest, yfeldblum
Differential Revision: D6342086
fbshipit-source-id: fb1bffd11f190b70e03e2ccbf2b30be08d655242
Summary:
Update TreeInode::computeCheckoutActions() to short circuit on non-materialized
directories that are identical to both the destination and source state. This
speeds up `hg update --clean` operations by allowing us to skip portions of the
tree that do not require changes.
This does not have much effect for update operations without `--clean`: those
already short-circuited the operation in `TreeInode::processCheckoutEntry()`
when the source and destination hashes were the same. That code cannot
short-circuit in the forced update case, since the forced update may have to
revert local modifications.
Reviewed By: bolinfest
Differential Revision: D6455970
fbshipit-source-id: 393acb3272745751a56e06dba0c7505ff2bfad44
Summary:
Previously, we used the Mercurial code `g` when faced with an `UNTRACKED_ADDED`
file conflict, but that was allowing merges to silently succeed that should not
have. This revision changes our logic to use the code `m` for merge, which
unearthed that we were not honoring the user's `update.check` setting properly.
Because we use `update.check=noconflict` internally at Facebook, we changed the
Eden integration tests to default to verifying Hg running with this setting. To
support it properly, we had to port this code from `update.py` in Mercurial to
our own `_determine_actions_for_conflicts()` function:
```
if updatecheck == 'noconflict':
for f, (m, args, msg) in actionbyfile.iteritems():
if m not in ('g', 'k', 'e', 'r', 'pr'):
msg = _("conflicting changes")
hint = _("commit or update --clean to discard changes")
raise error.Abort(msg, hint=hint)
```
However, this introduced an interesting issue where the `checkOutRevision()`
Thrift call from Hg would update the `SNAPSHOT` file on the server, but
`.hg/dirstate` would not get updated with the new parents until the update
completed on the client. With the new call to `raise error.Abort` on the client,
we could get in a state where the `SNAPSHOT` file had the hash of the commit
assuming the update succeeded, but `.hg/dirstate` reflected the reality where it
failed.
To that end, we changed `checkOutRevision()` to take a new parameter,
`checkoutMode`, which can take on one of three values: `NORMAL`, `DRY_RUN`, and
`FORCE`. Now if the user tries to do an ordinary `hg update` with
`update.check=noconflict`, we first do a `DRY_RUN` and examine the potential
conflicts. Only if the conflicts should not block the update do we proceed with
a call to `checkOutRevision()` in `NORMAL` mode.
To make this work, we had to make a number of changes to `CheckoutAction`,
`CheckoutContext`, `EdenMount`, and `TreeInode` to keep track of the
`checkoutMode` and ensure that no changes are made to the working copy when a
`DRY_RUN` is in effect.
One minor issue (for which there is a `TODO`) is that a `DRY_RUN` will not
report any `DIRECTORY_NOT_EMPTY` conflicts that may exist. As `TreeInode` is
implemented today, it is a bit messy to report this type of conflict without
modifying the working copy along the way.
Finally, any `UNTRACKED_ADDED` conflict should cause an update to
abort to match the behavior in stock Mercurial if the user has the following
config setting:
```
[commands]
update.check = noconflict
```
Though the original name for this setting was:
```
[experimental]
updatecheck = noconflict
```
Although I am on Mercurial 4.4.1, the `update.check` setting does not seem to
take effect when I run the integration tests, but the `updatecheck` setting
does, so for now, I set both in `hg_extension_test_base.py` with a `TODO` to
remove `updatecheck` once I can get `update.check` to do its job.
Reviewed By: simpkins
Differential Revision: D6366007
fbshipit-source-id: bb3ecb1270e77d59d7d9e7baa36ada61971bbc49
Summary:
This fixes a crash in EdenMount::destroy() if EdenMount::create() failed to
load the root inode. Previously the code called shutdownImpl() in this case
which tried to unload all inodes and crashed since the root inode was null.
This also fixes EdenMount::destroy() to properly handle the FUSE_ERROR and
FUSE_DONE cases.
Reviewed By: wez
Differential Revision: D6434355
fbshipit-source-id: 39c5f4472d6ebbcf881b4c9c8c8fd67686032ec1
Summary:
The goal is to provide a fast path for watchman to flesh
out the total set of changed files when it needs relay that information
on to consumers.
We choose not to include the full list in the Journal when checking out
between revisions because it will not always be needed and may be an
expensive `O(repo)` operation to compute. This means that watchman
needs to expand that information for itself, and that is currently
a fairly slow query to invoke through mercurial.
Since watchman is responding to journal events from eden we know that
we have tree data for the old and new hashes and thus we should be
able to efficiently compute that diff.
This implementation is slightly awful because it will instantiate an
unlinked TreeInode object for one side of the query, and will in
turn populate any children that differ as it walks down the tree.
A follow on diff will look at making a flavor of the diff code that
can diff raw Tree objects instead.
Reviewed By: bolinfest
Differential Revision: D6305844
fbshipit-source-id: 7506c9ba1f4febebcdc283c414261810a3951588
Summary:
This should have been removed as part of D6179950.
Ideally, Buck would error when this happens, but apparently `glob()` does not
complain when patterns do not match any files, even when the pattern does not
contain any wildcards. There appears to be some code at Facebook that is
exploiting this behavior.
Reviewed By: simpkins
Differential Revision: D6421529
fbshipit-source-id: c6f982624e0e12a911bc12ab1e8239ba4358ea56
Summary:
This corrects a bug where the in-memory timestamp for new files would
be set to the last checkout time instead of when the file was created.
Reviewed By: simpkins
Differential Revision: D6366189
fbshipit-source-id: c5fa8c779726d3a75c2d57b2a161293297eb9271
Summary:
Per a discussion with wez, let's make it explicit in the comments that the kernel handles
O_EXCL for us.
Reviewed By: simpkins
Differential Revision: D6365157
fbshipit-source-id: 638f67cd89a9450ff084e0ef77c731ef738bf518
Summary:
In some workloads we're seeing folks run out of file descriptors.
We forgot that we'd taken out the code that closes the underlying fds.
This diff takes a run at adding a simple counter of the open file handle
objects that is incremented when they are constructed and decremented
when they are destroyed.
When the count falls to zero we release the file handle.
Note that we unconditionally open files when we first load the inodes
from the overlay. I tried to defer that open attempt and it broke
the timestamp overlay test. I think we can revisit that aspect in
a follow on diff; for now we should be more resilient to transiently
opened files from things like ripgrep or similar.
Reviewed By: simpkins
Differential Revision: D6097090
fbshipit-source-id: 9a48220002e760fb1ffb8d7e2a68fa7036558b78
Summary:
This bug is part of a bigger issue in our Mercurial integration where
`UNTRACKED_ADDED` conflicts are being silently swallowed in our Hg extension
whereas stock Mercurial presents these as conflicts and forces the user to deal
with them. The Mercurial issues will be addressed in a follow-up change.
Reviewed By: simpkins
Differential Revision: D6365580
fbshipit-source-id: 831e27ce1da90ea605033b2b9988fe400ba404aa
Summary:
In preparation for bringing D6097090 across the finish line, we need a
more explicit mechanism for determining which state the inode is in.
The issue is that whether an inode is materialized was checked in a
couple ways: the nonexistence of the hash field or the definedness of
the file field. This diff introduces an explicit enum indicating the
state.
Reviewed By: simpkins
Differential Revision: D6325955
fbshipit-source-id: 3682a4ebc9330193baadbb33a4dd9845f26e59a6
Summary:
Eden attempts to initialize timestamps of newly loaded inodes with the time of
the last checkout operation performed in this mount. Unfortunately it had a
number of bugs in this logic:
EdenMount had two separate fields attempting to track the last checkout time:
`lastCheckoutTime_` and `parentInfo_.lastCheckoutTime`.
Unfortunately neither field was actually updated on checkout operations.
Additionally, `lastCheckoutTime_` did not have any locking to allow it to be
updated. `parentInfo_.lastCheckoutTime` did have locking, but it used the
mount point's checkout lock, so it could not be accessed during checkout
operations.
This diff removes `parentInfo_.lastCheckoutTime`, keeping only
`lastCheckoutTime_`. It also converts `lastCheckoutTime_` to a
`struct timespec` since this is most often needed as a `timespec`. It also
adds a new mountpoint-wide lock for synchronizing accessing to this variable.
Reviewed By: bolinfest
Differential Revision: D6356698
fbshipit-source-id: db54f9bb297b5febe4642e2b3fcc8055a6afc199
Summary: More work towards encapsulating a FileInode's internal state machine.
Reviewed By: wez
Differential Revision: D6316013
fbshipit-source-id: 9c8303b35a0de1ba69207c7f59be88c5fb037ad8
Summary:
This is preparation for a change I'm preparing that makes the state
transitions in FileInode clearer and safer.
Reviewed By: wez
Differential Revision: D6304272
fbshipit-source-id: 493ee80517443432f790abf9806000eecb03651c
Summary:
The gtest macros in this file were moved to folly/test/TestUtils.h
Update everything to just use folly/test/TestUtils.h directly.
Reviewed By: chadaustin
Differential Revision: D6301759
fbshipit-source-id: 7f2841c12d5bea15376f782fb3bf3bfef16039c7
Summary:
Moving the post-clone step out of C++ makes it so that
`ClientConfig` in C++ no longer knows about `hooks` and requires
`RepoConfig` to know about `hooks`. This helps us reduce
`ClientConfig`'s dependency on `ConfigData`, as my next step
is to move the remaining information that `ClientConfig` gets from
`ConfigData` directly into the client's `edenrc` file under
`~/local/.eden`.
Reviewed By: chadaustin
Differential Revision: D6310544
fbshipit-source-id: dec7a21281ab49e0416b8872757970a4eff2d943
Summary:
I created this change by running:
```
find eden -name TARGETS | grep -v eden/fs/fuse/TARGETS | grep -v eden/fs/service/TARGETS | xargs autodeps
```
apparently `eden/fs/fuse/TARGETS` and `eden/fs/service/TARGETS` have some
constructions that `autodeps` does not understand, so I filtered those out.
Reviewed By: StanislavGlebik
Differential Revision: D6319982
fbshipit-source-id: 7b3683d1507409dde6d6570e9b13811168aa6859
Summary:
This is follow-up to the lock ordering issues in
StreamingSubscriber. The Journal locks are now finer-grained and no
locks are held while the subscribers are invoked. This change
prevents future deadlocks.
Reviewed By: wez
Differential Revision: D6281410
fbshipit-source-id: 797c164395831752f61cc15928b6d4ce4dab1b68
Summary:
With the subsequent diff that enables multiple concurrent
hg importers, I was seeing this deadlock during rebase; each of
the worker threads was being blocked until it saturated the various
thread pools and locked up the server.
This removes the blocking call and replaces it with a SharedPromise
to allow multiple callers to wait for the load.
Note that this changes the semantics of ensureDataLoaded slightly:
previously the blob load was synchronous while the write lock was
held. Now we release the write lock until the load completes.
I've added some post-condition checks to validate that we don't
break any state. I believe that our usage is such that we don't
do anything that might mess things up. Am I missing anything?
Reviewed By: simpkins
Differential Revision: D6264900
fbshipit-source-id: 4aa2870d95f0f0ec48d87299978cb87af99e3969
Summary:
Address some feedback on D6264900 by only storing the sha-1 in the
overlay when it's immediately available. This avoids the possibility
of the FUSE thread getting blocked.
Reviewed By: simpkins
Differential Revision: D6292175
fbshipit-source-id: 06c2372724e58c485d9a302dde36c34885109acf
Summary:
The underlying issue is that we were reporting a `MODIFIED_REMOVED`
conflict as a `MODIFIED_MODIFIED` conflict. This put us in a state where
Mercurial expected to find a file in the new manifest, but failed because the
file was not present in that revision, so no such file could be found.
Somewhat surprisingly, the appropriate handler for a `MODIFIED_REMOVED`
conflict already existed in our Mercurial extension, but there was no logic on
the server that would generate a `MODIFIED_REMOVED` conflict previous to
this change.
Like D6204916, this was an issue I ran into when trying to create a repro case
for the issue that was fixed in D6199215.
Reviewed By: wez
Differential Revision: D6270272
fbshipit-source-id: 6604eea00b0794cd44b01d2ba6b9ea10db32d556
Summary: Make this function match our C++ guidelines.
Reviewed By: wez
Differential Revision: D6288591
fbshipit-source-id: 1a4f52a8c1e0497df938533fe29da10264eb1ccf
Summary:
Add log messages that will help debug the behavior of `TreeInode::diff()`
I added this since I saw a case where `getScmStatus()` was returning a file as
modified when it did not appear to have mode or contents changes.
Unfortunately we did not have any existing debug log messages for this, and the
problem went away after restarting edenfs, so I wasn't able to track it down.
Having these log messages will help debug this in the future if we run into it
again.
Reviewed By: bolinfest
Differential Revision: D6281905
fbshipit-source-id: 736028d6efe46a387bed6bf98d971685f83da3ec
Summary: The comments and the code did not agree on what fields were valid when, so I corrected them and added runtime enforcement.
Reviewed By: wez
Differential Revision: D6260555
fbshipit-source-id: 13e35e22c44b9a5b4cb30cdfd48e2bf7b0e116d3
Summary:
This is a major change to how we manage the dirstate in Eden's Hg extension.
Previously, the dirstate information was stored under `$EDEN_CONFIG_DIR`,
which is Eden's private storage. Any time the Mercurial extension wanted to
read or write the dirstate, it had to make a Thrift request to Eden to do so on
its behalf. The upside is that Eden could answer dirstate-related questions
independently of the Python code.
This was sufficiently different than how Mercurial's default dirstate worked
that our subclass, `eden_dirstate`, had to override quite a bit of behavior.
Failing to manage the `.hg/dirstate` file in a way similar to the way Mercurial
does has exposed some "unofficial contracts" that Mercurial has. For example,
tools like Nuclide rely on changes to the `.hg/dirstate` file as a heuristic to
determine when to invalidate its internal caches for Mercurial data.
Today, Mercurial has a well-factored `dirstatemap` abstraction that is primarily
responsible for the transactions with the dirstate's data. With this split, we can
focus on putting most of our customizations in our `eden_dirstate_map` subclass
while our `eden_dirstate` class has to override fewer methods. Because the
data is managed through the `.hg/dirstate` file, transaction logic in Mercurial that
relies on renaming/copying that file will work out-of-the-box. This change
also reduces the number of Thrift calls the Mercurial extension has to make
for operations like `hg status` or `hg add`.
In this revision, we introduce our own binary format for the `.hg/dirstate` file.
The logic to read and write this file is in `eden/py/dirstate.py`. After the first
40 bytes, which are used for the parent hashes, the next four bytes are
reserved for a version number for the file format so we can manage file format
changes going forward.
Admittedly one downside of this change is that it is a breaking change.
Ideally, users should commit all of their local changes in their existing mounts,
shutdown Eden, delete the old mounts, restart Eden, and re-clone.
In the end, this change deletes a number of Mercurial-specific code and Thrift
APIs from Eden. This is a better separation of concerns that makes Eden more
SCM-agnostic. For example, this change removes `Dirstate.cpp` and
`DirstatePersistance.cpp`, replacing them with the much simpler and more
general `Differ.cpp`. The Mercurial-specific logic from `Dirstate.cpp` that turned
a diff into an `hg status` now lives in the Mercurial extension in
`EdenThriftClient.getStatus()`, which is much more appropriate.
Note that this reverts the changes that were recently introduced in D6116105:
we now need to intercept `localrepo.localrepository.dirstate` once again.
Reviewed By: simpkins
Differential Revision: D6179950
fbshipit-source-id: 5b78904909b669c9cc606e2fe1fd118ef6eaab95
Summary: Continue moving the asynchrony of loading closer to the state access so, in a future world, we still behave if unloading occurs during access.
Reviewed By: simpkins
Differential Revision: D6238523
fbshipit-source-id: 722fe5bba90b55204d50393314d225f42680409b
Summary: To fix up the invariants in FileInode's API, we intend to remove ensureDataLoaded() and materializeForWrite(). But first I'm going to see if there is any pain caused by removing their calls.
Reviewed By: simpkins
Differential Revision: D6234049
fbshipit-source-id: c39c6d018d164b32903414d3750b875a897af210
Summary:
Per discussion with bolinfest, this brings Eden in line with clang-format.
This diff was generated with `find . \( -iname '*.cpp' -o -iname '*.h' \) -exec bash -c "yes | arc lint {}" \;`
Reviewed By: bolinfest
Differential Revision: D6232695
fbshipit-source-id: d54942bf1c69b5b0dcd4df629f1f2d5538c9e28c
Summary: If writing out the symlink file in the overlay fails, clean up the file before releasing the lock.
Reviewed By: wez
Differential Revision: D6234214
fbshipit-source-id: 91ccf917a26cc1a73c3963dea7dd87364857fa03
Summary:
Most functions in TreeInode that call Journal::addDelta() release the
TreeInode::contents_ lock before locking the journal. However, `doRename()`
locked the journal while still holding the contents_ locks for the directories
affected by the rename.
This updates `doRename()` to make sure we release the TreeInode locks properly
before attempting to update the journal. This should help ensure that we don't
have any lock ordering issues here.
Reviewed By: bolinfest
Differential Revision: D6212075
fbshipit-source-id: 84503acf47e9d97b81075932326825832cef9514
Summary:
For a while, the recommended approach (e.g. from folks on the thrift team, from folks on the javafoundations team) has been to use `languages = ["java-swift"]`. This renaming brings the code in line with the recommendation. There are no actual changes to any builds, this is just a rename.
This is the second diff in a small stack. It's a fully automated change that uses buildozer to rename `*-java` rules to `*-javadeprecated`.
Differential Revision: D6189950
fbshipit-source-id: a08ba86cb92293a07eceb4b6b29385c7bf647b72
Summary:
Now that we make use of the `.hg/dirstate` file, we should ensure that it is
initialized correctly. This takes the commit hash from the `SNAPSHOT` file and
passes it as an argument to the `post-clone` hook. In the Hg `post-clone` hook,
it writes the `.hg/dirstate` file such that the first 20 bytes are the binary
version of the commit hash and the next 20 bytes are nul bytes.
Reviewed By: simpkins
Differential Revision: D6174440
fbshipit-source-id: 29d17f88ca3f63fa6490e1db88bc2121aced74d6
Summary:
We have a couple of issues with the watchman/eden integration:
1. If you "win" the race, you can cause a segfault during shutdown
by changing files during unmount. This causes the journal updating
code to trigger a send to the client, but the associated eventBase
has already been destroyed.
2. We don't proactively send any signal to the subscriber (in practice: watchman)
when we unmount. Watchman detects the eden shutdown by noticing that its
socket has closed but has no way to detect an unmount.
This diff tries to connect the unmount with the set of subscribers and tries
to cause the thrift socket to close out.
Reviewed By: bolinfest
Differential Revision: D6162717
fbshipit-source-id: 42d4a005089cd9cddf204997b1643570488f04c3
Summary:
Previously, the `savebackup()` and `restorebackup()` methods in `eden_dirstate`
only retained the parent commit hashes. With this change, now the dirstate tuples
and entries in the copymap for the dirstate are also included as part of the saved
state.
Failing to restore all of the state caused issues when doing things like aborting
an `hg split`, as observed by one of our users. Although this fix works, we ultimately
plan to move the responsibility for persisting dirstate data out of Eden and into the
Hg extension. Then the data will live in `.hg/dirstate` like it would for the default
dirstate implementation.
Reviewed By: simpkins
Differential Revision: D6145420
fbshipit-source-id: baa077dee73847a47cc171cd980cdd272b3a3a99
Summary:
When performing an source control checkout operation, we attempt to remove
directories that are empty after the checkout. However, this code path was
missing a call to flush the kernel cache for these directories.
As a result, even though eden thought the directory not longer existed, and
would not report it in `readdir()` results, the kernel would return stale
information from its cache when explicitly accessing this path.
Reviewed By: bolinfest
Differential Revision: D6151543
fbshipit-source-id: 6031feb268ff6f980c885efb26c3d43243dec3f4
Summary:
Add a few extra debug logs to record `processCheckoutEntry()` and
`saveOverlayPostCheckout()` calls.
Reviewed By: bolinfest
Differential Revision: D6151544
fbshipit-source-id: ca6faa8fd1fe53df1e70305f5527360c918841d1
