On Fri 10-01-25 02:42:46, Al Viro wrote:
> 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: now that there is a lockless path where we might try to grab
> a reference to an already doomed external_name instance, it is no
> longer possible for external_name.u.count and external_name.u.head
> to share space (kudos to Linus for spotting that).
>
> To reduce the noice this commit just turns external_name.u into
> a struct (instead of union); the next commit will dissolve it.
>
> Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Cool. One less lock roundtrip on relatively hot fsnotify path :). Feel free
to add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
Honza
> ---
> fs/dcache.c | 35 +++++++++++++++++++++++++----------
> 1 file changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 52662a5d08e4..f387dc97df86 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,30 @@ 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 {
> + 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;
> + name->name.name = name->inline_name.string;
> + if (likely(s == dentry->d_shortname.string)) {
> name->inline_name = dentry->d_shortname;
> - name->name.name = name->inline_name.string;
> + } else {
> + struct external_name *p;
> + p = container_of(s, struct external_name, name[0]);
> + // get a valid reference
> + if (unlikely(!atomic_inc_not_zero(&p->u.count)))
> + goto retry;
> + name->name.name = s;
> }
> - spin_unlock(&dentry->d_lock);
> + if (read_seqcount_retry(&dentry->d_seq, seq)) {
> + release_dentry_name_snapshot(name);
> + goto retry;
> + }
> + rcu_read_unlock();
> }
> EXPORT_SYMBOL(take_dentry_name_snapshot);
>
> --
> 2.39.5
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
