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 <[email protected]>
>>>
>>> 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
