Re: [PATCH v4 5/6] kernfs: Use RCU to access kernfs_node::parent.

[Tejun Heo <tj@kernel.org>]

Hello,

On Mon, Jan 27, 2025 at 05:25:43PM +0100, Sebastian Andrzej Siewior wrote:
> > > -	return strscpy(buf, kn->parent ? kn->name : "/", buflen);
> > > +	return strscpy(buf, rcu_access_pointer(kn->__parent) ? kn->name : "/", buflen);
> > 
> > rcu_access_pointer() is for when only the pointer value is used without
> > dereferencing it. Here, the poiner is being dereferenced.
> 
> Is it? It checks if the pointer NULL and if so "/" is used otherwise
> "kn->name". The __parent pointer itself is not dereferenced. 

Ah, ignore me. I was misreading.

> > > +static inline struct kernfs_node *kernfs_parent(const struct kernfs_node *kn)
> > > +{
> > > +	return rcu_dereference_check(kn->__parent, kernfs_root_is_locked(kn));
> > > +}
> > 
> > AFAICS, all rules can be put into this helper, no?
> 
> This would work. kernfs_parent() is the "general purpose" access. It is
> used in most places (the kernfs_rename_ns() usage is moved to
> kernfs_parent() in the following patch, ended here open coded during the
> split, fixed now).
> 
> The "!atomic_read(&kn->count)" rule is a special case used only in
> kernfs_put() after the counter went to 0 and should not be used (used as
> in be valid) anywhere else. This is special because is going away and
> __parent can not be renamed/ replaced at this point. One user in total.
> 
> The "lockdep_is_held(&kernfs_rename_lock)" rule is only used in
> kernfs_get_parent(). One user in total.
> 
> Adding these two cases to kernfs_parent() will bloat the code a
> little in the debug case (where the check is expanded). Also it will
> require to make kernfs_rename_lock global so it be accessed outside of
> dir.c.
> All in all I don't think it is worth it. If you however prefer it that
> way, I sure can update it.

Hmm... maybe other people have different preferences here but I much prefer
documenting and enforcing RCU deref rules in a single place. It only adds
debug annotations that go away in prod builds while clarifying subtleties.
The trade-off seems pretty one-sided to me.

Thanks.

-- 
tejun