On Sun, Dec 13, 2020 at 11:46 AM Ian Kent <raven@xxxxxxxxxx> wrote: > > On Fri, 2020-12-11 at 10:17 +0800, Ian Kent wrote: > > On Fri, 2020-12-11 at 10:01 +0800, Ian Kent wrote: > > > > For the patches, there is a mutex_lock in kn->attr_mutex, as > > > > Tejun > > > > mentioned here > > > > (https://lore.kernel.org/lkml/X8fe0cmu+aq1gi7O@xxxxxxxxxxxxxxx/), > > > > maybe a global > > > > rwsem for kn->iattr will be better?? > > > > > > I wasn't sure about that, IIRC a spin lock could be used around the > > > initial check and checked again at the end which would probably > > > have > > > been much faster but much less conservative and a bit more ugly so > > > I just went the conservative path since there was so much change > > > already. > > > > Sorry, I hadn't looked at Tejun's reply yet and TBH didn't remember > > it. > > > > Based on what Tejun said it sounds like that needs work. > > Those attribute handling patches were meant to allow taking the rw > sem read lock instead of the write lock for kernfs_refresh_inode() > updates, with the added locking to protect the inode attributes > update since it's called from the VFS both with and without the > inode lock. Oh, understood. I was asking also because lock on kn->attr_mutex drags concurrent performance. > Looking around it looks like kernfs_iattrs() is called from multiple > places without a node database lock at all. > > I'm thinking that, to keep my proposed change straight forward > and on topic, I should just leave kernfs_refresh_inode() taking > the node db write lock for now and consider the attributes handling > as a separate change. Once that's done we could reconsider what's > needed to use the node db read lock in kernfs_refresh_inode(). You meant taking write lock of kernfs_rwsem for kernfs_refresh_inode()?? It may be a lot slower in my benchmark, let me test it. > It will reduce the effectiveness of the series but it would make > this change much more complicated, and is somewhat off-topic, and > could hamper the chances of reviewers spotting problem with it. > > Ian > thanks, fox
