Message-Id: <DCDC2M7N28X2.3Q8XYNEDOGK6A@kernel.org>
Date: Wed, 27 Aug 2025 17:59:49 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Vitaly Wool" <vitaly.wool@...sulko.se>
Cc: "rust-for-linux" <rust-for-linux@...r.kernel.org>, "LKML"
 <linux-kernel@...r.kernel.org>, "Uladzislau Rezki" <urezki@...il.com>,
 "Danilo Krummrich" <dakr@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Vlastimil Babka" <vbabka@...e.cz>, "Lorenzo Stoakes"
 <lorenzo.stoakes@...cle.com>, "Liam R . Howlett" <Liam.Howlett@...cle.com>,
 "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>,
 "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>, "Bjorn
 Roy Baron" <bjorn3_gh@...tonmail.com>, "Andreas Hindborg"
 <a.hindborg@...nel.org>, "Trevor Gross" <tmgross@...ch.edu>, "Johannes
 Weiner" <hannes@...xchg.org>, "Yosry Ahmed" <yosry.ahmed@...ux.dev>, "Nhat
 Pham" <nphamcs@...il.com>, <linux-mm@...ck.org>
Subject: Re: [PATCH v4 2/2] rust: zpool: add abstraction for zpool drivers

On Wed Aug 27, 2025 at 4:24 PM CEST, Vitaly Wool wrote:
>
>
>> On Aug 26, 2025, at 7:02 PM, Benno Lossin <lossin@...nel.org> wrote:
>> 
>> On Sat Aug 23, 2025 at 3:05 PM CEST, Vitaly Wool wrote:
>>> +pub trait ZpoolDriver {
>>> +    /// Opaque Rust representation of `struct zpool`.
>>> +    type Pool: ForeignOwnable;
>> 
>> I think this is the same question that Danilo asked a few versions ago,
>> but why do we need this? Why can't we just use `Self` instead?
>
> It’s convenient to use it in the backend implementation, like in the toy example supplied in the documentation part:
>
> +/// struct MyZpool {
> +///     name: &'static CStr,
> +///     bytes_used: AtomicU64,
> +/// }
> …
> +/// impl ZpoolDriver for MyZpoolDriver {
> +///     type Pool = KBox<MyZpool>;
>
> Does that make sense?

No, why can't it just be like this:

    struct MyZpool {
        name: &'static CStr,
        bytes_used: AtomicU64,
    }
    
    struct MyZpoolDriver;
    
    impl ZpoolDriver for MyZpoolDriver {
        type Error = Infallible;
    
        fn create(name: &'static CStr) -> impl PinInit<Self, Self::Error> {
            MyZpool { name, bytes_used: AtomicU64::new(0) }
        }
    
        fn malloc(&mut self, size: usize, gfp: Flags, _nid: NumaNode) -> Result<usize> {
            let mut pow: usize = 0;
            for n in 6..=PAGE_SHIFT {
                if size <= 1 << n {
                    pow = n;
                    break;
                }
            }
            match pow {
                0 => Err(EINVAL),
                _ => {
                    let vec = KVec::<u64>::with_capacity(1 << (pow - 3), gfp)?;
                    let (ptr, _len, _cap) = vec.into_raw_parts();
                    self.bytes_used.fetch_add(1 << pow, Ordering::Relaxed);
                    Ok(ptr as usize | (pow - 6))
                }
            }
        }
    
        unsafe fn free(&self, handle: usize) {
            let n = (handle & 0x3F) + 3;
            let uptr = handle & !0x3F;
    
            // SAFETY:
            // - uptr comes from handle which points to the KVec allocation from `alloc`.
            // - size == capacity and is coming from the first 6 bits of handle.
            let vec = unsafe { KVec::<u64>::from_raw_parts(uptr as *mut u64, 1 << n, 1 << n) };
            drop(vec);
            self.bytes_used.fetch_sub(1 << (n + 3), Ordering::Relaxed);
        }
    
        unsafe fn read_begin(&self, handle: usize) -> NonNull<u8> {
            let uptr = handle & !0x3F;
            // SAFETY: uptr points to a memory area allocated by KVec
            unsafe { NonNull::new_unchecked(uptr as *mut u8) }
        }
    
        unsafe fn read_end(&self, _handle: usize, _handle_mem: NonNull<u8>) {}
    
        unsafe fn write(&self, handle: usize, handle_mem: NonNull<u8>, mem_len: usize) {
            let uptr = handle & !0x3F;
            // SAFETY: handle_mem is a valid non-null pointer provided by zpool, uptr points to
            // a KVec allocated in `malloc` and is therefore also valid.
            unsafe {
                copy_nonoverlapping(handle_mem.as_ptr().cast(), uptr as *mut c_void, mem_len)
            };
        }
    
        fn total_pages(&self) -> u64 {
            self.bytes_used.load(Ordering::Relaxed) >> PAGE_SHIFT
        }
    }

Also using a `usize` as a handle seems like a bad idea. Use a newtype
wrapper of usize instead. You can also not implement `Copy` and thus get
rid of one of the safety requirements of the `free` function. Not sure
if we can remove the other one as well using more type system magic, but
we could try.


>>> +
>>> +    /// Create a pool.
>>> +    fn create(name: &'static CStr, gfp: Flags) -> Result<Self::Pool>;
>>> +
>>> +    /// Destroy the pool.
>>> +    fn destroy(pool: Self::Pool);
>> 
>> This should just be done via the normal `Drop` trait?
>
> Let me check if I’m getting you right here. I take what you are suggesting is that we require that Pool implements Drop trait and then just do something like:
>
>     extern "C" fn destroy_(pool: *mut c_void) {
>         // SAFETY: The pointer originates from an `into_foreign` call.
>         unsafe { drop(T::Pool::from_foreign(pool)) }
>     }
>
> Is that understanding correct?

Yes, but you don't need to require the type to implement drop.

---
Cheers,
Benno

Powered by blists - more mailing lists