Re: [pbs-devel] [PATCH v8 proxmox-backup 48/69] pxar: caching: add look-ahead cache

[Christian Ebner <c.ebner@proxmox.com>]

On 6/4/24 11:35, Fabian Grünbichler wrote:
> On May 28, 2024 11:42 am, Christian Ebner wrote:
>> Add a lookahead cache and the neccessary types to store the required
>> data and keep track of directory boundaries while traversing the
>> filesystem tree, in order to postpone a decision if to reuse or
>> reencode a given regular file with unchanged metadata.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>> changes since version 7:
>> - no changes
>>
>> changes since version 6:
>> - add PxarLookaheadCache and refactor some of the logic to be contained
>>    within this patch
>>
>>   pbs-client/src/pxar/create.rs           |   2 +-
>>   pbs-client/src/pxar/look_ahead_cache.rs | 165 ++++++++++++++++++++++++
>>   pbs-client/src/pxar/mod.rs              |   1 +
>>   3 files changed, 167 insertions(+), 1 deletion(-)
>>   create mode 100644 pbs-client/src/pxar/look_ahead_cache.rs
>>
>> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
>> index ac8827bb2..6127aa88f 100644
>> --- a/pbs-client/src/pxar/create.rs
>> +++ b/pbs-client/src/pxar/create.rs
>> @@ -131,7 +131,7 @@ impl fmt::Display for ArchiveError {
>>   }
>>   
>>   #[derive(Eq, PartialEq, Hash)]
>> -struct HardLinkInfo {
>> +pub(crate) struct HardLinkInfo {
>>       st_dev: u64,
>>       st_ino: u64,
>>   }
>> diff --git a/pbs-client/src/pxar/look_ahead_cache.rs b/pbs-client/src/pxar/look_ahead_cache.rs
>> new file mode 100644
>> index 000000000..539586271
>> --- /dev/null
>> +++ b/pbs-client/src/pxar/look_ahead_cache.rs
>> @@ -0,0 +1,165 @@
>> +use std::collections::HashSet;
>> +use std::ffi::CString;
>> +use std::ops::Range;
>> +use std::os::unix::io::OwnedFd;
>> +use std::path::PathBuf;
>> +
>> +use nix::sys::stat::FileStat;
>> +
>> +use pxar::encoder::PayloadOffset;
>> +use pxar::Metadata;
>> +
>> +use super::create::*;
>> +
>> +const DEFAULT_CACHE_SIZE: usize = 512;
>> +
>> +pub(crate) struct CacheEntryData {
>> +    pub(crate) fd: OwnedFd,
>> +    pub(crate) c_file_name: CString,
>> +    pub(crate) stat: FileStat,
>> +    pub(crate) metadata: Metadata,
>> +    pub(crate) payload_offset: PayloadOffset,
>> +}
>> +
>> +pub(crate) enum CacheEntry {
>> +    RegEntry(CacheEntryData),
>> +    DirEntry(CacheEntryData),
>> +    DirEnd,
>> +}
>> +
>> +pub(crate) struct PxarLookaheadCache {
>> +    // Current state of the cache
>> +    enabled: bool,
>> +    // Cached entries
>> +    entries: Vec<CacheEntry>,
>> +    // Entries encountered having more than one link given by stat
>> +    hardlinks: HashSet<HardLinkInfo>,
>> +    // Payload range covered by the currently cached entries
>> +    range: Range<u64>,
>> +    // Possible held back last chunk from last flush, used for possible chunk continuation
>> +    last_chunk: Option<ReusableDynamicEntry>,
>> +    // Path when started caching
>> +    start_path: PathBuf,
>> +    // Number of entries with file descriptors
>> +    fd_entries: usize,
>> +    // Max number of entries with file descriptors
>> +    cache_size: usize,
>> +}
>> +
>> +impl PxarLookaheadCache {
>> +    pub(crate) fn new(size: Option<usize>) -> Self {
>> +        Self {
>> +            enabled: false,
>> +            entries: Vec::new(),
>> +            hardlinks: HashSet::new(),
>> +            range: 0..0,
>> +            last_chunk: None,
>> +            start_path: PathBuf::new(),
>> +            fd_entries: 0,
>> +            cache_size: size.unwrap_or(DEFAULT_CACHE_SIZE),
>> +        }
>> +    }
>> +
>> +    pub(crate) fn is_full(&self) -> bool {
>> +        self.fd_entries >= self.cache_size
>> +    }
>> +
>> +    pub(crate) fn caching_enabled(&self) -> bool {
>> +        self.enabled
>> +    }
>> +
>> +    pub(crate) fn insert(
> 
> 2 out of 3 calls to this are preceded by the same call to
> update_start_path.. we could just add the path as parameter here, and
> inline that call and drop update_start_path altogether AFAICT?

Okay, I added this as parameter and moved the whole set if not enabled, 
together with the required pop if not a directory to be inside this 
function.

> 
>> +        &mut self,
>> +        fd: OwnedFd,
>> +        c_file_name: CString,
>> +        stat: FileStat,
>> +        metadata: Metadata,
>> +        payload_offset: PayloadOffset,
>> +    ) {
>> +        self.enabled = true;
>> +        self.fd_entries += 1;
>> +        if metadata.is_dir() {
>> +            self.entries.push(CacheEntry::DirEntry(CacheEntryData {
>> +                fd,
>> +                c_file_name,
>> +                stat,
>> +                metadata,
>> +                payload_offset,
>> +            }))
>> +        } else {
>> +            self.entries.push(CacheEntry::RegEntry(CacheEntryData {
>> +                fd,
>> +                c_file_name,
>> +                stat,
>> +                metadata,
>> +                payload_offset,
>> +            }))
>> +        }
>> +    }
>> +
>> +    pub(crate) fn insert_dir_end(&mut self) {
>> +        self.entries.push(CacheEntry::DirEnd);
>> +    }
>> +
>> +    pub(crate) fn take_and_reset(&mut self) -> Vec<CacheEntry> {
>> +        self.fd_entries = 0;
>> +        self.enabled = false;
>> +        self.start_path.clear();
> 
> start_path is cleared here, and take_and_reset is called
> 
>> +        self.clear_range();
>> +        std::mem::take(&mut self.entries)
>> +    }
>> +
>> +    pub(crate) fn update_start_path(&mut self, path: PathBuf) {
>> +        self.start_path = path;
>> +    }
>> +
>> +    pub(crate) fn start_path(&self) -> &PathBuf {
>> +        &self.start_path
> 
> right after the only call to this..
> 
> so take_and_reset could just take the path as well and return it, and we
> can drop this one here?

agreed, removed this and return it via take_and_reset instead

> 
>> +    }
>> +
>> +    pub(crate) fn contains_hardlink(&self, info: &HardLinkInfo) -> bool {
>> +        self.hardlinks.contains(info)
>> +    }
>> +
>> +    pub(crate) fn insert_hardlink(&mut self, info: HardLinkInfo) -> bool {
>> +        self.hardlinks.insert(info)
>> +    }
>> +
>> +    pub(crate) fn range(&self) -> &Range<u64> {
>> +        &self.range
>> +    }
>> +
>> +    pub(crate) fn update_range(&mut self, range: Range<u64>) {
>> +        self.range = range;
>> +    }
>> +
>> +    pub(crate) fn clear_range(&mut self) {
>> +        // keep end for possible continuation if cache has been cleared because
>> +        // it was full, but further caching would be fine
>> +        self.range = self.range.end..self.range.end
>> +    }
> 
> dangerous name.. clear to me always implies removing everything..
> especially since there is no doc comment on it that gives me such
> important information at the call site.
> 
> buuuut, thankfully this is only called once, and that call is a few
> lines above in take_and_reset, so maybe we can just inline it for now
> and not expose this to accidents?

Removed this as well by inlining as suggested.

> 
>> +
>> +    pub(crate) fn try_extend_range(&mut self, range: Range<u64>) -> bool {
>> +        if self.range.end == 0 {
>> +            // initialize first range to start and end with start of new range
>> +            self.range.start = range.start;
>> +            self.range.end = range.start;
>> +        }
>> +
>> +        // range continued, update end
>> +        if self.range.end == range.start {
>> +            self.range.end = range.end;
>> +            return true;
>> +        }
>> +
>> +        false
>> +    }
>> +
>> +    pub(crate) fn take_last_chunk(&mut self) -> Option<ReusableDynamicEntry> {
>> +        self.last_chunk.take()
>> +    }
>> +
>> +    pub(crate) fn update_last_chunk(&mut self, chunk: Option<ReusableDynamicEntry>) {
>> +        self.last_chunk = chunk;
>> +    }
>> +}
>> diff --git a/pbs-client/src/pxar/mod.rs b/pbs-client/src/pxar/mod.rs
>> index 5248a1956..334759df6 100644
>> --- a/pbs-client/src/pxar/mod.rs
>> +++ b/pbs-client/src/pxar/mod.rs
>> @@ -50,6 +50,7 @@
>>   pub(crate) mod create;
>>   pub(crate) mod dir_stack;
>>   pub(crate) mod extract;
>> +pub(crate) mod look_ahead_cache;
>>   pub(crate) mod metadata;
>>   pub(crate) mod tools;
>>   
>> -- 
>> 2.39.2
>>
>>
>>
>> _______________________________________________
>> pbs-devel mailing list
>> pbs-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>>
>>
>>
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel