Re: [pbs-devel] [RFC pxar 7/20] fix #3174: encoder: add helper to incr encoder pos

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

> On 28.09.2023 09:04 CEST Wolfgang Bumiller <w.bumiller@proxmox.com> wrote:
> 
>  
> On Wed, Sep 27, 2023 at 02:20:18PM +0200, Christian Ebner wrote:
> > 
> > > On 27.09.2023 14:07 CEST Wolfgang Bumiller <w.bumiller@proxmox.com> wrote:
> > > 
> > >  
> > > 'incr' :S
> > > 
> > > On Fri, Sep 22, 2023 at 09:16:08AM +0200, Christian Ebner wrote:
> > > > Adds a helper to allow to increase the encoder position by a given
> > > > size. This is used to increase the position when adding an appendix
> > > > section to the pxar stream, as these bytes are never encoded directly
> > > > but rather referenced by already existing chunks.
> > > 
> > > Exposing this seems like a weird choice to me. Why exactly is this
> > > needed? Why don't we instead expose methods to actually write the
> > > appendix section instead?
> > 
> > This is needed in order to increase the byte offset of the encoder itself.
> > The appendix section is a list of chunks which are injected in the chunk
> > stream on upload, but never really consumed by the encoder and subsequently
> > the chunker itself. So there is no direct writing of the appendix section to
> > the stream.
> > 
> > By adding the bytes, consistency with the rest of the pxar archive is assured,
> > as these chunks/bytes are present during decoding.
> 
> Ah so we inject the *contents* of the old pxar archive by way of sending
> the chunks a writing "layer" above. Initially I thought the archive
> would contain chunk ids, but this makes more sense. And is unfortunate
> for the API :-)

Yes, an initial approach was to store the chunk ids inline, but that is not
necessary and added unneeded storage overhead. As is, the chunks are appended
to a list to be injected after encoding the regular part of the archive,
while instead of the actual file payload the PXAR_APPENIDX_REF entry with
payload size and offset relative to the PXAR_APPENDIX entry is stored.

This section then contains the concatenated referenced chunks, allowing to
restore file payloads by sequentially skipping to the correct offset and
restoring the payload from there.

> 
> Maybe consider marking the position modification as `unsafe fn`, though?
> I mean it is a foot gun to break the resulting archive with, after all
> ;-)

You are right in that this is to be seen as an unsafe operation. Maybe instead
of the function to be unsafe, the interface could take the list of chunks as
input and shift the position accordingly?
Thereby consuming the chunks and store them for injection afterwards.

That way the ownership of the chunk list would be moved to the encoder rather than
being part of the archiver, as is now. The chunk list might then be passed from the
encoder to be injected to the backup upload stream, although I am not sure if and
how to bypass the chunker in that case.

> 
> But this means we don't have a direct way of creating incremental pxars
> without a PBS context, doesn't it?

This is correct. At the moment the only way to create an incremental pxar
archive is to use the PBS context. Both, index file and catalog are required,
which could in principle also be provided by a command line parameter, but
finally also the actual chunk data is needed. That is currently only provided
during restore of the archive from backup.

> Would it make sense to have a method here which returns a Writer to
> the `EncoderOutput` where we could in theory also just "dump in"
> contents of another actual pxar file (where the byte counting happens
> implicitly), which also has an extra `unsafe fn add_out_of_band_bytes()`
> to do the raw byte count modification?

Yes, this might be possible, but for creating the backup I completely want to
avoid that. This would require to download the chunk data just to inject it
for reuse, which is probably way more expensive and defies the purpose of
reusing the chunks to begin with.

If you intended this to be an addition to the current code, in order to create
a pxar archive with appendix locally, without the PBS context, then yes.
This might be possible by passing the data in form of a `MergedKnownChunk`,
which contains either the raw chunk data or the reused chunks hash and size,
allowing to pass either the data or the digest needed to index it.

> 
> One advantage of having a "starting point" for this type of operation is
> that we'd also force a `flush()` before out-of-band data gets written.
> Otherwise, if we don't need/want this, we should probably just add a
> `flush()` to the encoder we should call before adding any chunks out of
> band, given that Max already tried to sneak in a BufRead/Writers into
> the pxar crate for optimization purposes, IIRC ;-)

Good point, flushing is definitely  required if writes will be buffered to
not break the byte stream.