Messages in this thread | | | Date | Mon, 16 May 2022 09:35:07 -1000 | From | Tejun Heo <> | Subject | Re: [PATCH 2/5] kernfs: make ->attr.open RCU protected. |
| |
Hello,
On Mon, May 16, 2022 at 02:00:50AM +1000, Imran Khan wrote: > +/* > + * Deref RCU protected kn->attr.open. > + * If both @of->list and @kn->attr.open->files are non empty, we can safely > + * assume that @of is on @kn->attr.open and hence @kn->attr.open will not > + * vanish and derefeencing is safe here. > + */
Maybe use proper function comment starting with /**?
> +static struct kernfs_open_node * > +kernfs_deref_on_check(struct kernfs_open_file *of, struct kernfs_node *kn) > +{ > + struct kernfs_open_node *on; > + > + on = rcu_dereference_check(kn->attr.open, !list_empty(&of->list)); > + > + if (on && list_empty(&on->files)) > + return NULL; > + else > + return on; > +}
Why does it need to return NULL on empty on->files? We wanna trigger lockdep warning cuz that's a bug but it's not like the caller can recover from it (short of causing unexpected user visible error), so I don't see what the point is.
> If this looks okay then I can replace usage of kernfs_deref_on_raw with > kernfs_deref_on_check.
So, this is the main deref function without holding the mutex, right? Then name it kernfs_deref_open_node() (or on but it seem a bit confusing to me).
> > and in the check condition, add the conditions that you need to make this > > not trigger warning when used in all the places that you wanna use it. > > > > Since ->attr.open is always accessed/modified under kernfs_open_file_mutex, I > have included this check in helper which should be used only while holding this > mutex. Do you mean that I should include some additional checks as well in the > below helper:
Yeah, you're right. _protected makes sense for this one.
Thanks.
-- tejun
| |