Re: [PATCHSET 0/6] Enable NO_OFFLOAD support

[Pavel Begunkov <asml.silence@gmail.com>]

On 4/20/23 16:08, Jens Axboe wrote:
> On 4/19/23 6:43?PM, Pavel Begunkov wrote:
>> On 4/19/23 17:25, Jens Axboe wrote:
>>> Hi,
>>>
>>> This series enables support for forcing no-offload for requests that
>>> otherwise would have been punted to io-wq. In essence, it bypasses
>>> the normal non-blocking issue in favor of just letting the issue block.
>>> This is only done for requests that would've otherwise hit io-wq in
>>> the offload path, anything pollable will still be doing non-blocking
>>> issue. See patch 3 for details.
>>
>> That's shooting ourselves in the leg.
>>
>> 1) It has never been easier to lock up userspace. They might be able
>> to deal with simple cases like read(pipe) + write(pipe), though even
>> that in a complex enough framework would cause debugging and associated
>> headache.
>>
>> Now let's assume that the userspace submits nvme passthrough requests,
>> it exhausts tags and a request is left waiting there. To progress
>> forward one of the previous reqs should complete, but it's only putting
>> task in tw, which will never be run with DEFER_TASKRUN.
>>
>> It's not enough for the userspace to be careful, for DEFER_TASKRUN
>> there will always be a chance to get locked .
>>
>> 2) It's not limited only to requests we're submitting, but also
>> already queued async requests. Inline submission holds uring_lock,
>> and so if io-wq thread needs to grab a registered file for the
>> request, it'll io_ring_submit_lock() and wait until the submission
>> ends. Same for provided buffers and some other cases.
>>
>> Even task exit will actively try to grab the lock.
> 
> One thing I pondered was making the inline submissions similar to io-wq
> submissions - eg don't hold uring_lock over them. To make useful, I
> suspect we'd want to prep all SQ entries upfront, and then drop for
> submission.

That would need completion caches (ctx->submit_state) to be changed,
either by allowing multiple of them or limiting by some other mean
to only 1 inline submitter. Also, that will probably return the
request refcounting back, and DEFER_TASKRUN would probably need
to retake the lock for execution unless there are magic tricks
around it. Not an easy task if we don't want to hurt performance.


> We'd also want to make this mutually exclusive with IOPOLL, obviously.
> Doesn't make any sense to do anyway for IOPOLL, but it needs to be
> explicitly disallowed.

-- 
Pavel Begunkov