This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1654

treedirstate: ensure appended data is flushed to disk
ClosedPublic

Authored by mbthomas on Dec 11 2017, 10:12 AM.
Tags
None
Subscribers
jsgf

Details

Reviewers
ikostia
Group Reviewers
Restricted Project
Commits
rFBHGX37bccca1426f: treedirstate: ensure appended data is flushed to disk
Summary

A dirstate update that appends data to the treedirstate tree file, followed by
a hard reboot before the filesystem cache is flushed, can result in a dirstate
tree root that is referred to by the dirstate file, but does not contain the
correct data. Ensure the appended data is synced to disk before returning from
Store.flush().

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mbthomas created this revision.Dec 11 2017, 10:12 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 11 2017, 10:12 AM
ikostia accepted this revision.Dec 11 2017, 11:29 AM
This revision is now accepted and ready to land.Dec 11 2017, 11:29 AM
Closed by commit rFBHGX37bccca1426f: treedirstate: ensure appended data is flushed to disk (authored by mbthomas). · Explain WhyDec 12 2017, 7:25 PM
This revision was automatically updated to reflect the committed changes.
jsgf added a subscriber: jsgf.Dec 15 2017, 2:18 PM
jsgf added inline comments.
rust/treedirstate/src/filestore.rs
157

This should probably be sync_all(), since you do want to sync metadata as well (the file size). There's no guarantee that sync_data() will also sync the file size change, meaning that your data is safely written but inaccessible after the logical end of the file. (It is intended for overwrite cases where you want to write into the middle of a file and don't care whether the mtime update is durable.)

Revision Contents
Changeset List

PathPackages
Mrust/treedirstate/src/filestore.rs (4 lines)

Diff 4406

rust/treedirstate/src/filestore.rs

file.write_u32::<BigEndian>(data.len() as u32)?; file.write_u32::<BigEndian>(data.len() as u32)?;
file.write_all(data)?; file.write_all(data)?;
self.position += 4 + data.len() as u64; self.position += 4 + data.len() as u64;
debug_assert!(self.position == file.seek(SeekFrom::End(0))?); debug_assert!(self.position == file.seek(SeekFrom::End(0))?);
Ok(id) Ok(id)
} }
fn flush(&mut self) -> Result<()> { fn flush(&mut self) -> Result<()> {
self.file.get_mut().flush()?; let file = self.file.get_mut();
file.flush()?;
file.get_mut().sync_data()?;
jsgfUnsubmitted
Not Done

This should probably be sync_all(), since you do want to sync metadata as well (the file size). There's no guarantee that sync_data() will also sync the file size change, meaning that your data is safely written but inaccessible after the logical end of the file. (It is intended for overwrite cases where you want to write into the middle of a file and don't care whether the mtime update is durable.)

jsgf: This should probably be `sync_all()`, since you do want to sync metadata as well (the file…
Ok(()) Ok(())
} }
} }
impl StoreView for FileStore { impl StoreView for FileStore {
fn read<'a>(&'a self, id: BlockId) -> Result<Cow<'a, [u8]>> { fn read<'a>(&'a self, id: BlockId) -> Result<Cow<'a, [u8]>> {
// Check the ID is in range. // Check the ID is in range.
if id.0 < HEADER_LEN || id.0 > self.position - 4 { if id.0 < HEADER_LEN || id.0 > self.position - 4 {