From nobody Thu Dec 18 20:32:51 2025 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id AC26FC04A6A for ; Sat, 12 Aug 2023 19:27:48 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229874AbjHLT1m (ORCPT ); Sat, 12 Aug 2023 15:27:42 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40274 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229452AbjHLT1l (ORCPT ); Sat, 12 Aug 2023 15:27:41 -0400 Received: from out-72.mta1.migadu.com (out-72.mta1.migadu.com [95.215.58.72]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DC6E4171C for ; Sat, 12 Aug 2023 12:27:43 -0700 (PDT) X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1691868462; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=uFhnb5Aeyg7ibiHxps+oguX37hlZVERnmE8f4QyTYWo=; b=MPXU/W7jn5UrmkZ2doI71eQn2IdC73L/+OlCwe7eRjtlc7VvRGbUoaSTyXVuWo5bpk07s9 bG3PCR1GJ6GYW9WUXgqU9LzyY7npR1SVwixypDYuA8xKAzeUoFepwvDsfkWBQlkwq3bnvx rsHUhfEe6+zBCvno9/iprexYIqKGh+E= From: Kent Overstreet Cc: Kent Overstreet , Linus Torvalds , Boqun Feng , linux-bcachefs@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH] bcachefs: six locks: Fix missing barrier on wait->lock_acquired Date: Sat, 12 Aug 2023 15:27:20 -0400 Message-Id: <20230812192720.2703874-1-kent.overstreet@linux.dev> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Migadu-Flow: FLOW_OUT To: unlisted-recipients:; (no To-header on input) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Type: text/plain; charset="utf-8" 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 Cc: Linus Torvalds Cc: Boqun Feng Cc: linux-bcachefs@vger.kernel.org Cc: linux-kernel@vger.kernel.org Reviewed-by: Boqun Feng --- 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 <=3D 0) goto unlock; =20 - __list_del(w->list.prev, w->list.next); task =3D 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 =3D true; + smp_store_release(&w->lock_acquired, true); wake_up_process(task); } =20 @@ -502,17 +504,32 @@ static int six_lock_slowpath(struct six_lock *lock, e= num six_lock_type type, while (1) { set_current_state(TASK_UNINTERRUPTIBLE); =20 - if (wait->lock_acquired) + /* + * Ensures that writes to the waitlist entry happen after we see + * wait->lock_acquired: pairs with the smp_store_release in + * __six_lock_wakeup + */ + if (smp_load_acquire(&wait->lock_acquired)) break; =20 ret =3D 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 =3D wait->lock_acquired; + if (!acquired) list_del(&wait->list); raw_spin_unlock(&lock->wait_lock); =20 - if (unlikely(wait->lock_acquired)) + if (unlikely(acquired)) do_six_unlock_type(lock, type); break; } --=20 2.40.1