| Message ID | 20221111220614.991928-3-stephen.s.brennan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fsnotify: fix softlockups iterating over d_subdirs | expand |
On Sat, Nov 12, 2022 at 12:06 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> > --- > > Notes: > Changes in v4: > - Bail out if d_find_any_alias() returns NULL > - Rebase on Amir's patch > Changes in v3: > - 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 | 46 ++++++++++++++++++++++---------------------- > 1 file changed, 23 insertions(+), 23 deletions(-) > > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > index 2c50e9e50d35..409d479cbbc6 100644 > --- a/fs/notify/fsnotify.c > +++ b/fs/notify/fsnotify.c > @@ -105,35 +105,35 @@ void fsnotify_sb_delete(struct super_block *sb) > */ > void fsnotify_update_children_dentry_flags(struct inode *inode, bool watched) > { > - struct dentry *alias; > + struct dentry *alias, *child; > > if (!S_ISDIR(inode->i_mode)) > return; > > - 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); > + if (!alias) > + return; > > - 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) nit: this comment can probably fit in two nicer lines > + */ > + 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); > } > > /* > -- > 2.34.1 >
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c index 2c50e9e50d35..409d479cbbc6 100644 --- a/fs/notify/fsnotify.c +++ b/fs/notify/fsnotify.c @@ -105,35 +105,35 @@ void fsnotify_sb_delete(struct super_block *sb) */ void fsnotify_update_children_dentry_flags(struct inode *inode, bool watched) { - struct dentry *alias; + struct dentry *alias, *child; if (!S_ISDIR(inode->i_mode)) return; - 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); + if (!alias) + return; - 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); } /*