* [PATCH 2/5] dcache: back inline names with a struct-wrapped array of unsigned long
2024-12-10 2:48 ` [PATCH 1/5] make sure that DCACHE_INLINE_LEN is a multiple of word size Al Viro
@ 2024-12-10 2:48 ` Al Viro
2024-12-10 2:48 ` [PATCH 3/5] make take_dentry_name_snapshot() lockless Al Viro
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Al Viro @ 2024-12-10 2:48 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-fsdevel, linux-ext4
... so that they can be copied with struct assignment (which generates
better code) and accessed word-by-word. swap_names() used to do the
latter already, using casts, etc.; now that can be done cleanly.
Both dentry->d_iname and name_snapshot->inline_name got such treatment.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
fs/dcache.c | 19 +++++++------------
include/linux/dcache.h | 14 ++++++++++++--
2 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index ea0f0bea511b..007e582c3e68 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -334,8 +334,7 @@ void take_dentry_name_snapshot(struct name_snapshot *name, struct dentry *dentry
if (unlikely(dname_external(dentry))) {
atomic_inc(&external_name(dentry)->u.count);
} else {
- memcpy(name->inline_name, dentry->d_iname,
- dentry->d_name.len + 1);
+ name->inline_name_words = dentry->d_iname_words;
name->name.name = name->inline_name;
}
spin_unlock(&dentry->d_lock);
@@ -2729,9 +2728,8 @@ static void swap_names(struct dentry *dentry, struct dentry *target)
* dentry:internal, target:external. Steal target's
* storage and make target internal.
*/
- memcpy(target->d_iname, dentry->d_name.name,
- dentry->d_name.len + 1);
dentry->d_name.name = target->d_name.name;
+ target->d_iname_words = dentry->d_iname_words;
target->d_name.name = target->d_iname;
}
} else {
@@ -2740,18 +2738,16 @@ static void swap_names(struct dentry *dentry, struct dentry *target)
* dentry:external, target:internal. Give dentry's
* storage to target and make dentry internal
*/
- memcpy(dentry->d_iname, target->d_name.name,
- target->d_name.len + 1);
target->d_name.name = dentry->d_name.name;
+ dentry->d_iname_words = target->d_iname_words;
dentry->d_name.name = dentry->d_iname;
} else {
/*
* Both are internal.
*/
- for (int i = 0; i < DNAME_INLINE_WORDS; i++) {
- swap(((long *) &dentry->d_iname)[i],
- ((long *) &target->d_iname)[i]);
- }
+ for (int i = 0; i < DNAME_INLINE_WORDS; i++)
+ swap(dentry->d_iname_words.words[i],
+ target->d_iname_words.words[i]);
}
}
swap(dentry->d_name.hash_len, target->d_name.hash_len);
@@ -2766,8 +2762,7 @@ static void copy_name(struct dentry *dentry, struct dentry *target)
atomic_inc(&external_name(target)->u.count);
dentry->d_name = target->d_name;
} else {
- memcpy(dentry->d_iname, target->d_name.name,
- target->d_name.len + 1);
+ dentry->d_iname_words = target->d_iname_words;
dentry->d_name.name = dentry->d_iname;
dentry->d_name.hash_len = target->d_name.hash_len;
}
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 42dd89beaf4e..766a9156836e 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -79,6 +79,10 @@ extern const struct qstr dotdot_name;
#define DNAME_INLINE_LEN (DNAME_INLINE_WORDS*sizeof(unsigned long))
+struct shortname_store {
+ unsigned long words[DNAME_INLINE_WORDS];
+};
+
#define d_lock d_lockref.lock
struct dentry {
@@ -90,7 +94,10 @@ struct dentry {
struct qstr d_name;
struct inode *d_inode; /* Where the name belongs to - NULL is
* negative */
- unsigned char d_iname[DNAME_INLINE_LEN]; /* small names */
+ union {
+ unsigned char d_iname[DNAME_INLINE_LEN]; /* small names */
+ struct shortname_store d_iname_words;
+ };
/* --- cacheline 1 boundary (64 bytes) was 32 bytes ago --- */
/* Ref lookup also touches following */
@@ -591,7 +598,10 @@ static inline struct inode *d_real_inode(const struct dentry *dentry)
struct name_snapshot {
struct qstr name;
- unsigned char inline_name[DNAME_INLINE_LEN];
+ union {
+ unsigned char inline_name[DNAME_INLINE_LEN];
+ struct shortname_store inline_name_words;
+ };
};
void take_dentry_name_snapshot(struct name_snapshot *, struct dentry *);
void release_dentry_name_snapshot(struct name_snapshot *);
--
2.39.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/5] make take_dentry_name_snapshot() lockless
2024-12-10 2:48 ` [PATCH 1/5] make sure that DCACHE_INLINE_LEN is a multiple of word size Al Viro
2024-12-10 2:48 ` [PATCH 2/5] dcache: back inline names with a struct-wrapped array of unsigned long Al Viro
@ 2024-12-10 2:48 ` Al Viro
2024-12-10 2:48 ` [PATCH 4/5] dissolve external_name.u into separate members Al Viro
2024-12-10 2:48 ` [PATCH 5/5] ext4 fast_commit: make use of name_snapshot primitives Al Viro
3 siblings, 0 replies; 6+ messages in thread
From: Al Viro @ 2024-12-10 2:48 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-fsdevel, linux-ext4
Use ->d_seq instead of grabbing ->d_lock; in case of shortname dentries
that avoids any stores to shared data objects and in case of long names
we are down to (unavoidable) atomic_inc on the external_name refcount.
Makes the thing safer as well - the areas where ->d_seq is held odd are
all nested inside the areas where ->d_lock is held, and the latter are
much more numerous.
NOTE: we no longer can have external_name.u.count and external_name.u.head
sharing space, now that we have lockless path that might try to grab
a reference on already doomed instance (kudos to Linus for spotting that).
For now just turn that external_name.u into a struct (instead of union)
to reduce the noise in this commit; the next commit will dissolve it.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
fs/dcache.c | 38 ++++++++++++++++++++++++++++----------
1 file changed, 28 insertions(+), 10 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index 007e582c3e68..ae13e89ce7d7 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -296,9 +296,9 @@ static inline int dentry_cmp(const struct dentry *dentry, const unsigned char *c
}
struct external_name {
- union {
- atomic_t count;
- struct rcu_head head;
+ struct {
+ atomic_t count; // ->count and ->head can't be combined
+ struct rcu_head head; // see take_dentry_name_snapshot()
} u;
unsigned char name[];
};
@@ -329,15 +329,33 @@ static inline int dname_external(const struct dentry *dentry)
void take_dentry_name_snapshot(struct name_snapshot *name, struct dentry *dentry)
{
- spin_lock(&dentry->d_lock);
- name->name = dentry->d_name;
- if (unlikely(dname_external(dentry))) {
- atomic_inc(&external_name(dentry)->u.count);
- } else {
- name->inline_name_words = dentry->d_iname_words;
+ unsigned seq;
+ const unsigned char *s;
+
+ rcu_read_lock();
+retry:
+ seq = read_seqcount_begin(&dentry->d_seq);
+ s = READ_ONCE(dentry->d_name.name);
+ name->name.hash_len = dentry->d_name.hash_len;
+ if (likely(s == dentry->d_iname)) {
name->name.name = name->inline_name;
+ name->inline_name_words = dentry->d_iname_words;
+ if (read_seqcount_retry(&dentry->d_seq, seq))
+ goto retry;
+ } else {
+ struct external_name *p;
+ p = container_of(s, struct external_name, name[0]);
+ name->name.name = s;
+ // get a valid reference
+ if (unlikely(!atomic_inc_not_zero(&p->u.count)))
+ goto retry;
+ if (read_seqcount_retry(&dentry->d_seq, seq)) {
+ if (unlikely(atomic_dec_and_test(&p->u.count)))
+ kfree_rcu(p, u.head);
+ goto retry;
+ }
}
- spin_unlock(&dentry->d_lock);
+ rcu_read_unlock();
}
EXPORT_SYMBOL(take_dentry_name_snapshot);
--
2.39.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 4/5] dissolve external_name.u into separate members
2024-12-10 2:48 ` [PATCH 1/5] make sure that DCACHE_INLINE_LEN is a multiple of word size Al Viro
2024-12-10 2:48 ` [PATCH 2/5] dcache: back inline names with a struct-wrapped array of unsigned long Al Viro
2024-12-10 2:48 ` [PATCH 3/5] make take_dentry_name_snapshot() lockless Al Viro
@ 2024-12-10 2:48 ` Al Viro
2024-12-10 2:48 ` [PATCH 5/5] ext4 fast_commit: make use of name_snapshot primitives Al Viro
3 siblings, 0 replies; 6+ messages in thread
From: Al Viro @ 2024-12-10 2:48 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-fsdevel, linux-ext4
kept separate from the previous commit to keep the noise separate
from actual changes...
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
fs/dcache.c | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index ae13e89ce7d7..163bb0953b4f 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -296,10 +296,8 @@ static inline int dentry_cmp(const struct dentry *dentry, const unsigned char *c
}
struct external_name {
- struct {
- atomic_t count; // ->count and ->head can't be combined
- struct rcu_head head; // see take_dentry_name_snapshot()
- } u;
+ atomic_t count; // ->count and ->head can't be combined
+ struct rcu_head head; // see take_dentry_name_snapshot()
unsigned char name[];
};
@@ -347,11 +345,11 @@ void take_dentry_name_snapshot(struct name_snapshot *name, struct dentry *dentry
p = container_of(s, struct external_name, name[0]);
name->name.name = s;
// get a valid reference
- if (unlikely(!atomic_inc_not_zero(&p->u.count)))
+ if (unlikely(!atomic_inc_not_zero(&p->count)))
goto retry;
if (read_seqcount_retry(&dentry->d_seq, seq)) {
- if (unlikely(atomic_dec_and_test(&p->u.count)))
- kfree_rcu(p, u.head);
+ if (unlikely(atomic_dec_and_test(&p->count)))
+ kfree_rcu(p, head);
goto retry;
}
}
@@ -364,8 +362,8 @@ void release_dentry_name_snapshot(struct name_snapshot *name)
if (unlikely(name->name.name != name->inline_name)) {
struct external_name *p;
p = container_of(name->name.name, struct external_name, name[0]);
- if (unlikely(atomic_dec_and_test(&p->u.count)))
- kfree_rcu(p, u.head);
+ if (unlikely(atomic_dec_and_test(&p->count)))
+ kfree_rcu(p, head);
}
}
EXPORT_SYMBOL(release_dentry_name_snapshot);
@@ -403,7 +401,7 @@ static void dentry_free(struct dentry *dentry)
WARN_ON(!hlist_unhashed(&dentry->d_u.d_alias));
if (unlikely(dname_external(dentry))) {
struct external_name *p = external_name(dentry);
- if (likely(atomic_dec_and_test(&p->u.count))) {
+ if (likely(atomic_dec_and_test(&p->count))) {
call_rcu(&dentry->d_u.d_rcu, __d_free_external);
return;
}
@@ -1684,7 +1682,7 @@ static struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
kmem_cache_free(dentry_cache, dentry);
return NULL;
}
- atomic_set(&p->u.count, 1);
+ atomic_set(&p->count, 1);
dname = p->name;
} else {
dname = dentry->d_iname;
@@ -2777,15 +2775,15 @@ static void copy_name(struct dentry *dentry, struct dentry *target)
if (unlikely(dname_external(dentry)))
old_name = external_name(dentry);
if (unlikely(dname_external(target))) {
- atomic_inc(&external_name(target)->u.count);
+ atomic_inc(&external_name(target)->count);
dentry->d_name = target->d_name;
} else {
dentry->d_iname_words = target->d_iname_words;
dentry->d_name.name = dentry->d_iname;
dentry->d_name.hash_len = target->d_name.hash_len;
}
- if (old_name && likely(atomic_dec_and_test(&old_name->u.count)))
- kfree_rcu(old_name, u.head);
+ if (old_name && likely(atomic_dec_and_test(&old_name->count)))
+ kfree_rcu(old_name, head);
}
/*
--
2.39.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 5/5] ext4 fast_commit: make use of name_snapshot primitives
2024-12-10 2:48 ` [PATCH 1/5] make sure that DCACHE_INLINE_LEN is a multiple of word size Al Viro
` (2 preceding siblings ...)
2024-12-10 2:48 ` [PATCH 4/5] dissolve external_name.u into separate members Al Viro
@ 2024-12-10 2:48 ` Al Viro
3 siblings, 0 replies; 6+ messages in thread
From: Al Viro @ 2024-12-10 2:48 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-fsdevel, linux-ext4
... rather than open-coding them (and doing pointless work with extra
allocations, etc. for long names).
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
fs/ext4/fast_commit.c | 29 +++++------------------------
fs/ext4/fast_commit.h | 3 +--
2 files changed, 6 insertions(+), 26 deletions(-)
diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
index 26c4fc37edcf..da4263a14a20 100644
--- a/fs/ext4/fast_commit.c
+++ b/fs/ext4/fast_commit.c
@@ -322,9 +322,7 @@ void ext4_fc_del(struct inode *inode)
WARN_ON(!list_empty(&ei->i_fc_dilist));
spin_unlock(&sbi->s_fc_lock);
- if (fc_dentry->fcd_name.name &&
- fc_dentry->fcd_name.len > DNAME_INLINE_LEN)
- kfree(fc_dentry->fcd_name.name);
+ release_dentry_name_snapshot(&fc_dentry->fcd_name);
kmem_cache_free(ext4_fc_dentry_cachep, fc_dentry);
return;
@@ -449,22 +447,7 @@ static int __track_dentry_update(handle_t *handle, struct inode *inode,
node->fcd_op = dentry_update->op;
node->fcd_parent = dir->i_ino;
node->fcd_ino = inode->i_ino;
- if (dentry->d_name.len > DNAME_INLINE_LEN) {
- node->fcd_name.name = kmalloc(dentry->d_name.len, GFP_NOFS);
- if (!node->fcd_name.name) {
- kmem_cache_free(ext4_fc_dentry_cachep, node);
- ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_NOMEM, handle);
- mutex_lock(&ei->i_fc_lock);
- return -ENOMEM;
- }
- memcpy((u8 *)node->fcd_name.name, dentry->d_name.name,
- dentry->d_name.len);
- } else {
- memcpy(node->fcd_iname, dentry->d_name.name,
- dentry->d_name.len);
- node->fcd_name.name = node->fcd_iname;
- }
- node->fcd_name.len = dentry->d_name.len;
+ take_dentry_name_snapshot(&node->fcd_name, dentry);
INIT_LIST_HEAD(&node->fcd_dilist);
spin_lock(&sbi->s_fc_lock);
if (sbi->s_journal->j_flags & JBD2_FULL_COMMIT_ONGOING ||
@@ -832,7 +815,7 @@ static bool ext4_fc_add_dentry_tlv(struct super_block *sb, u32 *crc,
{
struct ext4_fc_dentry_info fcd;
struct ext4_fc_tl tl;
- int dlen = fc_dentry->fcd_name.len;
+ int dlen = fc_dentry->fcd_name.name.len;
u8 *dst = ext4_fc_reserve_space(sb,
EXT4_FC_TAG_BASE_LEN + sizeof(fcd) + dlen, crc);
@@ -847,7 +830,7 @@ static bool ext4_fc_add_dentry_tlv(struct super_block *sb, u32 *crc,
dst += EXT4_FC_TAG_BASE_LEN;
memcpy(dst, &fcd, sizeof(fcd));
dst += sizeof(fcd);
- memcpy(dst, fc_dentry->fcd_name.name, dlen);
+ memcpy(dst, fc_dentry->fcd_name.name.name, dlen);
return true;
}
@@ -1328,9 +1311,7 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)
list_del_init(&fc_dentry->fcd_dilist);
spin_unlock(&sbi->s_fc_lock);
- if (fc_dentry->fcd_name.name &&
- fc_dentry->fcd_name.len > DNAME_INLINE_LEN)
- kfree(fc_dentry->fcd_name.name);
+ release_dentry_name_snapshot(&fc_dentry->fcd_name);
kmem_cache_free(ext4_fc_dentry_cachep, fc_dentry);
spin_lock(&sbi->s_fc_lock);
}
diff --git a/fs/ext4/fast_commit.h b/fs/ext4/fast_commit.h
index 2fadb2c4780c..3bd534e4dbbf 100644
--- a/fs/ext4/fast_commit.h
+++ b/fs/ext4/fast_commit.h
@@ -109,8 +109,7 @@ struct ext4_fc_dentry_update {
int fcd_op; /* Type of update create / unlink / link */
int fcd_parent; /* Parent inode number */
int fcd_ino; /* Inode number */
- struct qstr fcd_name; /* Dirent name */
- unsigned char fcd_iname[DNAME_INLINE_LEN]; /* Dirent name string */
+ struct name_snapshot fcd_name; /* Dirent name */
struct list_head fcd_list;
struct list_head fcd_dilist;
};
--
2.39.5
^ permalink raw reply related [flat|nested] 6+ messages in thread