[PATCH v2] rust: zpool: add abstraction for zpool drivers

[PATCH v2] rust: zpool: add abstraction for zpool drivers

[Vitaly Wool]

Zpool is a common frontend for memory storage pool implementations.
These pools are typically used to store compressed memory objects,
e. g. for Zswap, the lightweight compressed cache for swap pages.

This patch provides the interface to use Zpool in Rust kernel code,
thus enabling Rust implementations of Zpool allocators for Zswap.

Co-developed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Vitaly Wool <vitaly.wool@konsulko.se>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/zpool.c            |   6 +
 rust/kernel/alloc.rs            |   5 +
 rust/kernel/lib.rs              |   2 +
 rust/kernel/zpool.rs            | 262 ++++++++++++++++++++++++++++++++
 6 files changed, 277 insertions(+)
 create mode 100644 rust/helpers/zpool.c
 create mode 100644 rust/kernel/zpool.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 84d60635e8a9..f0c4c454882b 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -75,6 +75,7 @@
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 #include <linux/xarray.h>
+#include <linux/zpool.h>
 #include <trace/events/rust_sample.h>
 
 #if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 7cf7fe95e41d..e1a7556cc700 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -51,3 +51,4 @@
 #include "wait.c"
 #include "workqueue.c"
 #include "xarray.c"
