Received-SPF: pass (google.com: domain of linux-nfs+bounces-3395-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: "Richard Kojedzinszky" <richard@kojedz.in>
Cc: "Richard Kojedzinszky" <richard+debian+bugreport@kojedz.in>,
 linux-nfs@vger.kernel.org, 1071501@bugs.debian.org
Subject: Re: Linux NFS client hangs in nfs4_lookup_revalidate
In-reply-to: <EB269D8D-03FF-4ED1-B64C-E41B111B4AF8@kojedz.in>
References: <>, <EB269D8D-03FF-4ED1-B64C-E41B111B4AF8@kojedz.in>
Date: Mon, 27 May 2024 14:50:08 +1000
Message-id: <171678540815.27191.15492517697816891799@noble.neil.brown.name>

On Mon, 27 May 2024, Richard Kojedzinszky wrote:
> Dear Neil,
>=20
> I was running it on arm64, may that be the reason?

That could certainly explain it.  I know x86_64 almost never needs
barriers, though I have seen cases where they matter.  ppc64 is very
sensitive.  A quick search suggests that arm64 does need barriers some
times.
I don't have arm64 hardware to test on but I'm happy with your
test results.

Thanks,
NeilBrown


>=20
> Regards,
> Richard
>=20
>=20
> On May 27, 2024 4:02:32 AM GMT+02:00, NeilBrown <neilb@suse.de> wrote:
>=20
>     On Sun, 26 May 2024, Richard Kojedzinszky wrote:
>         Dear Neil,
>        =20
>         According to my quick tests, your patch seems to fix this bug. Coul=
d you=20
>         also manage to try my attached code, could you also reproduce the b=
ug?
>    =20
>     Thanks for testing.
>    =20
>     I can run your test code but it isn't triggering the bug (90 minutes so
>     far).  Possibly a different compiler used for the kernel, possibly
>     hardware differences (I'm running under qemu).  Bugs related to barriers
>     (which this one seems to be) need just the right circumstances to
>     trigger so they can be hard to reproduce on a different system.
>    =20
>     I've made some cosmetic improvements to the patch and will post it to
>     the NFS maintainers.
>    =20
>     Thanks again,
>     NeilBrown
>    =20
>    =20
>        =20
>         Thanks,
>         Richard
>        =20
>         2024-05-24 07:29 id=C5=91pontban Richard Kojedzinszky ezt =C3=ADrta:
>             Dear Neil,
>            =20
>             I've applied your patch, and since then there are no lockups. B=
efore=20
>             that my application reported a lockup in a minute or two, now i=
t has=20
>             been running for half an hour, and still running.
>            =20
>             Thanks,
>             Richard
>            =20
>             2024-05-24 01:31 id=C5=91pontban NeilBrown ezt =C3=ADrta:
>                 On Fri, 24 May 2024, Richard Kojedzinszky wrote:
>                     Dear devs,
>                    =20
>                     I am attaching a stripped down version of the little pr=
ogram which
>                     triggers the bug very quickly, in a few minutes in my t=
est lab. It
>                     turned out that a single NFS mountpoint is enough. Just=
 start the
>                     program giving it the NFS mount as first argument. It w=
ill chdir=20
>                     there,
>                     and do file operations, which will trigger a lockup in =
a few minutes.
>                =20
>                 I couldn't get the go code to run.  But then it is a long t=
ime since I
>                 played with go and I didn't try very hard.
>                 If you could provide simple instructions and a list of pack=
age
>                 dependencies that I need to install (on Debian), I can give=
 it a try.
>                =20
>                 Or you could try this patch.  It might help, but I don't ha=
ve high
>                 hopes.  It adds some memory barriers and fixes a bug which =
would cause=20
>                 a
>                 problem if memory allocation failed (but memory allocation =
never=20
>                 fails).
>                =20
>                 NeilBrown
>                =20
>                 diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
>                 index ac505671efbd..5bcc0d14d519 100644
>                 --- a/fs/nfs/dir.c
>                 +++ b/fs/nfs/dir.c
>                 @@ -1804,7 +1804,7 @@ __nfs_lookup_revalidate(struct dentry=
 *dentry,=20
>                 unsigned int flags,
>                         } else {
>                                 /* Wait for unlink to complete */
>                                 wait_var_event(&dentry->d_fsdata,
>                 -                              dentry->d_fsdata !=3D NFS_FS=
DATA_BLOCKED);
>                 +                              smp_load_acquire(&dentry->d_=
fsdata) !=3D NFS_FSDATA_BLOCKED);
>                                 parent =3D dget_parent(dentry);
>                                 ret =3D reval(d_inode(parent), dentry, flag=
s);
>                                 dput(parent);
>                 @@ -2508,7 +2508,7 @@ int nfs_unlink(struct inode *dir, str=
uct dentry=20
>                 *dentry)
>                         spin_unlock(&dentry->d_lock);
>                         error =3D nfs_safe_remove(dentry);
>                         nfs_dentry_remove_handle_error(dir, dentry, error);
>                 -       dentry->d_fsdata =3D NULL;
>                 +       smp_store_release(&dentry->d_fsdata, NULL);
>                         wake_up_var(&dentry->d_fsdata);
>                  out:
>                         trace_nfs_unlink_exit(dir, dentry, error);
>                 @@ -2616,7 +2616,7 @@ nfs_unblock_rename(struct rpc_task *t=
ask, struct=20
>                 nfs_renamedata *data)
>                  {
>                         struct dentry *new_dentry =3D data->new_dentry;
>                =20
>                 -       new_dentry->d_fsdata =3D NULL;
>                 +       smp_store_release(&new_dentry->d_fsdata, NULL);
>                         wake_up_var(&new_dentry->d_fsdata);
>                  }
>                =20
>                 @@ -2717,6 +2717,10 @@ int nfs_rename(struct mnt_idmap *idm=
ap, struct=20
>                 inode *old_dir,
>                         task =3D nfs_async_rename(old_dir, new_dir, old_den=
try, new_dentry,
>                                                 must_unblock ? nfs_unblock_=
rename : NULL);
>                         if (IS_ERR(task)) {
>                 +               if (must_unlock) {
>                 +                       smp_store_release(&new_dentry->d_fs=
data, NULL);
>                 +                       wake_up_var(&new_dentry->d_fsdata);
>                 +               }
>                                 error =3D PTR_ERR(task);
>                                 goto out;
>                         }
>        =20
>    =20
>    =20
>=20
>=20
>=20