Re: [PATCH RFC v7 11/16] fuse: {uring} Allow to queue fg requests through io-uring

[Bernd Schubert <bernd.schubert@fastmail.fm>]

On 12/3/24 15:09, Pavel Begunkov wrote:
> On 11/27/24 13:40, Bernd Schubert wrote:
>> This prepares queueing and sending foreground requests through
>> io-uring.
>>
>> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
>> ---
>>   fs/fuse/dev.c         |   5 +-
>>   fs/fuse/dev_uring.c   | 159 ++++++++++++++++++++++++++++++++++++++++
>> ++++++++++
>>   fs/fuse/dev_uring_i.h |   8 +++
>>   fs/fuse/fuse_dev_i.h  |   5 ++
>>   4 files changed, 175 insertions(+), 2 deletions(-)
>>
> ...
>> +
>> +/*
>> + * This prepares and sends the ring request in fuse-uring task context.
>> + * User buffers are not mapped yet - the application does not have
>> permission
>> + * to write to it - this has to be executed in ring task context.
>> + */
>> +static void
>> +fuse_uring_send_req_in_task(struct io_uring_cmd *cmd,
>> +                unsigned int issue_flags)
>> +{
>> +    struct fuse_uring_cmd_pdu *pdu = (struct fuse_uring_cmd_pdu
>> *)cmd->pdu;
>> +    struct fuse_ring_ent *ring_ent = pdu->ring_ent;
>> +    struct fuse_ring_queue *queue = ring_ent->queue;
>> +    int err;
>> +
>> +    BUILD_BUG_ON(sizeof(pdu) > sizeof(cmd->pdu));
>> +
>> +    err = fuse_uring_prepare_send(ring_ent);
>> +    if (err)
>> +        goto err;
>> +
>> +    io_uring_cmd_done(cmd, 0, 0, issue_flags);
>> +
>> +    spin_lock(&queue->lock);
>> +    ring_ent->state = FRRS_USERSPACE;
> 
> I haven't followed the cancellation/teardown path well, but don't
> you need to set FRRS_USERSPACE before io_uring_cmd_done()?
> 
> E.g. while we're just before the spin_lock above here, can
> fuse_uring_stop_list_entries() find the request, see that the state
> is not FRRS_USERSPACE
> 
> bool need_cmd_done = ent->state != FRRS_USERSPACE;
> 
> and call io_uring_cmd_done a second time?

Sorry about the confusion, I had actually already fixed it in patch 14, 
the one that added handling of IO_URING_F_TASK_DEAD and that you asked
to merge into this patch here. Obviously at least that part should have
been part of this patch here.


Thanks again for your thorough review!


Bernd
Bernd