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@xxxxxxxxxx>
Overall LGTM.
Reviewed-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
> 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) });
> 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?
> @@ -110,9 +129,18 @@ impl<T: Operations> OperationsVTable<T> {
> ///
> /// # Safety
> ///
> - /// This function may only be called by blk-mq C infrastructure.
> - unsafe extern "C" fn commit_rqs_callback(_hctx: *mut bindings::blk_mq_hw_ctx) {
> - T::commit_rqs()
> + /// This function may only be called by blk-mq C infrastructure. The caller
> + /// must ensure that `hctx` is valid.
> + unsafe extern "C" fn commit_rqs_callback(hctx: *mut bindings::blk_mq_hw_ctx) {
> + // 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()) };
Ditto here.
Alice
