[pbs-devel] [PATCH proxmox-backup 0/5] GC: avoid multiple atime updates

[pbs-devel] [PATCH proxmox-backup 0/5] GC: avoid multiple atime updates

[Christian Ebner]

This patches implement the logic to greatly improve the performance
of phase 1 garbage collection by avoiding multiple atime updates on
the same chunk.

Currently, phase 1 GC iterates over all folders in the datastore
looking and collecting all image index files without taking any
logical assumptions (e.g. namespaces, groups, snapshots, ...). This
is to avoid accidentally missing image index files located in
unexpected paths and therefore not marking their chunks as in use,
leading to potential data losses.

This patches improve phase 1 by inserting encountered index image
paths into a data structure which allows to iterate the index files
in a more logical manner, following the same principle as for
incremental backup snapshots. The index files for the same namespace
and group as well as image filename can therefore be consecutevly
inspected.

Further, by keeping track of already seen and therefore updated chunk
atimes, it is now avoided to update the atime over and over again on the
chunks shared by consecutive backup snaphshots.

To give some ballpark figures, this reduced phase 1 garbage collection
on a real world datastore containing some of my backups from around
2 minutes to about 16 seconds.

Christian Ebner (5):
  datastore: restrict datastores list_images method scope to module
  garbage collection: refactor archive type based chunk marking logic
  garbage collection: add structure for optimized image iteration
  garbage collection: allow to keep track of already touched chunks
  fix #5331: garbage collection: avoid multiple chunk atime updates

 pbs-datastore/src/datastore.rs | 204 ++++++++++++++++++++++++++-------
 1 file changed, 160 insertions(+), 44 deletions(-)

-- 
2.39.5



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

[pbs-devel] [PATCH proxmox-backup 1/5] datastore: restrict datastores list_images method scope to module

[Christian Ebner]

Drop the pub scope for `DataStore`s `list_images` method.

This method is only used to generate a list of index files found in
the datastore for iteration during garbage collection. There are no
other call sites and this is intended to only be used within the
module itself. Allows to be more flexible for future method signature
adaptions.

No functional changes.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 75c0c16ab..a6a91ca79 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -970,7 +970,7 @@ impl DataStore {
         ListGroups::new(Arc::clone(self), ns)?.collect()
     }
 
