| Message ID | 20221021010310.29521-2-stephen.s.brennan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fsnotify: fix softlockups iterating over d_subdirs | expand |
On Fri, Oct 21, 2022 at 4:03 AM Stephen Brennan <stephen.s.brennan@oracle.com> wrote: > > 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> one nit below > --- > fs/notify/fsnotify.c | 41 ++++++++++++++++++----------------------- > 1 file changed, 18 insertions(+), 23 deletions(-) > > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > index 7974e91ffe13..6c338322f0c3 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,25 @@ 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) */ Please add new lines at the beginning and end of this multi line comment. Thanks, Amir.
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c index 7974e91ffe13..6c338322f0c3 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,25 @@ 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); } /* Are inode/sb/mount interested in parent and name info with this event? */
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> --- fs/notify/fsnotify.c | 41 ++++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 23 deletions(-)