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
