On Mon, Oct 27, 2014 at 09:06:54AM +0100, Miklos Szeredi wrote:
> [Paul McKenney added to CC]
>
> On Sat, Oct 25, 2014 at 7:06 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> > On Sat, Oct 25, 2014 at 11:53:52AM +0200, Miklos Szeredi wrote:
> >
> >> Yes, but it's not about race with copy-up (which the ovl_path_upper()
> >> protects against), but race of two fsync calls with each other. If
> >> there's no synchronization between them, then that od->upperfile does
> >> indeed count as lockless access, no matter that the assignment was
> >> done under lock.
> >
> > p = global;
> > if (!p) { // outside of lock
> > p = alloc();
> > grab lock
> > if (!global) {
> > global = p;
> > } else {
> > destroy(p);
> > p = global;
> > }
> > drop lock
> > }
> > is a very common pattern, especially if you look for cases when lock is
> > a spinlock and allocation is blocking (in those cases you'll often see
> > destroy() part done after dropping the lock; that's where what I fucked up in
> > what I'd originally pushed. And it wasn't even needed - fput() under
> > ->i_mutex is OK...)
>
> Being a very common pattern does not automatically make it correct...
>
> My understanding of these issues is very limited, but it's not clear
> to me what will order initialization of members of p with the storing
> of p into global. E.g. we start out with global == NULL and p->foo ==
> 0.
>
> CPU1:
> p->foo = 1
> grab lock
> if (!global)
> global = p
>
> CPU1:
If it is all the same to you, I will call this CPU2 to distinguish it from
the first CPU1. ;-)
> p = global
> if (p)
> q = p->foo
>
> Is it guaranteed that the above sequence (as is, without any barriers
> or ACCESS_ONCE() other than the lock acquisition) will result in q ==
> 1 if p != NULL?
Indeed, life is hard here. Keep in mind that lock acquisition is not
guaranteed to prevent prior operations from being reordered into the
critical section, possibly as follows:
CPU1:
grab lock
if (!global)
global = p;
/* Assume all of CPU2's accesses happen here. */
p->foo = 1;
This clearly allows CPU2 to execute as follows:
CPU2:
p = global; /* gets p */
if (p) /* non-NULL */
q = p->foo; /* might not be 1 */
Not only that, on DEC Alpha, even if CPU1's accesses are ordered, CPU2's
accesses can be misordered. You need rcu_dereference() or the combination
of ACCESS_ONCE() and smp_read_barrier_depends() to avoid this issue.
As always, see http://www.openvms.compaq.com/wizard/wiz_2637.html for
more info.
So no, there is no guarantee. I am assuming that the lock grabbed by
CPU1 guards all assignments to "global", otherwise the code needs further
help. I am further assuming that the memory pointed to by CPU1's "p"
is inaccessible to any other CPU, as in CPU1 just allocated the memory.
Otherwise, the assignment "p->foo = 1" is questionable. And finally,
I am assuming that p->foo stays constant once it has been made
accessible to readers.
But the following should work:
CPU1:
p->foo = 1; /* Assumes p is local. */
smp_mb__before_spinlock();
grab lock
if (!global) /* Assumes lock protects all assignments to global. */
global = p;
CPU2:
p = rcu_dereference(global);
if (p)
q = p->foo; /* Assumes p->foo constant once visible to readers. */
/* Also assumes p and q are local. */
If the assumptions called out in the comments do not hold, you at least
need ACCESS_ONCE(), and perhaps even more synchronization. For more info
on ACCESS_ONCE(), Jon's LWN article is at http://lwn.net/Articles/508991/.
Thanx, Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
