mirror of
https://github.com/narrrl/proton-drive-linux.git
synced 2026-11-03 16:41:26 +01:00
fix(fuse): a copy right after a save waits for the edit (B256)
close(2) returns before the kernel sends release, and release takes the write handle out of the open writes a few milliseconds before the revision is in the queue. A `pdfs copy` checked in that gap found the edit nowhere and copied the version Drive had. A folder copy looked only at the queue, so it also missed files below it still open with unsaved writes. release now keeps the inode in State::releasing until the revision is queued. A copy refuses a file or folder with unsaved writes, or a release still queueing, at it or below it, and a move checks the same set.
This commit is contained in:
1 parent
917e7268f9
commit
ea10a43216
5 files changed
+51
-4
No files matched your search
@@ -671,7 +671,7 @@ impl Filesystem for ProtonFs {
|
|||||||
reply: ReplyEmpty,
|
reply: ReplyEmpty,
|
||||||
) {
|
) {
|
||||||
let ino = self.tree(ino);
|
let ino = self.tree(ino);
|
||||||
let (handle, unlinked_uid) = {
|
let (handle, unlinked_uid, releasing) = {
|
||||||
let mut st = self.core.state();
|
let mut st = self.core.state();
|
||||||
let unlinked_uid = release_unlinked_entry(&mut st, ino.0);
|
let unlinked_uid = release_unlinked_entry(&mut st, ino.0);
|
||||||
let ino = st.handles.remove(&fh.0);
|
let ino = st.handles.remove(&fh.0);
|
||||||
@@ -684,7 +684,11 @@ impl Filesystem for ProtonFs {
|
|||||||
None
|
None
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
(h, unlinked_uid)
|
let releasing = ino.filter(|_| h.is_some());
|
||||||
|
if let Some(ino) = releasing {
|
||||||
|
st.releasing.insert(ino);
|
||||||
|
}
|
||||||
|
(h, unlinked_uid, releasing)
|
||||||
};
|
};
|
||||||
// Deliberately **not** handed to a worker, unlike the other slow
|
// Deliberately **not** handed to a worker, unlike the other slow
|
||||||
// handlers here. The kernel does not wait for a `release` reply before
|
// handlers here. The kernel does not wait for a `release` reply before
|
||||||
@@ -701,6 +705,9 @@ impl Filesystem for ProtonFs {
|
|||||||
// finish it (audit F1). What is left is a rename, two fsyncs, a database
|
// finish it (audit F1). What is left is a rename, two fsyncs, a database
|
||||||
// insert, and at most a bounded copy out of the local cache.
|
// insert, and at most a bounded copy out of the local cache.
|
||||||
self.finish_release(handle, unlinked_uid, reply);
|
self.finish_release(handle, unlinked_uid, reply);
|
||||||
|
if let Some(ino) = releasing {
|
||||||
|
self.core.state().releasing.remove(&ino);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
fn unlink(&self, _req: &Request, parent: INodeNo, name: &OsStr, reply: ReplyEmpty) {
|
fn unlink(&self, _req: &Request, parent: INodeNo, name: &OsStr, reply: ReplyEmpty) {
|
||||||
|
|||||||
@@ -5230,7 +5230,15 @@ impl Core {
|
|||||||
.entries
|
.entries
|
||||||
.get(&ino)
|
.get(&ino)
|
||||||
.ok_or_else(|| CoreError::not_found(format!("{} is gone", rel.display())))?;
|
.ok_or_else(|| CoreError::not_found(format!("{} is gone", rel.display())))?;
|
||||||
let unsaved = st.active_writes.get(&ino).is_some_and(|w| w.dirty);
|
// Unsaved writes, or a release still queueing them, to the file or,
|
||||||
|
// for a folder, to any file below it.
|
||||||
|
let unsaved = st
|
||||||
|
.active_writes
|
||||||
|
.iter()
|
||||||
|
.filter(|(_, w)| w.dirty)
|
||||||
|
.map(|(&open, _)| open)
|
||||||
|
.chain(st.releasing.iter().copied())
|
||||||
|
.any(|open| relocate::ino_is_under(&st, open, ino));
|
||||||
(entry.node.name.clone(), entry.node.is_folder(), unsaved)
|
(entry.node.name.clone(), entry.node.is_folder(), unsaved)
|
||||||
};
|
};
|
||||||
// Unlike a move, a copy cannot wait in the queue: it needs the node as
|
// Unlike a move, a copy cannot wait in the queue: it needs the node as
|
||||||
|
|||||||
@@ -133,7 +133,7 @@ fn rename_noreplace(from: &Path, to: &Path) -> std::io::Result<()> {
|
|||||||
|
|
||||||
/// Whether inode `ino` is `root` or lies under it in the tree. Bounded, so a
|
/// Whether inode `ino` is `root` or lies under it in the tree. Bounded, so a
|
||||||
/// parent cycle in corrupt data cannot hang the caller.
|
/// parent cycle in corrupt data cannot hang the caller.
|
||||||
fn ino_is_under(st: &State, mut ino: u64, root: u64) -> bool {
|
pub(crate) fn ino_is_under(st: &State, mut ino: u64, root: u64) -> bool {
|
||||||
for _ in 0..MAX_TREE_DEPTH {
|
for _ in 0..MAX_TREE_DEPTH {
|
||||||
if ino == root {
|
if ino == root {
|
||||||
return true;
|
return true;
|
||||||
@@ -456,6 +456,7 @@ impl Core {
|
|||||||
writing = st
|
writing = st
|
||||||
.active_writes
|
.active_writes
|
||||||
.keys()
|
.keys()
|
||||||
|
.chain(&st.releasing)
|
||||||
.any(|&open| ino_is_under(st, open, ino));
|
.any(|&open| ino_is_under(st, open, ino));
|
||||||
});
|
});
|
||||||
Ok(writing)
|
Ok(writing)
|
||||||
|
|||||||
@@ -266,6 +266,11 @@ pub(crate) struct State {
|
|||||||
pub(crate) next_ino: u64,
|
pub(crate) next_ino: u64,
|
||||||
/// Shared write state keyed by inode. Concurrent writers share the scratch file.
|
/// Shared write state keyed by inode. Concurrent writers share the scratch file.
|
||||||
pub(crate) active_writes: HashMap<u64, WriteHandle>,
|
pub(crate) active_writes: HashMap<u64, WriteHandle>,
|
||||||
|
/// Inodes whose last write handle `release` has taken out of
|
||||||
|
/// [`State::active_writes`] and is still queueing. `close(2)` returns
|
||||||
|
/// before that, and in between the edit is in neither the handles nor the
|
||||||
|
/// queue, so a check for changes not on Drive yet looks here too.
|
||||||
|
pub(crate) releasing: HashSet<u64>,
|
||||||
/// Maps file handle id (fh) to inode (ino). Read-only opens use fh 0 and
|
/// Maps file handle id (fh) to inode (ino). Read-only opens use fh 0 and
|
||||||
/// have no entry here.
|
/// have no entry here.
|
||||||
pub(crate) handles: HashMap<u64, u64>,
|
pub(crate) handles: HashMap<u64, u64>,
|
||||||
@@ -428,6 +433,7 @@ impl State {
|
|||||||
roots: HashSet::new(),
|
roots: HashSet::new(),
|
||||||
next_ino: FALLBACK_INO,
|
next_ino: FALLBACK_INO,
|
||||||
active_writes: HashMap::new(),
|
active_writes: HashMap::new(),
|
||||||
|
releasing: HashSet::new(),
|
||||||
handles: HashMap::new(),
|
handles: HashMap::new(),
|
||||||
next_fh: 1,
|
next_fh: 1,
|
||||||
dir_snapshots: HashMap::new(),
|
dir_snapshots: HashMap::new(),
|
||||||
|
|||||||
@@ -100,6 +100,7 @@ a live account since.
|
|||||||
|
|
||||||
| Entry | Title |
|
| Entry | Title |
|
||||||
|---|---|
|
|---|---|
|
||||||
|
| [B256](#b256--pdfs-copy-right-after-a-save-copies-the-old-version) | `pdfs copy` right after a save copies the old version |
|
||||||
| [B254](#b254--an-edit-in-the-same-second-as-a-new-upload-never-arrives) | An edit in the same second as a new upload never arrives |
|
| [B254](#b254--an-edit-in-the-same-second-as-a-new-upload-never-arrives) | An edit in the same second as a new upload never arrives |
|
||||||
| [B253](#b253--the-app-shows-the-sign-in-page-when-the-keyring-is-locked) | The app shows the sign-in page when the keyring is locked |
|
| [B253](#b253--the-app-shows-the-sign-in-page-when-the-keyring-is-locked) | The app shows the sign-in page when the keyring is locked |
|
||||||
| [B252](#b252--computers-sharing-a-device-overwrite-each-others-profile) | Computers sharing a device overwrite each other's profile |
|
| [B252](#b252--computers-sharing-a-device-overwrite-each-others-profile) | Computers sharing a device overwrite each other's profile |
|
||||||
@@ -357,6 +358,30 @@ a live account since.
|
|||||||
|
|
||||||
Newest first.
|
Newest first.
|
||||||
|
|
||||||
|
### B256 — `pdfs copy` right after a save copies the old version
|
||||||
|
|
||||||
|
**Status:** Fixed, not verified live
|
||||||
|
**Found:** 2026-10-26, the simulation `a_pdfs_copy_copies_on_drive_and_waits_for_unsynced_changes`
|
||||||
|
failing on most runs. The feature was not released yet.
|
||||||
|
**Where:** `release` in `crates/pdfs-fuse/src/filesystem.rs`; `copy_to` in
|
||||||
|
`crates/pdfs-fuse/src/lib.rs`; `has_pending_work_under` in `crates/pdfs-fuse/src/relocate.rs`
|
||||||
|
|
||||||
|
**Repro:** Save a file in the mount and copy it, or its folder, with `pdfs copy` straight away.
|
||||||
|
The copy goes ahead and Drive copies the version before the save.
|
||||||
|
|
||||||
|
**Cause.** `close(2)` returns before the kernel sends `release`. `release` takes the write handle
|
||||||
|
out of the open writes and then queues the revision, which takes a few milliseconds. A copy
|
||||||
|
checked in between found the edit in neither place. A folder copy looked only at the queue, so
|
||||||
|
it also missed a file still open with unsaved writes.
|
||||||
|
|
||||||
|
**Fix.** `release` keeps the inode in `State::releasing` until the revision is queued. A copy
|
||||||
|
refuses a file or folder with unsaved writes, or a release still queueing, at it or below it.
|
||||||
|
A move checks the same set.
|
||||||
|
|
||||||
|
**Test:** The simulation above passed 6 of 6 runs after the fix.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
### B254 — An edit in the same second as a new upload never arrives
|
### B254 — An edit in the same second as a new upload never arrives
|
||||||
|
|
||||||
**Status:** Fixed, not verified live
|
**Status:** Fixed, not verified live
|
||||||
|
|||||||
Reference in new issue
Block a user