On 2024-11-13 08:43:32 [+0100], To Tejun Heo wrote:
> On 2024-11-12 08:59:16 [-1000], Tejun Heo wrote:
> > Hello,
>
> Hi,
>
> > On Tue, Nov 12, 2024 at 04:52:39PM +0100, Sebastian Andrzej Siewior wrote:
> > ...
> > > /**
> > > - * pr_cont_kernfs_name - pr_cont name of a kernfs_node
> > > + * pr_cont_kernfs_name_rcu - pr_cont name of a kernfs_node
> > > * @kn: kernfs_node of interest
> > > *
> > > - * This function can be called from any context.
> > > + * This function can be called from any context. The root node must be with
> > > + * KERNFS_ROOT_SAME_PARENT.
> > > */
> > > -void pr_cont_kernfs_name(struct kernfs_node *kn)
> > > +void pr_cont_kernfs_name_rcu(struct kernfs_node *kn)
> >
> > Having to split the interface all the way up isn't great. While there are
> > also downsides, I wonder whether a better approach here is just making the
> > backend function (kernfs_path_from_node()) automatically use RCU locking if
> > the flag is set rather than propagating the difference by splitting the
> > interface. The distinction doesn't mean anything to most users after all.
>
> Indeed.
Now I see what the problems are. If we merge both into one, then I get
this:
| int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
| {
| struct kernfs_root *root;
| bool rcu_lookup;
|
| if (!kn)
| return strscpy(buf, "(null)", buflen);
|
| root = kernfs_root(kn);
This is the tricky part. For KERNFS_ROOT_INVARIANT_PARENT I don't worry
that the parent goes away and I need it to get a reference to the
kernfs_root node. For the !KERNFS_ROOT_INVARIANT_PARENT I need the lock
for kernfs_root() so I put the guard/ lock at the top.
I think that is why you suggested the two functions (or this is what I
understood). Looking at the remaining bits:
| rcu_lookup = root->flags & KERNFS_ROOT_INVARIANT_PARENT;
| if (rcu_lookup) {
| guard(rcu)();
| return strscpy(buf, kn->parent ? rcu_dereference(kn->name) : "/", buflen);
| }
| guard(read_lock_irqsave)(&kernfs_rename_lock);
| return strscpy(buf, kn->parent ? rcu_dereference(kn->name) : "/", buflen);
| }
This could collapse into the RCU version because read_lock_irqsave()
implies RCU protection. And since ->name is always RCU assigned/
deallocated I don't really need the lock here, RCU would be enough.
Except for the parent. The kn->parent does not matter here (it should be
always be != NULL if assigned), the problematic part is kernfs_root()
which checks the parent for the root node.
To make this simple I could avoid kernfs_root lookup and just have:
| int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
| {
| if (!kn)
| return strscpy(buf, "(null)", buflen);
|
| guard(rcu)();
| return strscpy(buf, kn->parent ? rcu_dereference(kn->name) : "/", buflen);
| }
That is the easy part. kernfs_path_from_node() is different as it
requires the parent pointer. In order to distinguish the RCU from the
non-RCU version I need kernfs_root for the flag and depending on it, the
lock so the parent does not go away.
Would it work to add the pointer to kernfs_root into kernfs_node? This
would shrink kernfs_elem_dir by a pointer but the union would remain the
same size due to kernfs_elem_attr so the struct would grow.
> > Thanks.
Sebastian
