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

["Theodore Ts'o" <tytso@mit.edu>]

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.

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.

Cheers,

					- Ted