Re: Possible circular dependency between i_data_sem and folio lock in ext4 filesystem

[Byungchul Park <byungchul@sk.com>]

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