This is an archive of the discontinued Mercurial Phabricator instance.

Differential D12580

rust-dirstate-v2: save proper data size if no new data on append
ClosedPublic

Authored by Alphare on Apr 21 2022, 9:40 AM.

Details

Reviewers
aalekseyev
martinvonz
Group Reviewers
hg-reviewers
Commits
rHGdd2503a63d33: rust-dirstate-v2: save proper data size if no new data on append
Summary

This is currently only triggered with the tests ran with --rhg without
--rust, by "luck", there probably always was something to write, like an
mtime when also using Rust extensions alongside rhg.

Diff Detail

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

Event Timeline

Alphare created this revision.Apr 21 2022, 9:40 AM
Alphare added a comment.Apr 21 2022, 9:42 AM

Note: the merge into default will require some small care (basically default has more unit tests, and we need to add an unused old_data_size). I can do it.

aalekseyev added a subscriber: aalekseyev.Apr 21 2022, 9:54 AM

I don't understand the justification: if it's not correct to store the file length when we're appending empty data, why is it correct to store it when data is not empty? Can't the garbage still be there?

Alphare added a comment.Apr 21 2022, 9:55 AM
In D12580#192697, @aalekseyev wrote:

I don't understand the justification: if it's not correct to store the file length when we're appending empty data, why is it correct to store it when data is not empty? Can't the garbage still be there?

It's never correct to store the file length, the comment just means that if we're appending, we return the position of the last write, which will always be the root node. If we're not appending, we don't write anything, the pointer is always 0, and poof.

aalekseyev added a comment.Apr 21 2022, 11:11 AM

Ok, I get it. You're saying size 0 is special because a write of size 0 does not update the current file pointer.
I did not know exactly how append mode interacted with lseek, so I did an experiment.
The following C program always prints "hi2" for me, even if redirected to a file in append mode.
I think this means that even though write in append mode always appends to the end of the file, it doesn't actually advance the file pointer to that position, so the returned value is effectively the amount you wrote, not the total size of the file.
(I did not try the same in rust)

#include <sys/types.h>                                                                                                         
#include <unistd.h>                                                                                                            
#include <stdio.h>                                                                                                             
int main() {                                                                                                                   
  write(1, "hi", 2);                                                                                                           
  off_t pos = lseek(1, 0, SEEK_CUR);                                                                                           
  printf("%ld\n", pos);                                                                                                        
  return 0;                                                                                                                    
}
aalekseyev added a comment.Apr 21 2022, 11:21 AM

Yep, same in rust:

use std::io::Seek;
use std::io::SeekFrom;
use std::io::Write as IoWrite;

fn main() -> Result<(), std::io::Error> {
  let mut options = std::fs::OpenOptions::new();
  options.append(true);
  let mut file = options.open("x")?;
  file.write_all(b"foobar")?;
  file.flush()?;
  println!("{}", file.seek(SeekFrom::Current(0))?);
  Ok(())
}

always prints 6.

So I'm guessing this doesn't work at all?

aalekseyev added a comment.Apr 21 2022, 11:31 AM

Wow, this is actually different on different filesystems: the behavior I described happens on ZFS, while a presumably-desirable behavior (different offset every time) happens on EXT4 and XFS. This is crazy!

aalekseyev added a comment.Apr 21 2022, 11:40 AM

