Re: [PATCH v7 1/6] fuse: decouple fuse_ring creation from ent registration

On 8/19/26 22:52, Bernd Schubert wrote:
> 
> 
> On 8/19/26 22:29, Joanne Koong wrote:
>> On Wed, Aug 19, 2026 at 1:05 PM Bernd Schubert <[email protected]> wrote:
>>>
>>>
>>>
>>> On 8/19/26 19:56, Joanne Koong wrote:
>>>> On Wed, Aug 19, 2026 at 4:35 AM Miklos Szeredi <[email protected]> wrote:
>>>>>
>>>>> On Fri, 14 Aug 2026 at 21:00, Joanne Koong <[email protected]> wrote:
>>>>>>
>>>>>> Currently, the connection's fuse_ring is created lazily on the first
>>>>>> FUSE_IO_URING_CMD_REGISTER command. A server registers entries from one
>>>>>> thread per queue (one per CPU) and those threads issue their first
>>>>>> REGISTER command concurrently. They then race to create the single
>>>>>> per-connection fuse_ring, which required open-coded handling in
>>>>>> fuse_uring_create() to detect and protect against concurrent creations.
>>>>>>
>>>>>> Decouple fuse_ring creation from ent registration and move it to
>>>>>> FUSE_INIT reply processing after a server has negotiated and set
>>>>>> FUSE_OVER_IO_URING. The ring is published before the connection is
>>>>>> marked initialized. fuse_uring_register() no longer creates the ring and
>>>>>> it instead uses the ring set up at init time.
>>>>>
>>>>> I tested this with loraw (a "raw" loopback tester that doesn't use
>>>>> libfuse) and it fails with
>>>>>
>>>>> root@kvm:~# ./loraw -u  /mnt/fuse
>>>>> loraw: loraw.c:1010: lo_start_uring: Assertion `!cqe->res' failed.
>>>>>
>>>>> cqe->res is -22 (EINVAL).
>>>>>
>>>>> Attaching the reproducer.  To compile:
>>>>>
>>>>> cp $(KERNEL_TREE)/include/uapi/linux/fuse.h fuse_kernel.h
>>>>> gcc loraw.c -oloraw -luring
>>>>>
>>>>
>>>> Thanks for attaching the repro.
>>>>
>>>> This is happening because this patch uses the FUSE_OVER_IO_URING init
>>>> reply as a signal that the ring should be created, but I missed that
>>>> the FUSE_OVER_IO_URING reply is *optional*.
>>>>
>>>> Prior to this patch, there's two scenarios:
>>>> a) server sets FUSE_OVER_IO_URING reply at init time - requests will
>>>> automatically block until fuse-io-uring is completely set up
>>>> b) server does not set FUSE_OVER_IO_URING but later sends uring
>>>> register request - requests will continue along /dev/fuse path until
>>>> fuse-io-uring is completely set up
>>>>
>>>> Libfuse sets FUSE_OVER_IO_URING in the reply, but the loraw.c server does not.
>>>>
>>>> I think the best way to fix this is to have the ring creation happen
>>>> when the kernel receives the first io-uring command instead of at
>>>> FUSE_INIT or at FUSE_IO_URING_CMD_REGISTER ent creation time, given
>>>> that FUSE_IO_URING_ADD_QUEUE needs the ring to exist:
>>>
>>> I don't think we should allow io-uring without FUSE_OVER_IO_URING and
>>> I really thought that was disabled.
>>
>> This is pre-existing behavior that's been there since the beginning
>> (kernel version 6.14) [1]. I don't think we can change this now, or
>> it'll break backwards compatibility, like Miklos's loraw program.
>>
>>>
>>> <... checking the code ...>
>>>
>>> I'm on a ublk branch without your commits a applied, i.e. plain upstream 7.2 fuse
>>>
>>> fuse_uring_cmd()
>>>         /* Once a connection has io-uring enabled on it, it can't be disabled */
>>>         if (!enable_uring && !fch->io_uring) {
>>>                 pr_info_ratelimited("fuse-io-uring is disabled\n");
>>>                 return -EOPNOTSUPP;
>>>         }
>>>
>>>
>>>
>>> In process_init_reply()
>>>
>>> if (flags & FUSE_OVER_IO_URING && fuse_uring_enabled())
>>>     fuse_chan_io_uring_enable(fc->chan);
>>>
>>
>> The condition check in fuse_uring_cmd() is an && and not an ||. It
> 
> Aaaarg. That makes the entire condition useless :(
> 
>> doesn't enforce that the server must have sent over FUSE_OVER_IO_URING
>> in the init reply to use fuse io-uring. With enable_uring=1 (which is
>> needed for the kernel to advertise FUSE_OVER_IO_URING in the init
>> request in the first place), the fuse io-uring logic proceeds
>> regardless of whether the server replied with the FUSE_OVER_IO_URING
>> flag or not.
>>
>> Thanks,
>> Joanne
>>
>> [1] https://elixir.bootlin.com/linux/v6.14-rc1/source/fs/fuse/inode.c#L1455
> 
> The comment there is definitely outdated, at least it was *supposed* to.
> 
> Well, then we have a problem because we have a lock order inversion
> issue. After adding in the reduce-queue series I had planned to work on
> distributing fch->num_background among queues, so that each queue gets
> its own num_background, so that holding two locks like in
> fuse_uring_queue_bq_req() wouldn't be needed anymore.
> 
> Also again, without FUSE_OVER_IO_URING reply the condition in
> fuse_block_alloc() doesn't make any sense.

Lock order inversion is this

 CPU 0 - io-uring completion              | CPU 1 - legacy background completion
                                           | fuse_request_end()
                                           |   takes fch->bg_lock
                                           |   fuse_request_bg_finish()
  fuse_uring_cmd()                         |
    fuse_uring_commit_fetch()              |
      fuse_uring_commit()                  |   
        fuse_uring_req_end()               |
          takes queue->lock                |
          wants fch->bg_lock               |
                                           |   flush_bg_queue()
                                           |     fuse_send_one()
                                           |       fiq->ops->send_req()
                                           |         fuse_uring_queue_fuse_req()
                                           |           wants queue->lock


(I queried AI about it to quickly generate that graph, but that is
exactly what I had seen back in development and which is why fuse
requests are supposed to be blocked until queue initialization is
complete, i.e. to avoid switching from /dev/fuse to io-uring at
run time.

The wrong "&&" in fuse_uring_cmd() also makes it possible that people
bypass the module option to enable io-uring, by just setting the flag
in their userspace implementation. Yikes :/