Received-SPF: pass (google.com: domain of linux-nfs+bounces-3480-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: "Trond Myklebust" <trondmy@hammerspace.com>
Cc: "anna@kernel.org" <anna@kernel.org>,
 "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
 "richard+debian+bugreport@kojedz.in" <richard+debian+bugreport@kojedz.in>,
 "1071501@bugs.debian.org" <1071501@bugs.debian.org>
Subject: Re: [PATCH] NFS: add barriers when testing for NFS_FSDATA_BLOCKED
In-reply-to: <808846871c4d5dc3410f610a704a7915d7cf5930.camel@hammerspace.com>
References:
 <>, <808846871c4d5dc3410f610a704a7915d7cf5930.camel@hammerspace.com>
Date: Wed, 29 May 2024 13:47:35 +1000
Message-id: <171695445579.27191.9734748642826473078@noble.neil.brown.name>

On Wed, 29 May 2024, Trond Myklebust wrote:
> On Tue, 2024-05-28 at 11:19 +1000, NeilBrown wrote:
> > On Tue, 28 May 2024, Trond Myklebust wrote:
> > > On Mon, 2024-05-27 at 13:04 +1000, NeilBrown wrote:
> > > >=20
> > > > dentry->d_fsdata is set to NFS_FSDATA_BLOCKED while unlinking or
> > > > renaming-over a file to ensure that no open succeeds while the
> > > > NFS
> > > > operation progressed on the server.
> > > >=20
> > > > Setting dentry->d_fsdata to NFS_FSDATA_BLOCKED is done under -
> > > > >d_lock
> > > > after checking the refcount is not elevated.=C2=A0 Any attempt to open
> > > > the
> > > > file (through that name) will go through lookp_open() which will
> > > > take
> > > > ->d_lock while incrementing the refcount, we can be sure that
> > > > once
> > > > the
> > > > new value is set, __nfs_lookup_revalidate() *will* see the new
> > > > value
> > > > and
> > > > will block.
> > > >=20
> > > > We don't have any locking guarantee that when we set ->d_fsdata
> > > > to
> > > > NULL,
> > > > the wait_var_event() in __nfs_lookup_revalidate() will notice.
> > > > wait/wake primitives do NOT provide barriers to guarantee order.=C2=A0
> > > > We
> > > > must use smp_load_acquire() in wait_var_event() to ensure we look
> > > > at
> > > > an
> > > > up-to-date value, and must use smp_store_release() before
> > > > wake_up_var().
> > > >=20
> > > > This patch adds those barrier functions and factors out
> > > > block_revalidate() and unblock_revalidate() far clarity.
> > > >=20
> > > > There is also a hypothetical bug in that if memory allocation
> > > > fails
> > > > (which never happens in practice) we might leave ->d_fsdata
> > > > locked.
> > > > This patch adds the missing call to unblock_revalidate().
> > > >=20
> > > > Reported-and-tested-by: Richard Kojedzinszky
> > > > <richard+debian+bugreport@kojedz.in>
> > > > Closes: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=3D1071501
> > > > Fixes: 3c59366c207e ("NFS: don't unhash dentry during
> > > > unlink/rename")
> > > > Signed-off-by: NeilBrown <neilb@suse.de>
> > > > ---
> > > > =C2=A0fs/nfs/dir.c | 44 +++++++++++++++++++++++++++++---------------
> > > > =C2=A01 file changed, 29 insertions(+), 15 deletions(-)
> > > >=20
> > > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > > > index ac505671efbd..c91dc36d41cc 100644
> > > > --- a/fs/nfs/dir.c
> > > > +++ b/fs/nfs/dir.c
> > > > @@ -1802,9 +1802,10 @@ __nfs_lookup_revalidate(struct dentry
> > > > *dentry,
> > > > unsigned int flags,
> > > > =C2=A0		if (parent !=3D READ_ONCE(dentry->d_parent))
> > > > =C2=A0			return -ECHILD;
> > > > =C2=A0	} else {
> > > > -		/* Wait for unlink to complete */
> > > > +		/* Wait for unlink to complete - see
> > > > unblock_revalidate() */
> > > > =C2=A0		wait_var_event(&dentry->d_fsdata,
> > > > -			=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 dentry->d_fsdata !=3D
> > > > NFS_FSDATA_BLOCKED);
> > > > +			=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 smp_load_acquire(&dentry-
> > > > >d_fsdata)
> > > > +			=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 !=3D NFS_FSDATA_BLOCKED);
> > >=20
> > > Doesn't this end up being a reversed ACQUIRE+RELEASE as described
> > > in
> > > the "LOCK ACQUISITION FUNCTIONS" section of Documentation/memory-
> > > barriers.txt?
> >=20
> > I don't think so.=C2=A0 That section is talking about STORE operations
> > which
> > can move backwards through ACQUIRE and forwards through RELEASE.
> >=20
> > Above we have a LOAD operation which mustn't move backwards through
> > ACQUIRE.=C2=A0 Below there is a STORE operation which mustn't move
> > forwards
> > through a RELEASE.=C2=A0 Both of those are guaranteed.
>=20
> It isn't necessary for the LOAD to move backwards through the ACQUIRE.
> As I understand it, the point is that the ACQUIRE can move through the
> RELEASE as per the following paragraph in that document:
>=20
>             Similarly, the reverse case of a RELEASE followed by an ACQUIRE=
 does
>             not imply a full memory barrier.  Therefore, the CPU's executio=
n of the
>             critical sections corresponding to the RELEASE and the ACQUIRE =
can cross,
>             so that:
>            =20
>                     *A =3D a;
>                     RELEASE M
>                     ACQUIRE N
>                     *B =3D b;
>            =20
>             could occur as:
>            =20
>                     ACQUIRE N, STORE *B, STORE *A, RELEASE M

On the wakeup side we have:

  STORE ->d_fsdata
  RELEASE
  spin_lock
  LOAD: check is waitq is empty

and on the wait side we have

  STORE: add to waitq
  spin_unlock
  ACQUIRE
  LOAD ->d_fsdata

I believe that spin_lock is an ACQUIRE operation and spin_unlock is a
RELEASE operation.  So both of these have "ACQUIRE ; RELEASE" which
creates a full barrier.

>=20
> This would presumably be why the function clear_and_wake_up_bit() needs
> a full memory barrier on most architectures, instead of being just an
> smp_wmb(). Is my understanding of this wrong?

clear_and_wake_up_bit() calls __wake_up_bit() which calls
waitqueue_active() without taking a lock.  So there is no ACQUIRE after
the RELEASE in clear_bit_unlock() and before testing for list-empty.  So
we need to explicitly add a barrier.

At least that is my understanding just now.  I hadn't worked through all
the details before, but I'm glad you prompted me to - thanks.

NeilBrown