Re: [PATCH RFC] io_uring: improve current file position IO

[Jens Axboe <axboe@kernel.dk>]

On 1/3/22 7:17 AM, Jann Horn wrote:
> On Fri, Dec 24, 2021 at 3:35 PM Jens Axboe <axboe@kernel.dk> wrote:
>> io_uring should be protecting the current file position with the
>> file position lock, ->f_pos_lock. Grab and track the lock state when
>> the request is being issued, and make the unlock part of request
>> cleaning.
>>
>> Fixes: ba04291eb66e ("io_uring: allow use of offset == -1 to mean file position")
>> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>
>> ---
>>
>> Main thing I don't like here:
>>
>> - We're holding the f_pos_lock across the kernel/user boundary, as
>>   it's held for the duration of the IO. Alternatively we could
>>   keep it local to io_read() and io_write() and lose REQ_F_CUR_POS_LOCK,
>>   but will messy up those functions more and add more items to the
>>   fast path (which current position read/write definitely is not).
>>
>> Suggestions welcome...
> 
> Oh, that's not pretty... is it guaranteed that the

Right, hence why it's an RFC :-)

> __f_unlock_pos(req->file) will happen in the same task as the
> io_file_pos_lock(req, false), and have you tried running this with

It might unlock from a thread off that task, depends on how the execution
happens. And as it stands, it'll also potentially exit the kernel with
the lock held until it completes.

> lockdep and mutex debugging enabled? Could a task deadlock if it tried
> to do a read() on a file while io_uring is already holding the
> position lock?

lockdep will complain about the leaving the kernel with it held aspect
for sure.

I think the better solution here is, as I suggested in the patch, to
keep it local to io_read() and io_write() rather than try and track it.
Which is a bit annoying in terms of adding mostly useless code to the
fast path, but... Don't think there's a better way.

-- 
Jens Axboe