[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <87pld778te.fsf@kernel.org>
Date: Thu, 07 Aug 2025 11:57:49 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Boqun Feng <boqun.feng@...il.com>, Miguel Ojeda <ojeda@...nel.org>, Alex
Gaynor <alex.gaynor@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy
Baron <bjorn3_gh@...tonmail.com>, Benno Lossin <lossin@...nel.org>, Trevor
Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>, Jens Axboe
<axboe@...nel.dk>, linux-block@...r.kernel.org,
rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 13/16] rust: block: add `GenDisk` private data support
"Alice Ryhl" <aliceryhl@...gle.com> writes:
> On Fri, Jul 11, 2025 at 01:43:14PM +0200, Andreas Hindborg wrote:
..
>> @@ -97,7 +98,14 @@ pub fn build<T: Operations>(
>> 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(|| {
>> + // SAFETY: T::QueueData was created by the call to `into_foreign()` above
>> + unsafe { T::QueueData::from_foreign(data) };
>
> I think this is clearer to read as
>
> drop(unsafe { T::QueueData::from_foreign(data) });
OK.
>
>> + });
>> +
>> // SAFETY: `bindings::queue_limits` contain only fields that are valid when zeroed.
>> let mut lim: bindings::queue_limits = unsafe { core::mem::zeroed() };
>>
>> @@ -112,7 +120,7 @@ pub fn build<T: Operations>(
>> bindings::__blk_mq_alloc_disk(
>> tagset.raw_tag_set(),
>> &mut lim,
>> - core::ptr::null_mut(),
>> + data.cast(),
>> static_lock_class!().as_ptr(),
>> )
>> })?;
>> @@ -166,8 +174,12 @@ pub fn build<T: Operations>(
>> },
>> )?;
>>
>> + recover_data.dismiss();
>> +
>> // INVARIANT: `gendisk` was initialized above.
>> // INVARIANT: `gendisk` was added to the VFS via `device_add_disk` above.
>> + // INVARIANT: `gendisk.queue.queue_data` is set to `data` in the call to
>> + // `__blk_mq_alloc_disk` above.
>> Ok(GenDisk {
>> _tagset: tagset,
>> gendisk,
>> @@ -179,9 +191,10 @@ pub fn build<T: Operations>(
>> ///
>> /// # Invariants
>> ///
>> -/// - `gendisk` must always point to an initialized and valid `struct gendisk`.
>> -/// - `gendisk` was added to the VFS through a call to
>> -/// `bindings::device_add_disk`.
>> +/// - `gendisk` must always point to an initialized and valid `struct gendisk`.
>> +/// - `gendisk` was added to the VFS through a call to
>> +/// `bindings::device_add_disk`.
>> +/// - `self.gendisk.queue.queuedata` is initialized by a call to `ForeignOwnable::into_foreign`.
>> pub struct GenDisk<T: Operations> {
>> _tagset: Arc<TagSet<T>>,
>> gendisk: *mut bindings::gendisk,
>> @@ -193,9 +206,20 @@ unsafe impl<T: Operations + Send> Send for GenDisk<T> {}
>>
>> 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) };
>> +
>> + // 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.
>> + let _queue_data = unsafe { T::QueueData::from_foreign(queue_data.cast()) };
>
> Ditto here.
>
> drop(unsafe { T::QueueData::from_foreign(queue_data.cast()) });
Thanks, I agree.
>
> Also, is this cast necessary as of
> https://lore.kernel.org/all/20250711-rnull-up-v6-16-v3-13-3a262b4e2921@kernel.org/
> ?
I think you have the wrong link, you probably refer to the recent
`ForeignOwnable` change. And yes, the cast is redundant.
Best regards,
Andreas Hindborg
Powered by blists - more mailing lists