+#include "zpool.c"
diff --git a/rust/helpers/zpool.c b/rust/helpers/zpool.c
new file mode 100644
index 000000000000..71ba173f917a
--- /dev/null
+++ b/rust/helpers/zpool.c
@@ -0,0 +1,6 @@
+#include <linux/zpool.h>
+
+void rust_helper_zpool_register_driver(struct zpool_driver *driver)
+{
+	zpool_register_driver(driver);
+}
diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
index b39c279236f5..0fec5337908c 100644
--- a/rust/kernel/alloc.rs
+++ b/rust/kernel/alloc.rs
@@ -41,6 +41,11 @@
 pub struct Flags(u32);
 
 impl Flags {
+    /// Create from the raw representation
+    pub fn new(f: u32) -> Self {
+        Self(f)
+    }
+
     /// Get the raw representation of this flag.
     pub(crate) fn as_raw(self) -> u32 {
         self.0
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index ed53169e795c..165d52feeea4 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -129,6 +129,8 @@
 pub mod uaccess;
 pub mod workqueue;
 pub mod xarray;
+#[cfg(CONFIG_ZPOOL)]
+pub mod zpool;
 
 #[doc(hidden)]
 pub use bindings;
diff --git a/rust/kernel/zpool.rs b/rust/kernel/zpool.rs
new file mode 100644
index 000000000000..1d143ff7fed1
--- /dev/null
+++ b/rust/kernel/zpool.rs
@@ -0,0 +1,262 @@
+use crate::{
+    bindings,
+    error::Result,
+    kernel::alloc::Flags,
+    str::CStr,
+    types::{ForeignOwnable, Opaque},
+};
+use core::ffi::{c_int, c_uchar, c_void};
+use core::ptr::{null_mut, NonNull};
+use kernel::alloc::NumaNode;
+use kernel::driver;
+use kernel::ThisModule;
+
+/// Zpool API.
+///
+/// The [`ZpoolDriver`] trait serves as an interface for Zpool drivers implemented in Rust.
+/// Such drivers implement memory storage pools in accordance with the zpool API.
+///
+/// # Example
+///
+/// A zpool driver implementation which does nothing but prints pool name on its creation and
+/// destruction, and panics if zswap tries to actually read from a pool's alleged object.
+///
+/// ```
+/// use core::ptr::NonNull;
+/// use kernel::alloc::{Flags, KBox, NumaNode};
+/// use kernel::zpool::*;
+///
+/// struct MyZpool {
+///     name: &'static CStr,
+/// }
+///
+/// struct MyZpoolDriver;
+///
+/// impl ZpoolDriver for MyZpoolDriver {
+///     type Pool = KBox<MyZpool>;
+///
+///     fn create(name: &'static CStr, gfp: Flags) -> Result<KBox<MyZpool>> {
+///         let myPool = MyZpool { name };
+///         let mut pool = KBox::new(myPool, gfp)?;
+///
+///         pr_info!("Created pool {}\n", pool.name);
+///         Ok(pool)
+///     }
+///     fn destroy(p: KBox<MyZpool>) {
+///         let pool = KBox::into_inner(p);
+///         pr_info!("Removed pool {}\n", pool.name);
+///     }
+///     fn malloc(_pool: &mut MyZpool, _size: usize, _gfp: Flags, _nid: NumaNode) -> Result<usize> {
+///         Ok(0) // TODO
+///     }
+///     fn free(_pool: &MyZpool, _handle: usize) {
+///         // TODO
+///     }
+///     fn read_begin(_pool: &MyZpool, _handle: usize) -> NonNull<u8> {
+///         panic!("read_begin not implemented\n"); // TODO
+///     }
+///     fn read_end(_pool: &MyZpool, _handle: usize, _handle_mem: NonNull<u8>) {}
+///     fn write(_pool: &MyZpool, _handle: usize, _handle_mem: NonNull<u8>, _mem_len: usize) {}
+///     fn total_pages(_pool: &MyZpool) -> u64 { 0 }
+/// }
+/// ```
+pub trait ZpoolDriver {
+    /// Opaque Rust representation of `struct zpool`.
+    type Pool: ForeignOwnable;
+
+    /// Create a pool.
+    fn create(name: &'static CStr, gfp: Flags) -> Result<Self::Pool>;
+
+    /// Destroy the pool.
+    fn destroy(pool: Self::Pool);
+
+    /// Allocate an object of size `size` using GFP flags `gfp` from the pool `pool`, wuth the
+    /// preferred NUMA node `nid`. If the allocation is successful, an opaque handle is returned.
+    fn malloc(
+        pool: <Self::Pool as ForeignOwnable>::BorrowedMut<'_>,
+        size: usize,
+        gfp: Flags,
+        nid: NumaNode,
+    ) -> Result<usize>;
+
+    /// Free a previously allocated from the `pool` object, represented by `handle`.
+    fn free(pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>, handle: usize);
+
+    /// Make all the necessary preparations for the caller to be able to read from the object
+    /// represented by `handle` and return a valid pointer to the `handle` memory to be read.
+    fn read_begin(pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>, handle: usize)
+        -> NonNull<u8>;
+
+    /// Finish reading from a previously allocated `handle`. `handle_mem` must be the pointer
+    /// previously returned by `read_begin`.
+    fn read_end(
+        pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>,
+        handle: usize,
+        handle_mem: NonNull<u8>,
+    );
+
+    /// Write to the object represented by a previously allocated `handle`. `handle_mem` points
+    /// to the memory to copy data from, and `mem_len` defines the length of the data block to
+    /// be copied.
+    fn write(
+        pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>,
+        handle: usize,
+        handle_mem: NonNull<u8>,
+        mem_len: usize,
+    );
+
+    /// Get the number of pages used by the `pool`.
+    fn total_pages(pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>) -> u64;
+}
+
+/// An "adapter" for the registration of zpool drivers.
+pub struct Adapter<T: ZpoolDriver>(T);
+
+impl<T: ZpoolDriver> Adapter<T> {
+    extern "C" fn create_(name: *const c_uchar, gfp: u32) -> *mut c_void {
+        // SAFETY: the memory pointed to by name is guaranteed by zpool to be a valid string
+        let pool = unsafe { T::create(CStr::from_char_ptr(name), Flags::new(gfp)) };
+        match pool {
+            Err(_) => null_mut(),
+            Ok(p) => T::Pool::into_foreign(p),
+        }
+    }
+    extern "C" fn destroy_(pool: *mut c_void) {
+        // SAFETY: The pointer originates from an `into_foreign` call.
+        T::destroy(unsafe { T::Pool::from_foreign(pool) })
+    }
+    extern "C" fn malloc_(
+        pool: *mut c_void,
+        size: usize,
+        gfp: u32,
+        handle: *mut usize,
+        nid: c_int,
+    ) -> c_int {
+        // SAFETY: The pointer originates from an `into_foreign` call. If `pool` is passed to
+        // `from_foreign`, then that happens in `_destroy` which will not be called during this
+        // method.
+        let pool = unsafe { T::Pool::borrow_mut(pool) };
+        let real_nid = match nid {
+            bindings::NUMA_NO_NODE => Ok(NumaNode::NO_NODE),
+            _ => NumaNode::new(nid),
+        };
+        if real_nid.is_err() {
+            return -(bindings::EINVAL as i32);
+        }
+
+        let result = T::malloc(pool, size, Flags::new(gfp), real_nid.unwrap());
+        match result {
+            Err(_) => -(bindings::ENOMEM as i32),
+            Ok(h) => {
+                // SAFETY: handle is guaranteed to be a valid pointer by zpool
+                unsafe { *handle = h };
+                0
+            }
+        }
+    }
+    extern "C" fn free_(pool: *mut c_void, handle: usize) {
+        // SAFETY: The pointer originates from an `into_foreign` call. If `pool` is passed to
+        // `from_foreign`, then that happens in `_destroy` which will not be called during this
+        // method.
+        let pool = unsafe { T::Pool::borrow(pool) };
+        T::free(pool, handle)
+    }
+    extern "C" fn obj_read_begin_(
+        pool: *mut c_void,
+        handle: usize,
+        _local_copy: *mut c_void,
+    ) -> *mut c_void {
+        // SAFETY: The pointer originates from an `into_foreign` call. If `pool` is passed to
+        // `from_foreign`, then that happens in `_destroy` which will not be called during this
+        // method.
+        let pool = unsafe { T::Pool::borrow(pool) };
+        let non_null_ptr = T::read_begin(pool, handle);
+        non_null_ptr.as_ptr().cast()
+    }
+    extern "C" fn obj_read_end_(pool: *mut c_void, handle: usize, handle_mem: *mut c_void) {
+        // SAFETY: The pointer originates from an `into_foreign` call. If `pool` is passed to
+        // `from_foreign`, then that happens in `_destroy` which will not be called during this
+        // method.
+        let pool = unsafe { T::Pool::borrow(pool) };
+
+        // SAFETY: handle_mem is guaranteed to be non-null by zpool
+        let handle_mem_ptr = unsafe { NonNull::new_unchecked(handle_mem.cast()) };
+        T::read_end(pool, handle, handle_mem_ptr)
+    }
+    extern "C" fn obj_write_(
+        pool: *mut c_void,
+        handle: usize,
+        handle_mem: *mut c_void,
+        mem_len: usize,
+    ) {
+        // SAFETY: The pointer originates from an `into_foreign` call. If `pool` is passed to
+        // `from_foreign`, then that happens in `_destroy` which will not be called during this
+        // method.
+        let pool = unsafe { T::Pool::borrow(pool) };
+
+        // SAFETY: handle_mem is guaranteed to be non-null by zpool
+        let handle_mem_ptr = unsafe { NonNull::new_unchecked(handle_mem.cast()) };
+        T::write(pool, handle, handle_mem_ptr, mem_len);
+    }
+    extern "C" fn total_pages_(pool: *mut c_void) -> u64 {
+        // SAFETY: The pointer originates from an `into_foreign` call. If `pool` is passed to
+        // `from_foreign`, then that happens in `_destroy` which will not be called during this
+        // method.
+        let pool = unsafe { T::Pool::borrow(pool) };
+        T::total_pages(pool)
+    }
+}
+
+// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid
+// because preceding call to `register` never fails for zpool.
+unsafe impl<T: ZpoolDriver + 'static> driver::RegistrationOps for Adapter<T> {
+    type RegType = bindings::zpool_driver;
+
+    unsafe fn register(
+        pdrv: &Opaque<Self::RegType>,
+        name: &'static CStr,
+        _module: &'static ThisModule,
+    ) -> Result {
+        // SAFETY: It's safe to set the fields of `struct zpool_driver` on initialization.
+        unsafe {
+            (*(pdrv.get())).type_ = name.as_char_ptr().cast_mut();
+            (*(pdrv.get())).create = Some(Self::create_);
+            (*(pdrv.get())).destroy = Some(Self::destroy_);
+            (*(pdrv.get())).malloc = Some(Self::malloc_);
+            (*(pdrv.get())).free = Some(Self::free_);
+            (*(pdrv.get())).obj_read_begin = Some(Self::obj_read_begin_);
+            (*(pdrv.get())).obj_read_end = Some(Self::obj_read_end_);
+            (*(pdrv.get())).obj_write = Some(Self::obj_write_);
+            (*(pdrv.get())).total_pages = Some(Self::total_pages_);
+
+            bindings::zpool_register_driver(pdrv.get());
+        }
+        Ok(())
+    }
+    unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
+            // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+            unsafe { bindings::zpool_unregister_driver(pdrv.get()) };
+    }
+}
+
+/// Declares a kernel module that exposes a zpool driver (i. e. an implementation of the zpool API)
+///
+/// # Examples
+///
+///```ignore
+/// kernel::module_zpool_driver! {
+///     type: MyDriver,
+///     name: "Module name",
+///     authors: ["Author name"],
+///     description: "Description",
+///     license: "GPL",
+/// }
+///```
+#[macro_export]
+macro_rules! module_zpool_driver {
+($($f:tt)*) => {
+    $crate::module_driver!(<T>, $crate::zpool::Adapter<T>, { $($f)* });
+};
+}
+
+
-- 
2.39.2

Re: [PATCH v2] rust: zpool: add abstraction for zpool drivers

[Danilo Krummrich]

On Thu Aug 21, 2025 at 1:17 PM CEST, Vitaly Wool wrote:
> Zpool is a common frontend for memory storage pool implementations.
> These pools are typically used to store compressed memory objects,
> e. g. for Zswap, the lightweight compressed cache for swap pages.
>
> This patch provides the interface to use Zpool in Rust kernel code,
> thus enabling Rust implementations of Zpool allocators for Zswap.
>
> Co-developed-by: Alice Ryhl <aliceryhl@google.com>

If this is co-developed by Alice it also needs her SoB. It's either both or none
of them in this case. :)

