Re: [RFC PATCH 1/5] rust: block: introduce `kernel::block::mq` module

From: Benno Lossin <benno.lossin@proton.me>
To: Andreas Hindborg <nmi@metaspace.dk>
Cc: "Jens Axboe" <axboe@kernel.dk>, "Christoph Hellwig" <hch@lst.de>,
	"Keith Busch" <kbusch@kernel.org>,
	"Damien Le Moal" <Damien.LeMoal@wdc.com>,
	"Bart Van Assche" <bvanassche@acm.org>,
	"Hannes Reinecke" <hare@suse.de>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	"Andreas Hindborg" <a.hindborg@samsung.com>,
	"Wedson Almeida Filho" <wedsonaf@gmail.com>,
	"Niklas Cassel" <Niklas.Cassel@wdc.com>,
	"Greg KH" <gregkh@linuxfoundation.org>,
	"Matthew Wilcox" <willy@infradead.org>,
	"Miguel Ojeda" <ojeda@kernel.org>,
	"Alex Gaynor" <alex.gaynor@gmail.com>,
	"Boqun Feng" <boqun.feng@gmail.com>,
	"Gary Guo" <gary@garyguo.net>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	"Alice Ryhl" <aliceryhl@google.com>,
	"Chaitanya Kulkarni" <chaitanyak@nvidia.com>,
	"Luis Chamberlain" <mcgrof@kernel.org>,
	"Yexuan Yang" <1182282462@bupt.edu.cn>,
	"Sergio González Collado" <sergio.collado@gmail.com>,
	"Joel Granados" <j.granados@samsung.com>,
	"Pankaj Raghav (Samsung)" <kernel@pankajraghav.com>,
	"Daniel Gomez" <da.gomez@samsung.com>,
	"open list" <linux-kernel@vger.kernel.org>,
	"rust-for-linux@vger.kernel.org" <rust-for-linux@vger.kernel.org>,
	"lsf-pc@lists.linux-foundation.org"
	<lsf-pc@lists.linux-foundation.org>,
	"gost.dev@samsung.com" <gost.dev@samsung.com>
Subject: Re: [RFC PATCH 1/5] rust: block: introduce `kernel::block::mq` module
Date: Tue, 02 Apr 2024 23:09:49 +0000	[thread overview]
Message-ID: <7ed2a8df-3088-42a1-b257-dba3c2c9fc92@proton.me> (raw)
In-Reply-To: <871q7o54el.fsf@metaspace.dk>

On 23.03.24 07:32, Andreas Hindborg wrote:
> Benno Lossin <benno.lossin@proton.me> writes:
>> On 3/13/24 12:05, Andreas Hindborg wrote:
>>> +//! implementations of the `Operations` trait.
>>> +//!
>>> +//! IO requests are passed to the driver as [`Request`] references. The
>>> +//! `Request` type is a wrapper around the C `struct request`. The driver must
>>> +//! mark start of request processing by calling [`Request::start`] and end of
>>> +//! processing by calling one of the [`Request::end`], methods. Failure to do so
>>> +//! can lead to IO failures.
>>
>> I am unfamiliar with this, what are "IO failures"?
>> Do you think that it might be better to change the API to use a
>> callback? So instead of calling start and end, you would do
>>
>>       request.handle(|req| {
>>           // do the stuff that would be done between start and end
>>       });
>>
>> I took a quick look at the rnull driver and there you are calling
>> `Request::end_ok` from a different function. So my suggestion might not
>> be possible, since you really need the freedom.
>>
>> Do you think that a guard approach might work better? ie `start` returns
>> a guard that when dropped will call `end` and you need the guard to
>> operate on the request.
> 
> I don't think that would fit, since the driver might not complete the
> request immediately. We might be able to call `start` on behalf of the
> driver.
> 
> At any rate, since the request is reference counted now, we can
> automatically fail a request when the last reference is dropped and it
> was not marked successfully completed. I would need to measure the
> performance implications of such a feature.

Are there cases where you still need access to the request after you
have called `end`? If no, I think it would be better for the request to
be consumed by the `end` function.
This is a bit difficult with `ARef`, since the user can just clone it
though... Do you think that it might be necessary to clone requests?

Also what happens if I call `end_ok` and then `end_err` or vice versa?

>>> +    pub fn data_ref(&self) -> &T::RequestData {
>>> +        let request_ptr = self.0.get().cast::<bindings::request>();
>>> +
>>> +        // SAFETY: `request_ptr` is a valid `struct request` because `ARef` is
>>> +        // `repr(transparent)`
>>> +        let p: *mut c_void = unsafe { bindings::blk_mq_rq_to_pdu(request_ptr) };
>>> +
>>> +        let p = p.cast::<T::RequestData>();
>>> +
>>> +        // SAFETY: By C API contract, `p` is initialized by a call to
>>> +        // `OperationsVTable::init_request_callback()`. By existence of `&self`
>>> +        // it must be valid for use as a shared reference.
>>> +        unsafe { &*p }
>>> +    }
>>> +}
>>> +
>>> +// SAFETY: It is impossible to obtain an owned or mutable `Request`, so we can
>>
>> What do you mean by "mutable `Request`"? There is the function to obtain
>> a `&mut Request`.
> 
> The idea behind this comment is that it is not possible to have an owned
> `Request` instance. You can only ever have something that will deref
> (shared) to `Request`. Construction of the `Request` type is not
> possible in safe driver code. At least that is the intention.
> 
> The `from_ptr_mut` is unsafe, and could be downgraded to
> `from_ptr`, since `Operations::complete` takes a shared reference
> anyway. Bottom line is that user code does not handle `&mut Request`.

Ah I see what you mean. But the user is able to have an `ARef<Request>`.
Which you own, if it is the only refcount currently held on that
request. When you drop it, you will run the destructor of the request.

A more suitable safety comment would be "SAFETY: A `struct request` may
be destroyed from any thread.".

-- 
Cheers,
Benno