Re: [PATCH 2/2] fs: nfs: dir.c: Fix sparse error

From: Paul E. McKenney
Date: Thu Dec 12 2019 - 20:16:13 EST


On Thu, Dec 12, 2019 at 04:55:34PM -0500, Joel Fernandes wrote:
> On Fri, Dec 06, 2019 at 08:02:38AM -0800, Paul E. McKenney wrote:
>
> Thanks for fixing these issues and I caught up with all the patches.
>
> >
> > o Create a list that is safe for bidirectional RCU traversal.
> > This can use list_head, and would need these functions,
> > give or take the exact names:
>
> On a related topic, I was trying to reason about how one could come up with
> bidirectional traversal without ever getting rid of poisoning.
>
> As you noted in another post, if during traversal, the node is deleted and
> poisoned, then the traverser can access a poisoned pointer. If the list is
> being traversed in reverse (by following prev), then poisioning could hurt
> it.
>
> Even with the below modifications, poisoning would still hurt it. No? Were
> you suggesting to remove poisoning for such bidirectional RCU list?

Yes. We removed forward poisoning from list_del_rcu(), and a
list_del_rcuprev() or whatever name would need to avoid poisoning both
pointers.

Thanx, Paul

> Sorry if I missed something.
> thanks,
>
> - Joel
>
>
> > list_add_tail_rcuprev(): This is like list_add_tail_rcu(),
> > but also has smp_store_release() for ->prev. (As in there is
> > also a __list_add_rcuprev() helper that actually contains the
> > additional smp_store_release().)
> >
> > list_del_rcuprev(): This can be exactly __list_del_entry(),
> > but with the assignment to ->prev in __list_del() becoming
> > WRITE_ONCE(). And it looks like callers to __list_del_entry()
> > and __list_del() might need some attention! And these might
> > result in additional users of *_rcuprev().
> >
> > list_prev_rcu() as in your first patch, but with READ_ONCE().
> > Otherwise DEC Alpha can fail. And more subtle compiler issues
> > can appear on other architectures.
> >
> > Note that list_move_tail() will be OK give or take *_ONCE().
> > It might be better to define a list_move_tail_rcuprev(), given
> > the large number of users of list_move_tail() -- some of these
> > users might not like even the possibility of added overhead due
> > to volatile accesses. ;-)
> >
> > Or am I missing something subtle here?
> >
> > Thanx, Paul