> Signed-off-by: Vitaly Wool <vitaly.wool@konsulko.se>
> +pub trait ZpoolDriver {
> +    /// Opaque Rust representation of `struct zpool`.
> +    type Pool: ForeignOwnable;
> +    /// Create a pool.
> +    fn create(name: &'static CStr, gfp: Flags) -> Result<Self::Pool>;
> +
> +    /// Destroy the pool.
> +    fn destroy(pool: Self::Pool);
> +
> +    /// Allocate an object of size `size` using GFP flags `gfp` from the pool `pool`, wuth the

typo: "with"

> +    /// preferred NUMA node `nid`. If the allocation is successful, an opaque handle is returned.
> +    fn malloc(
> +        pool: <Self::Pool as ForeignOwnable>::BorrowedMut<'_>,
> +        size: usize,
> +        gfp: Flags,
> +        nid: NumaNode,
> +    ) -> Result<usize>;

I still think we need a proper type representation of a zpool handle that
guarantees validity and manages its lifetime.

For instance, what prevents a caller from calling write() with a random handle?

Looking at zsmalloc(), if I call write() with a random number, I will most
likely oops the kernel. This is not acceptable for safe APIs.

Alternatively, all those trait functions have to be unsafe, which would be very
unfortunate.

> +    /// Free a previously allocated from the `pool` object, represented by `handle`.
> +    fn free(pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>, handle: usize);

What happens if I forget to call free()?

> +    /// Make all the necessary preparations for the caller to be able to read from the object
> +    /// represented by `handle` and return a valid pointer to the `handle` memory to be read.
> +    fn read_begin(pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>, handle: usize)
> +        -> NonNull<u8>;

Same for this, making it a NonNull<u8> is better than a *mut c_void, but it's
still a raw pointer. Nothing prevents users from using this raw pointer after
read_end() has been called.

This needs a type representation that only lives until read_end().

In general, I think this design doesn't really work out well. I think the design
should be something along the lines of:

  (1) We should only provide alloc() on the Zpool itself and which returns a
      Zmem instance. A Zmem instance must not outlive the Zpool it was allocated
      with.

  (2) Zmem should call free() when it is dropped. It should provide read_begin()
      and write() methods.

  (3) Zmem::read_begin() should return a Zslice which must not outlive Zmem and
      calls read_end() when dropped.

> +
> +    /// Finish reading from a previously allocated `handle`. `handle_mem` must be the pointer
> +    /// previously returned by `read_begin`.
> +    fn read_end(
> +        pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>,
> +        handle: usize,
> +        handle_mem: NonNull<u8>,
> +    );
> +
> +    /// Write to the object represented by a previously allocated `handle`. `handle_mem` points
> +    /// to the memory to copy data from, and `mem_len` defines the length of the data block to
> +    /// be copied.
> +    fn write(
> +        pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>,
> +        handle: usize,
> +        handle_mem: NonNull<u8>,
> +        mem_len: usize,
> +    );
> +
> +    /// Get the number of pages used by the `pool`.
> +    fn total_pages(pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>) -> u64;
> +}
> +
> +/// An "adapter" for the registration of zpool drivers.
> +pub struct Adapter<T: ZpoolDriver>(T);
> +
> +impl<T: ZpoolDriver> Adapter<T> {
> +    extern "C" fn create_(name: *const c_uchar, gfp: u32) -> *mut c_void {
> +        // SAFETY: the memory pointed to by name is guaranteed by zpool to be a valid string
> +        let pool = unsafe { T::create(CStr::from_char_ptr(name), Flags::new(gfp)) };
> +        match pool {
> +            Err(_) => null_mut(),
> +            Ok(p) => T::Pool::into_foreign(p),
> +        }
> +    }
> +    extern "C" fn destroy_(pool: *mut c_void) {
> +        // SAFETY: The pointer originates from an `into_foreign` call.
> +        T::destroy(unsafe { T::Pool::from_foreign(pool) })
> +    }
> +    extern "C" fn malloc_(
> +        pool: *mut c_void,
> +        size: usize,
> +        gfp: u32,
> +        handle: *mut usize,
> +        nid: c_int,
> +    ) -> c_int {
> +        // SAFETY: The pointer originates from an `into_foreign` call. If `pool` is passed to
> +        // `from_foreign`, then that happens in `_destroy` which will not be called during this
> +        // method.
> +        let pool = unsafe { T::Pool::borrow_mut(pool) };
> +        let real_nid = match nid {
> +            bindings::NUMA_NO_NODE => Ok(NumaNode::NO_NODE),
> +            _ => NumaNode::new(nid),
> +        };
> +        if real_nid.is_err() {
> +            return -(bindings::EINVAL as i32);
> +        }
> +
> +        let result = T::malloc(pool, size, Flags::new(gfp), real_nid.unwrap());

Please don't use unwrap() it may panic() the whole kernel. It is equivalent to

	if (ret)
		panic();
	else
		do_something();

in C.

Also, please use from_result() instead.

> +        match result {
> +            Err(_) => -(bindings::ENOMEM as i32),
> +            Ok(h) => {
> +                // SAFETY: handle is guaranteed to be a valid pointer by zpool
> +                unsafe { *handle = h };
> +                0
> +            }
> +        }
> +    }

<snip>

> +/// Declares a kernel module that exposes a zpool driver (i. e. an implementation of the zpool API)
> +///
> +/// # Examples
> +///
> +///```ignore
> +/// kernel::module_zpool_driver! {
> +///     type: MyDriver,
> +///     name: "Module name",
> +///     authors: ["Author name"],
> +///     description: "Description",
> +///     license: "GPL",
> +/// }
> +///```
> +#[macro_export]
> +macro_rules! module_zpool_driver {
> +($($f:tt)*) => {
> +    $crate::module_driver!(<T>, $crate::zpool::Adapter<T>, { $($f)* });
> +};
> +}

Thanks for sticking to the existing generic infrastructure, this looks much
better. :)

Re: [PATCH v2] rust: zpool: add abstraction for zpool drivers

[Danilo Krummrich]

On Thu Aug 21, 2025 at 2:03 PM CEST, Danilo Krummrich wrote:
> On Thu Aug 21, 2025 at 1:17 PM CEST, Vitaly Wool wrote:
>> +    /// preferred NUMA node `nid`. If the allocation is successful, an opaque handle is returned.
>> +    fn malloc(
>> +        pool: <Self::Pool as ForeignOwnable>::BorrowedMut<'_>,
>> +        size: usize,
>> +        gfp: Flags,
>> +        nid: NumaNode,
>> +    ) -> Result<usize>;
>
> I still think we need a proper type representation of a zpool handle that
> guarantees validity and manages its lifetime.
>
> For instance, what prevents a caller from calling write() with a random handle?
>
> Looking at zsmalloc(), if I call write() with a random number, I will most
> likely oops the kernel. This is not acceptable for safe APIs.
>
> Alternatively, all those trait functions have to be unsafe, which would be very
> unfortunate.

I just noticed that I confused something here. :)

