[RFC PATCH 1/2] fscrypt: remove d_lock on reading DCACHE_ENCRYPTED_WITH_KEY

[RFC PATCH 1/2] fscrypt: remove d_lock on reading DCACHE_ENCRYPTED_WITH_KEY

[Gao Xiang]

It seems there is no need to take d_lock when
accessing dentry->d_flags & DCACHE_ENCRYPTED_WITH_KEY
in fscrypt_d_revalidate. It only needs to be
serialized for updating.

Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
Hi,

At glance, I have no idea why fscrypt_d_revalidate disables
RCU-lookup. Therefore I made patches to raise a question and
do some basic test, and it seems to work.

Thanks,

 fs/crypto/crypto.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 0f46cf5..b38c574 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -329,9 +329,8 @@ static int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags)
 		return 0;
 	}
 
-	spin_lock(&dentry->d_lock);
-	cached_with_key = dentry->d_flags & DCACHE_ENCRYPTED_WITH_KEY;
-	spin_unlock(&dentry->d_lock);
+	cached_with_key = READ_ONCE(dentry->d_flags) &
+		DCACHE_ENCRYPTED_WITH_KEY;
 	dir_has_key = (d_inode(dir)->i_crypt_info != NULL);
 	dput(dir);
 
-- 
1.9.1

[RFC PATCH 2/2] fscrypt: enable RCU-walk path for .d_revalidate

[Gao Xiang]

