Message-ID: <428db714-5ec8-4259-b808-b8784153d4f2@ustc.edu>
Date: Sun, 18 Jan 2026 00:14:03 +0800
From: Chunsheng Luo <luochunsheng@...c.edu>
To: Amir Goldstein <amir73il@...il.com>
Cc: miklos@...redi.hu, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] fuse: add ioctl to cleanup all backing files



On 1/16/26 11:39 PM, Amir Goldstein wrote:
> On Fri, Jan 16, 2026 at 3:28 PM Chunsheng Luo <luochunsheng@...c.edu> wrote:
>>
>> To simplify crash recovery and reduce performance impact, backing_ids
>> are not persisted across daemon restarts. After crash recovery, this
>> may lead to resource leaks if backing file resources are not properly
>> cleaned up.
>>
>> Add FUSE_DEV_IOC_BACKING_CLOSE_ALL ioctl to release all backing_ids
>> and put backing files. When the FUSE daemon restarts, it can use this
>> ioctl to cleanup all backing file resources.
>>
>> Signed-off-by: Chunsheng Luo <luochunsheng@...c.edu>
>> ---
>>   fs/fuse/backing.c         | 19 +++++++++++++++++++
>>   fs/fuse/dev.c             | 16 ++++++++++++++++
>>   fs/fuse/fuse_i.h          |  1 +
>>   include/uapi/linux/fuse.h |  1 +
>>   4 files changed, 37 insertions(+)
>>
>> diff --git a/fs/fuse/backing.c b/fs/fuse/backing.c
>> index 4afda419dd14..e93d797a2cde 100644
>> --- a/fs/fuse/backing.c
>> +++ b/fs/fuse/backing.c
>> @@ -166,6 +166,25 @@ int fuse_backing_close(struct fuse_conn *fc, int backing_id)
>>          return err;
>>   }
>>
>> +static int fuse_backing_close_one(int id, void *p, void *data)
>> +{
>> +       struct fuse_conn *fc = data;
>> +
>> +       fuse_backing_close(fc, id);
>> +
>> +       return 0;
>> +}
>> +
>> +int fuse_backing_close_all(struct fuse_conn *fc)
>> +{
>> +       if (!fc->passthrough || !capable(CAP_SYS_ADMIN))
>> +               return -EPERM;
>> +
>> +       idr_for_each(&fc->backing_files_map, fuse_backing_close_one, fc);
>> +
>> +       return 0;
>> +}
>> +
> 
> This is not safe and not efficient.
> For safety from racing with _open/_close, iteration needs at least
> rcu_read_lock(),

Yes, you're absolutely right. Additionally, calling idr_remove within 
idr_for_each maybe presents safety risks.

> but I think it will be much more efficient to zap the entire map with
> fuse_backing_files_free()/fuse_backing_files_init().
> 
> This of course needs to be synchronized with concurrent _open/_close/_lookup.
> This could be done by making c->backing_files_map a struct idr __rcu *
> and replace the old and new backing_files_map under spin_lock(&fc->lock);
> 
> Then you can call fuse_backing_files_free() on the old backing_files_map
> without a lock.
> 
> As a side note, fuse_backing_files_free() iteration looks like it may need
> cond_resched() if there are a LOT of backing ids, but I am not sure and
> this is orthogonal to your change.
> 
> Thanks,
> Amir.
> 
> 

Thank you for your helpful suggestions. However, it cannot use 
fuse_backing_files_free() in the close_all implementation because it 
directly frees backing files without respecting reference counts. This 
function requires that no one is actively using the backing file (it 
even has WARN_ON_ONCE(refcount_read(&fb->count) != 1)), which cannot be 
guaranteed after a crash recovery scenario where backing files may still 
be in use.

Instead, the implementation uses fuse_backing_put() to safely decrement 
the reference count and allow the backing file to be freed when no 
longer in use.

Additionally, the implementation addresses two race conditions:

- Race between idr_for_each and lookup: Uses synchronize_rcu() to ensure 
all concurrent RCU readers (i.e., in-flight fuse_backing_lookup() calls) 
complete before releasing backing files, preventing use-after-free issues.

- Race with open/close operations: Uses fc->lock to atomically swap the 
old and new IDR maps, ensuring consistency with concurrent 
fuse_backing_open() and fuse_backing_close() operations.

This approach provides the same as the RCU pointer suggestion, but with 
less code and no changes to the struct fuse_conn data structures.

I've updated it and verified the implementation. Could you please review it?


diff --git a/fs/fuse/backing.c b/fs/fuse/backing.c
index 4afda419dd14..047d373684f9 100644
--- a/fs/fuse/backing.c
+++ b/fs/fuse/backing.c
@@ -166,6 +166,45 @@ int fuse_backing_close(struct fuse_conn *fc, int 
backing_id)
         return err;
  }

+static int fuse_backing_release_one(int id, void *p, void *data)
+{
+       struct fuse_backing *fb = p;
+
+       fuse_backing_put(fb);
+
+       return 0;
+}
+
+int fuse_backing_close_all(struct fuse_conn *fc)
+{
+       struct idr old_map;
+
+       if (!fc->passthrough || !capable(CAP_SYS_ADMIN))
+               return -EPERM;
+
+       /*
+        * Swap out the old backing_files_map with a new empty one under 
lock,
+        * then release all backing files outside the lock. This avoids long
+        * lock hold times and potential races with concurrent open/close
+        * operations.
+        */
+       idr_init(&old_map);
+       spin_lock(&fc->lock);
+       swap(fc->backing_files_map, old_map);
+       spin_unlock(&fc->lock);
+
+       /*
+        * Ensure all concurrent RCU readers complete before releasing 
backing
+        * files, so any in-flight lookups can safely take references.
+        */
+       synchronize_rcu();
+
+       idr_for_each(&old_map, fuse_backing_release_one, NULL);
+       idr_destroy(&old_map);
+
+       return 0;
+}
+

--

Thanks,
Chunsheng Luo

Powered by blists - more mailing lists