-    pub fn list_images(&self) -> Result<Vec<PathBuf>, Error> {
+    fn list_images(&self) -> Result<Vec<PathBuf>, Error> {
         let base = self.base_path();
 
         let mut list = vec![];
-- 
2.39.5



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

[pbs-devel] [PATCH proxmox-backup 2/5] garbage collection: refactor archive type based chunk marking logic

[Christian Ebner]

Move the logic for marking in-use chunks by image files based on
archive type and its error handling into its own dedicated method.

This is in preparation for optimizing the iteration order to avoid
multiple atime updates of chunks. The method can then be reused for
both cases, iteration over expected image file paths and unexpected
paths, the latter being iterated separately.

No functional changes.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 48 ++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 19 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index a6a91ca79..eda78193d 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1065,6 +1065,34 @@ impl DataStore {
         Ok(())
     }
 
+    fn mark_used_chunks_do(
+        &self,
+        img: &Path,
+        status: &mut GarbageCollectionStatus,
+        worker: &dyn WorkerTaskContext,
+    ) -> Result<(), Error> {
+        match std::fs::File::open(img) {
+            Ok(file) => {
+                if let Ok(archive_type) = ArchiveType::from_path(img) {
+                    if archive_type == ArchiveType::FixedIndex {
+                        let index = FixedIndexReader::new(file).map_err(|err| {
+                            format_err!("can't read index '{}' - {err}", img.to_string_lossy())
+                        })?;
+                        self.index_mark_used_chunks(index, img, status, worker)?;
+                    } else if archive_type == ArchiveType::DynamicIndex {
+                        let index = DynamicIndexReader::new(file).map_err(|err| {
+                            format_err!("can't read index '{}' - {err}", img.to_string_lossy())
+                        })?;
+                        self.index_mark_used_chunks(index, img, status, worker)?;
+                    }
+                }
+            }
+            Err(err) if err.kind() == io::ErrorKind::NotFound => (), // ignore vanished files
+            Err(err) => bail!("can't open index {} - {err}", img.to_string_lossy()),
+        }
+        Ok(())
+    }
+
     fn mark_used_chunks(
         &self,
         status: &mut GarbageCollectionStatus,
@@ -1090,25 +1118,7 @@ impl DataStore {
                 }
             }
 
-            match std::fs::File::open(&img) {
-                Ok(file) => {
-                    if let Ok(archive_type) = ArchiveType::from_path(&img) {
-                        if archive_type == ArchiveType::FixedIndex {
-                            let index = FixedIndexReader::new(file).map_err(|e| {
-                                format_err!("can't read index '{}' - {}", img.to_string_lossy(), e)
-                            })?;
-                            self.index_mark_used_chunks(index, &img, status, worker)?;
-                        } else if archive_type == ArchiveType::DynamicIndex {
-                            let index = DynamicIndexReader::new(file).map_err(|e| {
-                                format_err!("can't read index '{}' - {}", img.to_string_lossy(), e)
-                            })?;
-                            self.index_mark_used_chunks(index, &img, status, worker)?;
-                        }
-                    }
-                }
-                Err(err) if err.kind() == io::ErrorKind::NotFound => (), // ignore vanished files
-                Err(err) => bail!("can't open index {} - {}", img.to_string_lossy(), err),
-            }
+            self.mark_used_chunks_do(&img, status, worker)?;
 
             let percentage = (i + 1) * 100 / image_count;
             if percentage > last_percentage {
-- 
2.39.5



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

[pbs-devel] [PATCH proxmox-backup 3/5] garbage collection: add structure for optimized image iteration

[Christian Ebner]

Implements the GroupedImageList struct and methods, which groups
index files (image) paths by a hierarchy for optimized iteration
during phase 1 of garbage collection.

Currently, phase 1 of garbage collection iterates over all folders in
the datastore, without considering any logical organization. This is
to avoid missing image indices which might have unexpected paths,
thereby deleting chunks which are still in use by these indices in GC
phase 2.

The new structure helps to iterate over the index files in a more
logical way, without missing strange paths. The hierarchical
organization helps to avoid touching shared chunks of incremental
snapshot backups in a backup group multiple times, by allowing
tracking of these without excessive memory requirements.

Since deduplication happens on a per image basis for subsequent
snapshots, the hierarchy is chosen as follows:
- ns/group
- image filename
- snapshot timestamp

This allows to iterate over consecutive snapshots for the same images
in the same backup namespace and group.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 63 ++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index eda78193d..520f54548 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1,4 +1,5 @@
 use std::collections::{HashMap, HashSet};
+use std::ffi::OsString;
 use std::io::{self, Write};
 use std::os::unix::ffi::OsStrExt;
 use std::os::unix::io::AsRawFd;
@@ -1573,3 +1574,65 @@ impl DataStore {
         Ok(())
     }
 }
+
+struct GroupedImageList {
+    groups: HashMap<String, HashMap<OsString, Vec<(i64, PathBuf)>>>,
+    strange_path_images: Vec<PathBuf>,
+}
+
+impl GroupedImageList {
+    fn new() -> Self {
+        Self {
+            groups: HashMap::new(),
+            strange_path_images: Vec::new(),
+        }
+    }
+
+    fn insert(&mut self, img: &Path, base_path: &Path) -> Result<(), Error> {
+        let img = img.to_path_buf();
+
+        if let Some(backup_dir_path) = img.parent() {
+            let backup_dir_path = backup_dir_path.strip_prefix(base_path)?;
+
+            if let Some(backup_dir_str) = backup_dir_path.to_str() {
+                if let Ok((namespace, backup_dir)) =
+                    pbs_api_types::parse_ns_and_snapshot(backup_dir_str)
+                {
+                    if let Some(filename) = img.file_name() {
+                        let filename = filename.to_os_string();
+                        let group_key = format!("{namespace}/{group}", group = backup_dir.group);
+
+                        if let Some(images) = self.groups.get_mut(&group_key) {
+                            if let Some(snapshots) = images.get_mut(&filename) {
+                                snapshots.push((backup_dir.time, img));
+                            } else {
+                                let snapshots = vec![(backup_dir.time, img)];
+                                images.insert(filename, snapshots);
+                            }
+                        } else {
+                            // ns/group not present, insert new
+                            let snapshots = vec![(backup_dir.time, img)];
+                            let mut images = HashMap::new();
+                            images.insert(filename, snapshots);
+                            self.groups.insert(group_key, images);
+                        }
+                        return Ok(());
+                    }
+                }
+            }
+        }
+
+        self.strange_path_images.push(img);
+        Ok(())
+    }
+
+    fn len(&self) -> usize {
+        let mut count = self.strange_path_images.len();
+        for (_group, images) in self.groups.iter() {
+            for (_image, snapshots) in images.iter() {
+                count += snapshots.len();
+            }
+        }
+        count
+    }
+}
-- 
2.39.5



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

Re: [pbs-devel] [PATCH proxmox-backup 3/5] garbage collection: add structure for optimized image iteration

[Fabian Grünbichler]

On February 21, 2025 3:01 pm, Christian Ebner wrote:
> Implements the GroupedImageList struct and methods, which groups
> index files (image) paths by a hierarchy for optimized iteration
> during phase 1 of garbage collection.
> 
> Currently, phase 1 of garbage collection iterates over all folders in
> the datastore, without considering any logical organization. This is
> to avoid missing image indices which might have unexpected paths,
> thereby deleting chunks which are still in use by these indices in GC
> phase 2.
> 
> The new structure helps to iterate over the index files in a more
> logical way, without missing strange paths. The hierarchical
> organization helps to avoid touching shared chunks of incremental
> snapshot backups in a backup group multiple times, by allowing
> tracking of these without excessive memory requirements.
> 
> Since deduplication happens on a per image basis for subsequent
> snapshots, the hierarchy is chosen as follows:
> - ns/group
> - image filename
> - snapshot timestamp
> 
> This allows to iterate over consecutive snapshots for the same images
> in the same backup namespace and group.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  pbs-datastore/src/datastore.rs | 63 ++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index eda78193d..520f54548 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -1,4 +1,5 @@
>  use std::collections::{HashMap, HashSet};
> +use std::ffi::OsString;
>  use std::io::{self, Write};
>  use std::os::unix::ffi::OsStrExt;
>  use std::os::unix::io::AsRawFd;
> @@ -1573,3 +1574,65 @@ impl DataStore {
>          Ok(())
>      }
>  }
> +
> +struct GroupedImageList {
> +    groups: HashMap<String, HashMap<OsString, Vec<(i64, PathBuf)>>>,

this seems very complicated, couldn't we make it a lot simpler by doing

key: NS + Group (as tuple, using the actual types)
value: (snapshot => index paths)

or even a nested mapping of

NS -> Group -> Snapshot -> Images

and then simply reset when proceeding from one snapshot to the next? the
scope of "in-memory chunk digests" would still be bounded (in a way that
is in line with how we do it in many other parts, like when doing backup
+ restore), and the structure of this helper entity and the way it is
iterated over would feel much more natural.

we could go one step further, but that might eat some of our performance
gains here

- list all images like we did before and store the result in a
  collection that allows fast removal
- iterate normally over the datastore in a structured fashion using the
  existing helpers
-- when proceeding from one snapshot to the next, use the new "don't
retouch chunks" logic
-- remove each visited image path from the list of images
- if any images are left at the end in that list, mark those manually
  (strange or vanished paths, should hopefully be empty or irrelevant)

the main downside is that we'd have to iterate twice (well, not quite
twice, since the hierarchical iterators skip parts outside of the
"known" hierarchy), but we would save all this custom parse-back logic
here. if we keep the parse-back logic, then I think we want to have a
logical structure as well that follows how we normally do things.

I think that the list_images part of GC is by far the least expensive
though (for my test datastore where the GC runtime goes down from 52 to
17s with your patch series, listing images is 12ms of that ;)), so
effectively doing it twice might not hurt that much in practice..

