vfs: remove redundant smp_mb for thp handling in do_dentry_open

opening for write performs:

if (f->f_mode & FMODE_WRITE) {
[snip]
        smp_mb();
        if (filemap_nr_thps(inode->i_mapping)) {
[snip]
        }
}

filemap_nr_thps on kernels built without CONFIG_READ_ONLY_THP_FOR
expands to 0, allowing the compiler to eliminate the entire thing, with
exception of the fence (and the branch leading there).

So happens required synchronisation between i_writecount and nr_thps
changes is already provided by the full fence coming from
get_write_access -> atomic_inc_unless_negative, thus the smp_mb instance
above can be removed regardless of CONFIG_READ_ONLY_THP_FOR.

While I updated commentary in places claiming to match the now-removed
fence, I did not try to patch them to act on the compile option.

I did not bother benchmarking it, not issuing a spurious full fence in
the fast path does not warrant justification from perf standpoint.

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
Link: https://lore.kernel.org/r/20240624085402.493630-1-mjguzik@gmail.com
Signed-off-by: Christian Brauner <brauner@kernel.org>

diff --git a/fs/open.c b/fs/open.c
index a5c4f8a0f14388546a42c3905cbd1c9fc9635ec8..c4e9b01aafd8d00eaac146d73c526af57f5ecae8 100644 (file)
--- a/fs/open.c
+++ b/fs/open.c
@@ -986,12 +986,11 @@ static int do_dentry_open(struct file *f,
         */
        if (f->f_mode & FMODE_WRITE) {
                /*
-                * Paired with smp_mb() in collapse_file() to ensure nr_thps
-                * is up to date and the update to i_writecount by
-                * get_write_access() is visible. Ensures subsequent insertion
-                * of THPs into the page cache will fail.
+                * 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.
                 */
-               smp_mb();
                if (filemap_nr_thps(inode->i_mapping)) {
                        struct address_space *mapping = inode->i_mapping;
 

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 774a97e6e2da39f0c174b885f2ea2297a8df4c61..aab471791bd9574c3f25664a482b28ff00559538 100644 (file)
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -2000,9 +2000,9 @@ out_unlock:
        if (!is_shmem) {
                filemap_nr_thps_inc(mapping);
                /*
-                * Paired with smp_mb() in do_dentry_open() 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
+                * 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();
@@ -2190,8 +2190,8 @@ rollback:
        if (!is_shmem && result == SCAN_COPY_MC) {
                filemap_nr_thps_dec(mapping);
                /*
-                * Paired with smp_mb() in do_dentry_open() to
-                * ensure the update to nr_thps is visible.
+                * Paired with the fence in do_dentry_open() -> get_write_access()
+                * to ensure the update to nr_thps is visible.
                 */
                smp_mb();
        }