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

["Benno Lossin" <lossin@kernel.org>]

On Thu Aug 28, 2025 at 9:22 AM CEST, Vitaly Wool wrote:
>
>
> 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?

No, thanks for sharing that function. Then the question becomes, do you
really need `ForeignOwnable`? Or is `KBox` enough? Do you really want
people to use `Arc<MyZPool>`? Because `BorrowedMut` of `Arc` is the same
as it's `Borrowed` variant (it's read-only after all).

If you can get away with just `Box` (you might want people to choose
their allocator, which is fine IMO), then I'd do so.

---
Cheers,
Benno