Re: [PATCH 0/6] kernfs: proposed locking and concurrency improvement

[Fox Chen <foxhlchen@gmail.com>]

On Mon, Jan 11, 2021 at 12:20 PM Ian Kent <raven@themaw.net> 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@gmail.com/
> >
> > 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.

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.

> 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.



thanks,
fox