Re: [PATCH 4/8] io_uring: implement fixed buffers registration similar to fixed files

[Bijan Mottahedeh <bijan.mottahedeh@oracle.com>]

>>>> Apply fixed_rsrc functionality for fixed buffers support.
>>>
>>> I don't get it, requests with fixed files take a ref to a node (see
>>> fixed_file_refs) and put it on free, but I don't see anything similar
>>> here. Did you work around it somehow?
>>
>> No that's my oversight.  I think I wrongfully assumed that io_import_*fixed() would take care of that.
>>
>> Should I basically do something similar to io_file_get()/io_put_file()?
> 
> If done in a dumb way, that'd mean extra pair of percpu get/put
> and +8B in io_kiocb. Frankly, I don't like that idea.
> 
> However, if you don't split paths and make fixed_file_ref_node to
> supports all types of resources at the same time, it should be
> bearable. I.e. register removals of both types to a single node,
> and use ->fixed_file_refs for all request's resources.
> So you don't grow io_kiocb and do maximum one percpu_ref_get/put()
> pair per request.

So something like promoting fixed_file_ref_node to fixed_rsrc_ref_node, 
and ref counting all fixed resources for a given request together?

> 
> I'll send a small patch preparing grounds, because there is actually
> another nasty thing from past that needs to be reworked.

Ok, I'll wait for your patch then.

>> Are you ok with the rest of the patches or should I address anything else?
> 
> io_import_fixed() currently can be called twice, and that would give
> you 2 different bvecs. Hence after removing full quisce io_read()
> retrying short reads will probably be able to partially read into 2
> different buffers. That really have to be fixed.

Will you address that in your patch?

>>>>    static ssize_t io_import_fixed(struct io_kiocb *req, int rw,
>>>>                       struct iov_iter *iter)
>>>>    {
>>>> @@ -2959,10 +2982,15 @@ static ssize_t io_import_fixed(struct io_kiocb *req, int rw,
>>>>        size_t offset;
>>>>        u64 buf_addr;
>>>>    +    /* attempt to use fixed buffers without having provided iovecs */
>>>> +    if (unlikely(!ctx->buf_data))
>>>> +        return -EFAULT;
> 
> I removed it for files,
> because (ctx->buf_data) IFF (ctx->nr_user_bufs == 0),
> so the following ctx->nr_user_bufs check is enough.

Ok, will remove the check for buffers.