Hi Joanne,
On 2/3/25 19:50, Joanne Koong wrote:
> req->flags is set/tested/cleared atomically in fuse. When the FR_PENDING
> bit is cleared from the request flags when assigning a request to a
> uring entry, the fiq->lock does not need to be held.
>
> Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> Fixes: c090c8abae4b6 ("fuse: Add io-uring sqe commit and fetch support")
> ---
> fs/fuse/dev_uring.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> index ab8c26042aa8..42389d3e7235 100644
> --- a/fs/fuse/dev_uring.c
> +++ b/fs/fuse/dev_uring.c
> @@ -764,9 +764,7 @@ static void fuse_uring_add_req_to_ring_ent(struct fuse_ring_ent *ent,
> ent->state);
> }
>
> - spin_lock(&fiq->lock);
> clear_bit(FR_PENDING, &req->flags);
> - spin_unlock(&fiq->lock);
> ent->fuse_req = req;
> ent->state = FRRS_FUSE_REQ;
> list_move(&ent->list, &queue->ent_w_req_queue);
I think that would have an issue in request_wait_answer(). Let's say
task-A, request_wait_answer(),
spin_lock(&fiq->lock);
/* Request is not yet in userspace, bail out */
if (test_bit(FR_PENDING, &req->flags)) { // ========> if passed
list_del(&req->list); // --> removes from the list
task-B,
fuse_uring_add_req_to_ring_ent()
clear_bit(FR_PENDING, &req->flags);
ent->fuse_req = req;
ent->state = FRRS_FUSE_REQ;
list_move_tail(&ent->list, &queue->ent_w_req_queue);
fuse_uring_add_to_pq(ent, req); // ==> Add to list
What I mean is, task-A passes the if, but is then slower than task-B. I.e.
task-B runs fuse_uring_add_to_pq() before task-B does the list_del.
Now the ring entry gets handled by fuse-server, comes back to fuse-client
and does not find the request anymore, because both tasks raced.
The entire ring entry will be lost - it not be used anymore for
the live time of the connection.
And the other issue might be total list corruption, because two tasks might
access the list at the same time.
Thanks,
Bernd