Relevant issue: https://github.com/prometheus/prometheus/issues/484.
Presumably hg has reasonable expectations here, the bug is in ZFS. However this does mean that we in Jane Street will experience issues since we don't seem to have a patched ZFS.
This also means that this code path is untested, yes? (Since I'm not seeing any errors) Do you know how to reproduce the append behavior so we have a test that breaks on buggy ZFS?

aalekseyev accepted this revision.Apr 21 2022, 1:55 PM

ZFS bug and lack of tests notwithstanding, the change seems good to me.

martinvonz requested changes to this revision.Apr 21 2022, 3:50 PM
martinvonz added a subscriber: martinvonz.

It looks like the tests from your other long series need to be updated in this patch (they don't compile with this patch applied).

This revision now requires changes to proceed.Apr 21 2022, 3:50 PM
Alphare added a comment.Apr 22 2022, 10:25 AM
In D12580#192721, @martinvonz wrote:

It looks like the tests from your other long series need to be updated in this patch (they don't compile with this patch applied).

You may have missed my first comment here then. This patch is destined for stable where it should apply cleanly, only the merge will need to be adapted .

In D12580#192702, @aalekseyev wrote:

Relevant issue: https://github.com/prometheus/prometheus/issues/484.
Presumably hg has reasonable expectations here, the bug is in ZFS. However this does mean that we in Jane Street will experience issues since we don't seem to have a patched ZFS.
This also means that this code path is untested, yes? (Since I'm not seeing any errors) Do you know how to reproduce the append behavior so we have a test that breaks on buggy ZFS?

This is very interesting, thanks for the investigation. It is indeed untested, I can follow up with a test soon (but not this week).

martinvonz added a comment.Apr 22 2022, 11:46 AM
In D12580#192736, @Alphare wrote:
In D12580#192721, @martinvonz wrote:

It looks like the tests from your other long series need to be updated in this patch (they don't compile with this patch applied).

You may have missed my first comment here then. This patch is destined for stable where it should apply cleanly, only the merge will need to be adapted .

Ah, you're right that I missed that (well, I forgot about it). Sorry.

In D12580#192702, @aalekseyev wrote:

Relevant issue: https://github.com/prometheus/prometheus/issues/484.
Presumably hg has reasonable expectations here, the bug is in ZFS. However this does mean that we in Jane Street will experience issues since we don't seem to have a patched ZFS.
This also means that this code path is untested, yes? (Since I'm not seeing any errors) Do you know how to reproduce the append behavior so we have a test that breaks on buggy ZFS?

This is very interesting, thanks for the investigation. It is indeed untested, I can follow up with a test soon (but not this week).

Since @aalekseyev seemed happy with the patch in it's current form, I suppose I can queue it now and you add those tests in a separate patch. Thanks.

Alphare added a commit: rHGdd2503a63d33: rust-dirstate-v2: save proper data size if no new data on append.Apr 22 2022, 11:53 AM
This revision was not accepted when it landed; it landed in state Needs Revision.
Closed by commit rHGdd2503a63d33: rust-dirstate-v2: save proper data size if no new data on append (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 33305

rust/hg-core/src/dirstate_tree/dirstate_map.rs

/// `.copy_source.is_some()`. /// `.copy_source.is_some()`.
pub(super) nodes_with_copy_source_count: u32, pub(super) nodes_with_copy_source_count: u32,
/// See on_disk::Header /// See on_disk::Header
pub(super) ignore_patterns_hash: on_disk::IgnorePatternsHash, pub(super) ignore_patterns_hash: on_disk::IgnorePatternsHash,
/// How many bytes of `on_disk` are not used anymore /// How many bytes of `on_disk` are not used anymore
pub(super) unreachable_bytes: u32, pub(super) unreachable_bytes: u32,
/// Size of the data used to first load this `DirstateMap`. Used in case
/// we need to write some new metadata, but no new data on disk.
pub(super) old_data_size: usize,
} }
/// Using a plain `HgPathBuf` of the full path from the repository root as a /// Using a plain `HgPathBuf` of the full path from the repository root as a
/// map key would also work: all paths in a given map have the same parent /// map key would also work: all paths in a given map have the same parent
/// path, so comparing full paths gives the same result as comparing base /// path, so comparing full paths gives the same result as comparing base
/// names. However `HashMap` would waste time always re-hashing the same /// names. However `HashMap` would waste time always re-hashing the same
/// string prefix. /// string prefix.
pub(super) type NodeKey<'on_disk> = WithBasename<Cow<'on_disk, HgPath>>; pub(super) type NodeKey<'on_disk> = WithBasename<Cow<'on_disk, HgPath>>;
pub(super) fn empty(on_disk: &'on_disk [u8]) -> Self { pub(super) fn empty(on_disk: &'on_disk [u8]) -> Self {
Self { Self {
on_disk, on_disk,
root: ChildNodes::default(), root: ChildNodes::default(),
nodes_with_entry_count: 0, nodes_with_entry_count: 0,
nodes_with_copy_source_count: 0, nodes_with_copy_source_count: 0,
ignore_patterns_hash: [0; on_disk::IGNORE_PATTERNS_HASH_LEN], ignore_patterns_hash: [0; on_disk::IGNORE_PATTERNS_HASH_LEN],
unreachable_bytes: 0, unreachable_bytes: 0,
old_data_size: 0,
} }
} }
#[timed] #[timed]
pub fn new_v2( pub fn new_v2(
on_disk: &'on_disk [u8], on_disk: &'on_disk [u8],
data_size: usize, data_size: usize,
metadata: &[u8], metadata: &[u8],
} }
} }
Ok(packed) Ok(packed)
} }
/// Returns new data and metadata together with whether that data should be /// Returns new data and metadata together with whether that data should be
/// appended to the existing data file whose content is at /// appended to the existing data file whose content is at
/// `map.on_disk` (true), instead of written to a new data file /// `map.on_disk` (true), instead of written to a new data file
/// (false). /// (false), and the previous size of data on disk.
#[timed] #[timed]
pub fn pack_v2( pub fn pack_v2(
&self, &self,
can_append: bool, can_append: bool,
) -> Result<(Vec<u8>, on_disk::TreeMetadata, bool), DirstateError> { ) -> Result<(Vec<u8>, on_disk::TreeMetadata, bool, usize), DirstateError>
{
let map = self.get_map(); let map = self.get_map();
on_disk::write(map, can_append) on_disk::write(map, can_append)
} }
/// `callback` allows the caller to process and do something with the /// `callback` allows the caller to process and do something with the
/// results of the status. This is needed to do so efficiently (i.e. /// results of the status. This is needed to do so efficiently (i.e.
/// without cloning the `DirstateStatus` object with its paths) because /// without cloning the `DirstateStatus` object with its paths) because
/// we need to borrow from `Self`. /// we need to borrow from `Self`.

