Skip to content

Commit

Permalink
Filesystem store now delays before deleting temp file
Browse files Browse the repository at this point in the history
There was a rare condition that could happen where the store would
atomically swap the temp file and the final location then delete
the temp file immediately after. Normally this would be fine, however
during a hardlink, the interal kernel command first queries the inode
then creates a hardlink for the queried inode. If the file was not
swapped yet and the inode was queried, it'll get the inode of the
file about to be deleted, then if the file is deleted then the
hardlink will use an inode that doesn't exist any more.

This is solved by adding a 100ms delay. In testing 1ms would be
enough on my 16 core testbench, but to give plenty of leeway we add
a substantial delay.

This is also incredibly hard to write a test for, since it was a
flake condition and I don't have control over the innards of the
`hardlink` command, so this is one of the rare conditions there is
not a unit test for a bug, but it is documented now.
  • Loading branch information
allada committed May 5, 2022
1 parent 2546a77 commit 33d88c5
Showing 1 changed file with 16 additions and 5 deletions.
21 changes: 16 additions & 5 deletions cas/store/filesystem_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use std::path::Path;
use std::pin::Pin;
use std::sync::atomic::{AtomicU64, Ordering};
use std::sync::Arc;
use std::time::SystemTime;
use std::time::{Duration, SystemTime};

use async_trait::async_trait;
use bytes::BytesMut;
Expand All @@ -15,6 +15,7 @@ use nix::fcntl::{renameat2, RenameFlags};
use rand::{thread_rng, Rng};
use tokio::io::{AsyncReadExt, AsyncSeekExt, AsyncWriteExt, SeekFrom, Take};
use tokio::task::spawn_blocking;
use tokio::time::sleep;
use tokio_stream::wrappers::ReadDirStream;

use buf_channel::{DropCloserReadHalf, DropCloserWriteHalf};
Expand Down Expand Up @@ -348,7 +349,7 @@ impl FilesystemStore {

temp_file
.as_ref()
.sync_data()
.sync_all()
.await
.err_tip(|| format!("Failed to sync_data in filesystem store {}", temp_loc))?;

Expand Down Expand Up @@ -395,9 +396,19 @@ impl FilesystemStore {
.err_tip(|| "This could be due to the filesystem not supporting RENAME_EXCHANGE")?;

if let Some(old_item) = self.evicting_map.insert(digest, entry).await {
// At this point `temp_name_num` will be the file containing the old content we
// are going to delete.
old_item.flag_moved_to_temp_file(temp_name_num);
// Even though the move is atomic above, it is possible that during a hardlink operation
// the action will find the inode (which could be either the new or old file at this
// point), then create the hardlink with the queried inode. But if the file gets deleted
// after it grabs the inode but before the hardlink it could result in a NotFound error.
// By putting a delay before deleting the temp files we give some leeway so this error
// will be extremely rare.
tokio::spawn(async move {
const DELAY_TO_DELETE_TEMP_FILES: u64 = 100;
sleep(Duration::from_millis(DELAY_TO_DELETE_TEMP_FILES)).await;
// At this point `temp_name_num` will be the file containing the old content we
// are going to delete.
old_item.flag_moved_to_temp_file(temp_name_num);
});
}
Ok(())
}
Expand Down

0 comments on commit 33d88c5

Please sign in to comment.