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