rust/hg-core/src/dirstate_tree/on_disk.rs

root: dirstate_map::ChildNodes::OnDisk(read_nodes( root: dirstate_map::ChildNodes::OnDisk(read_nodes(
on_disk, on_disk,
meta.root_nodes, meta.root_nodes,
)?), )?),
nodes_with_entry_count: meta.nodes_with_entry_count.get(), nodes_with_entry_count: meta.nodes_with_entry_count.get(),
nodes_with_copy_source_count: meta.nodes_with_copy_source_count.get(), nodes_with_copy_source_count: meta.nodes_with_copy_source_count.get(),
ignore_patterns_hash: meta.ignore_patterns_hash, ignore_patterns_hash: meta.ignore_patterns_hash,
unreachable_bytes: meta.unreachable_bytes.get(), unreachable_bytes: meta.unreachable_bytes.get(),
old_data_size: on_disk.len(),
}; };
Ok(dirstate_map) Ok(dirstate_map)
} }
impl Node { impl Node {
pub(super) fn full_path<'on_disk>( pub(super) fn full_path<'on_disk>(
&self, &self,
on_disk: &'on_disk [u8], on_disk: &'on_disk [u8],
Ok(()) Ok(())
} }
recur(on_disk, meta.root_nodes, &mut f) recur(on_disk, meta.root_nodes, &mut f)
} }
/// Returns new data and metadata, together with whether that data should be /// Returns new data and metadata, together with whether that data should be
/// appended to the existing data file whose content is at /// appended to the existing data file whose content is at
/// `dirstate_map.on_disk` (true), instead of written to a new data file /// `dirstate_map.on_disk` (true), instead of written to a new data file
/// (false). /// (false), and the previous size of data on disk.
pub(super) fn write( pub(super) fn write(
dirstate_map: &DirstateMap, dirstate_map: &DirstateMap,
can_append: bool, can_append: bool,
) -> Result<(Vec<u8>, TreeMetadata, bool), DirstateError> { ) -> Result<(Vec<u8>, TreeMetadata, bool, usize), DirstateError> {
let append = can_append && dirstate_map.write_should_append(); let append = can_append && dirstate_map.write_should_append();
// This ignores the space for paths, and for nodes without an entry. // This ignores the space for paths, and for nodes without an entry.
// TODO: better estimate? Skip the `Vec` and write to a file directly? // TODO: better estimate? Skip the `Vec` and write to a file directly?
let size_guess = std::mem::size_of::<Node>() let size_guess = std::mem::size_of::<Node>()
* dirstate_map.nodes_with_entry_count as usize; * dirstate_map.nodes_with_entry_count as usize;
let mut writer = Writer { let mut writer = Writer {
nodes_with_entry_count: dirstate_map.nodes_with_entry_count.into(), nodes_with_entry_count: dirstate_map.nodes_with_entry_count.into(),
nodes_with_copy_source_count: dirstate_map nodes_with_copy_source_count: dirstate_map
.nodes_with_copy_source_count .nodes_with_copy_source_count
.into(), .into(),
unreachable_bytes: dirstate_map.unreachable_bytes.into(), unreachable_bytes: dirstate_map.unreachable_bytes.into(),
unused: [0; 4], unused: [0; 4],
ignore_patterns_hash: dirstate_map.ignore_patterns_hash, ignore_patterns_hash: dirstate_map.ignore_patterns_hash,
}; };
Ok((writer.out, meta, append)) Ok((writer.out, meta, append, dirstate_map.old_data_size))
} }
struct Writer<'dmap, 'on_disk> { struct Writer<'dmap, 'on_disk> {
dirstate_map: &'dmap DirstateMap<'on_disk>, dirstate_map: &'dmap DirstateMap<'on_disk>,
append: bool, append: bool,
out: Vec<u8>, out: Vec<u8>,
} }

