Hello Ian, Sorry for late reply. I was about to reply this week. On 27/7/2023 10:38 am, Ian Kent wrote: > On 20/7/23 10:03, Ian Kent wrote: >> On Wed, 2023-07-19 at 12:23 +0800, Ian Kent wrote: [...] >> I do see a problem with recent changes. >> >> I'll send this off to Greg after I've done some testing (primarily just >> compile and function). >> >> Here's a patch which describes what I found. >> >> Comments are, of course, welcome, ;) > > Anders I was hoping you would check if/what lockdep trace > > you get with this patch. > > > Imran, I was hoping you would comment on my change as it > > relates to the kernfs_iattr_rwsem changes. > > > Ian > >> >> kernfs: fix missing kernfs_iattr_rwsem locking >> >> From: Ian Kent <raven@xxxxxxxxxx> >> >> When the kernfs_iattr_rwsem was introduced a case was missed. >> >> The update of the kernfs directory node child count was also protected >> by the kernfs_rwsem and needs to be included in the change so that the >> child count (and so the inode n_link attribute) does not change while >> holding the rwsem for read. >> kernfs direcytory node's child count changes in kernfs_(un)link_sibling and these are getting invoked while adding (kernfs_add_one), removing(__kernfs_remove) or moving (kernfs_rename_ns)a node. Each of these operations proceed under kernfs_rwsem and I see each invocation of kernfs_link/unlink_sibling during the above mentioned operations is happening under kernfs_rwsem. So the child count should still be protected by kernfs_rwsem and we should not need to acquire kernfs_iattr_rwsem in kernfs_link/unlink_sibling. Kindly let me know your thoughts. I would still like to see new lockdep traces with this change. Thanks, Imran >> Fixes: 9caf696142 (kernfs: Introduce separate rwsem to protect inode >> attributes) >> >> Signed-off-by: Ian Kent <raven@xxxxxxxxxx> >> Cc: Anders Roxell <anders.roxell@xxxxxxxxxx> >> Cc: Imran Khan <imran.f.khan@xxxxxxxxxx> >> Cc: Arnd Bergmann <arnd@xxxxxxxx> >> Cc: Minchan Kim <minchan@xxxxxxxxxx> >> Cc: Eric Sandeen <sandeen@xxxxxxxxxxx> >> --- >> fs/kernfs/dir.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c >> index 45b6919903e6..6e84bb69602e 100644 >> --- a/fs/kernfs/dir.c >> +++ b/fs/kernfs/dir.c >> @@ -383,9 +383,11 @@ static int kernfs_link_sibling(struct kernfs_node >> *kn) >> rb_insert_color(&kn->rb, &kn->parent->dir.children); >> /* successfully added, account subdir number */ >> + down_write(&kernfs_root(kn)->kernfs_iattr_rwsem); >> if (kernfs_type(kn) == KERNFS_DIR) >> kn->parent->dir.subdirs++; >> kernfs_inc_rev(kn->parent); >> + up_write(&kernfs_root(kn)->kernfs_iattr_rwsem); >> return 0; >> } >> @@ -408,9 +410,11 @@ static bool kernfs_unlink_sibling(struct >> kernfs_node *kn) >> if (RB_EMPTY_NODE(&kn->rb)) >> return false; >> + down_write(&kernfs_root(kn)->kernfs_iattr_rwsem); >> if (kernfs_type(kn) == KERNFS_DIR) >> kn->parent->dir.subdirs--; >> kernfs_inc_rev(kn->parent); >> + up_write(&kernfs_root(kn)->kernfs_iattr_rwsem); >> rb_erase(&kn->rb, &kn->parent->dir.children); >> RB_CLEAR_NODE(&kn->rb); >>
