On 5/31/24 18:24, Jens Axboe wrote:
> On 5/29/24 12:00 PM, Bernd Schubert wrote:
>> This is to avoid using async completion tasks
>> (i.e. context switches) when not needed.
>>
>> Cc: io-uring@xxxxxxxxxxxxxxx
>> Signed-off-by: Bernd Schubert <bschubert@xxxxxxx>
>
> This patch is very confusing, even after having pulled the other
> changes. In general, would be great if the io_uring list was CC'ed on
Hmm, let me try to explain. And yes, I definitely need to add these details
to the commit message
Without the patch:
<sending a struct fuse_req>
fuse_uring_queue_fuse_req
fuse_uring_send_to_ring
io_uring_cmd_complete_in_task
<async task runs>
io_uring_cmd_done()
Now I would like to call io_uring_cmd_done() directly without another task
whenever possible. I didn't benchmark it, but another task is in general
against the entire concept. That is where the patch comes in
fuse_uring_queue_fuse_req() now adds the information if io_uring_cmd_done()
shall be called directly or via io_uring_cmd_complete_in_task().
Doing it directly requires the knowledge of issue_flags - these are the
conditions in fuse_uring_queue_fuse_req.
1) (current == queue->server_task)
fuse_uring_cmd (IORING_OP_URING_CMD) received a completion for a
previous fuse_req, after completion it fetched the next fuse_req and
wants to send it - for 'current == queue->server_task' issue flags
got stored in struct fuse_ring_queue::uring_cmd_issue_flags
2) 'else if (current->io_uring)'
(actually documented in the code)
2.1 This might be through IORING_OP_URING_CMD as well, but then server
side uses multiple threads to access the same ring - not nice. We only
store issue_flags into the queue for 'current == queue->server_task', so
we do not know issue_flags - sending through task is needed.
2.2 This might be an application request through the mount point, through
the io-uring interface. We do know issue flags either.
(That one was actually a surprise for me, when xfstests caught it.
Initially I had a condition to send without the extra task then lockdep
caught that.
In both cases it has to use a tasks.
My question here is if 'current->io_uring' is reliable.
3) everything else
3.1) For async requests, interesting are cached reads and writes here. At a minimum
writes a holding a spin lock and that lock conflicts with the mutex io-uring is taking -
we need a task as well
3.2) sync - no lock being hold, it can send without the extra task.
> the whole series, it's very hard to review just a single patch, when you
> don't have the full picture.
Sorry, I will do that for the next version.
>
> Outside of that, would be super useful to include a blurb on how you set
> things up for testing, and how you run the testing. That would really
> help in terms of being able to run and test it, and also to propose
> changes that might make a big difference.
>
Will do in the next version.
You basically need my libfuse uring branch
(right now commit history is not cleaned up) and follow
instructions in <libfuse>/xfstests/README.md how to run xfstests.
Missing is a slight patch for that dir to set extra daemon parameters,
like direct-io (fuse' FOPEN_DIRECT_IO) and io-uring. Will add that libfuse
during the next days.
Thanks,
Bernd
