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 <[email protected]>
Reviewed-by: Amir Goldstein <[email protected]>
---
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);
- 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
Stephen Brennan <[email protected]> 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 <[email protected]>
> Reviewed-by: Amir Goldstein <[email protected]>
> ---
>
> 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