Re: [PATCH v9 13/17] fuse: Allow to queue fg requests through io-uring

[Luis Henriques <luis@igalia.com>]

On Tue, Jan 07 2025, Bernd Schubert wrote:

> On 1/7/25 16:54, Luis Henriques wrote:
>
> [...]
>
>>> @@ -785,10 +830,22 @@ static void fuse_uring_do_register(struct fuse_ring_ent *ring_ent,
>>>   				   unsigned int issue_flags)
>>>   {
>>>   	struct fuse_ring_queue *queue = ring_ent->queue;
>>> +	struct fuse_ring *ring = queue->ring;
>>> +	struct fuse_conn *fc = ring->fc;
>>> +	struct fuse_iqueue *fiq = &fc->iq;
>>>     	spin_lock(&queue->lock);
>>>   	fuse_uring_ent_avail(ring_ent, queue);
>>>   	spin_unlock(&queue->lock);
>>> +
>>> +	if (!ring->ready) {
>>> +		bool ready = is_ring_ready(ring, queue->qid);
>>> +
>>> +		if (ready) {
>>> +			WRITE_ONCE(ring->ready, true);
>>> +			fiq->ops = &fuse_io_uring_ops;
>> Shouldn't we be taking the fiq->lock to protect the above operation?
>
> I switched the order and changed it to WRITE_ONCE. fiq->lock would
> require that doing the operations would also hold lock.
> Also see "[PATCH v9 16/17] fuse: block request allocation until",
> there should be no races anyone.

OK, great.  I still need to go read the code a few more times, I guess.
Thank you for your help understanding this code, Bernd.

Cheers,
-- 
Luís

>> 
>>> +		}
>>> +	}
>>>   }
>>>     /*
>>> @@ -979,3 +1036,119 @@ int __maybe_unused fuse_uring_cmd(struct io_uring_cmd *cmd,
>>>     	return -EIOCBQUEUED;
>>>   }
>>> +
>>> +/*
>>> + * 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_ring_ent *ent = uring_cmd_to_ring_ent(cmd);
>>> +	struct fuse_ring_queue *queue = ent->queue;
>>> +	int err;
>>> +
>>> +	if (unlikely(issue_flags & IO_URING_F_TASK_DEAD)) {
>>> +		err = -ECANCELED;
>>> +		goto terminating;
>>> +	}
>>> +
>>> +	err = fuse_uring_prepare_send(ent);
>>> +	if (err)
>>> +		goto err;
>> Suggestion: simplify this function flow.  Something like:
>> 	int err = 0;
>> 	if (unlikely(issue_flags & IO_URING_F_TASK_DEAD))
>> 		err = -ECANCELED;
>> 	else if (fuse_uring_prepare_send(ent)) {
>> 		fuse_uring_next_fuse_req(ent, queue, issue_flags);
>> 		return;
>> 	}
>> 	spin_lock(&queue->lock);
>>          [...]
>
> That makes it look like fuse_uring_prepare_send is not an
> error, but expected. How about like this?
>
> static void
> fuse_uring_send_req_in_task(struct io_uring_cmd *cmd,
> 			    unsigned int issue_flags)
> {
> 	struct fuse_ring_ent *ent = uring_cmd_to_ring_ent(cmd);
> 	struct fuse_ring_queue *queue = ent->queue;
> 	int err;
>
> 	if (!(issue_flags & IO_URING_F_TASK_DEAD)) {
> 		err = fuse_uring_prepare_send(ent);
> 		if (err) {
> 			fuse_uring_next_fuse_req(ent, queue, issue_flags);
> 			return;
> 		}
> 	} else {
> 		err = -ECANCELED;
> 	}
>
> 	spin_lock(&queue->lock);
> 	ent->state = FRRS_USERSPACE;
> 	list_move(&ent->list, &queue->ent_in_userspace);
> 	spin_unlock(&queue->lock);
>
> 	io_uring_cmd_done(cmd, err, 0, issue_flags);
> 	ent->cmd = NULL;
> }
>
>
>
> Thanks,
> Bernd