fix(fuse): download the base of a file open for writing on a worker

A read on a write handle was answered on the dispatch loop to keep its
order with the handle's writes, and when the bytes it asked for were
neither written nor cached it downloaded them there, holding every request
of the mount for up to the read deadline. The loop now answers from the
scratch file and the local caches only, and hands a read that needs a
download to a transfer worker with the handle as it was when the read came
in (B227).
This commit is contained in:
nvrl committed 2026-10-16 10:47:58 +02:00
1 parent f8c60c9753
commit d4ef9116c8
3 files changed
+203 -63

No files matched your search

+27 -6
View File
@@ -261,10 +261,13 @@ impl Filesystem for ProtonFs {
// in-flight (possibly unsaved) content: authored bytes come from the
// scratch file, untouched bytes from the base.
//
// This path stays on the dispatch loop. `write` runs there too, so
// keeping its reads there as well preserves the ordering the write path
// has always had between a write and a read of the same handle; a mostly
// local read is not worth reasoning about that for.
// Answered on the dispatch loop when everything it needs is on this
// machine, which keeps the order `write` has with reads of the same
// handle. A base range that has to be downloaded goes to a worker
// instead, with the handle as it was when the read came in: the kernel
// issued the read before any write that comes after it, so that is
// the content it asked for. On the loop, the download held every
// request of the mount (`docs/BUGS.md` B227).
let handle = {
let st = self.core.state();
st.active_writes.get(&ino.0).map(|h| {
@@ -279,7 +282,7 @@ impl Filesystem for ProtonFs {
})
};
if let Some((file, len, uid, base_mtime, base_size, written)) = handle {
match self.core.serve_open_read(
match self.core.serve_open_read_local(
&file,
len,
&uid,
@@ -289,7 +292,25 @@ impl Filesystem for ProtonFs {
offset,
size as u64,
) {
Ok(bytes) => reply.data(&bytes),
Ok(Some(bytes)) => reply.data(&bytes),
Ok(None) => {
let fs = self.clone();
self.core.workers.run(Lane::Transfer, "read", move || {
match fs.core.serve_open_read(
&file,
len,
&uid,
base_mtime,
base_size,
&written,
offset,
size as u64,
) {
Ok(bytes) => reply.data(&bytes),
Err(e) => reply.error(e),
}
});
}
Err(e) => reply.error(e),
}
return;
+123 -31
View File
@@ -1070,37 +1070,35 @@ impl Core {
offset: u64,
size: u64,
) -> Result<Vec<u8>, Errno> {
if offset >= len || size == 0 {
return Ok(Vec::new());
}
let end = offset.saturating_add(size).min(len);
let mut out = Vec::with_capacity((end - offset) as usize);
for (s, e, authored) in written.segments(offset, end) {
if authored {
let mut buf = vec![0u8; (e - s) as usize];
file.read_exact_at(&mut buf, s).map_err(|err| {
warn!(%uid, error = %err, "scratch read failed");
Errno::EIO
})?;
out.extend_from_slice(&buf);
} else {
let bend = e.min(base_size);
if s < bend {
out.extend_from_slice(&self.read_range(
uid,
base_mtime,
base_size,
s,
bend - s,
true,
)?);
}
// Anything past the base is a hole: zero-fill.
let zeros = e.saturating_sub(s.max(base_size));
out.resize(out.len() + zeros as usize, 0);
}
}
Ok(out)
let read = assemble_open_read(file, len, uid, base_size, written, offset, size, |s, n| {
self.read_range(uid, base_mtime, base_size, s, n, true)
.map(Some)
})?;
Ok(read.unwrap_or_default())
}
/// [`Core::serve_open_read`] from bytes already on this machine only:
/// `Ok(None)` the moment a range of the base would have to be downloaded.
///
/// The dispatch loop answers a read on a write handle with this, and hands
/// it to a worker only when it says `None`. Fetching the base there held
/// every request of the mount for as long as the download took, up to the
/// read deadline (`docs/BUGS.md` B227).
#[allow(clippy::too_many_arguments)]
pub(super) fn serve_open_read_local(
&self,
file: &Arc<File>,
len: u64,
uid: &NodeUid,
base_mtime: i64,
base_size: u64,
written: &Intervals,
offset: u64,
size: u64,
) -> Result<Option<Vec<u8>>, Errno> {
assemble_open_read(file, len, uid, base_size, written, offset, size, |s, n| {
Ok(self.cached_range(uid, base_mtime, base_size, s, n))
})
}
/// The largest amount [`Core::fill_gaps_cached`] will copy out of the local
@@ -1242,6 +1240,49 @@ impl Core {
}
}
/// The bytes `[offset, offset + size)` of a file open for writing: authored
/// ranges from its scratch `file`, the rest from its base through `base`,
/// and zeros past the base. `Ok(None)` when `base` says it cannot answer.
#[allow(clippy::too_many_arguments)]
fn assemble_open_read(
file: &File,
len: u64,
uid: &NodeUid,
base_size: u64,
written: &Intervals,
offset: u64,
size: u64,
mut base: impl FnMut(u64, u64) -> Result<Option<Vec<u8>>, Errno>,
) -> Result<Option<Vec<u8>>, Errno> {
if offset >= len || size == 0 {
return Ok(Some(Vec::new()));
}
let end = offset.saturating_add(size).min(len);
let mut out = Vec::with_capacity((end - offset) as usize);
for (s, e, authored) in written.segments(offset, end) {
if authored {
let mut buf = vec![0u8; (e - s) as usize];
file.read_exact_at(&mut buf, s).map_err(|err| {
warn!(%uid, error = %err, "scratch read failed");
Errno::EIO
})?;
out.extend_from_slice(&buf);
} else {
let bend = e.min(base_size);
if s < bend {
match base(s, bend - s)? {
Some(bytes) => out.extend_from_slice(&bytes),
None => return Ok(None),
}
}
// Anything past the base is a hole: zero-fill.
let zeros = e.saturating_sub(s.max(base_size));
out.resize(out.len() + zeros as usize, 0);
}
}
Ok(Some(out))
}
#[cfg(test)]
mod block_ring_tests {
use super::*;
@@ -1394,3 +1435,54 @@ mod prefetch_tests {
assert!(prefetch.streams.lock().len() <= MAX_PREFETCH_STREAMS + 1);
}
}
#[cfg(test)]
mod open_read_tests {
use super::*;
use proton_drive_rs::proton_sdk::ids::{LinkId, VolumeId};
fn scratch(bytes: &[u8]) -> (File, PathBuf) {
let path = std::env::temp_dir().join(format!(
"pdfs-open-read-{}-{}",
std::process::id(),
std::time::SystemTime::now()
.duration_since(std::time::UNIX_EPOCH)
.unwrap()
.as_nanos()
));
let mut file = File::options()
.create_new(true)
.read(true)
.write(true)
.open(&path)
.unwrap();
file.write_all(bytes).unwrap();
(file, path)
}
/// The dispatch loop answers from what is on this machine and says so
/// when the base would have to be downloaded, for a worker to do it
/// (`docs/BUGS.md` B227).
#[test]
fn a_read_needing_the_base_says_so_instead_of_fetching() {
let uid = NodeUid::new(VolumeId::from("v"), LinkId::from("f"));
let (file, path) = scratch(b"ABCDEFGH");
let mut written = Intervals::default();
written.add(0, 4);
let authored = assemble_open_read(&file, 8, &uid, 8, &written, 0, 4, |_, _| {
panic!("an authored range never asks the base")
});
assert_eq!(authored.unwrap().as_deref(), Some(&b"ABCD"[..]));
let missing = assemble_open_read(&file, 8, &uid, 8, &written, 2, 6, |_, _| Ok(None));
assert_eq!(missing.unwrap(), None);
let base = assemble_open_read(&file, 10, &uid, 8, &written, 2, 8, |s, n| {
assert_eq!((s, n), (4, 4));
Ok(Some(b"efgh".to_vec()))
});
assert_eq!(base.unwrap().as_deref(), Some(&b"CDefgh\0\0"[..]));
let _ = std::fs::remove_file(path);
}
}
+53 -26
View File
@@ -100,10 +100,11 @@ a live account since.
| Entry | Title |
|---|---|
| [B227](#b227--reading-a-file-open-for-writing-downloads-on-the-dispatch-loop) | Reading a file open for writing downloads on the dispatch loop |
| [B226](#b226--thumbnails-of-portrait-phone-photos-lie-on-their-side) | Thumbnails of portrait phone photos lie on their side |
| [B225](#b225--a-takeout-photo-without-a-sidecar-takes-a-siblings-name-and-date) | A Takeout photo without a sidecar takes a sibling's name and date |
| [B223](#b223--a-damaged-config-file-is-replaced-with-defaults-on-the-next-settings-change) | A damaged config file is replaced with defaults on the next settings change |
| [B224](#b224--fallocate-zero-ranges-are-undone-and-collapse-ranges-corrupt-the-file) | `fallocate` zero ranges are undone and collapse ranges corrupt the file |
| [B223](#b223--a-damaged-config-file-is-replaced-with-defaults-on-the-next-settings-change) | A damaged config file is replaced with defaults on the next settings change |
| [B222](#b222--a-full-disk-at-close-is-reported-to-nobody-and-fsync-on-a-read-only-file-fails) | A full disk at close is reported to nobody, and `fsync` on a read-only file fails |
| [B221](#b221--a-cold-listing-holds-the-global-lock-across-a-database-transaction) | A cold listing holds the global lock across a database transaction |
| [B220](#b220--thumbnails-on-a-stalled-link-hold-up-every-other-request) | Thumbnails on a stalled link hold up every other request |
@@ -329,6 +330,32 @@ a live account since.
Newest first.
### B227 — Reading a file open for writing downloads on the dispatch loop
**Status:** Fixed, not verified live
**Found:** 2026-10-16, architecture audit (`FINDINGS.md` 2.1.2).
**Where:** `read` in `crates/pdfs-fuse/src/filesystem.rs`; `Core::serve_open_read_local` and
`assemble_open_read` in `crates/pdfs-fuse/src/reads.rs`
**Repro:** On a slow link, open a large file that is only on Drive with `O_RDWR`, write a few
bytes, and read elsewhere in it, as SQLite and many editors do, while another program uses the
mount.
**Cause.** A read on a file open for writing was always answered on the dispatch loop, so that it
kept its order with the writes on the same handle. When the bytes it asked for were neither
written nor cached, it downloaded them there, for up to the 120 s read deadline plus the wait
for a connection. Every other request of the mount waited behind it.
**Fix.** The dispatch loop answers such a read from the scratch file and the local caches only.
When a range of the base would have to be downloaded, the read goes to a transfer worker, with
the handle as it was when the read came in. The kernel issued the read before any later write,
so that is the content it asked for.
**Test:** `reads::open_read_tests::a_read_needing_the_base_says_so_instead_of_fetching`; the
simulation runs cover the order of writes and reads on one handle.
---
### B226 — Thumbnails of portrait phone photos lie on their side
**Status:** Fixed, not verified live
@@ -379,31 +406,6 @@ by its own rule. It takes the capture time from that sidecar and keeps its own n
---
### B223 — A damaged config file is replaced with defaults on the next settings change
**Status:** Fixed, not verified live
**Found:** 2026-10-16, architecture audit (`FINDINGS.md` 1.10).
**Where:** `AppDirs::save_config`, `AppDirs::load_config` and `refuse_to_replace_unreadable` in
`crates/pdfs-core/src/config.rs`
**Repro:** Truncate `~/.config/proton-drive-linux/config.json`, then change any setting in the
app, or let the daemon record its device.
**Cause.** `load_config` gives defaults for a file it cannot parse, and logs that it leaves the
file alone. But every setter loads, changes one field and saves, in the daemon and in the app.
So the first change after the damage wrote the defaults over the file, losing the mountpoint,
the device the client had adopted, the ignore rules and the `open_with` choices. ARCHITECTURE
§10.5 says the file is never replaced with defaults. The first-run default was also written with a
plain `std::fs::write`, which is not atomic and takes the umask's mode.
**Fix.** `save_config` refuses while the file on disk exists and does not parse, and says which
file to fix. The first-run default goes through `save_config`, so it is written atomically with
mode 0600.
**Test:** `config::tests::an_unreadable_config_is_never_written_over`.
---
### B224 — `fallocate` zero ranges are undone and collapse ranges corrupt the file
**Status:** Fixed, not verified live
@@ -431,6 +433,31 @@ the lock is not held across it.
---
### B223 — A damaged config file is replaced with defaults on the next settings change
**Status:** Fixed, not verified live
**Found:** 2026-10-16, architecture audit (`FINDINGS.md` 1.10).
**Where:** `AppDirs::save_config`, `AppDirs::load_config` and `refuse_to_replace_unreadable` in
`crates/pdfs-core/src/config.rs`
**Repro:** Truncate `~/.config/proton-drive-linux/config.json`, then change any setting in the
app, or let the daemon record its device.
**Cause.** `load_config` gives defaults for a file it cannot parse, and logs that it leaves the
file alone. But every setter loads, changes one field and saves, in the daemon and in the app.
So the first change after the damage wrote the defaults over the file, losing the mountpoint,
the device the client had adopted, the ignore rules and the `open_with` choices. ARCHITECTURE
§10.5 says the file is never replaced with defaults. The first-run default was also written with a
plain `std::fs::write`, which is not atomic and takes the umask's mode.
**Fix.** `save_config` refuses while the file on disk exists and does not parse, and says which
file to fix. The first-run default goes through `save_config`, so it is written atomically with
mode 0600.
**Test:** `config::tests::an_unreadable_config_is_never_written_over`.
---
### B222 — A full disk at close is reported to nobody, and `fsync` on a read-only file fails
**Status:** Fixed, not verified live