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

[Tejun Heo <tj@kernel.org>]

Hello,

On Wed, Jan 29, 2025 at 02:23:11PM +0100, Sebastian Andrzej Siewior wrote:
> > > @@ -64,9 +64,9 @@ static size_t kernfs_depth(struct kernfs_node *from, struct kernfs_node *to)
> > >  {
> > >  	size_t depth = 0;
> > >  
> > > -	while (to->parent && to != from) {
> > > +	while (rcu_dereference(to->__parent) && to != from) {
> > 
> > Why not use kernfs_parent() here and other places?
> 
> Because it is from within RCU section and the other checks are not
> required. If you prefer this instead, I sure can update it.

Hmm... I would have gone with using the same accessor everywhere but am not
sure how strongly I feel about it. I don't think it's useful to worry about
the overhead of the extra lockdep annotations in debug builds. Ignoring that
and just considering code readability, what would you do?

> > > @@ -226,6 +227,7 @@ int kernfs_path_from_node(struct kernfs_node *to, struct kernfs_node *from,
> > >  	unsigned long flags;
> > >  	int ret;
> > >  
> > > +	guard(rcu)();
> > 
> > Doesn't irqsave imply rcu?
> 
> hmm. It kind of does based on the current implementation but it is not
> obvious. We had RCU-sched and RCU which got merged. From then on, the
> (implied) preempt-off part of IRQSAVE should imply RCU (section).
> It is good to be obvious about RCU.

There's that but it can also be confusing to have redundant annotations
especially because redundant things tend to become really inconsistent over
time. After the RCU type merges, ISTR removing annotations that became
redundant in several places.

> Also, rcu_dereference() will complain about missing RCU annotation. On
> PREEMPT_RT rcu_dereference_sched() will complain because irqsave (in
> this case) will not disable interrupts.

You know this a lot better than I do. If it's necessary for RT builds, it's
not redundant.

> > > diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
> > > index b42ee6547cdc1..c43bee18b79f7 100644
> > > --- a/fs/kernfs/kernfs-internal.h
> > > +++ b/fs/kernfs/kernfs-internal.h
> > > @@ -64,11 +66,14 @@ struct kernfs_root {
> > >   *
> > >   * Return: the kernfs_root @kn belongs to.
> > >   */
> > > -static inline struct kernfs_root *kernfs_root(struct kernfs_node *kn)
> > > +static inline struct kernfs_root *kernfs_root(const struct kernfs_node *kn)
> > >  {
> > > +	const struct kernfs_node *knp;
> > >  	/* if parent exists, it's always a dir; otherwise, @sd is a dir */
> > > -	if (kn->parent)
> > > -		kn = kn->parent;
> > > +	guard(rcu)();
> > > +	knp = rcu_dereference(kn->__parent);
> > > +	if (knp)
> > > +		kn = knp;
> > >  	return kn->dir.root;
> > >  }
> > 
> > This isn't a new problem but the addition of the rcu guard makes it stick
> > out more: What keeps the returned root safe to dereference?
> 
> As far as I understand it kernfs_root is around as long as the
> filesystem itself is around which means at least one node needs to stay.
> If you have a pointer to a kernfs_node you should own a reference.
> The RCU section is only needed to ensure that the (current) __parent is
> not replaced and then deallocated before the caller had a chance to
> obtain the root pointer.

That sounds reasonable to me.

Thanks.

-- 
tejun