Message-ID: <2df894de-8fa9-40c2-ba2c-f9ae65520656@collabora.com>
Date: Wed, 21 Aug 2024 12:10:23 +0300
From: Eugen Hristev <eugen.hristev@...labora.com>
To: Gabriel Krisman Bertazi <krisman@...e.de>
Cc: viro@...iv.linux.org.uk, brauner@...nel.org, tytso@....edu,
 linux-ext4@...r.kernel.org, jack@...e.cz, adilger.kernel@...ger.ca,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
 kernel@...labora.com, shreeya.patel@...labora.com
Subject: Re: [PATCH 1/2] fs/dcache: introduce d_alloc_parallel_check_existing

On 8/20/24 23:16, Gabriel Krisman Bertazi wrote:
> Eugen Hristev <eugen.hristev@...labora.com> 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,

Hello Krisman,

> 
> I have a hard time understanding what xfs has anything to do with this.

It has because xfs has been given as an example a few times about how a FS
implementation should use d_add_ci.

> 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.

That's right. So xfs cannot be given as an example, as it does not make
case-inexact dentries and lookup.

> 
>> 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@...labora.com>
>> ---
>>  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.

Yes, but we cannot add another dentry for the same file with a different case.
That would break everything about dentry lookups, etc.
We need to have the one dentry in the cache which use the right case. Regardless of
the case of the lookup.

As Al Viro said here :
https://lore.kernel.org/lkml/YVmyYP25kgGq9uEy@zeniv-ca.linux.org.uk/
we cannot have parallel lookups for names that would compare as equals (two
different dentries for the same file with different case).

So yes, I return the same dentry-under-lookup, because that's the purpose of that
search, return it, have it use the right case, and then splice it to the cache.

In the end we will have the dentry use the right case and not the case that we used
for the search, namely, the original filename from the disk. That was the purpose
of the patch.

Eugen

> 
>>  		/*
>>  		 * 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,
> 

Powered by blists - more mailing lists