Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx> writes:
> d_alloc_parallel currently looks for entries that match the name being
> added and return them if found.
> However if d_alloc_parallel is being called during the process of adding
> a new entry (that becomes in_lookup), the same entry is found by
> d_alloc_parallel in the in_lookup_hash and d_alloc_parallel will wait
> forever for it to stop being in lookup.
> To avoid this, it makes sense to check for an entry being currently
> added (e.g. by d_add_ci from a lookup func, like xfs is doing) and if this
> exact match is found, return the entry.
> This way, to add a new entry , as xfs is doing, is the following flow:
> _lookup_slow -> d_alloc_parallel -> entry is being created -> xfs_lookup ->
> d_add_ci -> d_alloc_parallel_check_existing(entry created before) ->
> d_splice_alias.
Hi Eugen,
I have a hard time understanding what xfs has anything to do with this.
xfs already users d_add_ci without problems. The issue is that
ext4/f2fs have case-insensitive d_compare/d_hash functions, so they will
find the dentry-under-lookup itself here. Xfs doesn't have that problem
at all because it doesn't try to match case-inexact names in the dcache.
> The initial entry stops being in_lookup after d_splice_alias finishes, and
> it's returned to d_add_ci by d_alloc_parallel_check_existing.
> Without d_alloc_parallel_check_existing, d_alloc_parallel would be called
> instead and wait forever for the entry to stop being in lookup, as the
> iteration through the in_lookup_hash matches the entry.
> Currently XFS does not hang because it creates another entry in the second
> call of d_alloc_parallel if the name differs by case as the hashing and
> comparison functions used by XFS are not case insensitive.
>
> Signed-off-by: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
> ---
> fs/dcache.c | 29 +++++++++++++++++++++++------
> include/linux/dcache.h | 4 ++++
> 2 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index a0a944fd3a1c..459a3d8b8bdb 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -2049,8 +2049,9 @@ struct dentry *d_add_ci(struct dentry *dentry, struct inode *inode,
> return found;
> }
> if (d_in_lookup(dentry)) {
> - found = d_alloc_parallel(dentry->d_parent, name,
> - dentry->d_wait);
> + found = d_alloc_parallel_check_existing(dentry,
> + dentry->d_parent, name,
> + dentry->d_wait);
> if (IS_ERR(found) || !d_in_lookup(found)) {
> iput(inode);
> return found;
> @@ -2452,9 +2453,10 @@ static void d_wait_lookup(struct dentry *dentry)
> }
> }
>
> -struct dentry *d_alloc_parallel(struct dentry *parent,
> - const struct qstr *name,
> - wait_queue_head_t *wq)
> +struct dentry *d_alloc_parallel_check_existing(struct dentry *d_check,
> + struct dentry *parent,
> + const struct qstr *name,
> + wait_queue_head_t *wq)
> {
> unsigned int hash = name->hash;
> struct hlist_bl_head *b = in_lookup_hash(parent, hash);
> @@ -2523,6 +2525,14 @@ struct dentry *d_alloc_parallel(struct dentry *parent,
> }
>
> rcu_read_unlock();
> +
> + /* if the entry we found is the same as the original we
> + * are checking against, then return it
> + */
> + if (d_check == dentry) {
> + dput(new);
> + return dentry;
> + }
The point of the patchset is to install a dentry with the disk-name in
the dcache if the name isn't an exact match to the name of the
dentry-under-lookup. But, since you return the same
dentry-under-lookup, d_add_ci will just splice that dentry into the
cache - which is exactly the same as just doing d_splice_alias(dentry) at
the end of ->d_lookup() like we currently do, no? Shreeya's idea in
that original patchset was to return a new dentry with the new name.
> /*
> * somebody is likely to be still doing lookup for it;
> * wait for them to finish
> @@ -2560,8 +2570,15 @@ struct dentry *d_alloc_parallel(struct dentry *parent,
> dput(dentry);
> goto retry;
> }
> -EXPORT_SYMBOL(d_alloc_parallel);
> +EXPORT_SYMBOL(d_alloc_parallel_check_existing);
>
> +struct dentry *d_alloc_parallel(struct dentry *parent,
> + const struct qstr *name,
> + wait_queue_head_t *wq)
> +{
> + return d_alloc_parallel_check_existing(NULL, parent, name, wq);
> +}
> +EXPORT_SYMBOL(d_alloc_parallel);
> /*
> * - Unhash the dentry
> * - Retrieve and clear the waitqueue head in dentry
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index bf53e3894aae..6eb21a518cb0 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -232,6 +232,10 @@ extern struct dentry * d_alloc(struct dentry *, const struct qstr *);
> extern struct dentry * d_alloc_anon(struct super_block *);
> extern struct dentry * d_alloc_parallel(struct dentry *, const struct qstr *,
> wait_queue_head_t *);
> +extern struct dentry * d_alloc_parallel_check_existing(struct dentry *,
> + struct dentry *,
> + const struct qstr *,
> + wait_queue_head_t *);
> extern struct dentry * d_splice_alias(struct inode *, struct dentry *);
> extern struct dentry * d_add_ci(struct dentry *, struct inode *, struct qstr *);
> extern bool d_same_name(const struct dentry *dentry, const struct dentry *parent,
--
Gabriel Krisman Bertazi
