On Mon, 2021-01-11 at 17:02 +0800, Fox Chen wrote: > On Mon, Jan 11, 2021 at 4:42 PM Ian Kent <raven@xxxxxxxxxx> wrote: > > On Mon, 2021-01-11 at 15:04 +0800, Fox Chen wrote: > > > On Mon, Jan 11, 2021 at 12:20 PM Ian Kent <raven@xxxxxxxxxx> > > > wrote: > > > > 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. > > > > > > Oh, got it. I missed the inode part. my bad. :( > > > > > > > > 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 ... > > > > > > IIUC only when dop->revalidate, iop->lookup being called, the > > > result > > > of rmdir/mkdir will be sync with vfs. > > > > Don't quite get what you mean here? > > > > Do you mean something like, VFS objects are created on user access > > to the file system. Given that user access generally means path > > resolution possibly followed by some operation. > > > > I guess those VFS objects will go away some time after the access > > but even thought the code looks like that should happen pretty > > quickly after I've observed that these objects stay around longer > > than expected. There wouldn't be any use in maintaining a least > > recently used list of dentry candidates eligible to discard. > > Yes, that is what I meant. I think the duration may depend on the > current ram pressure. though not quite sure, I'm still digging this > part of code. > > > > kernfs_node is detached from vfs inode/dentry to save ram. > > > > > > > > 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. > > > > > > um, It shouldn't be called with a NULL inode, right? > > > > > > inode->i_mode = kn->mode; > > > > > > otherwise will crash. > > > > Yes, you're right about that. > > > > > > 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?). > > > > > > > > > > you meant iattr to be allocated at inode creation time?? > > > yes, I think so. it's due to ram consumption. > > > > I did, yes. > > > > The actual problem is dealing with multiple concurrent updates to > > the inode link count, the rest can work. Umm ... maybe I've been trying to do this in the wrong place all along. You know the inode i_lock looks like the sensible thing to use to protect these updates. Something like this for that last patch should work: kernfs: use i_lock to protect concurrent inode updates From: Ian Kent <raven@xxxxxxxxxx> The inode operations .permission() and .getattr() use the kernfs node write lock but all that's needed is to keep the rb tree stable while updating the inode attributes as well as protecting the update itself against concurrent changes. And .permission() is called frequently during path walks and can cause quite a bit of contention between kernfs node opertations and path walks when the number of concurrant walks is high. To change kernfs_iop_getattr() and kernfs_iop_permission() to take the rw sem read lock instead of the write lock an addtional lock is needed to protect against multiple processes concurrently updating the inode attributes and link count in kernfs_refresh_inode(). The inode i_lock seems like the sensible thing to use to protect these inode attribute updates so use it in kernfs_refresh_inode(). Signed-off-by: Ian Kent <raven@xxxxxxxxxx> --- fs/kernfs/inode.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c index ddaf18198935..e26fa5115821 100644 --- a/fs/kernfs/inode.c +++ b/fs/kernfs/inode.c @@ -171,6 +171,7 @@ static void kernfs_refresh_inode(struct kernfs_node *kn, struct inode *inode) { struct kernfs_iattrs *attrs = kn->iattr; + spin_lock(inode->i_lock); inode->i_mode = kn->mode; if (attrs) /* @@ -181,6 +182,7 @@ static void kernfs_refresh_inode(struct kernfs_node *kn, struct inode *inode) if (kernfs_type(kn) == KERNFS_DIR) set_nlink(inode, kn->dir.subdirs + 2); + spin_unlock(inode->i_lock); } int kernfs_iop_getattr(const struct path *path, struct kstat *stat, @@ -189,9 +191,9 @@ int kernfs_iop_getattr(const struct path *path, struct kstat *stat, struct inode *inode = d_inode(path->dentry); struct kernfs_node *kn = inode->i_private; - down_write(&kernfs_rwsem); + down_read(&kernfs_rwsem); kernfs_refresh_inode(kn, inode); - up_write(&kernfs_rwsem); + up_read(&kernfs_rwsem); generic_fillattr(inode, stat); return 0; @@ -281,9 +283,9 @@ int kernfs_iop_permission(struct inode *inode, int mask) kn = inode->i_private; - down_write(&kernfs_rwsem); + down_read(&kernfs_rwsem); kernfs_refresh_inode(kn, inode); - up_write(&kernfs_rwsem); + up_read(&kernfs_rwsem); return generic_permission(inode, mask); }