So, for the backend driver this trait is obviously fine, since you have to implement
the C ops -- sorry for the confusion.

However, you still have to mark all functions except alloc() and total_pages()
as unsafe and document and justify the corresponding safety requirements.

>> +    /// Free a previously allocated from the `pool` object, represented by `handle`.
>> +    fn free(pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>, handle: usize);
>
> What happens if I forget to call free()?
>
>> +    /// Make all the necessary preparations for the caller to be able to read from the object
>> +    /// represented by `handle` and return a valid pointer to the `handle` memory to be read.
>> +    fn read_begin(pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>, handle: usize)
>> +        -> NonNull<u8>;
>
> Same for this, making it a NonNull<u8> is better than a *mut c_void, but it's
> still a raw pointer. Nothing prevents users from using this raw pointer after
> read_end() has been called.
>
> This needs a type representation that only lives until read_end().
>
> In general, I think this design doesn't really work out well. I think the design
> should be something along the lines of:
>
>   (1) We should only provide alloc() on the Zpool itself and which returns a
>       Zmem instance. A Zmem instance must not outlive the Zpool it was allocated
>       with.
>
>   (2) Zmem should call free() when it is dropped. It should provide read_begin()
>       and write() methods.
>
>   (3) Zmem::read_begin() should return a Zslice which must not outlive Zmem and
>       calls read_end() when dropped.

