Re: [pve-devel] [pbs-devel] [PATCH proxmox-backup-qemu 8/9] add shared_cache module

Re: [pve-devel] [pbs-devel] [PATCH proxmox-backup-qemu 8/9] add shared_cache module

[Wolfgang Bumiller]

> On 06/07/2021 10:03 AM Stefan Reiter <s.reiter@proxmox.com> wrote:
> 
>  
> On 6/4/21 2:16 PM, Wolfgang Bumiller wrote:
> > On Wed, Jun 02, 2021 at 04:38:32PM +0200, Stefan Reiter wrote:
> >> Provides a shared AsyncLruCache of 256MB (w/ 4MB chunks) that can be
> >> used by multiple readers at the same time. It is dropped once no more
> >> readers exist, so the memory gets freed if all QEMU block/pbs instances
> >> disappear.
> >>
> >> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> >> ---
> >>   src/lib.rs          |  7 ++++++-
> >>   src/shared_cache.rs | 36 ++++++++++++++++++++++++++++++++++++
> >>   2 files changed, 42 insertions(+), 1 deletion(-)
> >>   create mode 100644 src/shared_cache.rs
> >>
> >> diff --git a/src/lib.rs b/src/lib.rs
> >> index 05d7b58..aa167f7 100644
> >> --- a/src/lib.rs
> >> +++ b/src/lib.rs
> >> @@ -25,6 +25,7 @@ mod restore;
> >>   use restore::*;
> >>   
> >>   mod tools;
> >> +mod shared_cache;
> >>   
> >>   pub const PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE: u64 = 1024*1024*4;
> >>   
> >> @@ -804,7 +805,11 @@ pub extern "C" fn proxmox_restore_connect_async(
> >>   pub extern "C" fn proxmox_restore_disconnect(handle: *mut ProxmoxRestoreHandle) {
> >>   
> >>       let restore_task = handle as * mut Arc<RestoreTask>;
> >> -    unsafe { Box::from_raw(restore_task) }; //drop(restore_task)
> >> +    let restore_task = unsafe { Box::from_raw(restore_task) };
> >> +    drop(restore_task);
> >> +
> >> +    // after dropping, cache may be unused (if no other handles open)
> >> +    shared_cache::shared_chunk_cache_cleanup();
> >>   }
> >>   
> >>   /// Restore an image (sync)
> >> diff --git a/src/shared_cache.rs b/src/shared_cache.rs
> >> new file mode 100644
> >> index 0000000..bebae5b
> >> --- /dev/null
> >> +++ b/src/shared_cache.rs
> >> @@ -0,0 +1,36 @@
> >> +use once_cell::sync::OnceCell;
> >> +use proxmox_backup::backup::ChunkCache;
> >> +use proxmox_backup::tools::async_lru_cache::AsyncLruCache;
> >> +use std::sync::{Arc, Mutex};
> >> +
> >> +const SHARED_CACHE_CAPACITY: usize = 64; // 256 MB
> >> +static SHARED_CACHE: OnceCell<Mutex<Option<ChunkCache>>> = OnceCell::new();
> > 
> > OnceCell *and* Option is a bit too much, `get_shared_chunk_chache()` can
> > just initialize it in `get_or_init()` directly.
> > 
> 
> Then how do we drop the ChunkCache? I used OnceCell since 
> std::sync::Mutex doesn't seem to have a 'const new', not because of 
> 'Option' semantics.

Right. I forgot about the cleanup.
As for the const Mutex::new... don't we have parking lot in our dependency tree already? ;-)
But yeah, I guess it's fine.

[pve-devel] [PATCH 0/9] Improve live-restore speed and replace AsyncIndexReader

[Stefan Reiter]

This series is the third attempt[0] at substantially improving live-restore
performance. This time, a fully async- and concurrency safe LRU cache is
implemented, and a new CachedChunkReader is used to provide lock-free reading
from a remote chunk source. The big performance improvements come from removing
the global Mutex synchronising all read_image_at requests in the QEMU library
and the shared LRU cache.

The idea of using 'mmap' to alleviate memory pressure was dropped in favor of
using a shared cache between drives. This provides about the same performance
improvement in typical live-restore scenarios, but with a fixed memory
requirement, independant of drives. Adding 'mmap' caching is doable, but the
Rust code really doesn't look very nice with it, and I think it's not necessary
in this version...

Anyway, I figured out how to better benchmark the changes too, and can now
reproduce the performance gains very reliably. Below is a (gnu)plot of running
40 live-restores of a Windows 10 VM with a single 32GB disk, first with the
currently shipping libproxmox-backup-qemu0 (A) and then with the patched one
from this series (B). Testing was done with QEMU 6.0.


 seconds
 240 +-----------------------------------------------------------------------+
     |        +        +        +        +        +        +   A    +        |
 230 |-+                               A        A A                        +-|
     |   A  A A            AA       A                  A     A  A            |
 220 |-+        A AA              A        A                               +-|
     |    A          A   A    A A    A       AA       A             A        |
 210 |-+               A                            A      A      A   A  A A-|
     |                                                                  A    |
 200 |-+                                 A                                 +-|
     |                                                                       |
 190 |-+                                                                   +-|
     |                                                                       |
 180 |-A                                                                   +-|
     |                                                                       |
 170 |-+                                                                   +-|
     |                            B                             B   B        |
 160 |-B                    B   B      B B          B    B                 +-|
     |                   B    B               B                   B     B    |
 150 |-+ BB   B   BB B     B        B      B          B    B          B    B-|
     |      B + B      +        +    B   +   B  B +    B   + B B    +    B   |
 140 +-----------------------------------------------------------------------+
     0        5        10       15       20       25       30       35       40
                                     iteration
     1.0.3-1    A
     patched    B


[0] see: https://lists.proxmox.com/pipermail/pbs-devel/2021-April/002932.html


proxmox-backup: Stefan Reiter (7):
  tools/BroadcastFuture: add testcase for better understanding
  tools: add AsyncLruCache as a wrapper around sync LruCache
  backup: add CachedChunkReader utilizing AsyncLruCache
  backup: add AsyncRead/Seek to CachedChunkReader
  replace AsyncIndexReader with SeekableCachedChunkReader
  backup: remove AsyncIndexReader
  tools/lru_cache: make minimum capacity 1

 src/api2/admin/datastore.rs            |   4 +-
 src/backup.rs                          |   4 +-
 src/backup/async_index_reader.rs       | 215 -------------------------
 src/backup/cached_chunk_reader.rs      | 199 +++++++++++++++++++++++
 src/bin/proxmox_backup_client/mount.rs |   4 +-
 src/tools.rs                           |   1 +
 src/tools/async_lru_cache.rs           | 135 ++++++++++++++++
 src/tools/broadcast_future.rs          |  11 ++
 src/tools/lru_cache.rs                 |   1 +
 9 files changed, 353 insertions(+), 221 deletions(-)
 delete mode 100644 src/backup/async_index_reader.rs
 create mode 100644 src/backup/cached_chunk_reader.rs
 create mode 100644 src/tools/async_lru_cache.rs

proxmox-backup-qemu: Stefan Reiter (2):
  add shared_cache module
  access: use CachedChunkReader

 src/lib.rs          |  7 ++++++-
 src/restore.rs      | 30 ++++++++----------------------
 src/shared_cache.rs | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 23 deletions(-)
 create mode 100644 src/shared_cache.rs

-- 
2.30.2

[pve-devel] [PATCH proxmox-backup-qemu 8/9] add shared_cache module

[Stefan Reiter]

Provides a shared AsyncLruCache of 256MB (w/ 4MB chunks) that can be
used by multiple readers at the same time. It is dropped once no more
readers exist, so the memory gets freed if all QEMU block/pbs instances
disappear.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 src/lib.rs          |  7 ++++++-
 src/shared_cache.rs | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 1 deletion(-)
 create mode 100644 src/shared_cache.rs

diff --git a/src/lib.rs b/src/lib.rs
index 05d7b58..aa167f7 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -25,6 +25,7 @@ mod restore;
 use restore::*;
 
 mod tools;
+mod shared_cache;
 
 pub const PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE: u64 = 1024*1024*4;
 
@@ -804,7 +805,11 @@ pub extern "C" fn proxmox_restore_connect_async(
 pub extern "C" fn proxmox_restore_disconnect(handle: *mut ProxmoxRestoreHandle) {
 
     let restore_task = handle as * mut Arc<RestoreTask>;
-    unsafe { Box::from_raw(restore_task) }; //drop(restore_task)
+    let restore_task = unsafe { Box::from_raw(restore_task) };
+    drop(restore_task);
+
+    // after dropping, cache may be unused (if no other handles open)
+    shared_cache::shared_chunk_cache_cleanup();
 }
 
 /// Restore an image (sync)
diff --git a/src/shared_cache.rs b/src/shared_cache.rs
new file mode 100644
index 0000000..bebae5b
--- /dev/null
+++ b/src/shared_cache.rs
@@ -0,0 +1,36 @@
+use once_cell::sync::OnceCell;
+use proxmox_backup::backup::ChunkCache;
+use proxmox_backup::tools::async_lru_cache::AsyncLruCache;
+use std::sync::{Arc, Mutex};
+
+const SHARED_CACHE_CAPACITY: usize = 64; // 256 MB
+static SHARED_CACHE: OnceCell<Mutex<Option<ChunkCache>>> = OnceCell::new();
+
+pub fn get_shared_chunk_cache() -> ChunkCache {
+    let mut guard = SHARED_CACHE
+        .get_or_init(|| Mutex::new(None))
+        .lock()
+        .unwrap();
+    match &*guard {
+        Some(cache) => Arc::clone(cache),
+        None => {
+            let cache = Arc::new(AsyncLruCache::new(SHARED_CACHE_CAPACITY));
+            *guard = Some(Arc::clone(&cache));
+            cache
+        }
+    }
+}
+
+pub fn shared_chunk_cache_cleanup() {
+    let mut guard = SHARED_CACHE
+        .get_or_init(|| Mutex::new(None))
+        .lock()
+        .unwrap();
+    if let Some(arc) = guard.as_ref() {
+        let refcount = Arc::strong_count(arc);
+        if refcount == 1 {
+            // no one else is using the cache anymore, drop it
+            let _drop = guard.take();
+        }
+    }
+}
-- 
2.30.2

Re: [pve-devel] [pbs-devel] [PATCH proxmox-backup-qemu 8/9] add shared_cache module

[Wolfgang Bumiller]

On Wed, Jun 02, 2021 at 04:38:32PM +0200, Stefan Reiter wrote:
> Provides a shared AsyncLruCache of 256MB (w/ 4MB chunks) that can be
> used by multiple readers at the same time. It is dropped once no more
> readers exist, so the memory gets freed if all QEMU block/pbs instances
> disappear.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  src/lib.rs          |  7 ++++++-
>  src/shared_cache.rs | 36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+), 1 deletion(-)
>  create mode 100644 src/shared_cache.rs
> 
> diff --git a/src/lib.rs b/src/lib.rs
> index 05d7b58..aa167f7 100644
> --- a/src/lib.rs
> +++ b/src/lib.rs
> @@ -25,6 +25,7 @@ mod restore;
>  use restore::*;
>  
>  mod tools;
> +mod shared_cache;
>  
>  pub const PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE: u64 = 1024*1024*4;
>  
> @@ -804,7 +805,11 @@ pub extern "C" fn proxmox_restore_connect_async(
>  pub extern "C" fn proxmox_restore_disconnect(handle: *mut ProxmoxRestoreHandle) {
>  
>      let restore_task = handle as * mut Arc<RestoreTask>;
> -    unsafe { Box::from_raw(restore_task) }; //drop(restore_task)
> +    let restore_task = unsafe { Box::from_raw(restore_task) };
> +    drop(restore_task);
> +
> +    // after dropping, cache may be unused (if no other handles open)
> +    shared_cache::shared_chunk_cache_cleanup();
>  }
>  
>  /// Restore an image (sync)
> diff --git a/src/shared_cache.rs b/src/shared_cache.rs
> new file mode 100644
> index 0000000..bebae5b
> --- /dev/null
> +++ b/src/shared_cache.rs
> @@ -0,0 +1,36 @@
> +use once_cell::sync::OnceCell;
> +use proxmox_backup::backup::ChunkCache;
> +use proxmox_backup::tools::async_lru_cache::AsyncLruCache;
> +use std::sync::{Arc, Mutex};
> +
> +const SHARED_CACHE_CAPACITY: usize = 64; // 256 MB
> +static SHARED_CACHE: OnceCell<Mutex<Option<ChunkCache>>> = OnceCell::new();

OnceCell *and* Option is a bit too much, `get_shared_chunk_chache()` can
just initialize it in `get_or_init()` directly.

> +
> +pub fn get_shared_chunk_cache() -> ChunkCache {
> +    let mut guard = SHARED_CACHE
> +        .get_or_init(|| Mutex::new(None))
> +        .lock()
> +        .unwrap();
> +    match &*guard {
> +        Some(cache) => Arc::clone(cache),
> +        None => {
> +            let cache = Arc::new(AsyncLruCache::new(SHARED_CACHE_CAPACITY));
> +            *guard = Some(Arc::clone(&cache));
> +            cache
> +        }
> +    }
> +}
> +
> +pub fn shared_chunk_cache_cleanup() {
> +    let mut guard = SHARED_CACHE
> +        .get_or_init(|| Mutex::new(None))
> +        .lock()
> +        .unwrap();
> +    if let Some(arc) = guard.as_ref() {
> +        let refcount = Arc::strong_count(arc);
> +        if refcount == 1 {
> +            // no one else is using the cache anymore, drop it
> +            let _drop = guard.take();
> +        }
> +    }
> +}
> -- 
> 2.30.2

Re: [pve-devel] [pbs-devel] [PATCH proxmox-backup-qemu 8/9] add shared_cache module

[Stefan Reiter]

On 6/4/21 2:16 PM, Wolfgang Bumiller wrote:
> On Wed, Jun 02, 2021 at 04:38:32PM +0200, Stefan Reiter wrote:
>> Provides a shared AsyncLruCache of 256MB (w/ 4MB chunks) that can be
>> used by multiple readers at the same time. It is dropped once no more
>> readers exist, so the memory gets freed if all QEMU block/pbs instances
>> disappear.
>>
>> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
>> ---
>>   src/lib.rs          |  7 ++++++-
>>   src/shared_cache.rs | 36 ++++++++++++++++++++++++++++++++++++
>>   2 files changed, 42 insertions(+), 1 deletion(-)
>>   create mode 100644 src/shared_cache.rs
>>
>> diff --git a/src/lib.rs b/src/lib.rs
>> index 05d7b58..aa167f7 100644
>> --- a/src/lib.rs
>> +++ b/src/lib.rs
>> @@ -25,6 +25,7 @@ mod restore;
>>   use restore::*;
>>   
>>   mod tools;
>> +mod shared_cache;
>>   
>>   pub const PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE: u64 = 1024*1024*4;
>>   
>> @@ -804,7 +805,11 @@ pub extern "C" fn proxmox_restore_connect_async(
>>   pub extern "C" fn proxmox_restore_disconnect(handle: *mut ProxmoxRestoreHandle) {
>>   
>>       let restore_task = handle as * mut Arc<RestoreTask>;
>> -    unsafe { Box::from_raw(restore_task) }; //drop(restore_task)
>> +    let restore_task = unsafe { Box::from_raw(restore_task) };
>> +    drop(restore_task);
>> +
>> +    // after dropping, cache may be unused (if no other handles open)
>> +    shared_cache::shared_chunk_cache_cleanup();
>>   }
>>   
>>   /// Restore an image (sync)
>> diff --git a/src/shared_cache.rs b/src/shared_cache.rs
>> new file mode 100644
>> index 0000000..bebae5b
>> --- /dev/null
>> +++ b/src/shared_cache.rs
>> @@ -0,0 +1,36 @@
>> +use once_cell::sync::OnceCell;
>> +use proxmox_backup::backup::ChunkCache;
>> +use proxmox_backup::tools::async_lru_cache::AsyncLruCache;
>> +use std::sync::{Arc, Mutex};
>> +
>> +const SHARED_CACHE_CAPACITY: usize = 64; // 256 MB
>> +static SHARED_CACHE: OnceCell<Mutex<Option<ChunkCache>>> = OnceCell::new();
> 
> OnceCell *and* Option is a bit too much, `get_shared_chunk_chache()` can
> just initialize it in `get_or_init()` directly.
> 

Then how do we drop the ChunkCache? I used OnceCell since 
std::sync::Mutex doesn't seem to have a 'const new', not because of 
'Option' semantics.

>> +
>> +pub fn get_shared_chunk_cache() -> ChunkCache {
>> +    let mut guard = SHARED_CACHE
>> +        .get_or_init(|| Mutex::new(None))
>> +        .lock()
>> +        .unwrap();
>> +    match &*guard {
>> +        Some(cache) => Arc::clone(cache),
>> +        None => {
>> +            let cache = Arc::new(AsyncLruCache::new(SHARED_CACHE_CAPACITY));
>> +            *guard = Some(Arc::clone(&cache));
>> +            cache
>> +        }
>> +    }
>> +}
>> +
>> +pub fn shared_chunk_cache_cleanup() {
>> +    let mut guard = SHARED_CACHE
>> +        .get_or_init(|| Mutex::new(None))
>> +        .lock()
>> +        .unwrap();
>> +    if let Some(arc) = guard.as_ref() {
>> +        let refcount = Arc::strong_count(arc);
>> +        if refcount == 1 {
>> +            // no one else is using the cache anymore, drop it
>> +            let _drop = guard.take();
>> +        }
>> +    }
>> +}
>> -- 
>> 2.30.2