> +    strange_path_images: Vec<PathBuf>,
> +}
> +
> +impl GroupedImageList {
> +    fn new() -> Self {
> +        Self {
> +            groups: HashMap::new(),
> +            strange_path_images: Vec::new(),
> +        }
> +    }
> +
> +    fn insert(&mut self, img: &Path, base_path: &Path) -> Result<(), Error> {
> +        let img = img.to_path_buf();
> +
> +        if let Some(backup_dir_path) = img.parent() {
> +            let backup_dir_path = backup_dir_path.strip_prefix(base_path)?;
> +
> +            if let Some(backup_dir_str) = backup_dir_path.to_str() {
> +                if let Ok((namespace, backup_dir)) =
> +                    pbs_api_types::parse_ns_and_snapshot(backup_dir_str)
> +                {
> +                    if let Some(filename) = img.file_name() {
> +                        let filename = filename.to_os_string();
> +                        let group_key = format!("{namespace}/{group}", group = backup_dir.group);
> +
> +                        if let Some(images) = self.groups.get_mut(&group_key) {
> +                            if let Some(snapshots) = images.get_mut(&filename) {
> +                                snapshots.push((backup_dir.time, img));
> +                            } else {
> +                                let snapshots = vec![(backup_dir.time, img)];
> +                                images.insert(filename, snapshots);
> +                            }
> +                        } else {
> +                            // ns/group not present, insert new
> +                            let snapshots = vec![(backup_dir.time, img)];
> +                            let mut images = HashMap::new();
> +                            images.insert(filename, snapshots);
> +                            self.groups.insert(group_key, images);
> +                        }

this whole part could then be simplified using the Entry API, e.g. if 

    groups: HashMap<(BackupNamespace, String), HashMap<i64, Vec<PathBuf>>>,

then we can just do

                    let group_key = (namespace, backup_dir.group.to_string());

                    let snapshot_map = self.groups.entry(group_key).or_default();
                    let images = snapshot_map.entry(backup_dir.time).or_default();
                    images.push(img);

or similar if we split NS and group..

> +                        return Ok(());
> +                    }
> +                }
> +            }
> +        }
> +
> +        self.strange_path_images.push(img);
> +        Ok(())
> +    }
> +
> +    fn len(&self) -> usize {
> +        let mut count = self.strange_path_images.len();
> +        for (_group, images) in self.groups.iter() {
> +            for (_image, snapshots) in images.iter() {
> +                count += snapshots.len();
> +            }
> +        }
> +        count
> +    }
> +}
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> 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

Re: [pbs-devel] [PATCH proxmox-backup 3/5] garbage collection: add structure for optimized image iteration

[Christian Ebner]

On 3/5/25 14:47, Fabian Grünbichler wrote:
> On February 21, 2025 3:01 pm, Christian Ebner wrote:
>> Implements the GroupedImageList struct and methods, which groups
>> index files (image) paths by a hierarchy for optimized iteration
>> during phase 1 of garbage collection.
>>
>> Currently, phase 1 of garbage collection iterates over all folders in
>> the datastore, without considering any logical organization. This is
>> to avoid missing image indices which might have unexpected paths,
>> thereby deleting chunks which are still in use by these indices in GC
>> phase 2.
>>
>> The new structure helps to iterate over the index files in a more
>> logical way, without missing strange paths. The hierarchical
>> organization helps to avoid touching shared chunks of incremental
>> snapshot backups in a backup group multiple times, by allowing
>> tracking of these without excessive memory requirements.
>>
>> Since deduplication happens on a per image basis for subsequent
>> snapshots, the hierarchy is chosen as follows:
>> - ns/group
>> - image filename
>> - snapshot timestamp
>>
>> This allows to iterate over consecutive snapshots for the same images
>> in the same backup namespace and group.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>>   pbs-datastore/src/datastore.rs | 63 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 63 insertions(+)
>>
>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
>> index eda78193d..520f54548 100644
>> --- a/pbs-datastore/src/datastore.rs
>> +++ b/pbs-datastore/src/datastore.rs
>> @@ -1,4 +1,5 @@
>>   use std::collections::{HashMap, HashSet};
>> +use std::ffi::OsString;
>>   use std::io::{self, Write};
>>   use std::os::unix::ffi::OsStrExt;
>>   use std::os::unix::io::AsRawFd;
>> @@ -1573,3 +1574,65 @@ impl DataStore {
>>           Ok(())
>>       }
>>   }
>> +
>> +struct GroupedImageList {
>> +    groups: HashMap<String, HashMap<OsString, Vec<(i64, PathBuf)>>>,
> 
> this seems very complicated, couldn't we make it a lot simpler by doing
> 
> key: NS + Group (as tuple, using the actual types)
> value: (snapshot => index paths)
> 
> or even a nested mapping of
> 
> NS -> Group -> Snapshot -> Images
> 
> and then simply reset when proceeding from one snapshot to the next? the
> scope of "in-memory chunk digests" would still be bounded (in a way that
> is in line with how we do it in many other parts, like when doing backup
> + restore), and the structure of this helper entity and the way it is
> iterated over would feel much more natural.

The type signature looks more complex than what it is in the end, but I 
do agree that this can be reduced/optimized.

> 
> we could go one step further, but that might eat some of our performance
> gains here
> 
> - list all images like we did before and store the result in a
>    collection that allows fast removal
> - iterate normally over the datastore in a structured fashion using the
>    existing helpers
> -- when proceeding from one snapshot to the next, use the new "don't
> retouch chunks" logic
> -- remove each visited image path from the list of images
> - if any images are left at the end in that list, mark those manually
>    (strange or vanished paths, should hopefully be empty or irrelevant)
> 
> the main downside is that we'd have to iterate twice (well, not quite
> twice, since the hierarchical iterators skip parts outside of the
> "known" hierarchy), but we would save all this custom parse-back logic
> here. if we keep the parse-back logic, then I think we want to have a
> logical structure as well that follows how we normally do things.
> 
> I think that the list_images part of GC is by far the least expensive
> though (for my test datastore where the GC runtime goes down from 52 to
> 17s with your patch series, listing images is 12ms of that ;)), so
> effectively doing it twice might not hurt that much in practice..

