Re: [PATCH v4 12/15] rust: block: add `GenDisk` private data support

[Andreas Hindborg <a.hindborg@kernel.org>]

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Tue, Aug 12, 2025 at 10:44:30AM +0200, Andreas Hindborg wrote:
>> Allow users of the rust block device driver API to install private data in
>> the `GenDisk` structure.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>
> Overall LGTM.
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>
>>          self,
>>          name: fmt::Arguments<'_>,
>>          tagset: Arc<TagSet<T>>,
>> +        queue_data: T::QueueData,
>>      ) -> Result<GenDisk<T>> {
>> +        let data = queue_data.into_foreign();
>> +        let recover_data = ScopeGuard::new(|| {
>> +            drop(
>> +                // SAFETY: T::QueueData was created by the call to `into_foreign()` above
>> +                unsafe { T::QueueData::from_foreign(data) },
>> +            );
>
> This is usually formatted as:
>
> // SAFETY: T::QueueData was created by the call to `into_foreign()` above
> drop(unsafe { T::QueueData::from_foreign(data) });

I don't really have a preference, my optimization function was to
minimize distance to the unsafe block. Are there any rust guidelines on this?

>
>>  impl<T: Operations> Drop for GenDisk<T> {
>>      fn drop(&mut self) {
>> +        // SAFETY: By type invariant of `Self`, `self.gendisk` points to a valid
>> +        // and initialized instance of `struct gendisk`, and, `queuedata` was
>> +        // initialized with the result of a call to
>> +        // `ForeignOwnable::into_foreign`.
>> +        let queue_data = unsafe { (*(*self.gendisk).queue).queuedata };
>> +
>>          // SAFETY: By type invariant, `self.gendisk` points to a valid and
>>          // initialized instance of `struct gendisk`, and it was previously added
>>          // to the VFS.
>>          unsafe { bindings::del_gendisk(self.gendisk) };
>> +
>> +        drop(
>> +            // SAFETY: `queue.queuedata` was created by `GenDiskBuilder::build` with
>> +            // a call to `ForeignOwnable::into_foreign` to create `queuedata`.
>> +            // `ForeignOwnable::from_foreign` is only called here.
>> +            unsafe { T::QueueData::from_foreign(queue_data) },
>> +        );
>
> Ditto here.
>
>>          //    reference counted by `ARef` until then.
>>          let rq = unsafe { Request::aref_from_raw((*bd).rq) };
>>
>> +        // SAFETY: `hctx` is valid as required by this function.
>> +        let queue_data = unsafe { (*(*hctx).queue).queuedata };
>> +
>> +        // SAFETY: `queue.queuedata` was created by `GenDisk::try_new()` with a
>> +        // call to `ForeignOwnable::into_pointer()` to create `queuedata`.
>> +        // `ForeignOwnable::from_foreign()` is only called when the tagset is
>> +        // dropped, which happens after we are dropped.
>> +        let queue_data = unsafe { T::QueueData::borrow(queue_data.cast()) };
>
> Is this cast necessary? Is it not a void pointer?

Leftover from old `ForeignOwnable` I think. I'll remove it.


Best regards,
Andreas Hindborg