On Mon, 2021-01-11 at 11:19 +0800, Ian Kent wrote: > On Wed, 2021-01-06 at 10:38 +0800, Fox Chen wrote: > > Hi Ian, > > > > I am rethinking this problem. Can we simply use a global lock? > > > > In your original patch 5, you have a global mutex attr_mutex to > > protect attr, if we change it to a rwsem, is it enough to protect > > both > > inode and attr while having the concurrent read ability? > > > > like this patch I submitted. ( clearly, I missed __kernfs_iattrs > > part, > > but just about that idea ) > > https://lore.kernel.org/lkml/20201207084333.179132-1-foxhlchen@xxxxxxxxx/ > > I don't think so. > > kernfs_refresh_inode() writes to the inode so taking a read lock > will allow multiple processes to concurrently update it which is > what we need to avoid. > > It's possibly even more interesting. > > For example, kernfs_iop_rmdir() and kernfs_iop_mkdir() might alter > the inode link count (I don't know if that would be the sort of thing > they would do but kernfs can't possibly know either). Both of these > functions rely on the VFS locking for exclusion but the inode link > count is updated in kernfs_refresh_inode() too. > > That's the case now, without any patches. So it's not so easy to get the inode from just the kernfs object so these probably aren't a problem ... > > I'm not entirely sure what's going on in kernfs_refresh_inode(). > > It could be as simple as being called with a NULL inode because > the dentry concerned is negative at that point. I haven't had > time to look closely at it TBH but I have been thinking about it. Certainly this can be called without a struct iattr having been allocated ... and given it probably needs to remain a pointer rather than embedded in the node the inode link count update can't easily be protected from concurrent updates. If it was ok to do the allocation at inode creation the problem becomes much simpler to resolve but I thought there were concerns about ram consumption (although I don't think that was exactly what was said?). Ian