I do like this approach more, as this will also detect snapshots as 
"strange paths" when the helpers themself do fail to detect an index 
file for some reason, not just the snapshot being off.

So I will reiterate the patches using this approach, thanks!


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

Re: [pbs-devel] [PATCH proxmox-backup 3/5] garbage collection: add structure for optimized image iteration

[Fabian Grünbichler]

> Christian Ebner <c.ebner@proxmox.com> hat am 07.03.2025 09:24 CET geschrieben:
> 
>  
> On 3/5/25 14:47, Fabian Grünbichler wrote:
> > On February 21, 2025 3:01 pm, Christian Ebner wrote:
> >> Implements the GroupedImageList struct and methods, which groups
> >> index files (image) paths by a hierarchy for optimized iteration
> >> during phase 1 of garbage collection.
> >>
> >> Currently, phase 1 of garbage collection iterates over all folders in
> >> the datastore, without considering any logical organization. This is
> >> to avoid missing image indices which might have unexpected paths,
> >> thereby deleting chunks which are still in use by these indices in GC
> >> phase 2.
> >>
> >> The new structure helps to iterate over the index files in a more
> >> logical way, without missing strange paths. The hierarchical
> >> organization helps to avoid touching shared chunks of incremental
> >> snapshot backups in a backup group multiple times, by allowing
> >> tracking of these without excessive memory requirements.
> >>
> >> Since deduplication happens on a per image basis for subsequent
> >> snapshots, the hierarchy is chosen as follows:
> >> - ns/group
> >> - image filename
> >> - snapshot timestamp
> >>
> >> This allows to iterate over consecutive snapshots for the same images
> >> in the same backup namespace and group.
> >>
> >> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> >> ---
> >>   pbs-datastore/src/datastore.rs | 63 ++++++++++++++++++++++++++++++++++
> >>   1 file changed, 63 insertions(+)
> >>
> >> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> >> index eda78193d..520f54548 100644
> >> --- a/pbs-datastore/src/datastore.rs
> >> +++ b/pbs-datastore/src/datastore.rs
> >> @@ -1,4 +1,5 @@
> >>   use std::collections::{HashMap, HashSet};
> >> +use std::ffi::OsString;
> >>   use std::io::{self, Write};
> >>   use std::os::unix::ffi::OsStrExt;
> >>   use std::os::unix::io::AsRawFd;
> >> @@ -1573,3 +1574,65 @@ impl DataStore {
> >>           Ok(())
> >>       }
> >>   }
> >> +
> >> +struct GroupedImageList {
> >> +    groups: HashMap<String, HashMap<OsString, Vec<(i64, PathBuf)>>>,
> > 
> > this seems very complicated, couldn't we make it a lot simpler by doing
> > 
> > key: NS + Group (as tuple, using the actual types)
> > value: (snapshot => index paths)
> > 
> > or even a nested mapping of
> > 
> > NS -> Group -> Snapshot -> Images
> > 
> > and then simply reset when proceeding from one snapshot to the next? the
> > scope of "in-memory chunk digests" would still be bounded (in a way that
> > is in line with how we do it in many other parts, like when doing backup
> > + restore), and the structure of this helper entity and the way it is
> > iterated over would feel much more natural.
> 
> The type signature looks more complex than what it is in the end, but I 
> do agree that this can be reduced/optimized.

it's not so much the type itself, but the elements (too much String-like
types where we could have proper ones) and the way of grouping things ;)

in the end, it is either a nested map or one with a complex key, no way
around it (well, other than the alternate approach below :))

> > 
> > we could go one step further, but that might eat some of our performance
> > gains here
> > 
> > - list all images like we did before and store the result in a
> >    collection that allows fast removal
> > - iterate normally over the datastore in a structured fashion using the
> >    existing helpers
> > -- when proceeding from one snapshot to the next, use the new "don't
> > retouch chunks" logic
> > -- remove each visited image path from the list of images
> > - if any images are left at the end in that list, mark those manually
> >    (strange or vanished paths, should hopefully be empty or irrelevant)
> > 
> > the main downside is that we'd have to iterate twice (well, not quite
> > twice, since the hierarchical iterators skip parts outside of the
> > "known" hierarchy), but we would save all this custom parse-back logic
> > here. if we keep the parse-back logic, then I think we want to have a
> > logical structure as well that follows how we normally do things.
> > 
> > I think that the list_images part of GC is by far the least expensive
> > though (for my test datastore where the GC runtime goes down from 52 to
> > 17s with your patch series, listing images is 12ms of that ;)), so
> > effectively doing it twice might not hurt that much in practice..
> 
> I do like this approach more, as this will also detect snapshots as 
> "strange paths" when the helpers themself do fail to detect an index 
> file for some reason, not just the snapshot being off.
> 
> So I will reiterate the patches using this approach, thanks!