This design is obiously for when you want to use a Zpool, but not implement its
backend. :)

Re: [PATCH v2] rust: zpool: add abstraction for zpool drivers

[Vitaly Wool]

On 8/21/25 14:32, Danilo Krummrich wrote:
> On Thu Aug 21, 2025 at 2:03 PM CEST, Danilo Krummrich wrote:
>> On Thu Aug 21, 2025 at 1:17 PM CEST, Vitaly Wool wrote:
>>> +    /// preferred NUMA node `nid`. If the allocation is successful, an opaque handle is returned.
>>> +    fn malloc(
>>> +        pool: <Self::Pool as ForeignOwnable>::BorrowedMut<'_>,
>>> +        size: usize,
>>> +        gfp: Flags,
>>> +        nid: NumaNode,
>>> +    ) -> Result<usize>;
>>
>> I still think we need a proper type representation of a zpool handle that
>> guarantees validity and manages its lifetime.
>>
>> For instance, what prevents a caller from calling write() with a random handle?
>>
>> Looking at zsmalloc(), if I call write() with a random number, I will most
>> likely oops the kernel. This is not acceptable for safe APIs.
>>
>> Alternatively, all those trait functions have to be unsafe, which would be very
>> unfortunate.
> 
> I just noticed that I confused something here. :)
> 
> So, for the backend driver this trait is obviously fine, since you have to implement
> the C ops -- sorry for the confusion.
> 
> However, you still have to mark all functions except alloc() and total_pages()
> as unsafe and document and justify the corresponding safety requirements.

