[pbs-devel] [PATCH v4 pxar proxmox-backup 0/3] fix fuse mount performance for split archives

[pbs-devel] [PATCH v4 pxar proxmox-backup 0/3] fix fuse mount performance for split archives

[Christian Ebner]

Fuse mounts for split pxar archives currently greatly suffer from the
consistency check between metadata and payload data archives, as
these happen already during decoding of the payload reference entry
in the metadata archive. By moving this check to the content reader
instantiation, the performance can be improved significantly, as now
the payload data chunks only need to be fetched and decoded when
actually accessing the file payloads.

Changes since version 3:
- fix missing adaption to tests for already applied patches
- move ContentRange to accessor
- move payload header check to FileContentsImpl new method, make it private
- drop unsafe from `open_contents_at_range`

Changes since version 2:
- Make helper and ContentRange pub(crate) only
- Fix typo in commit message
- Move incorrectly squashed cargo fmt patch into own patch

Changes since version 1:
- Add previously missing check when accessing contents via the accessor
  instead of the decoder.
- Add missing context and refactor file entry extraction branch in pxar
  extract according to suggestions

pxar:

Christian Ebner (2):
  tests: adapt tests to decoder interface changes
  accessor: adapt and restrict contents access

 src/accessor/aio.rs  | 16 ++++++-----
 src/accessor/mod.rs  | 68 ++++++++++++++++++++++++++++++++------------
 src/accessor/sync.rs | 16 ++++++-----
 tests/compat.rs      |  1 +
 tests/simple/fs.rs   |  2 +-
 5 files changed, 70 insertions(+), 33 deletions(-)

proxmox-backup:

Christian Ebner (1):
  client: pxar: fix fuse mount performance for split archives

 pbs-client/src/pxar/extract.rs | 62 ++++++++++++++++------------------
 pbs-pxar-fuse/src/lib.rs       | 19 ++++++-----
 src/api2/tape/restore.rs       |  2 +-
 3 files changed, 41 insertions(+), 42 deletions(-)

-- 
2.39.2



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

[pbs-devel] [PATCH v4 pxar 1/3] tests: adapt tests to decoder interface changes

[Christian Ebner]

The `Decoder`s `contents` method call can fail because of an added
consistency check when using split variant inputs.

Therefore, the additional error has to be handled by the callers.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 3:
- not present

 tests/compat.rs    | 1 +
 tests/simple/fs.rs | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/compat.rs b/tests/compat.rs