perf numbers for bigger datastores and the usual pathological candidates
(NFS/Samba, single spinning rust) would be nice in that case - I guess
that with iterating twice we should still end up being faster, as the
reduced chunk accesses should by far outweigh the metadata accesses for
listing (unless you only have a single snapshot per group I guess), but
hard numbers are better than gut feelings :) both duration and number of
file operations might be interesting (maybe for stock, this version and
the next one?)


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

Re: [pbs-devel] [PATCH proxmox-backup 3/5] garbage collection: add structure for optimized image iteration

[Christian Ebner]

On 3/7/25 09:53, Fabian Grünbichler wrote:
> 
>> Christian Ebner <c.ebner@proxmox.com> hat am 07.03.2025 09:24 CET geschrieben:
>>
>>   
>> On 3/5/25 14:47, Fabian Grünbichler wrote:
>>> On February 21, 2025 3:01 pm, Christian Ebner wrote:
>>>> Implements the GroupedImageList struct and methods, which groups
>>>> index files (image) paths by a hierarchy for optimized iteration
>>>> during phase 1 of garbage collection.
>>>>
>>>> Currently, phase 1 of garbage collection iterates over all folders in
>>>> the datastore, without considering any logical organization. This is
>>>> to avoid missing image indices which might have unexpected paths,
>>>> thereby deleting chunks which are still in use by these indices in GC
>>>> phase 2.
>>>>
>>>> The new structure helps to iterate over the index files in a more
>>>> logical way, without missing strange paths. The hierarchical
>>>> organization helps to avoid touching shared chunks of incremental
>>>> snapshot backups in a backup group multiple times, by allowing
>>>> tracking of these without excessive memory requirements.
>>>>
>>>> Since deduplication happens on a per image basis for subsequent
>>>> snapshots, the hierarchy is chosen as follows:
>>>> - ns/group
>>>> - image filename
>>>> - snapshot timestamp
>>>>
>>>> This allows to iterate over consecutive snapshots for the same images
>>>> in the same backup namespace and group.
>>>>
>>>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>>>> ---
>>>>    pbs-datastore/src/datastore.rs | 63 ++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 63 insertions(+)
>>>>
>>>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
>>>> index eda78193d..520f54548 100644
>>>> --- a/pbs-datastore/src/datastore.rs
>>>> +++ b/pbs-datastore/src/datastore.rs
>>>> @@ -1,4 +1,5 @@
>>>>    use std::collections::{HashMap, HashSet};
>>>> +use std::ffi::OsString;
>>>>    use std::io::{self, Write};
>>>>    use std::os::unix::ffi::OsStrExt;
>>>>    use std::os::unix::io::AsRawFd;
>>>> @@ -1573,3 +1574,65 @@ impl DataStore {
>>>>            Ok(())
>>>>        }
>>>>    }
>>>> +
>>>> +struct GroupedImageList {
>>>> +    groups: HashMap<String, HashMap<OsString, Vec<(i64, PathBuf)>>>,
>>>
>>> this seems very complicated, couldn't we make it a lot simpler by doing
>>>
>>> key: NS + Group (as tuple, using the actual types)
>>> value: (snapshot => index paths)
>>>
>>> or even a nested mapping of
>>>
>>> NS -> Group -> Snapshot -> Images
>>>
>>> and then simply reset when proceeding from one snapshot to the next? the
>>> scope of "in-memory chunk digests" would still be bounded (in a way that
>>> is in line with how we do it in many other parts, like when doing backup
>>> + restore), and the structure of this helper entity and the way it is
>>> iterated over would feel much more natural.
>>
>> The type signature looks more complex than what it is in the end, but I
>> do agree that this can be reduced/optimized.
> 
> it's not so much the type itself, but the elements (too much String-like
> types where we could have proper ones) and the way of grouping things ;)
> 
> in the end, it is either a nested map or one with a complex key, no way
> around it (well, other than the alternate approach below :))
> 
>>>
>>> we could go one step further, but that might eat some of our performance
>>> gains here
>>>
>>> - list all images like we did before and store the result in a
>>>     collection that allows fast removal
>>> - iterate normally over the datastore in a structured fashion using the
>>>     existing helpers
>>> -- when proceeding from one snapshot to the next, use the new "don't
>>> retouch chunks" logic
>>> -- remove each visited image path from the list of images
>>> - if any images are left at the end in that list, mark those manually
>>>     (strange or vanished paths, should hopefully be empty or irrelevant)
>>>
>>> the main downside is that we'd have to iterate twice (well, not quite
>>> twice, since the hierarchical iterators skip parts outside of the
>>> "known" hierarchy), but we would save all this custom parse-back logic
>>> here. if we keep the parse-back logic, then I think we want to have a
>>> logical structure as well that follows how we normally do things.
>>>
>>> I think that the list_images part of GC is by far the least expensive
>>> though (for my test datastore where the GC runtime goes down from 52 to
>>> 17s with your patch series, listing images is 12ms of that ;)), so
>>> effectively doing it twice might not hurt that much in practice..
>>
>> I do like this approach more, as this will also detect snapshots as
>> "strange paths" when the helpers themself do fail to detect an index
>> file for some reason, not just the snapshot being off.
>>
>> So I will reiterate the patches using this approach, thanks!
> 
> perf numbers for bigger datastores and the usual pathological candidates
> (NFS/Samba, single spinning rust) would be nice in that case - I guess
> that with iterating twice we should still end up being faster, as the
> reduced chunk accesses should by far outweigh the metadata accesses for
> listing (unless you only have a single snapshot per group I guess), but
> hard numbers are better than gut feelings :) both duration and number of
> file operations might be interesting (maybe for stock, this version and
> the next one?)

