On Fri, Dec 18, 2020 at 3:36 PM Ian Kent <raven@xxxxxxxxxx> wrote: > > On Thu, 2020-12-17 at 10:14 -0500, Tejun Heo wrote: > > Hello, > > > > On Thu, Dec 17, 2020 at 07:48:49PM +0800, Ian Kent wrote: > > > > What could be done is to make the kernfs node attr_mutex > > > > a pointer and dynamically allocate it but even that is too > > > > costly a size addition to the kernfs node structure as > > > > Tejun has said. > > > > > > I guess the question to ask is, is there really a need to > > > call kernfs_refresh_inode() from functions that are usually > > > reading/checking functions. > > > > > > Would it be sufficient to refresh the inode in the write/set > > > operations in (if there's any) places where things like > > > setattr_copy() is not already called? > > > > > > Perhaps GKH or Tejun could comment on this? > > > > My memory is a bit hazy but invalidations on reads is how sysfs > > namespace is > > implemented, so I don't think there's an easy around that. The only > > thing I > > can think of is embedding the lock into attrs and doing xchg dance > > when > > attaching it. > > Sounds like your saying it would be ok to add a lock to the > attrs structure, am I correct? > > Assuming it is then, to keep things simple, use two locks. > > One global lock for the allocation and an attrs lock for all the > attrs field updates including the kernfs_refresh_inode() update. > > The critical section for the global lock could be reduced and it > changed to a spin lock. > > In __kernfs_iattrs() we would have something like: > > take the allocation lock > do the allocated checks > assign if existing attrs > release the allocation lock > return existing if found > othewise > release the allocation lock > > allocate and initialize attrs > > take the allocation lock > check if someone beat us to it > free and grab exiting attrs > otherwise > assign the new attrs > release the allocation lock > return attrs > > Add a spinlock to the attrs struct and use it everywhere for > field updates. > > Am I on the right track or can you see problems with this? > > Ian > umm, we update the inode in kernfs_refresh_inode, right?? So I guess the problem is how can we protect the inode when kernfs_refresh_inode is called, not the attrs?? thanks, fox
