Re: [PATCH] bcachefs: six locks: Fix missing barrier on wait->lock_acquired

[Boqun Feng]

On Sat, Aug 12, 2023 at 03:27:20PM -0400, Kent Overstreet wrote:
> Six locks do lock handoff via the wakeup path: the thread doing the
> wakeup also takes the lock on behalf of the waiter, which means the
> waiter only has to look at its waitlist entry, and doesn't have to touch
> the lock cacheline while another thread is using it.
> 
> Linus noticed that this needs a real barrier, which this patch fixes.
> 
> Also add a comment for the should_sleep_fn() error path.
> 
> Signed-off-by: Kent Overstreet <kent.overstreet@xxxxxxxxx>
> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Cc: Boqun Feng <boqun.feng@xxxxxxxxx>
> Cc: linux-bcachefs@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> ---
>  fs/bcachefs/six.c | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/bcachefs/six.c b/fs/bcachefs/six.c
> index 581aee565e..b6ca53c852 100644
> --- a/fs/bcachefs/six.c
> +++ b/fs/bcachefs/six.c
> @@ -223,14 +223,16 @@ static void __six_lock_wakeup(struct six_lock *lock, enum six_lock_type lock_typ
>  		if (ret <= 0)
>  			goto unlock;
>  
> -		__list_del(w->list.prev, w->list.next);
>  		task = w->task;
> +		__list_del(w->list.prev, w->list.next);
>  		/*
> -		 * Do no writes to @w besides setting lock_acquired - otherwise
> -		 * we would need a memory barrier:
> +		 * The release barrier here ensures the ordering of the
> +		 * __list_del before setting w->lock_acquired; @w is on the
> +		 * stack of the thread doing the waiting and will be reused
> +		 * after it sees w->lock_acquired with no other locking:
> +		 * pairs with smp_load_acquire() in six_lock_slowpath()
>  		 */
> -		barrier();
> -		w->lock_acquired = true;
> +		smp_store_release(&w->lock_acquired, true);
>  		wake_up_process(task);
>  	}
>  
> @@ -502,17 +504,32 @@ static int six_lock_slowpath(struct six_lock *lock, enum six_lock_type type,
>  	while (1) {
>  		set_current_state(TASK_UNINTERRUPTIBLE);
>  
> -		if (wait->lock_acquired)
> +		/*
> +		 * Ensures that writes to the waitlist entry happen after we see

Maybe my English, but "happen after" here is a little confusing: writes
happen after the read of ->lock_acquired? How about

		/*
		 * Ensures once we observe the write to
		 * wait->lock_acquired, we must observe the writes to
		 * the waitlist entry: pairs with smp_store_release in
		 * __six_lock_wakeup
		 */

?

I haven't finished my review on the SIX lock, but this patch looks good
to me, feel free to add:

Reviewed-by: Boqun Feng <boqun.feng@xxxxxxxxx>

Regards,
Boqun

> +		 * wait->lock_acquired: pairs with the smp_store_release in
> +		 * __six_lock_wakeup
> +		 */
> +		if (smp_load_acquire(&wait->lock_acquired))
>  			break;
>  
>  		ret = should_sleep_fn ? should_sleep_fn(lock, p) : 0;
>  		if (unlikely(ret)) {
> +			bool acquired;
> +
> +			/*
> +			 * If should_sleep_fn() returns an error, we are
> +			 * required to return that error even if we already
> +			 * acquired the lock - should_sleep_fn() might have
> +			 * modified external state (e.g. when the deadlock cycle
> +			 * detector in bcachefs issued a transaction restart)
> +			 */
>  			raw_spin_lock(&lock->wait_lock);
> -			if (!wait->lock_acquired)
> +			acquired = wait->lock_acquired;
> +			if (!acquired)
>  				list_del(&wait->list);
>  			raw_spin_unlock(&lock->wait_lock);
>  
> -			if (unlikely(wait->lock_acquired))
> +			if (unlikely(acquired))
>  				do_six_unlock_type(lock, type);
>  			break;
>  		}
> -- 
> 2.40.1
>