Acked, will also look into how to get more representative performance 
statistics for this as well (maybe by setting up a datastore in tmpfs 
and using perf counters), as I noticed that just looking a the runtime 
for this on a regular datastore is not really telling. There are to much 
moving parts involved, leading to a huge spread in the observables.


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

[pbs-devel] [PATCH proxmox-backup 4/5] garbage collection: allow to keep track of already touched chunks

[Christian Ebner]

Implements the `TouchedChunks` struct and methods to keep track of
already touched chunks during garbage collection phase 1, to avoid
multiple computational and I/O intensive atime updates via a syscall.

By inserting a digest, the chunk will be considered as touched and
can be ignored for subsequent encounters. To limit memory usage, the
structure allows to reset the chunk status, flagging them as seen
previous to the reset. A subsequent insert will then flag it as seen
after the reset. Chunks not seen after a reset, will be cleared from
the structure by the next reset call, eliminating them from memory.

This allows to reset the tracking stat after each processes image
index file, to mimic the incremental backup behaviour of known chunks
and limit memory footprint.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 520f54548..f9047820a 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1636,3 +1636,32 @@ impl GroupedImageList {
         count
     }
 }
+
+struct TouchedChunks {
+    list: HashMap<[u8; 32], bool>,
+}
+
+impl TouchedChunks {
+    fn new() -> Self {
+        Self {
+            list: HashMap::new(),
+        }
+    }
+
+    // Clear untouched chunks and reset the touched marker for others.
+    fn reset(&mut self) {
+        let mut new_list = HashMap::new();
+        for (digest, touched) in self.list.drain() {
+            if touched {
+                new_list.insert(digest, false);
+            }
+        }
+        self.list = new_list;
+    }
+
+    // Insert the digest in the list of touched chunks.
+    // Returns true if the chunk was already present, false otherwise.
+    fn insert(&mut self, digest: [u8; 32]) -> bool {
+        self.list.insert(digest, true).is_some()
+    }
+}
-- 
2.39.5



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

[pbs-devel] [PATCH proxmox-backup 5/5] fix #5331: garbage collection: avoid multiple chunk atime updates

[Christian Ebner]

Reduce the number of atime updates on the same chunk by logically
iterating over image index files, following the incremental backup
logic. By inserting paths for encountered images during
`list_images` using the GroupedImageList structure, the iteration
happens now for the same image filenames in the same image namespace
and group in a order based on the snapshot timestamp. For each image,
keep track of the encountered chunk digests, and remember these as
seen for the next snapshot. Chunks which have been encountered in the
previous image index, but are not present anymore are removed from
the list after each image, in order to reduce memory footprint.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5331
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 70 ++++++++++++++++++++--------------
 1 file changed, 42 insertions(+), 28 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index f9047820a..992812269 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -971,14 +971,14 @@ impl DataStore {
         ListGroups::new(Arc::clone(self), ns)?.collect()
     }
 