How is destroy() different from alloc() in terms of safety? I believe 
it's only free, read_{begin|end}, write that should be marked as unsafe.

>>> +    /// Free a previously allocated from the `pool` object, represented by `handle`.
>>> +    fn free(pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>, handle: usize);
>>
>> What happens if I forget to call free()?
>>
>>> +    /// Make all the necessary preparations for the caller to be able to read from the object
>>> +    /// represented by `handle` and return a valid pointer to the `handle` memory to be read.
>>> +    fn read_begin(pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>, handle: usize)
>>> +        -> NonNull<u8>;
>>
>> Same for this, making it a NonNull<u8> is better than a *mut c_void, but it's
>> still a raw pointer. Nothing prevents users from using this raw pointer after
>> read_end() has been called.
>>
>> This needs a type representation that only lives until read_end().
>>
>> In general, I think this design doesn't really work out well. I think the design
>> should be something along the lines of:
>>
>>    (1) We should only provide alloc() on the Zpool itself and which returns a
>>        Zmem instance. A Zmem instance must not outlive the Zpool it was allocated
>>        with.
>>
>>    (2) Zmem should call free() when it is dropped. It should provide read_begin()
>>        and write() methods.
>>
>>    (3) Zmem::read_begin() should return a Zslice which must not outlive Zmem and
>>        calls read_end() when dropped.
> 
> This design is obiously for when you want to use a Zpool, but not implement its
> backend. :)

