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

["Paul E. McKenney" <paulmck@kernel.org>]

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