linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: Al Viro <viro@zeniv.linux.org.uk>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Linux-Fsdevel <linux-fsdevel@vger.kernel.org>,
	Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-unionfs@vger.kernel.org
Subject: Re: [GIT PULL] overlay filesystem v25
Date: Mon, 27 Oct 2014 08:59:01 -0700	[thread overview]
Message-ID: <20141027155901.GE5718@linux.vnet.ibm.com> (raw)
In-Reply-To: <CAJfpegucroRzo0NHTz=4N0WcrB2oDC0eDiZXk=aKSjc8vdYDjw@mail.gmail.com>

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@zeniv.linux.org.uk> 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

  reply	other threads:[~2014-10-27 15:59 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-10-23 23:25 [GIT PULL] overlay filesystem v25 Miklos Szeredi
2014-10-24  2:20 ` Al Viro
2014-10-24  3:24   ` Al Viro
2014-10-24  7:24     ` Miklos Szeredi
2014-10-25  8:18       ` Al Viro
2014-10-25  9:53         ` Miklos Szeredi
2014-10-25 17:06           ` Al Viro
2014-10-27  8:06             ` Miklos Szeredi
2014-10-27 15:59               ` Paul E. McKenney [this message]
2014-10-27 17:28                 ` Al Viro
2014-10-27 17:36                   ` Paul E. McKenney
2014-10-28  1:12                     ` Al Viro
2014-10-28  4:11                       ` Paul E. McKenney
2014-10-28 22:55                         ` Al Viro
2014-10-28 23:25                           ` Paul E. McKenney
2014-10-24  7:28     ` Geert Uytterhoeven

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20141027155901.GE5718@linux.vnet.ibm.com \
    --to=paulmck@linux.vnet.ibm.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-unionfs@vger.kernel.org \
    --cc=miklos@szeredi.hu \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).