Hi David, Sorry for the late reply. Busy for other business in work. On 12/9/2022 7:26 PM, David Howells wrote: > Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote: > >>> clear_bit(FSCACHE_VOLUME_ACQUIRE_PENDING, &cursor->flags); > Maybe this should be clear_bit_unlock() instead. I'm not sure about that. In my understanding, clear_bit_unlock() is usually paired with test_and_set_bit_lock() to implement bit lock to make sure the writes before clear_bit_unlock() are visible to read access in concurrent process, right ? But now the caller of fscache_wake_pending_volume() only modify cursor->flags and nothing else, so I don't think it is needed here. If its intended purpose is to provide the missing smp_mb() for wake_up_bit(), I also don't think it is right, because the release barrier provided by clear_bit_unlock() doesn't guarantee the order of cursor->flags and wq_head, so I think one extra smp_mb_after_atomic() is also needed after clear_bit(FSCACHE_VOLUME_ACQUIRE_PENDING, &cursor->flags). If the above reasoning makes sense to you, I think we also need to add smp_mb_after_atomic() for wake_up_bit() in fscache_create_volume_work(). > And I wonder if: > > set_bit(FSCACHE_VOLUME_ACQUIRE_PENDING, &candidate->flags); > > in fscache_hash_volume() needs a barrier before it. I also don't get it. The barrier is used to guarantee the order between cursor->flags and candidate->flags, right ? But the write and read of cursor->flags and candidate->flags are protected by the same hash lock. > >>> - wake_up_bit(&cursor->flags, FSCACHE_VOLUME_ACQUIRE_PENDING); >>> + /* >>> + * Paired with barrier in wait_var_event(). Check >>> + * waitqueue_active() and wake_up_var() for details. >>> + */ >>> + smp_mb__after_atomic(); >>> + wake_up_var(&cursor->flags); > That doesn't seem right. > > wake_up_bit() is more selective, so should be preferred to wake_up_var(). OK. Will update fscache_wait_on_volume_collision() to use wait_on_bit() accordingly. > David > > > . -- Linux-cachefs mailing list Linux-cachefs@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/linux-cachefs
