Re: [pbs-devel] [PATCH proxmox-backup 2/2] client: pxar: fix fuse mount performance for split archives

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On June 10, 2024 5:11 pm, Christian Ebner wrote:
> Adapt to the decoder method changes introduced in the pxar library,
> which were introduced in order to move the consistency check for
> metadata and payload data archives.
> 
> The new location of the check allows to access the pxar archive via
> a `Split` variant reader instance, without penalization when just
> accessing the metadata, not reading any payload data.
> 
> This greatly improves performance when accessing fuse mounted
> archives.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  pbs-client/src/pxar/extract.rs | 33 ++++++++++++++++++---------------
>  src/api2/tape/restore.rs       |  2 +-
>  2 files changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/pbs-client/src/pxar/extract.rs b/pbs-client/src/pxar/extract.rs
> index 38d4ca89a..b3545650e 100644
> --- a/pbs-client/src/pxar/extract.rs
> +++ b/pbs-client/src/pxar/extract.rs
> @@ -374,7 +374,10 @@ where
>                  }
>              }
>              (true, EntryKind::File { size, .. }) => {
> -                let contents = self.decoder.contents();
> +                let contents = match self.decoder.contents() {
> +                    Ok(contents) => contents,
> +                    Err(err) => return Some(Err(err.into())),
> +                };

the error handling below adds context which this part here is lacking.

I think the whole match arm would be easier to understand, while
preserving the context, if written as:

            (true, EntryKind::File { size, .. }) => match self.decoder.contents() {
                Ok(Some(mut contents)) => self.extractor.extract_file(
                    &file_name,
                    metadata,
                    *size,
                    &mut contents,
                    self.extractor
                        .overwrite_flags
                        .contains(OverwriteFlags::FILE),
                ),
                Ok(None) => Err(format_err!(
                    "found regular file entry without contents in archive"
                )),
                Err(err) => Err(err.into()),
            }
            .context(PxarExtractContext::ExtractFile),


>  
>                  if let Some(mut contents) = contents {
>                      self.extractor.extract_file(
> @@ -872,7 +875,8 @@ where
>              match entry.kind() {
>                  EntryKind::File { .. } => {
>                      let size = decoder.content_size().unwrap_or(0);
> -                    tar_add_file(&mut tarencoder, decoder.contents(), size, metadata, path).await?
> +                    let contents = decoder.contents().await?;
> +                    tar_add_file(&mut tarencoder, contents, size, metadata, path).await?
>                  }
>                  EntryKind::Hardlink(link) => {
>                      if !link.data.is_empty() {
> @@ -894,14 +898,9 @@ where
>                                      path
>                                  } else {
>                                      let size = decoder.content_size().unwrap_or(0);
> -                                    tar_add_file(
> -                                        &mut tarencoder,
> -                                        decoder.contents(),
> -                                        size,
> -                                        metadata,
> -                                        path,
> -                                    )
> -                                    .await?;
> +                                    let contents = decoder.contents().await?;
> +                                    tar_add_file(&mut tarencoder, contents, size, metadata, path)
> +                                        .await?;
>                                      hardlinks.insert(realpath.to_owned(), path.to_owned());
>                                      continue;
>                                  }
> @@ -1038,7 +1037,8 @@ where
>                          metadata.stat.mode as u16,
>                          true,
>                      );
> -                    zip.add_entry(entry, decoder.contents())
> +                    let contents = decoder.contents().await?;
> +                    zip.add_entry(entry, contents)
>                          .await
>                          .context("could not send file entry")?;
>                  }
> @@ -1056,7 +1056,8 @@ where
>                          metadata.stat.mode as u16,
>                          true,
>                      );
> -                    zip.add_entry(entry, decoder.contents())
> +                    let contents = decoder.contents().await?;
> +                    zip.add_entry(entry, contents)
>                          .await
>                          .context("could not send file entry")?;
>                  }
> @@ -1279,14 +1280,16 @@ where
>                          .with_context(|| format!("error at entry {file_name_os:?}"))?;
>                  }
>                  EntryKind::File { size, .. } => {
> +                    let mut contents = decoder
> +                        .contents()
> +                        .await?
> +                        .context("found regular file entry without contents in archive")?;
>                      extractor
>                          .async_extract_file(
>                              &file_name,
>                              metadata,
>                              *size,
> -                            &mut decoder
> -                                .contents()
> -                                .context("found regular file entry without contents in archive")?,
> +                            &mut contents,
>                              extractor.overwrite_flags.contains(OverwriteFlags::FILE),
>                          )
>                          .await?
> diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
> index 382909647..2b2025f6d 100644
> --- a/src/api2/tape/restore.rs
> +++ b/src/api2/tape/restore.rs
> @@ -1748,7 +1748,7 @@ fn try_restore_snapshot_archive<R: pxar::decoder::SeqRead>(
>          }
>  
>          let filename = entry.file_name();
> -        let mut contents = match decoder.contents() {
> +        let mut contents = match decoder.contents()? {
>              None => bail!("missing file content"),
>              Some(contents) => contents,
>          };
> -- 
> 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