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

["Vlastimil Babka (SUSE)" <vbabka@kernel.org>]

On 7/12/24 7:31 AM, Byungchul Park wrote:
> On Fri, Jul 12, 2024 at 01:44:20PM +0900, Byungchul Park wrote:
>> 
>> 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.

enabling means building with CONFIG_DEPT right?

>> > 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 doubt anyone will object making struct page larger for a non-production
debugging config option, which AFAIU DEPT is, i.e. in the same area as
LOCKDEP or KASAN etc... I can see at least KMSAN already adds some fields to
struct page already.

>> 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.

page_ext is useful if you have a debugging feature that can be compiled in
but adds no overhead (memory, nor cpu thanks to static keys) unless enabled
on boot time, i.e. page_owner... so for DEPT it seems it would be an
unnecessary complication.

> 	Byungchul
>