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

[Zi Yan <ziy@nvidia.com>]

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:
>> On 3/27/26 13:23, Lorenzo Stoakes (Oracle) wrote:
>>> On Thu, Mar 26, 2026 at 09:42:48PM -0400, Zi Yan wrote:
>>>> They are used by READ_ONLY_THP_FOR_FS to handle writes to FSes without
>>>> large folio support, so that read-only THPs created in these FSes are not
>>>> seen by the FSes when the underlying fd becomes writable. Now read-only PMD
>>>> THPs only appear in a FS with large folio support and the supported orders
>>>> include PMD_ORDRE.
>>>
>>> Typo: PMD_ORDRE -> PMD_ORDER
>>>
>>>>
>>>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>>>
>>> This looks obviously-correct since this stuff wouldn't have been invoked for
>>> large folio file systems before + they already had to handle it separately, and
>>> this function is only tied to CONFIG_READ_ONLY_THP_FOR_FS (+ a quick grep
>>> suggests you didn't miss anything), so:
>>
>> 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?

>
> -                       filemap_nr_thps_dec(mapping);
> -               }
> -       }
>
> For below:
>
> -       /*
> -        * Undo the updates of filemap_nr_thps_inc for non-SHMEM
> -        * file only. This undo is not needed unless failure is
> -        * due to SCAN_COPY_MC.
> -        */
> -       if (!is_shmem && result == SCAN_COPY_MC) {
> -               filemap_nr_thps_dec(mapping);
> -               /*
> -                * Paired with the fence in do_dentry_open() -> get_write_access()
> -                * to ensure the update to nr_thps is visible.
> -                */
> -               smp_mb();
> -       }
>
> Here is probably fine to remove if barrier _only_ for nr_thps.
>
>>
>> --
>> Cheers,
>>
>> David
>
> Sorry Zi, R-b tag withdrawn... :( I missed that 1 functional change there.
>
> Cheers, Lorenzo


Best Regards,
Yan, Zi