From a05fd8b30920fff1b52ac7890269988bcd584222 Mon Sep 17 00:00:00 2001 From: Joaqim Planstedt Date: Mon, 12 May 2025 15:54:06 +0200 Subject: [PATCH 1/2] feat: Attempt to re-implement previous commit that allows for read-only for seeding --- crates/librqbit/src/storage/filesystem/fs.rs | 34 +++++++++++---- .../src/storage/filesystem/opened_file.rs | 41 ++++++++++++++++++- 2 files changed, 65 insertions(+), 10 deletions(-) diff --git a/crates/librqbit/src/storage/filesystem/fs.rs b/crates/librqbit/src/storage/filesystem/fs.rs index d4051a753..400df4a88 100644 --- a/crates/librqbit/src/storage/filesystem/fs.rs +++ b/crates/librqbit/src/storage/filesystem/fs.rs @@ -88,6 +88,7 @@ impl TorrentStorage for FilesystemStorage { fn pwrite_all(&self, file_id: usize, offset: u64, buf: &[u8]) -> anyhow::Result<()> { let of = self.opened_files.get(file_id).context("no such file")?; + of.ensure_writeable()?; #[cfg(target_family = "unix")] { use std::os::unix::fs::FileExt; @@ -148,7 +149,7 @@ impl TorrentStorage for FilesystemStorage { if !path.is_dir() { anyhow::bail!("cannot remove dir: {path:?} is not a directory") } - if std::fs::read_dir(&path)?.count() == 0 { + if std::fs::read_dir(&path)?.next().is_none() { std::fs::remove_dir(&path).with_context(|| format!("error removing {path:?}")) } else { warn!("did not remove {path:?} as it was not empty"); @@ -172,17 +173,34 @@ impl TorrentStorage for FilesystemStorage { continue; }; std::fs::create_dir_all(full_path.parent().context("bug: no parent")?)?; - let f = if shared.options.allow_overwrite { - OpenOptions::new() + if shared.options.allow_overwrite { + // ensure file exists + let (file, writeable) = match OpenOptions::new() .create(true) - .truncate(false) .read(true) .write(true) + .append(false) + .truncate(false) .open(&full_path) - .with_context(|| format!("error opening {full_path:?} in read/write mode"))? + { + Ok(file) => (file, true), + Err(e) => { + warn!(?full_path, "error opening file in create+write mode: {e:?}"); + // open the file in read-only mode, will reopen in write mode later. + ( + OpenOptions::new() + .create(false) + .read(true) + .open(&full_path) + .with_context(|| format!("error opening {full_path:?}"))?, + false, + ) + } + }; + files.push(OpenedFile::new(full_path.clone(), file, writeable)); } else { // create_new does not seem to work with read(true), so calling this twice. - OpenOptions::new() + let file = OpenOptions::new() .create_new(true) .write(true) .open(&full_path) @@ -192,9 +210,9 @@ impl TorrentStorage for FilesystemStorage { &full_path ) })?; - OpenOptions::new().read(true).write(true).open(&full_path)? + OpenOptions::new().read(true).write(true).open(&full_path)?; + files.push(OpenedFile::new(full_path.clone(), file, true)); }; - files.push(OpenedFile::new(f)); } self.opened_files = files; diff --git a/crates/librqbit/src/storage/filesystem/opened_file.rs b/crates/librqbit/src/storage/filesystem/opened_file.rs index 051c652e1..3c8fcc82e 100644 --- a/crates/librqbit/src/storage/filesystem/opened_file.rs +++ b/crates/librqbit/src/storage/filesystem/opened_file.rs @@ -1,22 +1,34 @@ -use std::fs::File; +use std::{ + fs::File, + path::PathBuf, + sync::atomic::{AtomicBool, Ordering}, +}; + +use anyhow::Context; use parking_lot::RwLock; #[derive(Debug)] pub(crate) struct OpenedFile { + pub filename: PathBuf, pub file: RwLock>, + pub is_writeable: AtomicBool, } impl OpenedFile { - pub fn new(f: File) -> Self { + pub fn new(filename: PathBuf, f: File, is_writeable: bool) -> Self { Self { + filename, file: RwLock::new(Some(f)), + is_writeable: AtomicBool::new(is_writeable), } } pub fn new_dummy() -> Self { Self { + filename: PathBuf::new(), file: RwLock::new(None), + is_writeable: AtomicBool::new(false), } } @@ -28,7 +40,32 @@ impl OpenedFile { pub fn take_clone(&self) -> anyhow::Result { let f = self.take()?; Ok(Self { + filename: self.filename.clone(), file: RwLock::new(f), + is_writeable: AtomicBool::new(self.is_writeable.load(Ordering::SeqCst)), }) } + + pub fn ensure_writeable(&self) -> anyhow::Result<()> { + match self + .is_writeable + .compare_exchange(false, true, Ordering::SeqCst, Ordering::Relaxed) + { + Ok(_) => { + // Updated, need to reopen writeable + let mut g = self.file.write(); + let new_file = std::fs::OpenOptions::new() + .write(true) + .create(false) + .open(&self.filename) + .with_context(|| format!("error opening {:?} in write mode", self.filename))?; + *g = Some(new_file); + } + Err(_) => { + // Didn't update, no need to reopen + } + } + + Ok(()) + } } From 3d7f9752a4019ebba53adfdeaa4bf1739d49678e Mon Sep 17 00:00:00 2001 From: Joaqim Planstedt Date: Fri, 16 May 2025 14:50:21 +0200 Subject: [PATCH 2/2] feat!: OpenedFile: Replace Option with Option struct which stores boolean whether file is writeable or not * New FileHandle struct: { file: File, is_writeable: bool } * Replace `OpenedFile::file: RwLock>` with `::file_handle RwLock>` --- crates/librqbit/src/storage/filesystem/fs.rs | 17 +++-- .../librqbit/src/storage/filesystem/mmap.rs | 8 +-- .../src/storage/filesystem/opened_file.rs | 69 +++++++++++-------- 3 files changed, 54 insertions(+), 40 deletions(-) diff --git a/crates/librqbit/src/storage/filesystem/fs.rs b/crates/librqbit/src/storage/filesystem/fs.rs index 400df4a88..0a6da217f 100644 --- a/crates/librqbit/src/storage/filesystem/fs.rs +++ b/crates/librqbit/src/storage/filesystem/fs.rs @@ -62,10 +62,11 @@ impl TorrentStorage for FilesystemStorage { { use std::os::unix::fs::FileExt; Ok(of - .file + .file_handle .read() .as_ref() .context("file is None")? + .file .read_exact_at(buf, offset)?) } #[cfg(target_family = "windows")] @@ -93,18 +94,19 @@ impl TorrentStorage for FilesystemStorage { { use std::os::unix::fs::FileExt; Ok(of - .file + .file_handle .read() .as_ref() .context("file is None")? + .file .write_all_at(buf, offset)?) } #[cfg(target_family = "windows")] { use std::os::windows::fs::FileExt; let mut remaining = buf.len(); - let g = of.file.read(); - let f = g.as_ref().context("file is None")?; + let g = of.file_handle.read(); + let f = g.as_ref().context("file is None")?.file; while remaining > 0 { remaining -= f.seek_write(buf, offset)?; } @@ -113,8 +115,8 @@ impl TorrentStorage for FilesystemStorage { #[cfg(not(any(target_family = "unix", target_family = "windows")))] { use std::io::{Read, Seek, SeekFrom, Write}; - let mut g = of.file.write(); - let mut f = g.as_ref().context("file is None")?; + let mut g = of.file_handle.write(); + let mut f = g.as_ref().context("file is None")?.file; f.seek(SeekFrom::Start(offset))?; Ok(f.write_all(buf)?) } @@ -126,10 +128,11 @@ impl TorrentStorage for FilesystemStorage { fn ensure_file_length(&self, file_id: usize, len: u64) -> anyhow::Result<()> { Ok(self.opened_files[file_id] - .file + .file_handle .write() .as_ref() .context("file is None")? + .file .set_len(len)?) } diff --git a/crates/librqbit/src/storage/filesystem/mmap.rs b/crates/librqbit/src/storage/filesystem/mmap.rs index a1824c677..b3945e70c 100644 --- a/crates/librqbit/src/storage/filesystem/mmap.rs +++ b/crates/librqbit/src/storage/filesystem/mmap.rs @@ -109,11 +109,11 @@ impl TorrentStorage for MmapFilesystemStorage { self.fs.init(shared, metadata)?; let mut mmaps = Vec::new(); for (idx, file) in self.fs.opened_files.iter().enumerate() { - let fg = file.file.write(); - let fg = fg.as_ref().context("file is None")?; - fg.set_len(metadata.file_infos[idx].len) + let mut fh = file.file_handle.write(); + let file = &fh.as_mut().context("file is None")?.file; + file.set_len(metadata.file_infos[idx].len) .context("mmap storage: error setting length")?; - let mmap = unsafe { MmapOptions::new().map_mut(fg) }.context("error mapping file")?; + let mmap = unsafe { MmapOptions::new().map_mut(file) }.context("error mapping file")?; mmaps.push(RwLock::new(mmap)); } diff --git a/crates/librqbit/src/storage/filesystem/opened_file.rs b/crates/librqbit/src/storage/filesystem/opened_file.rs index 3c8fcc82e..35be70387 100644 --- a/crates/librqbit/src/storage/filesystem/opened_file.rs +++ b/crates/librqbit/src/storage/filesystem/opened_file.rs @@ -1,71 +1,82 @@ use std::{ fs::File, + fs::OpenOptions, path::PathBuf, - sync::atomic::{AtomicBool, Ordering}, }; use anyhow::Context; use parking_lot::RwLock; +#[derive(Debug)] +pub(crate) struct FileHandle { + pub file: File, + pub is_writeable: bool, +} + #[derive(Debug)] pub(crate) struct OpenedFile { pub filename: PathBuf, - pub file: RwLock>, - pub is_writeable: AtomicBool, + pub file_handle: RwLock>, } impl OpenedFile { - pub fn new(filename: PathBuf, f: File, is_writeable: bool) -> Self { - Self { - filename, - file: RwLock::new(Some(f)), - is_writeable: AtomicBool::new(is_writeable), - } + pub fn new(filename: PathBuf, file: File, is_writeable: bool) -> Self { + let file_handle = RwLock::new(Some(FileHandle { + file, + is_writeable, + })); + Self { filename, file_handle } } pub fn new_dummy() -> Self { Self { filename: PathBuf::new(), - file: RwLock::new(None), - is_writeable: AtomicBool::new(false), + file_handle: None.into(), } } pub fn take(&self) -> anyhow::Result> { - let mut f = self.file.write(); - Ok(f.take()) + let mut fh = self.file_handle.write(); + if let Some(file_handle) = fh.take() { + Ok(Some(file_handle.file)) + } else { + Ok(None) + } + } + + pub fn is_writeable(&self) -> bool { + self.file_handle.read().as_ref().and_then(|fh| Some(fh.is_writeable)).unwrap_or(false) } pub fn take_clone(&self) -> anyhow::Result { - let f = self.take()?; + let file = self.take().unwrap().with_context(|| format!("error taking file for {:?}", self.filename))?; Ok(Self { filename: self.filename.clone(), - file: RwLock::new(f), - is_writeable: AtomicBool::new(self.is_writeable.load(Ordering::SeqCst)), + file_handle: RwLock::new( + Some(FileHandle { + file, + is_writeable: self.is_writeable(), + }) + ), }) } pub fn ensure_writeable(&self) -> anyhow::Result<()> { - match self - .is_writeable - .compare_exchange(false, true, Ordering::SeqCst, Ordering::Relaxed) - { - Ok(_) => { - // Updated, need to reopen writeable - let mut g = self.file.write(); - let new_file = std::fs::OpenOptions::new() + let mut fh = self.file_handle.write(); + if let Some(file_handle) = fh.as_mut() { + if !file_handle.is_writeable { + let new_file = OpenOptions::new() .write(true) .create(false) .open(&self.filename) .with_context(|| format!("error opening {:?} in write mode", self.filename))?; - *g = Some(new_file); - } - Err(_) => { - // Didn't update, no need to reopen + *file_handle = FileHandle { + file: new_file, + is_writeable: true, + }; } } - Ok(()) } }