Re: [PATCH v3] kernfs: allocate inodes and dentry outside kernfs_rwsem

[Shakeel Butt]

Hi Jori and thanks a lot for taking a look.

On Sun, Oct 18, 2026 at 10:47:56PM +0200, Jori Koolstra wrote:
> On Fri, Oct 16, 2026 at 03:00:29PM -0700, Shakeel Butt wrote:
[...]
> > +/**
> > + * kernfs_get_inode_locked - get the inode of @kn under kernfs_rwsem
> > + * @sb: super block
> > + * @kn: kernfs_node to get the inode for
> > + * @new: points to an inode from new_inode(@sb), or to %NULL if that failed
> > + *
> > + * The caller allocates *@new before taking kernfs_rwsem, as the allocation
> > + * can stall in memcg reclaim.  If @kn needs a new inode, *@new is used and
> 
> I wouldn't mention memcg here per se, it is not relevant to grasp its
> use. In another series I have implemented almost the same thing (but you
> did it better!).

Ack, will remove memcg here.

> 
> > + * set to %NULL.  Otherwise the caller frees it with iput() after dropping
> > + * the lock.
> > + *
> > + * A new inode is added only with kernfs_rwsem held and @kn active, so
> > + * __kernfs_remove() always finds it to clear i_nlink.
> > + *
> > + * Context: kernfs_rwsem held for read.
> > + *
> > + * Return: the inode, %NULL if @kn is inactive and has no inode, or
> > + * ERR_PTR(-ENOMEM) if @kn needs an inode and *@new is %NULL.
> > + */
> > +struct inode *kernfs_get_inode_locked(struct super_block *sb,
> > +				      struct kernfs_node *kn,
> > +				      struct inode **new)
> 
> I understand this is intended to have some kind of ownership semantics,
> but I must say for such a simple function, I find the contract quite
> complex to check. Do we really need an out parameter here?

Not really. I just used it simply call iput(new) without a check. I can change
it and make it more like inode_insert5() and let caller do check before calling
iput().

> 
[...]
> > @@ -294,7 +298,7 @@ static int kernfs_fill_super(struct super_block *sb, struct kernfs_fs_context *k
> >  {
> >  	struct kernfs_super_info *info = kernfs_info(sb);
> >  	struct kernfs_root *kf_root = kfc->root;
> > -	struct inode *inode;
> > +	struct inode *inode, *new;
> >  	struct dentry *root;
> >  
> >  	info->sb = sb;
> > @@ -314,10 +318,12 @@ static int kernfs_fill_super(struct super_block *sb, struct kernfs_fs_context *k
> >  	sb->s_shrink->seeks = 0;
> >  
> >  	/* get root inode, initialize and unlock it */
> > +	new = new_inode(sb);
> >  	down_read(&kf_root->kernfs_rwsem);
> > -	inode = kernfs_get_inode(sb, info->root->kn);
> > +	inode = kernfs_get_inode_locked(sb, info->root->kn, &new);
> 
> What are you serializing against here? I've been looking at this as well
> recently, and I thought we could just get rid of the locking here. But
> on second look, I am not so sure what is correct.
> 
> Is there anything that serializes kernfs_get_tree() with root node
> removal?

That is a good question and honestly I didn't know the answer. After digging a
bit, I think 07/12 patch [1] of your series is right.

Superblock is still private here and it is added to the supers list after
kernfs_fill_super. Root removal is not serialized by kernfs and kernfs users
keep the root alive across the mount.

I can drop the kernfs_fill_super change from my patch and I think your patch
will cover it better.

[1] https://lore.kernel.org/20261016143806.2946578-8-jkoolstra@xxxxxxxxx