[pbs-devel] [PATCH v3 proxmox-backup 47/58] client: pxar: add look-ahead caching

[Fabian Grünbichler]

On April 9, 2024 4:53 pm, Christian Ebner wrote:
> On 4/5/24 10:33, Fabian Grünbichler wrote:
>> Quoting Christian Ebner (2024-03-28 13:36:56)
>>> +
>>> +                let boundary = encoder.payload_position()?;
>>> +                let offset =
>>> +                    self.reused_chunks
>>> +                        .insert(indices, boundary, start_padding, end_padding);
>>> +
>>> +                self.caching_enabled = true;
>>> +                let cache_entry = CacheEntry::RegEntry(CacheEntryData::new(
>>> +                    fd,
>>> +                    c_file_name.into(),
>>> +                    *stat,
>>> +                    metadata.clone(),
>>> +                    offset,
>>> +                ));
>>> +                self.cached_entries.push(cache_entry);
>>> +
>>> +                match self.reused_chunks.suggested() {
>>> +                    Suggested::Reuse => self.flush_cached_to_archive(encoder, true, true).await?,
>>> +                    Suggested::Reencode => {
>>> +                        self.flush_cached_to_archive(encoder, false, true).await?
>>> +                    }
>>> +                    Suggested::CheckNext => {}
>>> +                }
>>> +
>>> +                return Ok(());
>>> +            }
>>> +        }
>>> +
>>> +        self.flush_cached_to_archive(encoder, false, true).await?;
>>> +        self.add_entry(encoder, previous_metadata_accessor, fd.as_raw_fd(), c_file_name, stat)
>>> +            .await
>> 
>> this part here is where I think we mishandle some edge cases, like mentioned in
>> the ReusedChunks patch comments.. even keeping back the last chunk doesn't save
>> us from losing some re-usable files sometimes..
> 
> Still need to have a closer look at what you mean exactly here... The 
> code path itself should be fine I think, or am I missing your point here?
> 
> It is rather the match against the `suggested` (now called `action`) 
> where the wrong decision might be made.

yes, sorry for not phrasing that more explicitly - I just meant to say
that we mishandle those wrong "Suggestions" here (because "CheckNext" is
suggested too often, we then proceed with re-encoding if the next file
is not re-usable. there also is the second issue about re-usable file
sequences crossing chunk boundaries and padding that can cause the
suggestion to take a wrong turn).

not like "this code here is broken", but "it does the wrong thing based
on wrong information" ;) the fix is (most likely) in the suggestion
part, not in the handling part here.. but depending on the solution, it
might also be necessary to adapt something here :)