* [PATCH 2/2] fs: track the inode having file locks with a flag in ->i_opflags
2025-11-09 17:25 [PATCH 1/2] filelock: use a consume fence in locks_inode_context() Mateusz Guzik
@ 2025-11-09 17:25 ` Mateusz Guzik
0 siblings, 0 replies; 2+ messages in thread
From: Mateusz Guzik @ 2025-11-09 17:25 UTC (permalink / raw)
To: brauner, jlayton; +Cc: viro, jack, linux-kernel, linux-fsdevel, Mateusz Guzik
Opening and closing an inode dirties the ->i_readcount field.
There is false-sharing with both of these operations, extent of which
depends on how misaligned the inode happens to be.
This notably concerns the ->i_flctx field. Since most inodes don't have
the field populated, this bit can be managed with a flag in ->i_opflags
instead which bypasses the problem.
Here are results I obtained while opening a file read-only in a loop
with 24 cores doing the work on Sapphire Rapids. Utilizing the flag as
opposed to reading ->i_flctx field was toggled at runtime as the benchmark
was running, to make sure both results come from the same alignment.
before: 3233740
after: 3373346 (+4%)
before: 3284313
after: 3518711 (+7%)
before: 3505545
after: 4092806 (+16%)
Or to put it differently, this varies wildly depending on how (un)lucky
you get.
The primary bottleneck before and after is the avoidable lockref trip in
do_dentry_open(), afterwards it's a toss up between apparmor and
->i_readcount.
Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
of course someone(tm) should sort out struct inode, but that's a longer
story and even then it may be beneficial to not load i_flctx.
bench code for will-it-scale below.
It differs from open3 in that it is not read-write. Opening a file
read-write in parallel suffers *more* false-sharing and is less common,
thus a more likely case of read-only.
# cat tests/openro3.c
#include <stdlib.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <assert.h>
static char tmpfile[] = "/tmp/willitscale.XXXXXX";
char *testcase_description = "Same file open/close read-only";
void testcase_prepare(unsigned long nr_tasks)
{
int fd = mkstemp(tmpfile);
assert(fd >= 0);
close(fd);
}
void testcase(unsigned long long *iterations, unsigned long nr)
{
while (1) {
int fd = open(tmpfile, O_RDONLY);
assert(fd >= 0);
close(fd);
(*iterations)++;
}
}
void testcase_cleanup(void)
{
unlink(tmpfile);
}
fs/locks.c | 14 ++++++++++++--
include/linux/filelock.h | 15 +++++++++++----
include/linux/fs.h | 1 +
3 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index 04a3f0e20724..9cd23dd2a74e 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -178,7 +178,6 @@ locks_get_lock_context(struct inode *inode, int type)
{
struct file_lock_context *ctx;
- /* paired with cmpxchg() below */
ctx = locks_inode_context(inode);
if (likely(ctx) || type == F_UNLCK)
goto out;
@@ -196,7 +195,18 @@ locks_get_lock_context(struct inode *inode, int type)
* Assign the pointer if it's not already assigned. If it is, then
* free the context we just allocated.
*/
- if (cmpxchg(&inode->i_flctx, NULL, ctx)) {
+ spin_lock(&inode->i_lock);
+ if (!(inode->i_opflags & IOP_FLCTX)) {
+ VFS_BUG_ON_INODE(inode->i_flctx, inode);
+ WRITE_ONCE(inode->i_flctx, ctx);
+ /*
+ * Paired with locks_inode_context().
+ */
+ smp_store_release(&inode->i_opflags, inode->i_opflags | IOP_FLCTX);
+ spin_unlock(&inode->i_lock);
+ } else {
+ VFS_BUG_ON_INODE(!inode->i_flctx, inode);
+ spin_unlock(&inode->i_lock);
kmem_cache_free(flctx_cache, ctx);
ctx = locks_inode_context(inode);
}
diff --git a/include/linux/filelock.h b/include/linux/filelock.h
index 37e1b33bd267..e23cbc562534 100644
--- a/include/linux/filelock.h
+++ b/include/linux/filelock.h
@@ -233,8 +233,12 @@ static inline struct file_lock_context *
locks_inode_context(const struct inode *inode)
{
/*
- * Paired with the fence in locks_get_lock_context().
+ * Paired with smp_store_release in locks_get_lock_context().
+ *
+ * Ensures ->i_flctx will be visible if we spotted the flag.
*/
+ if (likely(!(smp_load_acquire(&inode->i_opflags) & IOP_FLCTX)))
+ return 0;
return READ_ONCE(inode->i_flctx);
}
@@ -441,7 +445,7 @@ static inline int break_lease(struct inode *inode, unsigned int mode)
* could end up racing with tasks trying to set a new lease on this
* file.
*/
- flctx = READ_ONCE(inode->i_flctx);
+ flctx = locks_inode_context(inode);
if (!flctx)
return 0;
smp_mb();
@@ -460,7 +464,7 @@ static inline int break_deleg(struct inode *inode, unsigned int mode)
* could end up racing with tasks trying to set a new lease on this
* file.
*/
- flctx = READ_ONCE(inode->i_flctx);
+ flctx = locks_inode_context(inode);
if (!flctx)
return 0;
smp_mb();
@@ -493,8 +497,11 @@ static inline int break_deleg_wait(struct inode **delegated_inode)
static inline int break_layout(struct inode *inode, bool wait)
{
+ struct file_lock_context *flctx;
+
smp_mb();
- if (inode->i_flctx && !list_empty_careful(&inode->i_flctx->flc_lease))
+ flctx = locks_inode_context(inode);
+ if (flctx && !list_empty_careful(&flctx->flc_lease))
return __break_lease(inode,
wait ? O_WRONLY : O_WRONLY | O_NONBLOCK,
FL_LAYOUT);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 7c2aa286ef87..314a1349747b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -655,6 +655,7 @@ is_uncached_acl(struct posix_acl *acl)
#define IOP_MGTIME 0x0020
#define IOP_CACHED_LINK 0x0040
#define IOP_FASTPERM_MAY_EXEC 0x0080
+#define IOP_FLCTX 0x0100
/*
* Inode state bits. Protected by inode->i_lock
--
2.48.1
^ permalink raw reply related [flat|nested] 2+ messages in thread