index 8f1b778..c9e681a 100644
--- a/tests/compat.rs
+++ b/tests/compat.rs
@@ -125,6 +125,7 @@ fn test_archive() {
     let mut content = Vec::new();
     decoder
         .contents()
+        .expect("failed to get contents from decoder")
         .expect("failed to get contents for file entry")
         .read_to_end(&mut content)
         .expect("failed to read test file contents");
diff --git a/tests/simple/fs.rs b/tests/simple/fs.rs
index 96fcee9..39d1294 100644
--- a/tests/simple/fs.rs
+++ b/tests/simple/fs.rs
@@ -235,7 +235,7 @@ impl Entry {
                 PxarEntryKind::File { size, .. } => {
                     let mut data = Vec::new();
                     decoder
-                        .contents()
+                        .contents()?
                         .ok_or_else(|| {
                             format_err!("failed to get contents for file entry: {:?}", item.path())
                         })?
-- 
2.39.2



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

[pbs-devel] [PATCH v4 pxar 2/3] accessor: adapt and restrict contents access

[Christian Ebner]

Add checks for split variant inputs when accessing the payload
contents via the accessor instance. Both cases, accessing via the
safe `contents` method and via the previousely unsafe
`open_contents_at_range` call are covered.

Reduce possible misuse by wrapping the current plain content range
into an opaque `ContentRange` type with an additional optional
payload reference field to check consistency between the payload
reference encoded in the metadata archive and the payload header'
found in the payload data archive.

Because of the additional type wrapping and the payload header check,
the `open_contents_at_range` is considered safe now, dropping the
previously unsafe implementation.
The corresponding interfaces have been adapted accordingly.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 3:
- move `ContentRange` to accessor
- move payload header check to `FileContentsImpl` new, make it private
- drop unsafe for `open_contents_at_range`
- refactor

 src/accessor/aio.rs  | 16 ++++++-----
 src/accessor/mod.rs  | 68 ++++++++++++++++++++++++++++++++------------
 src/accessor/sync.rs | 16 ++++++-----
 3 files changed, 68 insertions(+), 32 deletions(-)

diff --git a/src/accessor/aio.rs b/src/accessor/aio.rs
index 73b1025..eb89f8f 100644
--- a/src/accessor/aio.rs
+++ b/src/accessor/aio.rs
@@ -7,14 +7,13 @@
 use std::future::Future;
 use std::io;
 use std::mem;
-use std::ops::Range;
 use std::os::unix::fs::FileExt;
 use std::path::Path;
 use std::pin::Pin;
 use std::sync::Arc;
 use std::task::{Context, Poll};
 
-use crate::accessor::{self, cache::Cache, MaybeReady, ReadAt, ReadAtOperation};
+use crate::accessor::{self, cache::Cache, ContentRange, MaybeReady, ReadAt, ReadAtOperation};
 use crate::decoder::aio::Decoder;
 use crate::format::GoodbyeItem;
 use crate::util;
@@ -153,13 +152,16 @@ impl<T: Clone + ReadAt> Accessor<T> {
     ///
     /// This will provide a reader over an arbitrary range of the archive file, so unless this
     /// comes from a actual file entry data, the contents might not make much sense.
-    pub unsafe fn open_contents_at_range(&self, range: Range<u64>) -> FileContents<T> {
-        FileContents {
-            inner: unsafe { self.inner.open_contents_at_range(range) },
+    pub async fn open_contents_at_range(
+        &self,
+        range: &ContentRange,
+    ) -> io::Result<FileContents<T>> {
+        Ok(FileContents {
+            inner: self.inner.open_contents_at_range(range).await?,
             at: 0,
             buffer: Vec::new(),
             future: None,
-        }
+        })
     }
 
     /// Following a hardlink.
@@ -235,7 +237,7 @@ impl<T: Clone + ReadAt> FileEntry<T> {
     }
 
     /// For use with unsafe accessor methods.
-    pub fn content_range(&self) -> io::Result<Option<Range<u64>>> {
+    pub fn content_range(&self) -> io::Result<Option<ContentRange>> {
         self.inner.content_range()
     }
 
diff --git a/src/accessor/mod.rs b/src/accessor/mod.rs
index 92d689d..48605eb 100644
--- a/src/accessor/mod.rs
+++ b/src/accessor/mod.rs
@@ -17,7 +17,7 @@ use endian_trait::Endian;
 
 use crate::binary_tree_array;
 use crate::decoder::{self, DecoderImpl};
-use crate::format::{self, FormatVersion, GoodbyeItem};
+use crate::format::{self, FormatVersion, GoodbyeItem, PayloadRef};
 use crate::util;
 use crate::{Entry, EntryKind, PxarVariant};
 
@@ -54,6 +54,16 @@ impl EntryRangeInfo {
     }
 }
 
+/// Stores a content range to be accessed via the `Accessor` as well as the payload reference to
+/// perform consistency checks on payload references for archives accessed via split variant input.
+#[derive(Clone)]
+pub struct ContentRange {
+    // Range of the content
+    content: Range<u64>,
+    // Optional payload ref
+    payload_ref: Option<PayloadRef>,
+}
+
 /// awaitable version of `ReadAt`.
 async fn read_at<T>(input: &T, buf: &mut [u8], offset: u64) -> io::Result<usize>
 where
@@ -335,13 +345,12 @@ impl<T: Clone + ReadAt> AccessorImpl<T> {
         })
     }
 
-    /// Allow opening arbitrary contents from a specific range.
-    pub unsafe fn open_contents_at_range(&self, range: Range<u64>) -> FileContentsImpl<T> {
-        if let Some((payload_input, _)) = &self.input.payload() {
-            FileContentsImpl::new(payload_input.clone(), range)
-        } else {
-            FileContentsImpl::new(self.input.archive().clone(), range)
-        }
+    /// Open contents at provided range
+    pub async fn open_contents_at_range(
+        &self,
+        range: &ContentRange,
+    ) -> io::Result<FileContentsImpl<T>> {
+        FileContentsImpl::new(&self.input, range).await
     }
 
     /// Following a hardlink breaks a couple of conventions we otherwise have, particularly we will
@@ -758,7 +767,7 @@ impl<T: Clone + ReadAt> FileEntryImpl<T> {
     }
 
     /// For use with unsafe accessor methods.
-    pub fn content_range(&self) -> io::Result<Option<Range<u64>>> {
+    pub fn content_range(&self) -> io::Result<Option<ContentRange>> {
         match self.entry.kind {
             EntryKind::File { offset: None, .. } => {
                 io_bail!("cannot open file, reader provided no offset")
@@ -767,7 +776,10 @@ impl<T: Clone + ReadAt> FileEntryImpl<T> {
                 size,
                 offset: Some(offset),
                 payload_offset: None,
-            } => Ok(Some(offset..(offset + size))),
+            } => Ok(Some(ContentRange {
+                content: offset..(offset + size),
+                payload_ref: None,
+            })),
             // Payload offset beats regular offset if some
             EntryKind::File {
                 size,
@@ -775,7 +787,13 @@ impl<T: Clone + ReadAt> FileEntryImpl<T> {
                 payload_offset: Some(payload_offset),
             } => {
                 let start_offset = payload_offset + size_of::<format::Header>() as u64;
-                Ok(Some(start_offset..start_offset + size))
+                Ok(Some(ContentRange {
+                    content: start_offset..start_offset + size,
+                    payload_ref: Some(PayloadRef {
+                        offset: payload_offset,
+                        size,
+                    }),
+                }))
             }
             _ => Ok(None),
         }
@@ -785,11 +803,8 @@ impl<T: Clone + ReadAt> FileEntryImpl<T> {
         let range = self
             .content_range()?
             .ok_or_else(|| io_format_err!("not a file"))?;
-        if let Some((ref payload_input, _)) = self.input.payload() {
-            Ok(FileContentsImpl::new(payload_input.clone(), range))
-        } else {
-            Ok(FileContentsImpl::new(self.input.archive().clone(), range))
-        }
+
+        FileContentsImpl::new(&self.input, &range).await
     }
 
     #[inline]
@@ -897,8 +912,25 @@ pub(crate) struct FileContentsImpl<T> {
 }
 
 impl<T: Clone + ReadAt> FileContentsImpl<T> {
-    pub fn new(input: T, range: Range<u64>) -> Self {
-        Self { input, range }
+    async fn new(
+        input: &PxarVariant<T, (T, Range<u64>)>,
+        range: &ContentRange,
+    ) -> io::Result<Self> {
+        let (input, range) = if let Some((payload_input, payload_range)) = input.payload() {
+            if let Some(payload_ref) = &range.payload_ref {
+                let header: format::Header =
+                    read_entry_at(payload_input, payload_ref.offset).await?;
+                format::check_payload_header_and_size(&header, payload_ref.size)?;
+            }
+            if payload_range.start > range.content.start || payload_range.end < range.content.end {
+                io_bail!("out of range access for payload");
+            }
+            (payload_input.clone(), range.content.clone())
+        } else {
+            (input.archive().clone(), range.content.clone())
+        };
+
+        Ok(Self { input, range })
     }
 
     #[inline]
diff --git a/src/accessor/sync.rs b/src/accessor/sync.rs
index df2ed23..76e8c03 100644
--- a/src/accessor/sync.rs
+++ b/src/accessor/sync.rs
@@ -1,14 +1,13 @@
 //! Blocking `pxar` random access handling.
 
 use std::io;
-use std::ops::Range;
 use std::os::unix::fs::FileExt;
 use std::path::Path;
 use std::pin::Pin;
 use std::sync::Arc;
 use std::task::Context;
 
-use crate::accessor::{self, cache::Cache, MaybeReady, ReadAt, ReadAtOperation};
+use crate::accessor::{self, cache::Cache, ContentRange, MaybeReady, ReadAt, ReadAtOperation};
 use crate::decoder::Decoder;
 use crate::format::GoodbyeItem;
 use crate::util::poll_result_once;
@@ -142,11 +141,14 @@ impl<T: Clone + ReadAt> Accessor<T> {
     ///
     /// This will provide a reader over an arbitrary range of the archive file, so unless this
     /// comes from a actual file entry data, the contents might not make much sense.
-    pub unsafe fn open_contents_at_range(&self, range: Range<u64>) -> FileContents<T> {
-        FileContents {
-            inner: unsafe { self.inner.open_contents_at_range(range) },
+    pub unsafe fn open_contents_at_range(
+        &self,
+        range: &ContentRange,
+    ) -> io::Result<FileContents<T>> {
+        Ok(FileContents {
+            inner: poll_result_once(self.inner.open_contents_at_range(range))?,
             at: 0,
-        }
+        })
     }
 
     /// Following a hardlink.
@@ -291,7 +293,7 @@ impl<T: Clone + ReadAt> FileEntry<T> {
     }
 
     /// For use with unsafe accessor methods.
-    pub fn content_range(&self) -> io::Result<Option<Range<u64>>> {
+    pub fn content_range(&self) -> io::Result<Option<ContentRange>> {
         self.inner.content_range()
     }
 
-- 
2.39.2



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

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

[Christian Ebner]

Adapt to the decoder/accessor 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 checks 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>
---
changes since version 3:
- adapt to dropped unsafe for `open_contents_at_range`

 pbs-client/src/pxar/extract.rs | 62 ++++++++++++++++------------------
 pbs-pxar-fuse/src/lib.rs       | 19 ++++++-----
 src/api2/tape/restore.rs       |  2 +-
 3 files changed, 41 insertions(+), 42 deletions(-)

diff --git a/pbs-client/src/pxar/extract.rs b/pbs-client/src/pxar/extract.rs
index 38d4ca89a..e1192c73f 100644
--- a/pbs-client/src/pxar/extract.rs
+++ b/pbs-client/src/pxar/extract.rs
@@ -373,26 +373,22 @@ where
                     Ok(())
                 }
             }
-            (true, EntryKind::File { size, .. }) => {
-                let contents = self.decoder.contents();
-
-                if let Some(mut contents) = contents {
-                    self.extractor.extract_file(
-                        &file_name,
-                        metadata,
-                        *size,
-                        &mut contents,
-                        self.extractor
-                            .overwrite_flags
-                            .contains(OverwriteFlags::FILE),
-                    )
-                } else {
-                    Err(format_err!(
-                        "found regular file entry without contents in archive"
-                    ))
-                }
-                .context(PxarExtractContext::ExtractFile)
+            (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),
             (false, _) => Ok(()), // skip this
         };
 
@@ -872,7 +868,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 +891,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 +1030,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 +1049,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 +1273,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/pbs-pxar-fuse/src/lib.rs b/pbs-pxar-fuse/src/lib.rs
index 780a4ddbe..4c26de200 100644
--- a/pbs-pxar-fuse/src/lib.rs
+++ b/pbs-pxar-fuse/src/lib.rs
@@ -5,7 +5,6 @@ use std::ffi::{OsStr, OsString};
 use std::future::Future;
 use std::io;
 use std::mem;
-use std::ops::Range;
 use std::os::unix::ffi::OsStrExt;
 use std::path::Path;
 use std::pin::Pin;
@@ -20,7 +19,7 @@ use futures::sink::SinkExt;
 use futures::stream::{StreamExt, TryStreamExt};
 
 use proxmox_io::vec;
-use pxar::accessor::{self, EntryRangeInfo, ReadAt};
+use pxar::accessor::{self, ContentRange, EntryRangeInfo, ReadAt};
 
 use proxmox_fuse::requests::{self, FuseRequest};
 use proxmox_fuse::{EntryParam, Fuse, ReplyBufState, Request, ROOT_ID};
@@ -130,7 +129,7 @@ struct Lookup {
     inode: u64,
     parent: u64,
     entry_range_info: EntryRangeInfo,
-    content_range: Option<Range<u64>>,
+    content_range: Option<ContentRange>,
 }
 
 impl Lookup {
@@ -138,7 +137,7 @@ impl Lookup {
         inode: u64,
         parent: u64,
         entry_range_info: EntryRangeInfo,
-        content_range: Option<Range<u64>>,
+        content_range: Option<ContentRange>,
     ) -> Box<Lookup> {
         Box::new(Self {
             refs: AtomicUsize::new(1),
@@ -433,13 +432,17 @@ impl SessionImpl {
         }
     }
 
-    fn open_content(&self, lookup: &LookupRef) -> Result<FileContents, Error> {
+    async fn open_content<'a>(&'a self, lookup: &'a LookupRef<'a>) -> Result<FileContents, Error> {
         if is_dir_inode(lookup.inode) {
             io_return!(libc::EISDIR);
         }
 
-        match lookup.content_range.clone() {
-            Some(range) => Ok(unsafe { self.accessor.open_contents_at_range(range) }),
+        match &lookup.content_range {
+            Some(range) => self
+                .accessor
+                .open_contents_at_range(range)
+                .await
+                .map_err(|err| err.into()),
             None => io_return!(libc::EBADF),
         }
     }
@@ -581,7 +584,7 @@ impl SessionImpl {
 
     async fn read(&self, inode: u64, len: usize, offset: u64) -> Result<Vec<u8>, Error> {
         let file = self.get_lookup(inode)?;
-        let content = self.open_content(&file)?;
+        let content = self.open_content(&file).await?;
         let mut buf = vec::undefined(len);
         let mut pos = 0;
         // fuse' read is different from normal read - no short reads allowed except for EOF!
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] applied-series: [PATCH v4 pxar proxmox-backup 0/3] fix fuse mount performance for split archives

[Fabian Grünbichler]

thanks!

On June 12, 2024 3:17 pm, Christian Ebner wrote:
> Fuse mounts for split pxar archives currently greatly suffer from the
> consistency check between metadata and payload data archives, as
> these happen already during decoding of the payload reference entry
> in the metadata archive. By moving this check to the content reader
> instantiation, the performance can be improved significantly, as now
> the payload data chunks only need to be fetched and decoded when
> actually accessing the file payloads.
> 
> Changes since version 3:
> - fix missing adaption to tests for already applied patches
> - move ContentRange to accessor
> - move payload header check to FileContentsImpl new method, make it private
> - drop unsafe from `open_contents_at_range`
> 
> Changes since version 2:
> - Make helper and ContentRange pub(crate) only
> - Fix typo in commit message
> - Move incorrectly squashed cargo fmt patch into own patch
> 
> Changes since version 1:
> - Add previously missing check when accessing contents via the accessor
>   instead of the decoder.
> - Add missing context and refactor file entry extraction branch in pxar
>   extract according to suggestions
> 
> pxar:
> 
> Christian Ebner (2):
>   tests: adapt tests to decoder interface changes
>   accessor: adapt and restrict contents access
> 
>  src/accessor/aio.rs  | 16 ++++++-----
>  src/accessor/mod.rs  | 68 ++++++++++++++++++++++++++++++++------------
>  src/accessor/sync.rs | 16 ++++++-----
>  tests/compat.rs      |  1 +
>  tests/simple/fs.rs   |  2 +-
>  5 files changed, 70 insertions(+), 33 deletions(-)
> 
> proxmox-backup:
> 
> Christian Ebner (1):
>   client: pxar: fix fuse mount performance for split archives
> 
>  pbs-client/src/pxar/extract.rs | 62 ++++++++++++++++------------------
>  pbs-pxar-fuse/src/lib.rs       | 19 ++++++-----
>  src/api2/tape/restore.rs       |  2 +-
>  3 files changed, 41 insertions(+), 42 deletions(-)
> 
> -- 
> 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