From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f48.google.com (mail-wm1-f48.google.com [209.85.128.48]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1ADDE18DB37 for ; Mon, 18 May 2026 02:52:24 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.48 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779072746; cv=none; b=XH/THL3u5956kaej6I9EifkTgtAZcbmRDccN8Pu6tJgo17GwURGkJbzQawYNGXMPF6BZSce8GVtRLykIQhh2KlcwmoRcbVqZuHq6LL++1T+xM6rDbgAGERJ6k2A88LMV8+qPHkw98dRa0tdxkPUb9RvGeHDX924kn6KAx8qHYGU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779072746; c=relaxed/simple; bh=srjcodAozv4JLuO7X9acfYWYMGPSwx6Uadd2oOcjSA0=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=QSQiFgamXHQzjJYqFX0GfM3XlOjFdE0DCf+RHfYyDBgquU7XY4eODnGjZC3svjeiAlrUpGhdNH64fxtON5mdUbC764Cb4/FP0p9Zyg/GVDbcoysG1Dxk2kqPoT1vznTUetz8zyqINfGmr2bl7DD3YJTOLMWCLr6JKlJQoHRGUZc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=suse.com; spf=pass smtp.mailfrom=suse.com; dkim=pass (2048-bit key) header.d=suse.com header.i=@suse.com header.b=W5mKygOp; arc=none smtp.client-ip=209.85.128.48 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=suse.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=suse.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=suse.com header.i=@suse.com header.b="W5mKygOp" Received: by mail-wm1-f48.google.com with SMTP id 5b1f17b1804b1-4891b02a0acso444705e9.3 for ; Sun, 17 May 2026 19:52:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1779072743; x=1779677543; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=BJLlaxjhYndtoJyGrs9NNxoX5Vmo11cQYCIpaiSe9Hw=; b=W5mKygOpERfHYe8Tot7WPTxrjdc5kbQ0OlxSZrmbj3KqoTTQncR3GMeBb4t96ZSB7b FDQtOQmL8sKc7vOOQnCBwn/pQNNm0rbtlsgtP+n2fk2gJMi+dBSl8cwT20FJpwBx6CTU vjz3/eX2Vo47nGni/0kmti4B1NSaVTuaQyoE0AkmXt/7D1Jm4NQR1D6FIfZ9vgq0rxJL I+jSytJhJIwsnoZnBbXlBG/0ILYDDCIyBFtJPYbXB3qejLqfml/cHlFaCHVXjbwC4I2P Wl4t5RsihzWJeDefWTckNGVcH4eQVg67iE4qRxI2rYdtz2ekyYP3DtXSk+I4XmISWatJ 9utw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1779072743; x=1779677543; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=BJLlaxjhYndtoJyGrs9NNxoX5Vmo11cQYCIpaiSe9Hw=; b=lGsmH4VQhGN+H90OyndHLnhZZ7GyX9G/+XqSKB2ncllISNiD6soQHamrKLGC42tjt1 jkN3ee4iFRC4Tu+CtqMpuuCGY5yQD1NWkqz6kshbK+AZIZ+8Sp43lTDPUsx/0nwLIrM3 bVy1htmfUpprsfyGyPTXIBO2P5PqxrHCFzM6ElUG0rc3HDDlbnBugpU7KLNgTtMwV3JO J5xm3GDqGsr2f/eQSCHfuSF84uDtgkdvLRThao2+MLAtcBv9oHCyU4f4AC4FUNhW+Zr2 /7bh3A1E9pkdoQax3dQXypsVDIR/8UwOwY9709CnlPpRFEzgBwhPvdEagPs+RuFTVQgl Dt/A== X-Forwarded-Encrypted: i=1; AFNElJ8sMz0hpsFYAkkFsIMS09p7EXQajF66GVZRMM8QZmb+0lQPJNRy347S4+yaAGACFcim1Ju/racV6XxZtWs=@vger.kernel.org X-Gm-Message-State: AOJu0YyYaAuvNtlmE+XfBmaNoFKL6L2H1hWcyGFbTU5gRmtHENVB4hb1 EW/OhkTwPYpMyk3UaFG/mZCf2T/JGdS+nYQwxQUg0AeVbSuwch4yqkJEuO2VoHv1WTuAL8n7tfI pEvaug7c= X-Gm-Gg: Acq92OEciFyFknoaoVxCo4YEGt3LzIZaZAdnCnIbGdtjpeBqdBlD1L1nkwxmJ7EsZ6l TnChSUFAOZmfWVTGevWa3560INv7kcm0+Ob0Iro4PCMf1MCXUY1x+2VUMc/m5PuHAsJYDg5Hbou GMA0CtWvuQ8xgRcDzo4wjMvAy42dandiu3gkH6bINVMNyDuVhC51io1mhbEzpYu0ywZKjtv2K9v A0sZ94fc+bjj6+dxK9FU9HpNy/N7UVF6bAqex2+TJvHN80lhL6c1u2hM6KAnh6K3wML8FKGBaTf YTKgIQ+wjhQhpTKGdvGdqBrd0e5gEqjoaOGRRfWEY0QHEBPNI4EG7qxZ1clSxXx9Jo2zu8jlRSX bGSnUJVa3lh4cjD1yQFSadX8n8Vox7gHKe/BkGVG5iLTSa51si6OTMheNZg+rYDbGpowpnbynFE KSX3aPSFgxshI0s/Pe26mEBA== X-Received: by 2002:a05:600c:198d:b0:489:6c28:dbb4 with SMTP id 5b1f17b1804b1-48fe62f7cbbmr109725735e9.5.1779072743330; Sun, 17 May 2026 19:52:23 -0700 (PDT) Received: from localhost ([202.127.77.110]) by smtp.gmail.com with ESMTPSA id 41be03b00d2f7-c82bb114a70sm14642994a12.22.2026.05.17.19.52.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 17 May 2026 19:52:22 -0700 (PDT) Date: Mon, 18 May 2026 10:52:19 +0800 From: Heming Zhao To: Tetsuo Handa Cc: Mark Fasheh , Joel Becker , Joseph Qi , jiangyiwen , Andrew Morton , ocfs2-devel@lists.linux.dev, LKML Subject: Re: [PATCH] ocfs2: kill osb->system_file_mutex lock Message-ID: References: <934355dd-a0b1-4e53-93ac-0a7ae7458051@I-love.SAKURA.ne.jp> <831c4fc1-c89f-48bc-84c6-25b2cefc2b20@I-love.SAKURA.ne.jp> <670882aa-b637-4565-adf0-ddcd9d7a588b@I-love.SAKURA.ne.jp> 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=us-ascii Content-Disposition: inline In-Reply-To: <670882aa-b637-4565-adf0-ddcd9d7a588b@I-love.SAKURA.ne.jp> On Sat, May 16, 2026 at 10:10:44PM +0900, Tetsuo Handa wrote: > On 2026/05/16 21:27, Heming Zhao wrote: > >>>> In my opinion, the problem with the current code is that the scope of > >>>> mutex_lock(&osb->system_file_mutex) is too broad. This mutex only needs to be > >>>> held prior to calling _ocfs2_get_system_file_inode(). I previously highlighted > >>>> this point in my initial review comment on the patch. > >> > >> If you meant > >> > >> struct inode *ocfs2_get_system_file_inode(struct ocfs2_super *osb, > >> int type, > >> u32 slot) > >> { > >> struct inode *inode = NULL; > >> struct inode **arr = NULL; > >> > >> /* avoid the lookup if cached in local system file array */ > >> if (is_global_system_inode(type)) { > >> arr = &(osb->global_system_inodes[type]); > >> } else > >> arr = get_local_system_inode(osb, type, slot); > >> > >> - mutex_lock(&osb->system_file_mutex); > >> if (arr && ((inode = *arr) != NULL)) { > >> /* get a ref in addition to the array ref */ > >> inode = igrab(inode); > >> mutex_unlock(&osb->system_file_mutex); > >> BUG_ON(!inode); > >> > >> return inode; > >> } > >> > >> + mutex_lock(&osb->system_file_mutex); > >> + > >> /* this gets one ref thru iget */ > >> inode = _ocfs2_get_system_file_inode(osb, type, slot); > >> > >> /* add one more if putting into array for first time */ > >> if (arr && inode) { > >> *arr = igrab(inode); > >> BUG_ON(!*arr); > >> } > >> mutex_unlock(&osb->system_file_mutex); > >> return inode; > >> } > >> > > > > Yes, this is what I expected. > > > >> , that brings us back to before commit 43b10a20372d, for two threads can hit > >> the "*arr = igrab(inode)" line. > > > > I don't think this bring us back, because igrab() uses a spinlock to protect the > > refcount. > > You are missing the point. What is wrong with above change is that two threads can > sequentially enter into the "if (arr && inode) { }" block, for we are not checking > whether the slot is already NULL or not. The cmpxchg() is the magic that guarantees > that only one thread can enter into that block. To avoid my code issue, the code should be: mutex_lock(&osb->system_file_mutex); if (arr && !*arr) { /* this gets one ref thru iget */ inode = _ocfs2_get_system_file_inode(osb, type, slot); /* add one more if putting into array for first time */ if (inode) { *arr = igrab(inode); BUG_ON(!*arr); } } else if (arr && *arr) { /* get a ref in addition to the array ref */ inode = igrab(*arr); BUG_ON(!inode); } mutex_unlock(&osb->system_file_mutex); If apply this change, the code will look too complicated and hard to maintain. Regarding the removal of the mutex, your patch looks good to me now. My new comment for your patch: Because this issue was triggered by syzbot testing Diogo Jahchan Koike 's patch for bug[1], I think it's better to removed the 'Reported-by' and close' tags, and describe the history in commit log. Current 'Reported-by' & 'close' tags are misleading. [1]: https://syzkaller.appspot.com/bug?extid=1fed2de07d8e11a3ec1b Thanks, Heming > > >> > >> Even if you also change the "if (arr && inode) {" check in order to make sure that > >> the second-reached thread won't again hit the "*arr = igrab(inode)" line when the > >> first-reached thread already hit the "*arr = igrab(inode)" line, the second-reached > >> thread is fully blocked by the first-reached thread. Not serializing two threads > >> allows these threads to return as quick as serializing these threads. > > > > with the mutex protection removed, and relying on igrab()'s internal spinlock, > > the code logic is already correct for concurrency. > > > > there are two kinds of locks involved in this discussion: > > - inode->i_lock: protects the inode's refcount (used by igrab()) > > - osb->system_file_mutex: protects the array slot. > > You are missing the point. The lock used inside igrab() is irrelevant. > The cmpxchg() is the lock that protects the array slot (in other words, > serializes two threads) without using the system_file_mutex lock. >