[PATCH] bcachefs: Fix lost wake up

[PATCH] bcachefs: Fix lost wake up

[Alan Huang]

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 <mmpgouride@gmail.com>
---
 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();
 		ret = !pcpu_read_count(lock);
 
 		if (try && !ret) {
-- 
2.45.2

Re: [PATCH] bcachefs: Fix lost wake up

[Kent Overstreet]

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 <mmpgouride@gmail.com>
> ---
>  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
> 

Re: [PATCH] bcachefs: Fix lost wake up

[Alan Huang]

> 2024年9月1日 09:09，Kent Overstreet <kent.overstreet@linux.dev> wrote：
> 
> 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 <mmpgouride@gmail.com>
>> ---
>> 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.

Make sure six_set_bitmask(lock, SIX_LOCK_WAITING_read << type)

happens before pcpu_read_count()

> 
>> ret = !pcpu_read_count(lock);
>> 
>> if (try && !ret) {
>> -- 
>> 2.45.2

Re: [PATCH] bcachefs: Fix lost wake up

[Kent Overstreet]

On Sun, Sep 01, 2024 at 10:50:37AM GMT, Alan Huang wrote:
> > 2024年9月1日 09:09，Kent Overstreet <kent.overstreet@linux.dev> wrote：
> > 
> > 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 <mmpgouride@gmail.com>
> >> ---
> >> 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.
> 
> Make sure six_set_bitmask(lock, SIX_LOCK_WAITING_read << type)
> 
> happens before pcpu_read_count()

that's in another thread, a memory barrier orders things in the same
thread

Re: [PATCH] bcachefs: Fix lost wake up

[Alan Huang]

2024年9月1日 11:10，Kent Overstreet <kent.overstreet@linux.dev> wrote：
> 
> On Sun, Sep 01, 2024 at 10:50:37AM GMT, Alan Huang wrote:
>>> 2024年9月1日 09:09，Kent Overstreet <kent.overstreet@linux.dev> wrote：
>>> 
>>> 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 <mmpgouride@gmail.com>
>>>> ---
>>>> 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.
>> 
>> Make sure six_set_bitmask(lock, SIX_LOCK_WAITING_read << type)
>> 
>> happens before pcpu_read_count()
> 
> that's in another thread, a memory barrier orders things in the same
> thread

six_set_bitmask(lock, SIX_LOCK_WAITING_read << type) is called before __do_six_trylock()