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

[Byungchul Park <byungchul@sk.com>]

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.

	Byungchul

> Cheers,
> 
> 					- Ted