rust/hg-core/src/repo.rs

let map = self.dirstate_map()?; let map = self.dirstate_map()?;
// TODO: Maintain a `DirstateMap::dirty` flag, and return early here if // TODO: Maintain a `DirstateMap::dirty` flag, and return early here if
// it’s unset // it’s unset
let parents = self.dirstate_parents()?; let parents = self.dirstate_parents()?;
let packed_dirstate = if self.has_dirstate_v2() { let packed_dirstate = if self.has_dirstate_v2() {
let uuid = self.dirstate_data_file_uuid.get_or_init(self)?; let uuid = self.dirstate_data_file_uuid.get_or_init(self)?;
let mut uuid = uuid.as_ref(); let mut uuid = uuid.as_ref();
let can_append = uuid.is_some(); let can_append = uuid.is_some();
let (data, tree_metadata, append) = map.pack_v2(can_append)?; let (data, tree_metadata, append, old_data_size) =
map.pack_v2(can_append)?;
if !append { if !append {
uuid = None uuid = None
} }
let uuid = if let Some(uuid) = uuid { let uuid = if let Some(uuid) = uuid {
std::str::from_utf8(uuid) std::str::from_utf8(uuid)
.map_err(|_| { .map_err(|_| {
HgError::corrupted("non-UTF-8 dirstate data file ID") HgError::corrupted("non-UTF-8 dirstate data file ID")
})? })?
} else { } else {
options.write(true).create_new(true); options.write(true).create_new(true);
} }
let data_size = (|| { let data_size = (|| {
// TODO: loop and try another random ID if !append and this // TODO: loop and try another random ID if !append and this
// returns `ErrorKind::AlreadyExists`? Collision chance of two // returns `ErrorKind::AlreadyExists`? Collision chance of two
// random IDs is one in 2**32 // random IDs is one in 2**32
let mut file = options.open(&data_filename)?; let mut file = options.open(&data_filename)?;
if data.is_empty() {
// If we're not appending anything, the data size is the
// same as in the previous docket. It is *not* the file
// length, since it could have garbage at the end.
// We don't have to worry about it when we do have data
// to append since we rewrite the root node in this case.
Ok(old_data_size as u64)
} else {
file.write_all(&data)?; file.write_all(&data)?;
file.flush()?; file.flush()?;
// TODO: use https://doc.rust-lang.org/std/io/trait.Seek.html#method.stream_position when we require Rust 1.51+ // TODO: use https://doc.rust-lang.org/std/io/trait.Seek.html#method.stream_position when we require Rust 1.51+
file.seek(SeekFrom::Current(0)) file.seek(SeekFrom::Current(0))
}
})() })()
.when_writing_file(&data_filename)?; .when_writing_file(&data_filename)?;
DirstateDocket::serialize( DirstateDocket::serialize(
parents, parents,
tree_metadata, tree_metadata,
data_size, data_size,
uuid.as_bytes(), uuid.as_bytes(),
) )

rust/hg-cpython/src/dirstate/dirstate_map.rs

/// instead of written to a new data file (False). /// instead of written to a new data file (False).
def write_v2( def write_v2(
&self, &self,
can_append: bool, can_append: bool,
) -> PyResult<PyObject> { ) -> PyResult<PyObject> {
let inner = self.inner(py).borrow(); let inner = self.inner(py).borrow();
let result = inner.pack_v2(can_append); let result = inner.pack_v2(can_append);
match result { match result {
Ok((packed, tree_metadata, append)) => { Ok((packed, tree_metadata, append, _old_data_size)) => {
let packed = PyBytes::new(py, &packed); let packed = PyBytes::new(py, &packed);
let tree_metadata = PyBytes::new(py, tree_metadata.as_bytes()); let tree_metadata = PyBytes::new(py, tree_metadata.as_bytes());
let tuple = (packed, tree_metadata, append); let tuple = (packed, tree_metadata, append);
Ok(tuple.to_py_object(py).into_object()) Ok(tuple.to_py_object(py).into_object())
}, },
Err(_) => Err(PyErr::new::<exc::OSError, _>( Err(_) => Err(PyErr::new::<exc::OSError, _>(
py, py,
"Dirstate error".to_string(), "Dirstate error".to_string(),