Re: [PATCH RFC v2 19/19] fuse: {uring} Optimize async sends

[Bernd Schubert <bschubert@ddn.com>]

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@vger.kernel.org
>> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
> 
> 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