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

["Danilo Krummrich" <dakr@kernel.org>]

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. :)