This patch attempts to enable RCU-walk for fscrypt.
It looks harmless at glance and could have better
performance than do ref-walk only.

Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 fs/crypto/crypto.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index b38c574..9bd21c0 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -319,20 +319,24 @@ static int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags)
 {
 	struct dentry *dir;
 	int dir_has_key, cached_with_key;
-
-	if (flags & LOOKUP_RCU)
-		return -ECHILD;
-
-	dir = dget_parent(dentry);
-	if (!IS_ENCRYPTED(d_inode(dir))) {
-		dput(dir);
+	struct inode *dir_inode;
+
+	rcu_read_lock();
+repeat:
+	dir = READ_ONCE(dentry->d_parent);
+	dir_inode = d_inode_rcu(dir);
+	if (!IS_ENCRYPTED(dir_inode)) {
+		rcu_read_unlock();
 		return 0;
 	}
+	dir_has_key = (dir_inode->i_crypt_info != NULL);
+	if (unlikely(READ_ONCE(dir->d_lockref.count) < 0 ||
+		READ_ONCE(dir->d_parent) != dir))
+		goto repeat;
+	rcu_read_unlock();
 
 	cached_with_key = READ_ONCE(dentry->d_flags) &
 		DCACHE_ENCRYPTED_WITH_KEY;
-	dir_has_key = (d_inode(dir)->i_crypt_info != NULL);
-	dput(dir);
 
 	/*
 	 * If the dentry was cached without the key, and it is a
-- 
1.9.1

[RFC PATCH v2 2/2] fscrypt: enable RCU-walk path for .d_revalidate

[Gao Xiang]

This patch attempts to enable RCU-walk for fscrypt.
It looks harmless at glance and could have better
performance than do ref-walk only.

Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
change log v2:
	- READ_ONCE(dir->d_parent) -> READ_ONCE(dentry->d_parent)

 fs/crypto/crypto.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index b38c574..9bd21c0 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -319,20 +319,24 @@ static int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags)
 {
 	struct dentry *dir;
 	int dir_has_key, cached_with_key;
-
-	if (flags & LOOKUP_RCU)
-		return -ECHILD;
-
-	dir = dget_parent(dentry);
-	if (!IS_ENCRYPTED(d_inode(dir))) {
-		dput(dir);
+	struct inode *dir_inode;
+
+	rcu_read_lock();
+repeat:
+	dir = READ_ONCE(dentry->d_parent);
+	dir_inode = d_inode_rcu(dir);
+	if (!IS_ENCRYPTED(dir_inode)) {
+		rcu_read_unlock();
 		return 0;
 	}
+	dir_has_key = (dir_inode->i_crypt_info != NULL);
+	if (unlikely(READ_ONCE(dir->d_lockref.count) < 0 ||
+		READ_ONCE(dentry->d_parent) != dir))
+		goto repeat;
+	rcu_read_unlock();
 
 	cached_with_key = READ_ONCE(dentry->d_flags) &
 		DCACHE_ENCRYPTED_WITH_KEY;
-	dir_has_key = (d_inode(dir)->i_crypt_info != NULL);
-	dput(dir);
 
 	/*
 	 * If the dentry was cached without the key, and it is a
-- 
1.9.1

Re: [RFC PATCH v2 2/2] fscrypt: enable RCU-walk path for .d_revalidate

[Eric Biggers]

Hi Gao,

On Mon, Sep 10, 2018 at 09:08:57PM +0800, Gao Xiang wrote:
> This patch attempts to enable RCU-walk for fscrypt.
> It looks harmless at glance and could have better
> performance than do ref-walk only.
> 
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> ---
> change log v2:
> 	- READ_ONCE(dir->d_parent) -> READ_ONCE(dentry->d_parent)
> 
>  fs/crypto/crypto.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> index b38c574..9bd21c0 100644
> --- a/fs/crypto/crypto.c
> +++ b/fs/crypto/crypto.c
> @@ -319,20 +319,24 @@ static int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags)
>  {
>  	struct dentry *dir;
>  	int dir_has_key, cached_with_key;
> -
> -	if (flags & LOOKUP_RCU)
> -		return -ECHILD;
> -
> -	dir = dget_parent(dentry);
> -	if (!IS_ENCRYPTED(d_inode(dir))) {
> -		dput(dir);
> +	struct inode *dir_inode;
> +
> +	rcu_read_lock();
> +repeat:
> +	dir = READ_ONCE(dentry->d_parent);
> +	dir_inode = d_inode_rcu(dir);
> +	if (!IS_ENCRYPTED(dir_inode)) {
> +		rcu_read_unlock();
>  		return 0;
>  	}
> +	dir_has_key = (dir_inode->i_crypt_info != NULL);
> +	if (unlikely(READ_ONCE(dir->d_lockref.count) < 0 ||
> +		READ_ONCE(dentry->d_parent) != dir))
>
>
> +	rcu_read_unlock();
>  
>  	cached_with_key = READ_ONCE(dentry->d_flags) &
>  		DCACHE_ENCRYPTED_WITH_KEY;
> -	dir_has_key = (d_inode(dir)->i_crypt_info != NULL);
> -	dput(dir);
>  

I think you're right that we don't have to drop out of RCU mode here, but can
you please Cc linux-fsdevel so that people more knowledgeable about path lookup
can review this too?  This kind of stuff is very tricky.  Please resend both
patches.

Also please indent properly:

	if (unlikely(READ_ONCE(dir->d_lockref.count) < 0 ||
                     READ_ONCE(dentry->d_parent) != dir))
		goto repeat;

Why read d_lockref.count directly instead of using __lockref_is_dead()?

Also since there's no longer any reference to the parent dentry taken, how do
you know it's still positive (non-NULL d_inode), i.e. that the directory hasn't
been removed and turned into a negative dentry (NULL d_inode)?

I'm also wondering whether the retry loop is actually needed.  Can you explain
your thoughts more?  But if it is needed, in principle you'd actually need to
wait until after the loop before taking any action based on dir_inode, right?
That would mean the 'rcu_read_unlock(); return 0;' is in the wrong place.

Thanks,

- Eric

Re: [RFC PATCH v2 2/2] fscrypt: enable RCU-walk path for .d_revalidate

[Gao Xiang]

Hi Eric,

On 2018/9/11 7:20, Eric Biggers wrote:
> Hi Gao,
> 
> On Mon, Sep 10, 2018 at 09:08:57PM +0800, Gao Xiang wrote:
>> This patch attempts to enable RCU-walk for fscrypt.
>> It looks harmless at glance and could have better
>> performance than do ref-walk only.
>>
>> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
>> ---
>> change log v2:
>> 	- READ_ONCE(dir->d_parent) -> READ_ONCE(dentry->d_parent)
>>
>>  fs/crypto/crypto.c | 22 +++++++++++++---------
>>  1 file changed, 13 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
>> index b38c574..9bd21c0 100644
>> --- a/fs/crypto/crypto.c
>> +++ b/fs/crypto/crypto.c
>> @@ -319,20 +319,24 @@ static int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags)
>>  {
>>  	struct dentry *dir;
>>  	int dir_has_key, cached_with_key;
>> -
>> -	if (flags & LOOKUP_RCU)
>> -		return -ECHILD;
>> -
>> -	dir = dget_parent(dentry);
>> -	if (!IS_ENCRYPTED(d_inode(dir))) {
>> -		dput(dir);
>> +	struct inode *dir_inode;
>> +
>> +	rcu_read_lock();
>> +repeat:
>> +	dir = READ_ONCE(dentry->d_parent);
>> +	dir_inode = d_inode_rcu(dir);
>> +	if (!IS_ENCRYPTED(dir_inode)) {
>> +		rcu_read_unlock();
>>  		return 0;
>>  	}
>> +	dir_has_key = (dir_inode->i_crypt_info != NULL);
>> +	if (unlikely(READ_ONCE(dir->d_lockref.count) < 0 ||
>> +		READ_ONCE(dentry->d_parent) != dir))
>>
>>
>> +	rcu_read_unlock();
>>  
>>  	cached_with_key = READ_ONCE(dentry->d_flags) &
>>  		DCACHE_ENCRYPTED_WITH_KEY;
>> -	dir_has_key = (d_inode(dir)->i_crypt_info != NULL);
>> -	dput(dir);
>>  
> 
> I think you're right that we don't have to drop out of RCU mode here, but can
> you please Cc linux-fsdevel so that people more knowledgeable about path lookup
> can review this too?  This kind of stuff is very tricky.  Please resend both
> patches.
> 
> Also please indent properly:
> 
> 	if (unlikely(READ_ONCE(dir->d_lockref.count) < 0 ||
>                      READ_ONCE(dentry->d_parent) != dir))
> 		goto repeat;
> 
> Why read d_lockref.count directly instead of using __lockref_is_dead()?

will be fixed in the next version, thanks.

> 
> Also since there's no longer any reference to the parent dentry taken, how do
> you know it's still positive (non-NULL d_inode), i.e. that the directory hasn't
> been removed and turned into a negative dentry (NULL d_inode)?

I think you are right. I saw this fscrypt piece of code when I was locating a
problem related to fscrypt (I am still taking part in it since the problem is urgent).
It seems that it could be turned into a negative dentry by d_delete() etc.

I will rethink this flow more, make the next patch later and Cc linux-devel
the next time.

> 
> I'm also wondering whether the retry loop is actually needed.  Can you explain
> your thoughts more?  But if it is needed, in principle you'd actually need to
> wait until after the loop before taking any action based on dir_inode, right?
> That would mean the 'rcu_read_unlock(); return 0;' is in the wrong place.
What I thought was that I guess it needs to be more strict to claim the dentry is
still valid than other cases (therefore IS_ENCRYPTED is not so strict, that is
my personal thought tho.)

If the parent dentry just sampled is invalid, since the dentry and inode are
protected by rcu, so there is no way to READ_ONCE(dentry->d_parent) == dir.

Therefore I sampled (IS_ENCRYPTED, dir_has_key) and do a final basic validity
check at last --- currently dentry itself (maybe inode later), and I tend to
try again especially for ref-walk case (which not governed by d_seq) since it is
more lightweight (like a seqlock) than taking & releasing d_lock (or even
return 0 to do real lookup again) I think.

That is my personal thought, could not be accurate, and I am trying to learn
more about the fscrypt due to the urgent problem.

If any error, please kindly point out, thanks...

Thanks,
Gao Xiang

> 
> Thanks,
> 
> - Eric
> 

[RFC PATCH v2 1/2] fscrypt: remove d_lock on reading DCACHE_ENCRYPTED_WITH_KEY

[Gao Xiang]

It seems there is no need to take d_lock when accessing
dentry->d_flags & DCACHE_ENCRYPTED_WITH_KEY in
fscrypt_d_revalidate. It only needs to be serialized for updating.

Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---

Change log v2:
 - Fix negative dentry (NULL d_inode) issue as Eric pointed out;
 - Fix to use '__lockref_is_dead()' as Eric pointed out before;

[original message]
Hi,
 At glance, I have no idea why fscrypt_d_revalidate disables
RCU-lookup. Therefore I made patches to raise a question and
do some test (eg. fsstress w/ and w/o drop_caches) and it
seems to work.

Thanks,
Gao Xiang

 fs/crypto/crypto.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 0f46cf550907..b38c574f70ac 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -329,9 +329,8 @@ static int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags)
 		return 0;
 	}
 
-	spin_lock(&dentry->d_lock);
-	cached_with_key = dentry->d_flags & DCACHE_ENCRYPTED_WITH_KEY;
-	spin_unlock(&dentry->d_lock);
+	cached_with_key = READ_ONCE(dentry->d_flags) &
+		DCACHE_ENCRYPTED_WITH_KEY;
 	dir_has_key = (d_inode(dir)->i_crypt_info != NULL);
 	dput(dir);
 
-- 
2.14.4

[RFC PATCH v2 2/2] fscrypt: enable RCU-walk path for .d_revalidate

[Gao Xiang]

This patch attempts to enable RCU-walk for fscrypt.
It looks harmless at glance and could have better
performance than do ref-walk only.

Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---

p.s.
  In my opinion, it is better to retry for the case of
  READ_ONCE(dentry->d_parent) != dir rather than just
  return 0; and then do real lookup for ref-walk path...
  It behaves much like a seqlock and I tend to avoid
  taking d_lock as well..
  Please kindly correct me if I am wrong... Thanks in advance.

Thanks,
Gao Xiang

 fs/crypto/crypto.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index b38c574f70ac..2cc26fe9c43d 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -319,20 +319,29 @@ static int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags)
 {
 	struct dentry *dir;
 	int dir_has_key, cached_with_key;
+	struct inode *dir_inode;
 
-	if (flags & LOOKUP_RCU)
-		return -ECHILD;
+repeat:
+	rcu_read_lock();
+	dir = READ_ONCE(dentry->d_parent);
 
-	dir = dget_parent(dentry);
-	if (!IS_ENCRYPTED(d_inode(dir))) {
-		dput(dir);
+	dir_inode = d_inode_rcu(dir);
+	if (!dir_inode || !IS_ENCRYPTED(dir_inode)) {
+		rcu_read_unlock();
 		return 0;
 	}
+	dir_has_key = (dir_inode->i_crypt_info != NULL);
+
+	/* original dir becomes invalid after sampling all? */
+	if (unlikely(__lockref_is_dead(&dir->d_lockref) ||
+		     READ_ONCE(dentry->d_parent) != dir)) {
+		rcu_read_unlock();
+		goto repeat;
+	}
+	rcu_read_unlock();
 
 	cached_with_key = READ_ONCE(dentry->d_flags) &
 		DCACHE_ENCRYPTED_WITH_KEY;
-	dir_has_key = (d_inode(dir)->i_crypt_info != NULL);
-	dput(dir);
 
 	/*
 	 * If the dentry was cached without the key, and it is a
-- 
2.14.4