Re: [PATCH v2] rust: zpool: add abstraction for zpool drivers

[Danilo Krummrich]

On Thu Aug 21, 2025 at 4:15 PM CEST, Vitaly Wool wrote:
>
>
> On 8/21/25 14:32, Danilo Krummrich wrote:
>> On Thu Aug 21, 2025 at 2:03 PM CEST, Danilo Krummrich wrote:
>>> On Thu Aug 21, 2025 at 1:17 PM CEST, Vitaly Wool wrote:
>>>> +    /// preferred NUMA node `nid`. If the allocation is successful, an opaque handle is returned.
>>>> +    fn malloc(
>>>> +        pool: <Self::Pool as ForeignOwnable>::BorrowedMut<'_>,
>>>> +        size: usize,
>>>> +        gfp: Flags,
>>>> +        nid: NumaNode,
>>>> +    ) -> Result<usize>;
>>>
>>> I still think we need a proper type representation of a zpool handle that
>>> guarantees validity and manages its lifetime.
>>>
>>> For instance, what prevents a caller from calling write() with a random handle?
>>>
>>> Looking at zsmalloc(), if I call write() with a random number, I will most
>>> likely oops the kernel. This is not acceptable for safe APIs.
>>>
>>> Alternatively, all those trait functions have to be unsafe, which would be very
>>> unfortunate.
>> 
>> I just noticed that I confused something here. :)
>> 
>> So, for the backend driver this trait is obviously fine, since you have to implement
>> the C ops -- sorry for the confusion.
>> 
>> However, you still have to mark all functions except alloc() and total_pages()
>> as unsafe and document and justify the corresponding safety requirements.
>
> How is destroy() different from alloc() in terms of safety? I believe 
> it's only free, read_{begin|end}, write that should be marked as unsafe.

destroy() should be fine.