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