Skip to content

Commit eefe49d

Browse files
authored
Unrolled build for #147821
Rollup merge of #147821 - iximeow:ixi/session-gc-vs-flock, r=nnethercote Do not GC the current active incremental session directory when building a relatively large repo (https://github.com/oxidecomputer/omicron) on illumos under heavy CPU pressure, i saw some rustc invocations die like: ``` [..]/target/debug/incremental/<crate>-<hash>/<name>/dep-graph.part.bin: No such file or directory (os error 2) ``` a bit of debugging later and it seems that if the system is very slow, Unix-flavored `flock::Lock::new()` doesn't quite get the mutual exclusion `garbage_collect_session_directories` expects. before this patch i could reproduce this with the crate `nexus_db_queries` (in that repo) by pinning the full `cargo build` to one core and having a busy loop fighting on that same core. with this patch i cannot reproduce the issue. i took a look at how `flock::Lock` is used and i think this is the only problematic use, so i figure i'll propose this change particularly since i don't think file locking can be made.. good... for Unix in general. ------ In `setup_dep_graph`, we set up a session directory for the current incremental compilation session, load the dep graph, and then GC stale incremental compilation sessions for the crate. The freshly-created session directory ends up in this list of potentially-GC'd directories but in practice is not typically even considered for GC because the new directory is neither finalized nor `is_old_enough_to_be_collected`. Unfortunately, `is_old_enough_to_be_collected` is a simple time check, and if `load_dep_graph` is slow enough it's possible for the freshly-created session directory to be tens of seconds old already. Then, old enough to be *eligible* to GC, we try to `flock::Lock` it as proof it is not owned by anyone else, and so is a stale working directory. Because we hold the lock in the same process, the behavior of `flock::Lock` is dependent on platform-specifics about file locking APIs. `fcntl(F_SETLK)`-style locks used on non-Linux Unices do not provide mutual exclusion internal to a process. `fcntl_locking(2)` on Linux describes some relevant problems: ``` The record locks described above are associated with the process (unlike the open file description locks described below). This has some unfortunate consequences: * If a process closes any file descriptor referring to a file, then all of the process's locks on that file are released, [...] * The threads in a process share locks. In other words, a multithreaded program can't use record locking to ensure that threads don't simultaneously access the same region of a file. ``` `fcntl`-locks will appear to succeed to lock the fresh incremental compilation directory, at which point we can remove it just before using it later for incremental compilation. Saving incremental compilation state later fails and takes rustc with it with an error like ``` [..]/target/debug/incremental/crate-<hash>/<name>/dep-graph.part.bin: No such file or directory (os error 2) ``` The release-lock-on-close behavior has uncomfortable consequences for the freshly-opened file description for the lock, but I think in practice isn't an issue. If we would close the file, we failed to acquire the lock, so someone else had the lock ad we're not releasing locks prematurely. `flock(LOCK_EX)` doesn't seem to have these same issues, and because `flock::Lock::new` always opens a new file description when locking, I don't think Linux can have this issue. From reading `LockFileEx` on MSDN I *think* Windows has locking semantics similar to `flock`, but I haven't tested there at all. My conclusion is that there is no way to write a pure-POSIX `flock::Lock::new` which guarantees mutual exclusion across different file descriptions of the same file in the same process, and `flock::Lock::new` must not be used for that purpose. So, instead, avoid considering the current incremental session directory for GC in the first place. Our own `sess` is evidence we're alive and using it.
2 parents c0c37ca + 4e816d8 commit eefe49d

File tree

1 file changed

+26
-0
lines changed
  • compiler/rustc_incremental/src/persist

1 file changed

+26
-0
lines changed

compiler/rustc_incremental/src/persist/fs.rs

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -721,11 +721,37 @@ pub(crate) fn garbage_collect_session_directories(sess: &Session) -> io::Result<
721721
}
722722
}
723723

724+
let current_session_directory_name =
725+
session_directory.file_name().expect("session directory is not `..`");
726+
724727
// Now garbage collect the valid session directories.
725728
let deletion_candidates =
726729
lock_file_to_session_dir.items().filter_map(|(lock_file_name, directory_name)| {
727730
debug!("garbage_collect_session_directories() - inspecting: {}", directory_name);
728731

732+
if directory_name.as_str() == current_session_directory_name {
733+
// Skipping our own directory is, unfortunately, important for correctness.
734+
//
735+
// To summarize #147821: we will try to lock directories before deciding they can be
736+
// garbage collected, but the ability of `flock::Lock` to detect a lock held *by the
737+
// same process* varies across file locking APIs. Then, if our own session directory
738+
// has become old enough to be eligible for GC, we are beholden to platform-specific
739+
// details about detecting the our own lock on the session directory.
740+
//
741+
// POSIX `fcntl(F_SETLK)`-style file locks are maintained across a process. On
742+
// systems where this is the mechanism for `flock::Lock`, there is no way to
743+
// discover if an `flock::Lock` has been created in the same process on the same
744+
// file. Attempting to set a lock on the lockfile again will succeed, even if the
745+
// lock was set by another thread, on another file descriptor. Then we would
746+
// garbage collect our own live directory, unable to tell it was locked perhaps by
747+
// this same thread.
748+
//
749+
// It's not clear that `flock::Lock` can be fixed for this in general, and our own
750+
// incremental session directory is the only one which this process may own, so skip
751+
// it here and avoid the problem. We know it's not garbage anyway: we're using it.
752+
return None;
753+
}
754+
729755
let Ok(timestamp) = extract_timestamp_from_session_dir(directory_name) else {
730756
debug!(
731757
"found session-dir with malformed timestamp: {}",

0 commit comments

Comments
 (0)