On Tue, Aug 27, 2024 at 11:14:48PM GMT, Alan Huang wrote:
> If the reader acquires the read lock and then the writer enters the slow
> path, while the reader proceeds to the unlock path, the following scenario
> can occur without the change:
>
> writer: pcpu_read_count(lock) return 1 (so __do_six_trylock will return 0)
> reader: this_cpu_dec(*lock->readers)
> reader: smp_mb()
> reader: state = atomic_read(&lock->state) (there is no waiting flag set)
> writer: six_set_bitmask()
>
> then the writer will sleep forever.
>
> Signed-off-by: Alan Huang <[email protected]>
> ---
> fs/bcachefs/six.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/fs/bcachefs/six.c b/fs/bcachefs/six.c
> index 3a494c5d1247..b3583c50ef04 100644
> --- a/fs/bcachefs/six.c
> +++ b/fs/bcachefs/six.c
> @@ -169,11 +169,17 @@ static int __do_six_trylock(struct six_lock *lock, enum
> six_lock_type type,
> ret = -1 - SIX_LOCK_write;
> }
> } else if (type == SIX_LOCK_write && lock->readers) {
> - if (try) {
> + if (try)
> atomic_add(SIX_LOCK_HELD_write, &lock->state);
> - smp_mb__after_atomic();
> - }
>
> + /*
> + * Make sure atomic_add happens before pcpu_read_count and
> + * six_set_bitmask in slow path happens before pcpu_read_count.
> + *
> + * Paired with the smp_mb() in read lock fast path (per-cpu
> mode)
> + * and the one before atomic_read in read unlock path.
> + */
> + smp_mb();
What is this barrier ordering?
This is only a change in the !try case, and we didn't do anything before
checking pcpu_read_count() - except way back in six_lock_slowpath()
where we set SIX_LOCK_HELD_write, but that does have a barrier.
> ret = !pcpu_read_count(lock);
>
> if (try && !ret) {
> --
> 2.45.2
>
