linux-kernel - Re: [PATCH v1] fs/dax: fix folio splitting issue by resetting old folio order + _nr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <6e1a9ad5-c1e1-4f04-af67-cfc05246acbc@redhat.com>
Date: Fri, 11 Apr 2025 10:37:17 +0200
From: David Hildenbrand <david@...hat.com>
To: Alistair Popple <apopple@...dia.com>
Cc: linux-mm@...ck.org, linux-fsdevel@...r.kernel.org,
 nvdimm@...ts.linux.dev, Alison Schofield <alison.schofield@...el.com>,
 Alexander Viro <viro@...iv.linux.org.uk>,
 Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
 Dan Williams <dan.j.williams@...el.com>, Matthew Wilcox
 <willy@...radead.org>, Andrew Morton <akpm@...ux-foundation.org>,
 Alistair Popple <apopple@...dia.com>, Christoph Hellwig <hch@...radead.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1] fs/dax: fix folio splitting issue by resetting old
 folio order + _nr_pages

(adding CC list again, because I assume it was dropped by accident)

>> diff --git a/fs/dax.c b/fs/dax.c
>> index af5045b0f476e..676303419e9e8 100644
>> --- a/fs/dax.c
>> +++ b/fs/dax.c
>> @@ -396,6 +396,7 @@ static inline unsigned long dax_folio_put(struct folio *folio)
>>   	order = folio_order(folio);
>>   	if (!order)
>>   		return 0;
>> +	folio_reset_order(folio);
> 
> Wouldn't it be better to also move the loop below into this function? The intent
> of this loop was to reinitialise the small folios after splitting which is what
> I think this helper should be doing.

As the function does nothing on small folios (as documented), I think 
this is good enough for now.

Once we decouple folio from page, this code will likely have to change 
either way ...

The first large folio will become a small folio (so resetting kind-of 
makes sense), but the other small folios would have to allocate a new 
"struct folio" for small folios.

> 
>>   	for (i = 0; i < (1UL << order); i++) {
>>   		struct dev_pagemap *pgmap = page_pgmap(&folio->page);
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index b7f13f087954b..bf55206935c46 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -1218,6 +1218,23 @@ static inline unsigned int folio_order(const struct folio *folio)
>>   	return folio_large_order(folio);
>>   }
>>   
>> +/**
>> + * folio_reset_order - Reset the folio order and derived _nr_pages
>> + * @folio: The folio.
>> + *
>> + * Reset the order and derived _nr_pages to 0. Must only be used in the
>> + * process of splitting large folios.
>> + */
>> +static inline void folio_reset_order(struct folio *folio)
>> +{
>> +	if (WARN_ON_ONCE(!folio_test_large(folio)))
>> +		return;
>> +	folio->_flags_1 &= ~0xffUL;
>> +#ifdef NR_PAGES_IN_LARGE_FOLIO
>> +	folio->_nr_pages = 0;
>> +#endif
>> +}
>> +


I'm still not sure if this splitting code in fs/dax.c is more similar to 
THP splitting or to "splitting when freeing in the buddy". I think it's 
something in between: we want small folios, but the new folios are 
essentially free.

Likely, to be future-proof, we should also look into doing

folio->_flags_1 &= ~PAGE_FLAGS_SECOND;

Or alternatively (better?)

new_folio->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;


... but that problem will go away once we decouple page from folio (see 
above), so I'm not sure if we should really do that at this point unless 
there is an issue.

-- 
Cheers,

David / dhildenb