Remove limit maximum number of files #11
Conversation
DavidVentura
force-pushed
the
remove-file-limit
branch
from
d932b8a to
066d081
Compare
src/main.rs
Outdated
| // Resize the file to its final size already: | ||
| // * So that the file system can do a better job of allocating | ||
| // a single extent for it, and it doesn't have to fragment | ||
| // the file. | ||
| // * If we run out of space, we learn about that before we waste | ||
| // time on the transfer (although then maybe we should do it | ||
| // before we receive a chunk after all?). | ||
| // This can make debugging a bit harder, because when you look | ||
| // at just the file size you might think it's fully transferred. |
There was a problem hiding this comment.
Hm.. this is not resume friendly. If transfer fails in the middle, based on size we wouldn't know transfer was not done. I am not sure is this worth it.
There was a problem hiding this comment.
If we want to implement the ability to resume we need to make more changes (eg: file list now does not send metadata), so I wouldn't worry about it now
There was a problem hiding this comment.
I guess it would be fine, if target is temporary location, which will be renamed into place after the transfer. That would be better.
| let (events_tx, events_rx) = std::sync::mpsc::channel::<SenderEvent>(); | ||
| let cwd = env::current_dir().unwrap(); | ||
| thread::spawn(|| { | ||
| let td = TempDir::new_in(".").unwrap(); |
There was a problem hiding this comment.
Probably better to run this on a ramdisk, I didn’t do the math to check if my worry is justified, but I am worried about burning through my SSD if I run this on my laptop.
There was a problem hiding this comment.
It's now done in this weird fashion because we don't allow sending absolute paths
There was a problem hiding this comment.
Ah, right, yes. But we could chdir into a directory in /tmp and run from there? (Or does that break the Rust test runner which by default runs tests in parallel?) (Sorry for being so paranoid, but I seem to be very good at corrupting btrfs file systems even when I’m not actively trying, and my root is on btrfs, I’m not very eager to tempt fate like this 🙊)
There was a problem hiding this comment.
no worries, it is now like that (chdir to /tmp) which is what I meant by "a weird fashion"
| // we effectively limit the amount of open files to the amount of send threads. | ||
| // However, this now introduces the possibility of files getting deleted between | ||
| // getting listed and their turn for transfer. | ||
| // A vanishing file is expected, it is not a transfer-terminating event. |
There was a problem hiding this comment.
It does mess up the progress measurement, but I suppose we could treat a vanished file as instantly completing all of its size bytes. (Which would mess up transfer rate estimates, but I think that’s okay. Ah oh actually we could decrement the total we expect if we see a vanishing file.)
DavidVentura
force-pushed
the
remove-file-limit
branch
from
c696830 to
40320a4
Compare
DavidVentura
force-pushed
the
remove-file-limit
branch
from
40320a4 to
76156ab
Compare
ruuda
dismissed
kucharskim’s stale review
“Not ready yet” comment no longer applies.
Fixes #10.
After the naive implementation (just bumping the u16 and protocol version),
fastsynccrashed with "too many open files" -- this is why I changed the fds to be opened on demand (leading to vanishing file precautions)The test with 20k files runs in 0.8s on my machine.