diff mbox series

[v3,1/3] fsnotify: Use d_find_any_alias to get dentry associated with inode

Message ID 20221028001016.332663-2-stephen.s.brennan@oracle.com
State New
Headers
Series fsnotify: fix softlockups iterating over d_subdirs |

Commit Message

Stephen Brennan Oct. 28, 2022, 12:10 a.m. UTC 
  Rather than iterating over the inode's i_dentry (requiring holding the
i_lock for the entire duration of the function), we know that there
should be only one item in the list. Use d_find_any_alias() and no
longer hold i_lock.

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---

Notes:
    Changes since v2:
    - Add newlines in block comment
    - d_find_any_alias() returns a reference, which I was leaking. Add
      a dput(alias) at the end.
    - Add Amir's R-b

 fs/notify/fsnotify.c | 44 +++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

Comments

Stephen Brennan Nov. 10, 2022, 1:12 a.m. UTC | #1 
Stephen Brennan <stephen.s.brennan@oracle.com> writes:

> Rather than iterating over the inode's i_dentry (requiring holding the
> i_lock for the entire duration of the function), we know that there
> should be only one item in the list. Use d_find_any_alias() and no
> longer hold i_lock.
>
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
>
> Notes:
>     Changes since v2:
>     - Add newlines in block comment
>     - d_find_any_alias() returns a reference, which I was leaking. Add
>       a dput(alias) at the end.
>     - Add Amir's R-b
>
>  fs/notify/fsnotify.c | 44 +++++++++++++++++++++-----------------------
>  1 file changed, 21 insertions(+), 23 deletions(-)
>
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 7974e91ffe13..7939aa911931 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -105,7 +105,7 @@ void fsnotify_sb_delete(struct super_block *sb)
>   */
>  void __fsnotify_update_child_dentry_flags(struct inode *inode)
>  {
> -	struct dentry *alias;
> +	struct dentry *alias, *child;
>  	int watched;
>  
>  	if (!S_ISDIR(inode->i_mode))
> @@ -114,30 +114,28 @@ void __fsnotify_update_child_dentry_flags(struct inode *inode)
>  	/* determine if the children should tell inode about their events */
>  	watched = fsnotify_inode_watches_children(inode);
>  
> -	spin_lock(&inode->i_lock);
> -	/* run all of the dentries associated with this inode.  Since this is a
> -	 * directory, there damn well better only be one item on this list */
> -	hlist_for_each_entry(alias, &inode->i_dentry, d_u.d_alias) {
> -		struct dentry *child;
> -
> -		/* run all of the children of the original inode and fix their
> -		 * d_flags to indicate parental interest (their parent is the
> -		 * original inode) */
> -		spin_lock(&alias->d_lock);
> -		list_for_each_entry(child, &alias->d_subdirs, d_child) {
> -			if (!child->d_inode)
> -				continue;
> +	/* Since this is a directory, there damn well better only be one child */
> +	alias = d_find_any_alias(inode);

Unfortunately, even this patch is not as simple as we would have liked
to believe. Running LTP, fanotify10 test, I reliably panic here because
d_find_any_alias(inode) returns NULL. I've fixed this in my branch
already with a simple if (alias) return watched;

Will continue to work through LTP tests (with lockdep enabled) and work
on the existing dnotify lock ordering issue.

>  
> -			spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED);
> -			if (watched)
> -				child->d_flags |= DCACHE_FSNOTIFY_PARENT_WATCHED;
> -			else
> -				child->d_flags &= ~DCACHE_FSNOTIFY_PARENT_WATCHED;
> -			spin_unlock(&child->d_lock);
> -		}
> -		spin_unlock(&alias->d_lock);
> +	/*
> +	 * run all of the children of the original inode and fix their
> +	 * d_flags to indicate parental interest (their parent is the
> +	 * original inode)
> +	 */
> +	spin_lock(&alias->d_lock);
> +	list_for_each_entry(child, &alias->d_subdirs, d_child) {
> +		if (!child->d_inode)
> +			continue;
> +
> +		spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED);
> +		if (watched)
> +			child->d_flags |= DCACHE_FSNOTIFY_PARENT_WATCHED;
> +		else
> +			child->d_flags &= ~DCACHE_FSNOTIFY_PARENT_WATCHED;
> +		spin_unlock(&child->d_lock);
>  	}
> -	spin_unlock(&inode->i_lock);
> +	spin_unlock(&alias->d_lock);
> +	dput(alias);
>  }
>  
>  /* Are inode/sb/mount interested in parent and name info with this event? */
> -- 
> 2.34.1
diff mbox series

Patch

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 7974e91ffe13..7939aa911931 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -105,7 +105,7 @@  void fsnotify_sb_delete(struct super_block *sb)
  */
 void __fsnotify_update_child_dentry_flags(struct inode *inode)
 {
-	struct dentry *alias;
+	struct dentry *alias, *child;
 	int watched;
 
 	if (!S_ISDIR(inode->i_mode))
@@ -114,30 +114,28 @@  void __fsnotify_update_child_dentry_flags(struct inode *inode)
 	/* determine if the children should tell inode about their events */
 	watched = fsnotify_inode_watches_children(inode);
 
-	spin_lock(&inode->i_lock);
-	/* run all of the dentries associated with this inode.  Since this is a
-	 * directory, there damn well better only be one item on this list */
-	hlist_for_each_entry(alias, &inode->i_dentry, d_u.d_alias) {
-		struct dentry *child;
-
-		/* run all of the children of the original inode and fix their
-		 * d_flags to indicate parental interest (their parent is the
-		 * original inode) */
-		spin_lock(&alias->d_lock);
-		list_for_each_entry(child, &alias->d_subdirs, d_child) {
-			if (!child->d_inode)
-				continue;
+	/* Since this is a directory, there damn well better only be one child */
+	alias = d_find_any_alias(inode);
 
-			spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED);
-			if (watched)
-				child->d_flags |= DCACHE_FSNOTIFY_PARENT_WATCHED;
-			else
-				child->d_flags &= ~DCACHE_FSNOTIFY_PARENT_WATCHED;
-			spin_unlock(&child->d_lock);
-		}
-		spin_unlock(&alias->d_lock);
+	/*
+	 * run all of the children of the original inode and fix their
+	 * d_flags to indicate parental interest (their parent is the
+	 * original inode)
+	 */
+	spin_lock(&alias->d_lock);
+	list_for_each_entry(child, &alias->d_subdirs, d_child) {
+		if (!child->d_inode)
+			continue;
+
+		spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED);
+		if (watched)
+			child->d_flags |= DCACHE_FSNOTIFY_PARENT_WATCHED;
+		else
+			child->d_flags &= ~DCACHE_FSNOTIFY_PARENT_WATCHED;
+		spin_unlock(&child->d_lock);
 	}
-	spin_unlock(&inode->i_lock);
+	spin_unlock(&alias->d_lock);
+	dput(alias);
 }
 
 /* Are inode/sb/mount interested in parent and name info with this event? */