Re: [PATCH v4 2/2] rust: zpool: add abstraction for zpool drivers

[Vitaly Wool <vitaly.wool@konsulko.se>]

On 8/27/25 17:59, Benno Lossin wrote:
> On Wed Aug 27, 2025 at 4:24 PM CEST, Vitaly Wool wrote:
>>
>>
>>> On Aug 26, 2025, at 7:02 PM, Benno Lossin <lossin@kernel.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
>          }
>      }

It can indeed but then the ZpoolDriver trait will have to be extended 
with functions like into_raw() and from_raw(), because zpool expects 
*mut c_void, so on the Adapter side it will look like

     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::from_raw(gfp)) };
         match pool {
             Err(_) => null_mut(),
             Ok(p) => T::into_raw(p).cast(),
         }
     }

The question is, why does this make it better?

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

Thanks, I'll surely look into this.

>>>> +
>>>> +    /// 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