[v2] fs/namei.c: Remove unlikely of status being -ECHILD in lookup_fast()

Commit Message

Steven Rostedt Dec. 9, 2020, 10:09 p.m. UTC

From:  Steven Rostedt (VMware) <[e-mail]>

Running my yearly branch profiling code, it detected a 100% wrong branch
condition in name.c for lookup_fast(). The code in question has:

		status = d_revalidate(dentry, nd->flags);
		if (likely(status > 0))
			return dentry;
		if (unlazy_child(nd, dentry, seq))
			return ERR_PTR(-ECHILD);
		if (unlikely(status == -ECHILD))
			/* we'd been told to redo it in non-rcu mode */
			status = d_revalidate(dentry, nd->flags);

If the status of the d_revalidate() is greater than zero, then the function
finishes. Otherwise, if it is an "unlazy_child" it returns with -ECHILD.
After the above two checks, the status is compared to -ECHILD, as that is
what is returned if the original d_revalidate() needed to be done in a
non-rcu mode.

Especially this path is called in a condition of:

	if (nd->flags & LOOKUP_RCU) {

And most of the d_revalidate() functions have:

	if (flags & LOOKUP_RCU)
		return -ECHILD;

It appears that that is the only case that this if statement is triggered
on two of my machines, running in production.

As it is dependent on what filesystem mix is configured in the running
kernel, simply remove the unlikely() from the if statement.

Signed-off-by: Steven Rostedt (VMware) <[e-mail]>
---
Changes since v1:

 - Remove unlikely() instead of making it a likely()

Comments

Steven Rostedt Jan. 13, 2021, 11:13 p.m. UTC | #1

Ping?

-- Steve


On Wed, 9 Dec 2020 17:09:28 -0500
Steven Rostedt <[e-mail]> wrote:

> From:  Steven Rostedt (VMware) <[e-mail]>
> 
> Running my yearly branch profiling code, it detected a 100% wrong branch
> condition in name.c for lookup_fast(). The code in question has:
> 
> 		status = d_revalidate(dentry, nd->flags);
> 		if (likely(status > 0))
> 			return dentry;
> 		if (unlazy_child(nd, dentry, seq))
> 			return ERR_PTR(-ECHILD);
> 		if (unlikely(status == -ECHILD))
> 			/* we'd been told to redo it in non-rcu mode */
> 			status = d_revalidate(dentry, nd->flags);
> 
> If the status of the d_revalidate() is greater than zero, then the function
> finishes. Otherwise, if it is an "unlazy_child" it returns with -ECHILD.
> After the above two checks, the status is compared to -ECHILD, as that is
> what is returned if the original d_revalidate() needed to be done in a
> non-rcu mode.
> 
> Especially this path is called in a condition of:
> 
> 	if (nd->flags & LOOKUP_RCU) {
> 
> And most of the d_revalidate() functions have:
> 
> 	if (flags & LOOKUP_RCU)
> 		return -ECHILD;
> 
> It appears that that is the only case that this if statement is triggered
> on two of my machines, running in production.
> 
> As it is dependent on what filesystem mix is configured in the running
> kernel, simply remove the unlikely() from the if statement.
> 
> Signed-off-by: Steven Rostedt (VMware) <[e-mail]>
> ---
> Changes since v1:
> 
>  - Remove unlikely() instead of making it a likely()
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index d4a6dd772303..c7b7e83853f3 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1495,7 +1495,7 @@ static struct dentry *lookup_fast(struct nameidata *nd,
>  			return dentry;
>  		if (unlazy_child(nd, dentry, seq))
>  			return ERR_PTR(-ECHILD);
> -		if (unlikely(status == -ECHILD))
> +		if (status == -ECHILD)
>  			/* we'd been told to redo it in non-rcu mode */
>  			status = d_revalidate(dentry, nd->flags);
>  	} else {

Patch

diff --git a/fs/namei.c b/fs/namei.c
index d4a6dd772303..c7b7e83853f3 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1495,7 +1495,7 @@  static struct dentry *lookup_fast(struct nameidata *nd,
 			return dentry;
 		if (unlazy_child(nd, dentry, seq))
 			return ERR_PTR(-ECHILD);
-		if (unlikely(status == -ECHILD))
+		if (status == -ECHILD)
 			/* we'd been told to redo it in non-rcu mode */
 			status = d_revalidate(dentry, nd->flags);
 	} else {