From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp153-166.sina.com.cn (smtp153-166.sina.com.cn [61.135.153.166]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7A610285C8A for ; Mon, 6 Oct 2025 13:16:01 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=61.135.153.166 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1759756570; cv=none; b=eN+UjKLdzEk+6gc7AfF1qyKCXO+rhHDv9aYzCb1khkzt2brmAFAm/cY6PSgJ+0wbZnZPeuawM79RtEUE2Q9YEuV3bYdqv3BJp1woZcKXXzeSDIBIxgq4cplpSErbg1hNR+8Oh2gORtuvOWS5jTWQZFyXDgFHIZqyJGoWUKmbVQQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1759756570; c=relaxed/simple; bh=u3cKwQ/hTEFRDr4aDr8z7FfCeyXmAvEuSX0PEbU89WQ=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=fqCFP5TEFPNEGaooYpvCPZqxXd+ZrVRrCVMoBO6BVfI2uxlRBHSZnuHHpfdGiimfaNBBLZiQByst7xu8y3tTUyqDjUbL6XFufafiGKMOg3mOagX0IqxZ/Dr9c0nfOy1hR6C67GksHAhHhEYm0YuejiHvFNF4hR6Z9eyM68eg32I= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=sina.com; spf=pass smtp.mailfrom=sina.com; dkim=pass (1024-bit key) header.d=sina.com header.i=@sina.com header.b=CVk8qiBu; arc=none smtp.client-ip=61.135.153.166 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=sina.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=sina.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=sina.com header.i=@sina.com header.b="CVk8qiBu" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sina.com; s=201208; t=1759756561; bh=fgLvmbN/6fP7SxWS3f2gpTaHJkF8Nxr/COMwyxv8VTw=; h=From:Subject:Date:Message-ID; b=CVk8qiBuEfe4t3Br2uaeI90X6CHQDdd5IeeVXvOtCqUFLOcOHc2TglLGCUTkOniBD wiz2vSy+XOID/HleUf9pAoRO895bfXuwSxCuNObRC6yEj/vxAAMn4VDynSdXrOBUPx 3TVsbVDtbtG8aBCLaVFBMlVgfB2hpfdEYgYC2Ic4= X-SMAIL-HELO: localhost.localdomain Received: from unknown (HELO localhost.localdomain)([114.249.58.236]) by sina.com (10.54.253.33) with ESMTP id 68E3C106000075F8; Mon, 6 Oct 2025 21:15:52 +0800 (CST) X-Sender: hdanton@sina.com X-Auth-ID: hdanton@sina.com Authentication-Results: sina.com; spf=none smtp.mailfrom=hdanton@sina.com; dkim=none header.i=none; dmarc=none action=none header.from=hdanton@sina.com X-SMAIL-MID: 4891596685193 X-SMAIL-UIID: 0E362BD497144BB5BF50782985302B98-20251006-211552-1 From: Hillf Danton To: Mateusz Guzik Cc: brauner@kernel.org, viro@zeniv.linux.org.uk, jack@suse.cz, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org Subject: Re: [PATCH] fs: add missing fences to I_NEW handling Date: Mon, 6 Oct 2025 21:15:38 +0800 Message-ID: <20251006131543.8283-1-hdanton@sina.com> In-Reply-To: References: Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Mon, 6 Oct 2025 12:30:01 +0200 Mateusz Guzik wrote: > On Mon, Oct 6, 2025 at 9:21 AM Hillf Danton wrote: > > On Mon, 6 Oct 2025 03:16:43 +0200 Mateusz Guzik wrote: > > > That fence synchronizes against threads which went to sleep. > > > > > > In the example I'm providing this did not happen. > > > > > > 193 static inline void wait_on_inode(struct inode *inode) > > > 194 { > > > 195 wait_var_event(inode_state_wait_address(inode, __I_NEW), > > > 196 !(READ_ONCE(inode->i_state) & I_NEW)); > > > > > > 303 #define wait_var_event(var, condition) \ > > > 304 do { \ > > > 305 might_sleep(); \ > > > 306 if (condition) \ > > > 307 break; \ > > > > > > I_NEW is tested here without any locks or fences. > > > > > Thanks, got it but given the comment of the current mem barrier in > > unlock_new_inode(), why did peterZ/Av leave such a huge chance behind? > > > My guess is nobody is perfect -- mistakes happen. > I do not think you are so lucky -- the code has been there for quite a while. > > The condition check in waitqueue_active() matches waitqueue_active() in > > __wake_up_bit(), and both make it run fast on both the waiter and the > > waker sides, no? > Your quotation here is different from my reply [2]. Weird. The condition check in wait_var_event() matches waitqueue_active() in __wake_up_bit(), and both make it run fast on both the waiter and the waker sides, no? [2] https://lore.kernel.org/lkml/20251006072136.8236-1-hdanton@sina.com/ > So happens the commentary above wait_var_event() explicitly mentions > you want an acquire fence: > 299 * The condition should normally use smp_load_acquire() or a similarly > 300 * ordered access to ensure that any changes to memory made before the > 301 * condition became true will be visible after the wait completes. What is missed is recheck -- adding self on to wait queue with a true condition makes no sense. * Wait for a @condition to be true, only re-checking when a wake up is * received for the given @var (an arbitrary kernel address which need * not be directly related to the given condition, but usually is). > > The commentary about waitqueue_active() says you want and even > stronger fence (smp_mb) for that one: Yes, it matches the current code, so your change is not needed. > 104 * Use either while holding wait_queue_head::lock or when used for wakeups > 105 * with an extra smp_mb() like:: > 106 * > 107 * CPU0 - waker CPU1 - waiter > 108 * > 109 * for (;;) { > 110 * @cond = true; prepare_to_wait(&wq_head, &wait, state); > 111 * smp_mb(); // smp_mb() from set_current_state() > 112 * if (waitqueue_active(wq_head)) if (@cond) > 113 * wake_up(wq_head); break; > 114 * schedule(); > 115 * } > 116 * finish_wait(&wq_head, &wait);