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

[Benno Lossin <benno.lossin@proton.me>]

On 04.04.24 07:44, Andreas Hindborg wrote:
> Benno Lossin <benno.lossin@proton.me> writes:
> 
>> On 03.04.24 10:46, Andreas Hindborg wrote:
>>> Benno Lossin <benno.lossin@proton.me> writes:
>>>
>>>> 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`?
>>>
>>> In general no, there is no need to handle the request after calling end.
>>> C drivers are not allowed to, because this transfers ownership of the
>>> request back to the block layer. This patch series defer the transfer of
>>> ownership to the point when the ARef<Request> refcount goes to zero, so
>>> there should be no danger associated with touching the `Request` after
>>> 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?
>>>
>>> Looking into the details now I see that calling `Request::end` more than
>>> once will trigger UAF, because C code decrements the refcount on the
>>> request. When we have `ARef<Request>` around, that is a problem. It
>>> probably also messes with other things in C land. Good catch.
>>>
>>> I did implement `Request::end` to consume the request at one point
>>> before I fell back on reference counting. It works fine for simple
>>> drivers. However, most drivers will need to use the block layer tag set
>>> service, that allows conversion of an integer id to a request pointer.
>>> The abstraction for this feature is not part of this patch set. But the
>>> block layer manages a mapping of integer to request mapping, and drivers
>>> typically use this to identify the request that corresponds to
>>> completion messages that arrive from hardware. When drivers are able to
>>> turn integers into requests like this, consuming the request in the call
>>> to `end` makes little sense (because we can just construct more).
>>
>> How do you ensure that this is fine?:
>>
>>       let r1 = tagset.get(0);
>>       let r2 = tagset.get(0);
>>       r1.end_ok();
>>       r2.do_something_that_would_only_be_done_while_active();
>>
>> One thing that comes to my mind would be to only give out `&Request`
>> from the tag set. And to destroy, you could have a separate operation
>> that also removes the request from the tag set. (I am thinking of a tag
>> set as a `HashMap<u64, Request>`.
> 
> This would be similar to
> 
>    let r1 = tagset.get(0)?;
>    ler r2 = r1.clone();
>    r1.end_ok();
>    r2.do_something_requires_active();
> 
> but it is not a problem because we do not implement any actions that are
> illegal in that position (outside of `end` - that _is_ a problem).

Makes sense, but I think it's a bit weird to still be able to access it
after `end`ing.

> 
> 
>>>
>>> What I do now is issue the an `Option<ARef<Request>>` with
>>> `bindings::req_ref_inc_not_zero(rq_ptr)`, to make sure that the request
>>> is currently owned by the driver.
>>>
>>> I guess we can check the absolute value of the refcount, and only issue
>>> a request handle if the count matches what we expect. Then we can be certain
>>> that the handle is unique, and we can require transfer of ownership of
>>> the handle to `Request::end` to make sure it can never be called more
>>> than once.
>>>
>>> Another option is to error out in `Request::end` if the
>>> refcount is not what we expect.
>>
>> I am a bit confused, why does the refcount matter in this case? Can't
>> the user just have multiple `ARef`s?
> 
> Because we want to assert that we are consuming the last handle to the
> request. After we do that, the user cannot call `Request::end` again.
> `TagSet::get` will not issue a request reference if the request is not
> in flight. Although there might be a race condition to watch out for.
> 
> When the block layer hands over ownership to Rust, the reference count
> is 1. The first `ARef<Request>` we create increments the count to 2. To
> complete the request, we must have ownership of all reference counts
> above 1. The block layer takes the last reference count when it takes
> back ownership of the request.
> 
>> I think it would be weird to use `ARef<Request>` if you expect the
>> refcount to be 1.
> 
> Yes, that would require a custom smart pointer with a `try_into_unique`
> method that succeeds when the refcount is exactly 2. It would consume
> the instance and decrement the refcount to 1. But as I said, there is a
> potential race with `TagSet::get` when the refcount is 1 that needs to
> be handled.
> 
>> Maybe the API should be different?
> 
> I needs to change a little, yes.
> 
>> As I understand it, a request has the following life cycle (please
>> correct me if I am wrong):
>> 1. A new request is created, it is given to the driver via `queue_rq`.
>> 2. The driver can now decide what to do with it (theoretically it can
>>      store it somewhere and later do something with it), but it should at
>>      some point call `Request::start`.
>> 3. Work happens and eventually the driver calls `Request::end`.
>>
>> To me this does not seem like something where we need a refcount (we
>> still might need one for safety, but it does not need to be exposed to
>> the user).
> 
> It would not need to be exposed to the user, other than a) ending a request
> can fail OR b) `TagSet::get` can fail.
> 
> a) would require that ending a request must be done with a unique
> reference. This could be done by the user by the user calling
> `try_into_unique` or by making the `end` method fallible.
> 
> b) would make the reference handle `!Clone` and add a failure mode to
> `TagSet::get`, so it fails to construct a `Request` handle if there are
> already one in existence.
> 
> I gravitate towards a) because it allows the user to clone the Request
> reference without adding an additional `Arc`.

This confuses me a little, since I thought that `TagSet::get` returns
`Option<ARef<Request>>`. (I tried to find the abstractions in your
github, but I did not find them)

I think that this could work: `queue_rq` takes a `OwnedRequest`, which
the user can store in a `TagSet`, transferring ownership. `TagSet::get`
returns `Option<&Request>` and you can call `TagSet::remove` to get
`Option<OwnedRequest>`. `OwnedRequest::end` consumes `self`.
With this pattern we also do not need to take an additional refcount.

-- 
Cheers,
Benno