Re: [PATCH] ublk: remove io_cmds list in ublk_queue

[Jens Axboe <axboe@kernel.dk>]

On 3/18/25 7:04 PM, Ming Lei wrote:
> On Tue, Mar 18, 2025 at 12:48:44PM -0600, Jens Axboe wrote:
>> On 3/18/25 12:43 PM, Uday Shankar wrote:
>>> On Tue, Mar 18, 2025 at 12:22:57PM -0600, Jens Axboe wrote:
>>>>>  struct ublk_rq_data {
>>>>> -	struct llist_node node;
>>>>> -
>>>>>  	struct kref ref;
>>>>>  };
>>>>
>>>> Can we get rid of ublk_rq_data then? If it's just a ref thing, I'm sure
>>>> we can find an atomic_t of space in struct request and avoid it. Not a
>>>> pressing thing, just tossing it out there...
>>>
>>> Yeah probably - we do need a ref since one could complete a request
>>> concurrently with another code path which references it (user copy and
>>> zero copy). I see that struct request has a refcount in it already,
>>
>> Right, at least with the current usage, we still do need that kref, or
>> something similar. I would've probably made it just use refcount_t
>> though, rather than rely on the indirect calls. kref doesn't really
>> bring us anything here in terms of API.
>>
>>> though I don't see any examples of drivers using it. Would it be a bad
>>> idea to try and reuse that?
>>
>> We can't reuse that one, and it's not for driver use - purely internal.
>> But I _think_ you could easily grab space in the union that has the hash
>> and ipi_list for it. And then you could dump needing this extra data per
>> request.
> 
> It should be fine to reuse request->ref, since the payload shares same
> lifetime with request.
> 
> But if it is exported, the interface is likely to be misused...

Exactly, that's why I don't want to have drivers mess with this.

And following up on the location, as I forgot to do that in the reply to
Uday - the end_io_data is not a bad idea either, drivers get to use that
as they wish. Then you can't use it with an end_io handler, but it's not
like we've had a need to do that before anyway... It is a bit odd,
however.

But the ipi_list is only used completion side, at which point the driver
is by definition done with the ref. And hash is just for io scheduling,
which we'd never do with requests like this. So still think that's the
best option.

-- 
Jens Axboe