Skip to content

Commit

Permalink
Return invalidation list of inodes after checkout on NFS
Browse files Browse the repository at this point in the history
Summary:
## Context
S439820 is an ongoing SEV that has been challenging to reproduce. Initially, we had some repros, but they suddenly disappeared. We suspect that NFS cache invalidation might be causing stale files on macOS. There is no direct way to invalidate cache in NFS. Therefore, we have to use some hacky ways to invalidate cache on macOS see this diff summary for detailed explanation D35435764

## This Diff Stack
This diff stack aims to implement an invalidation check method to verify the invalidation after each checkout on macOS. This will help us detect how many users are affected by the SEV and confirm whether our NFS cache invalidation approach is working correctly.
Also, if we find a large number of users are affected by the SEV, and we try any fix for macOS invalidation this check can confirm if any fix works in prod.

## This diff
Finaly `CheckoutContext::finish()` returns the invalidation list along with the checkout conflicts in a pair to process in the next diff.

Reviewed By: jdelliot, genevievehelsel

Differential Revision: D68631239

fbshipit-source-id: 0d17e7af8b3e202d744dabb7eaa3e2b4d03df84d
  • Loading branch information
kavehahmadi60 authored and facebook-github-bot committed Jan 30, 2025
1 parent 1b8fd55 commit ee57405
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 11 deletions.
12 changes: 9 additions & 3 deletions eden/fs/inodes/CheckoutContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,8 @@ void CheckoutContext::start(
}
}

ImmediateFuture<vector<CheckoutConflict>> CheckoutContext::finish(
RootId newSnapshot) {
ImmediateFuture<CheckoutContext::CheckoutConflictsAndInvalidations>
CheckoutContext::finish(const RootId& newSnapshot) {
auto config = mount_->getCheckoutConfig();

auto parentCommit = config->getParentCommit();
Expand All @@ -100,7 +100,13 @@ ImmediateFuture<vector<CheckoutConflict>> CheckoutContext::finish(
// This allows any filesystem unlink() or rename() operations to proceed.
renameLock_.unlock();

return flush();
return flush().thenValue(
[invalidations = extractFilesToVerfy()](auto&& conflicts) mutable {
CheckoutConflictsAndInvalidations result;
result.conflicts = std::move(conflicts);
result.invalidations = std::move(invalidations);
return result;
});
}

ImmediateFuture<vector<CheckoutConflict>> CheckoutContext::flush() {
Expand Down
22 changes: 16 additions & 6 deletions eden/fs/inodes/CheckoutContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,16 @@ class CheckoutContext {

~CheckoutContext();

/**
* The list of conflicts that were encountered as well as some sample paths
* that were invalidated during the checkout.
* TODO: The invalidated sample paths are used for S439820. It can be deleted
* when the SEV closed*/
struct CheckoutConflictsAndInvalidations {
std::vector<CheckoutConflict> conflicts;
std::vector<InodeNumber> invalidations;
};

/**
* Returns true if the checkout operation should do a dry run, looking for
* conflicts without actually updating the inode contents. If it returns
Expand Down Expand Up @@ -96,13 +106,13 @@ class CheckoutContext {
/**
* Complete the checkout operation
*
* Returns the list of conflicts and errors that were encountered during the
* operation as well as some sample paths that were invalidated during the
* checkout that can be used to validate that invalidation is working
* correctly.
TODO: The invalidated sample paths are used for S439820.
* Returns the list of conflicts and errors that were encountered as well as
some sample paths that were invalidated during the checkout.
TODO: The invalidations can be used to validate that NFS invalidation is
working correctly.The invalidated sample paths are used for S439820.
*/
ImmediateFuture<std::vector<CheckoutConflict>> finish(RootId newSnapshot);
ImmediateFuture<CheckoutConflictsAndInvalidations> finish(
const RootId& newSnapshot);

/**
* Flush the invalidation if needed.
Expand Down
6 changes: 4 additions & 2 deletions eden/fs/inodes/EdenMount.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1598,12 +1598,14 @@ ImmediateFuture<CheckoutResult> EdenMount::checkout(
stopWatch,
oldParent,
snapshotHash,
journalDiffCallback](std::vector<CheckoutConflict>&& conflicts) {
journalDiffCallback](
CheckoutContext::CheckoutConflictsAndInvalidations&& conflicts) {
checkoutTimes->didFinish = stopWatch.elapsed();

CheckoutResult result;
result.times = *checkoutTimes;
result.conflicts = std::move(conflicts);
result.conflicts = std::move(conflicts.conflicts);
result.sampleInodesToValidate = std::move(conflicts.invalidations);
if (ctx->isDryRun()) {
// This is a dry run, so all we need to do is tell the caller
// about the conflicts: we should not modify any files or add
Expand Down
1 change: 1 addition & 0 deletions eden/fs/inodes/EdenMount.h
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ struct SetPathObjectIdTimes {
struct CheckoutResult {
std::vector<CheckoutConflict> conflicts;
CheckoutTimes times;
std::vector<InodeNumber> sampleInodesToValidate;
};

struct SetPathObjectIdResultAndTimes {
Expand Down

0 comments on commit ee57405

Please sign in to comment.