Message-ID: <20240712053150.GA68384@system.software.com>
Date: Fri, 12 Jul 2024 14:31:50 +0900
From: Byungchul Park <byungchul@...com>
To: Theodore Ts'o <tytso@....edu>
Cc: Hyeonggon Yoo <42.hyeyoo@...il.com>,
	Linux Memory Management List <linux-mm@...ck.org>,
	linux-fsdevel@...r.kernel.org, linux-ext4@...r.kernel.org,
	max.byungchul.park@...com,
	Gwan-gyeong Mun <gwan-gyeong.mun@...el.com>,
	kernel_team@...ynix.com
Subject: Re: Possible circular dependency between i_data_sem and folio lock
 in ext4 filesystem

On Fri, Jul 12, 2024 at 01:44:20PM +0900, Byungchul Park wrote:
> On Thu, Jul 11, 2024 at 11:38:46AM -0400, Theodore Ts'o wrote:
> > On Thu, Jul 11, 2024 at 09:07:53PM +0900, Hyeonggon Yoo wrote:
> > > Hi folks,
> > > 
> > > Byungchul, Gwan-gyeong and I are investigating possible circular
> > > dependency reported by a dependency tracker named DEPT [1], which is
> > > able to report possible circular dependencies involving folio locks
> > > and other forms of dependencies that are not locks (i.e., wait for
> > > completion).
> > > 
> > > Below are two similar reports from DEPT where one context takes
> > > i_data_sem and then folio lock in ext4_map_blocks(), while the other
> > > context takes folio lock and then i_data_sem during processing of
> > > pwrite64() system calls. We're reaching out due to a lack of
> > > understanding of ext4 and file system internals.
> > > 
> > > The points in question are:
> > > 
> > > - Can the two contexts actually create a dependency between each other
> > > in ext4? In other words, do their uses of folio lock make them belong
> > > to the same lock classes?
> > 
> > No.
> > 
> > > - Are there any locking rules in ext4 that ensure these two contexts
> > > will never be considered as the same lock class?
> > 
> > It's inherent is the code path.  In one of the stack traces, we are
> > using the page cache for the bitmap allocation block (in other words, a metadata
> > block).  In the other stack trace, the page cache belongs to a regular
> > file (in other words, a data block).
> > 
> > So this is a false positive with DEPT, which has always been one of
> > the reasons why I've been dubious about the value of DEPT in terms of
> > potential for make-work for mantainer once automated systems like
> > syzbot try to blindly use and it results in huge numbers of false
> > positive reports that we then have to work through as an unfunded
> > mandate.
> 
> What a funny guy...  He did neither 1) insisting it's a bug in your code
> nor 3) insisting DEPT is a great tool, but just asking if there's any
> locking rules based on the *different acqusition order* between folio
> lock and i_data_sem that he observed anyway.
> 
> I don't think you are a guy who introduces bugs, but the thing is it's
> hard to find a *document* describing locking rules.  Anyone could get
> fairly curious about the different acquisition order.  It's an open
> source project.  You are responsible for appropriate document as well.
> 
> I don't understand why you act to DEPT like that by the way.  You don't
> have to becasue:
> 
>    1. I added the *EXPERIMENTAL* tag in Kconfig as you suggested, which
>       will prevent autotesting until it's considered stable.  However,
>       the report from DEPT can be a good hint to someone.
> 
>    2. DEPT can locate code where needs to be documented even if it's not
>       a real bug.  It could even help better documentation.
> 
> DEPT hurts neither code nor performance unless enabling it.
> 
> > If you want to add lock annotations into the struct page or even
> > struct folio, I cordially invite you to try running that by the mm
> > developers, who will probably tell you why that is a terrible idea
> > since it bloats a critical data structure.
> 
> I already said several times.  Doesn't consume struct page.

Sorry for that.  I've changed the code so the current version consumes
it by about two words if enabled.  I can place it to page_ext as before
if needed.

	Byungchul

Powered by blists - more mailing lists