| Message ID | 20221028001016.332663-2-stephen.s.brennan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fsnotify: fix softlockups iterating over d_subdirs | expand |
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 --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? */