Re: [PATCH v3 1/4] fuse: Make the fuse unique value a per-cpu counter

[Bernd Schubert <bschubert@xxxxxxx>]

On 4/4/25 14:43, Miklos Szeredi wrote:
> On Thu, 3 Apr 2025 at 22:23, Bernd Schubert <bschubert@xxxxxxx> wrote:
> 
>> +/**
>> + * Get the next unique ID for a request
>> + */
>> +static inline u64 fuse_get_unique(struct fuse_iqueue *fiq)
>> +{
>> +       int step = FUSE_REQ_ID_STEP * (task_cpu(current));
>> +       u64 cntr = this_cpu_inc_return(*fiq->reqctr);
>> +
>> +       return cntr * FUSE_REQ_ID_STEP * NR_CPUS + step;
> 
> Thinking a bit... this looks wrong.
> 
> The reason is that the task could be migrated to a different CPU
> between the task_cpu() and the this_cpu_inc_return(), resulting in a
> possibly duplicated value.
> 
> This could be fixed with a preempt_disable()/preempt_enable() pair,
> but I think it would be cleaner to go with my original idea and
> initialize the percpu counters to  CPUID and increment by NR_CPU *
> FUSE_REQ_ID_STEP when fetching a new value.
> 

Oh right, I guess something like this

diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 80a526eaba38..eac26ee654ca 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1074,10 +1074,7 @@ static inline void fuse_sync_bucket_dec(struct fuse_sync_bucket *bucket)
  */
 static inline u64 fuse_get_unique(struct fuse_iqueue *fiq)
 {
-       int step = FUSE_REQ_ID_STEP * (task_cpu(current));
-       u64 cntr = this_cpu_inc_return(*fiq->reqctr);
-
-       return cntr * FUSE_REQ_ID_STEP * NR_CPUS + step;
+       return this_cpu_add_return(*fiq->reqctr, FUSE_REQ_ID_STEP * NR_CPUS);
 }
 
 /** Device operations */
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 7e1066c174d0..463cf7797e1b 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -930,7 +930,13 @@ static void fuse_iqueue_init(struct fuse_iqueue *fiq,
        memset(fiq, 0, sizeof(struct fuse_iqueue));
        spin_lock_init(&fiq->lock);
        init_waitqueue_head(&fiq->waitq);
+       int cpu;
+
        fiq->reqctr = alloc_percpu(u64);
+       for_each_possible_cpu(cpu) {
+               *per_cpu_ptr(fiq->reqctr, cpu) = cpu * FUSE_REQ_ID_STEP;
+       }
+
        INIT_LIST_HEAD(&fiq->pending);
        INIT_LIST_HEAD(&fiq->interrupts);
        fiq->forget_list_tail = &fiq->forget_list_head;




First need to test and think about it again and currently busy
with something else - new version follows later.


Thanks,
Bernd