Re: [PATCH v1 03/10] mm: fs: remove filemap_nr_thps*() functions and their users

[David Hildenbrand (Arm)]

On 3/27/26 16:05, Zi Yan wrote:
> On 27 Mar 2026, at 10:23, Lorenzo Stoakes (Oracle) wrote:
> 
>> On Fri, Mar 27, 2026 at 02:58:12PM +0100, David Hildenbrand (Arm) wrote:
>>>
>>> There could now be a race between collapsing and the file getting opened
>>> r/w.
>>>
>>> Are we sure that all code can really deal with that?
>>>
>>> IOW, "they already had to handle it separately" -- is that true?
>>> khugepaged would have never collapse in writable files, so I wonder if
>>> all code paths are prepared for that.
>>
>> OK I guess I overlooked a part of this code... :) see below.
>>
>> This is fine and would be a no-op anyway
>>
>> -       if (f->f_mode & FMODE_WRITE) {
>> -               /*
>> -                * Depends on full fence from get_write_access() to synchronize
>> -                * against collapse_file() regarding i_writecount and nr_thps
>> -                * updates. Ensures subsequent insertion of THPs into the page
>> -                * cache will fail.
>> -                */
>> -               if (filemap_nr_thps(inode->i_mapping)) {
>>
>> But this:
>>
>> -       if (!is_shmem) {
>> -               filemap_nr_thps_inc(mapping);
>> -               /*
>> -                * Paired with the fence in do_dentry_open() -> get_write_access()
>> -                * to ensure i_writecount is up to date and the update to nr_thps
>> -                * is visible. Ensures the page cache will be truncated if the
>> -                * file is opened writable.
>> -                */
>> -               smp_mb();
>>
>> We can drop barrier
>>
>> -               if (inode_is_open_for_write(mapping->host)) {
>> -                       result = SCAN_FAIL;
>>
>> But this is a functional change!
>>
>> Yup missed this.
> 
> But I added
> 
> +	if (!is_shmem && inode_is_open_for_write(mapping->host))
> +		result = SCAN_FAIL;
> 
> That keeps the original bail out, right?

Independent of that, are we sure that the possible race we allow is ok?

-- 
Cheers,

David