-    fn list_images(&self) -> Result<Vec<PathBuf>, Error> {
+    fn list_images(&self) -> Result<GroupedImageList, Error> {
         let base = self.base_path();
 
-        let mut list = vec![];
+        let mut list = GroupedImageList::new();
 
         use walkdir::WalkDir;
 
-        let walker = WalkDir::new(base).into_iter();
+        let walker = WalkDir::new(&base).into_iter();
 
         // make sure we skip .chunks (and other hidden files to keep it simple)
         fn is_hidden(entry: &walkdir::DirEntry) -> bool {
@@ -1022,7 +1022,7 @@ impl DataStore {
                 if archive_type == ArchiveType::FixedIndex
                     || archive_type == ArchiveType::DynamicIndex
                 {
-                    list.push(path);
+                    list.insert(&path, &base)?;
                 }
             }
         }
@@ -1035,6 +1035,7 @@ impl DataStore {
         &self,
         index: I,
         file_name: &Path, // only used for error reporting
+        touched_chunks: &mut TouchedChunks,
         status: &mut GarbageCollectionStatus,
         worker: &dyn WorkerTaskContext,
     ) -> Result<(), Error> {
@@ -1045,6 +1046,12 @@ impl DataStore {
             worker.check_abort()?;
             worker.fail_on_shutdown()?;
             let digest = index.index_digest(pos).unwrap();
+
+            // Avoid multiple expensive atime updates by utimensat
+            if touched_chunks.insert(*digest) {
+                continue;
+            }
+
             if !self.inner.chunk_store.cond_touch_chunk(digest, false)? {
                 let hex = hex::encode(digest);
                 warn!(
@@ -1069,6 +1076,7 @@ impl DataStore {
     fn mark_used_chunks_do(
         &self,
         img: &Path,
+        touched_chunks: &mut TouchedChunks,
         status: &mut GarbageCollectionStatus,
         worker: &dyn WorkerTaskContext,
     ) -> Result<(), Error> {
@@ -1079,12 +1087,12 @@ impl DataStore {
                         let index = FixedIndexReader::new(file).map_err(|err| {
                             format_err!("can't read index '{}' - {err}", img.to_string_lossy())
                         })?;
-                        self.index_mark_used_chunks(index, img, status, worker)?;
+                        self.index_mark_used_chunks(index, img, touched_chunks, status, worker)?;
                     } else if archive_type == ArchiveType::DynamicIndex {
                         let index = DynamicIndexReader::new(file).map_err(|err| {
                             format_err!("can't read index '{}' - {err}", img.to_string_lossy())
                         })?;
-                        self.index_mark_used_chunks(index, img, status, worker)?;
+                        self.index_mark_used_chunks(index, img, touched_chunks, status, worker)?;
                     }
                 }
             }
@@ -1099,38 +1107,44 @@ impl DataStore {
         status: &mut GarbageCollectionStatus,
         worker: &dyn WorkerTaskContext,
     ) -> Result<(), Error> {
-        let image_list = self.list_images()?;
+        let mut image_list = self.list_images()?;
         let image_count = image_list.len();
 
         let mut last_percentage: usize = 0;
 
-        let mut strange_paths_count: u64 = 0;
-
-        for (i, img) in image_list.into_iter().enumerate() {
-            worker.check_abort()?;
-            worker.fail_on_shutdown()?;
-
-            if let Some(backup_dir_path) = img.parent() {
-                let backup_dir_path = backup_dir_path.strip_prefix(self.base_path())?;
-                if let Some(backup_dir_str) = backup_dir_path.to_str() {
-                    if pbs_api_types::parse_ns_and_snapshot(backup_dir_str).is_err() {
-                        strange_paths_count += 1;
+        // Optimize for avoiding updates of chunks atime in same group with same
+        // image names multiple times.
+        let mut touched_chunks = TouchedChunks::new();
+        let mut processed_images = 0;
+        for (_group, images) in image_list.groups.iter_mut() {
+            for (_image, snapshots) in images.iter_mut() {
+                // Sort by snapshot timestamp to iterate over consecutive snapshots for each image.
+                snapshots.sort_by(|a, b| a.0.cmp(&b.0));
+                for (_timestamp, img) in snapshots {
+                    worker.check_abort()?;
+                    worker.fail_on_shutdown()?;
+
+                    self.mark_used_chunks_do(img, &mut touched_chunks, status, worker)?;
+                    touched_chunks.reset();
+
+                    let percentage = (processed_images + 1) * 100 / image_count;
+                    if percentage > last_percentage {
+                        info!(
+                            "marked {percentage}% ({} of {image_count} index files)",
+                            processed_images + 1,
+                        );
+                        last_percentage = percentage;
                     }
+                    processed_images += 1;
                 }
             }
+        }
 
-            self.mark_used_chunks_do(&img, status, worker)?;
-
-            let percentage = (i + 1) * 100 / image_count;
-            if percentage > last_percentage {
-                info!(
-                    "marked {percentage}% ({} of {image_count} index files)",
-                    i + 1,
-                );
-                last_percentage = percentage;
-            }
+        for img in &image_list.strange_path_images {
+            self.mark_used_chunks_do(img, &mut touched_chunks, status, worker)?;
         }
 
+        let strange_paths_count = image_list.strange_path_images.len();
         if strange_paths_count > 0 {
             info!(
                 "found (and marked) {strange_paths_count} index files outside of expected directory scheme"
-- 
2.39.5



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

Re: [pbs-devel] [PATCH proxmox-backup 0/5] GC: avoid multiple atime updates

[Roland]

hello,

this looks like this relates to or adresses what's being mentioned here ?

https://forum.proxmox.com/threads/better-understand-proxmox-garbage-collection-lots-of-avoidable-utimensat-calls.101417/
?

it came to my mind when reading this.  not sure if i did an RFE for
this, guess i forgot...

 >for my curiosity, i see there is one chunk in both of my homeserver
repos which is accessed orders of magnitudes more often then all
 >the other chunks (for one repo roundabout 1000x more often) during gc
run. anyhow, many chunks are being acessed repeatedly, but at a much
lower rate.

ah, now i understand why that file is touched 34742 times - it's that
"all zeroes" chunk, which does occur in the vm image much more often.

34742 proxmox-backup-(1428): R
/backup/pve-t620_backup/.chunks/bb9f/bb9f8df61474d25e71fa00722318cd387396ca1736605e1248821cc0de3d3af8

any clue what's this one,  which is being touched an order of magnitude
more often then the zeroes chunk ?

940079 proxmox-backup-(1428): R
/backup/pve-maker-bonn_backup/.chunks/7b27/7b27b1eb4febae3273321255d8304e9b3e7938d9e254564bef859a4307a88638

regards
roland


Am 21.02.25 um 15:01 schrieb Christian Ebner:
> This patches implement the logic to greatly improve the performance
> of phase 1 garbage collection by avoiding multiple atime updates on
> the same chunk.
>
> Currently, phase 1 GC iterates over all folders in the datastore
> looking and collecting all image index files without taking any
> logical assumptions (e.g. namespaces, groups, snapshots, ...). This
> is to avoid accidentally missing image index files located in
> unexpected paths and therefore not marking their chunks as in use,
> leading to potential data losses.
>
> This patches improve phase 1 by inserting encountered index image
> paths into a data structure which allows to iterate the index files
> in a more logical manner, following the same principle as for
> incremental backup snapshots. The index files for the same namespace
> and group as well as image filename can therefore be consecutevly
> inspected.
>
> Further, by keeping track of already seen and therefore updated chunk
> atimes, it is now avoided to update the atime over and over again on the
> chunks shared by consecutive backup snaphshots.
>
> To give some ballpark figures, this reduced phase 1 garbage collection
> on a real world datastore containing some of my backups from around
> 2 minutes to about 16 seconds.
>
> Christian Ebner (5):
>    datastore: restrict datastores list_images method scope to module
>    garbage collection: refactor archive type based chunk marking logic
>    garbage collection: add structure for optimized image iteration
>    garbage collection: allow to keep track of already touched chunks
>    fix #5331: garbage collection: avoid multiple chunk atime updates
>
>   pbs-datastore/src/datastore.rs | 204 ++++++++++++++++++++++++++-------
>   1 file changed, 160 insertions(+), 44 deletions(-)
>

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

Re: [pbs-devel] [PATCH proxmox-backup 0/5] GC: avoid multiple atime updates

[Christian Ebner]

On 2/21/25 16:35, Roland wrote:
> hello,
> 
> this looks like this relates to or adresses what's being mentioned here ?
> 
> https://forum.proxmox.com/threads/better-understand-proxmox-garbage- 
> collection-lots-of-avoidable-utimensat-calls.101417/
> ?
> 
> it came to my mind when reading this.  not sure if i did an RFE for
> this, guess i forgot...

Yes, this has been requested also by others, see the discussion in 
https://bugzilla.proxmox.com/show_bug.cgi?id=5331

The (still initial) title of the issue is a bit misleading, as the 
implementation in the patch series works for all chunks, not just the 
zero chunks. ;)

> 
>  >for my curiosity, i see there is one chunk in both of my homeserver
> repos which is accessed orders of magnitudes more often then all
>  >the other chunks (for one repo roundabout 1000x more often) during gc
> run. anyhow, many chunks are being acessed repeatedly, but at a much
> lower rate.
> 
> ah, now i understand why that file is touched 34742 times - it's that
> "all zeroes" chunk, which does occur in the vm image much more often.

Haven't check that yet, but most likely yes, I do have the same chunk in 
my datastore.

> 
> 34742 proxmox-backup-(1428): R
> /backup/pve-t620_backup/.chunks/bb9f/ 
> bb9f8df61474d25e71fa00722318cd387396ca1736605e1248821cc0de3d3af8
> 
> any clue what's this one,  which is being touched an order of magnitude
> more often then the zeroes chunk ?

I do not have that particular chunk digest in my datastores, but if the 
chunk is not encrypted, you could just decompress it and inspect the 
contents via a hex editor.

> 
> 940079 proxmox-backup-(1428): R
> /backup/pve-maker- 
> bonn_backup/.chunks/7b27/7b27b1eb4febae3273321255d8304e9b3e7938d9e254564bef859a4307a88638
> 
> regards
> roland

Cheers,
Chris



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

Re: [pbs-devel] [PATCH proxmox-backup 0/5] GC: avoid multiple atime updates

[Roland]

 > 34742 proxmox-backup-(1428): R
 > /backup/pve-t620_backup/.chunks/bb9f/
bb9f8df61474d25e71fa00722318cd387396ca1736605e1248821cc0de3d3af8
 > any clue what's this one,  which is being touched an order of magnitude
 > more often then the zeroes chunk ?
 > I do not have that particular chunk digest in my datastores, but if
the chunk is not encrypted, you could just decompress it and inspect the
contents via a hex editor.

apparently, this is also an "all zero" chunk, but from a different
backup datastore where encryption is active.
after thinking twice, this looks logical to me....

Am 21.02.25 um 16:49 schrieb Christian Ebner:
> On 2/21/25 16:35, Roland wrote:
>> hello,
>>
>> this looks like this relates to or adresses what's being mentioned
>> here ?
>>
>> https://forum.proxmox.com/threads/better-understand-proxmox-garbage-
>> collection-lots-of-avoidable-utimensat-calls.101417/
>> ?
>>
>> it came to my mind when reading this.  not sure if i did an RFE for
>> this, guess i forgot...
>
> Yes, this has been requested also by others, see the discussion in
> https://bugzilla.proxmox.com/show_bug.cgi?id=5331
>
> The (still initial) title of the issue is a bit misleading, as the
> implementation in the patch series works for all chunks, not just the
> zero chunks. ;)
>
>>
>>  >for my curiosity, i see there is one chunk in both of my homeserver
>> repos which is accessed orders of magnitudes more often then all
>>  >the other chunks (for one repo roundabout 1000x more often) during gc
>> run. anyhow, many chunks are being acessed repeatedly, but at a much
>> lower rate.
>>
>> ah, now i understand why that file is touched 34742 times - it's that
>> "all zeroes" chunk, which does occur in the vm image much more often.
>
> Haven't check that yet, but most likely yes, I do have the same chunk
> in my datastore.
>
>>
>> 34742 proxmox-backup-(1428): R
>> /backup/pve-t620_backup/.chunks/bb9f/
>> bb9f8df61474d25e71fa00722318cd387396ca1736605e1248821cc0de3d3af8
>>
>> any clue what's this one,  which is being touched an order of magnitude
>> more often then the zeroes chunk ?
>
> I do not have that particular chunk digest in my datastores, but if
> the chunk is not encrypted, you could just decompress it and inspect
> the contents via a hex editor.
>
>>
>> 940079 proxmox-backup-(1428): R
>> /backup/pve-maker-
>> bonn_backup/.chunks/7b27/7b27b1eb4febae3273321255d8304e9b3e7938d9e254564bef859a4307a88638
>>
>> regards
>> roland
>
> Cheers,
> Chris
>
>

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

Re: [pbs-devel] [PATCH proxmox-backup 0/5] GC: avoid multiple atime updates

[Christian Ebner]

superseded-by version 2:
https://lore.proxmox.com/pbs-devel/20250310111634.162156-1-c.ebner@proxmox.com/T/


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