Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
MIME-Version: 1.0
References: <20230917230551.30483-1-trondmy@kernel.org> <20230917230551.30483-2-trondmy@kernel.org>
In-Reply-To: <20230917230551.30483-2-trondmy@kernel.org>
From:   Anna Schumaker <schumaker.anna@gmail.com>
Date:   Wed, 20 Sep 2023 15:38:50 -0400
Message-ID: <CAFX2Jfn-6J1RAiz7Vjjet+EW4jDFVRcQ9ahsZVp69AW=MC5tpg@mail.gmail.com>
Subject: Re: [PATCH 2/2] NFSv4: Fix a state manager thread deadlock regression
To:     trondmy@kernel.org
Cc:     Anna Schumaker <Anna.Schumaker@netapp.com>,
        linux-nfs@vger.kernel.org, Neil Brown <neilb@suse.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Trond,

On Sun, Sep 17, 2023 at 7:12=E2=80=AFPM <trondmy@kernel.org> wrote:
>
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>
> Commit 4dc73c679114 reintroduces the deadlock that was fixed by commit
> aeabb3c96186 ("NFSv4: Fix a NFSv4 state manager deadlock") because it
> prevents the setup of new threads to handle reboot recovery, while the
> older recovery thread is stuck returning delegations.

I'm seeing a possible deadlock with xfstests generic/472 on NFS v4.x
after applying this patch. The test itself checks for various swapfile
edge cases, so it seems likely something is going on there.

Let me know if you need more info
Anna

>
> Fixes: 4dc73c679114 ("NFSv4: keep state manager thread active if swap is =
enabled")
> Cc: stable@vger.kernel.org
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfs/nfs4proc.c  |  4 +++-
>  fs/nfs/nfs4state.c | 38 ++++++++++++++++++++++++++------------
>  2 files changed, 29 insertions(+), 13 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 5deeaea8026e..a19e809cad16 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -10652,7 +10652,9 @@ static void nfs4_disable_swap(struct inode *inode=
)
>          */
>         struct nfs_client *clp =3D NFS_SERVER(inode)->nfs_client;
>
> -       nfs4_schedule_state_manager(clp);
> +       set_bit(NFS4CLNT_RUN_MANAGER, &clp->cl_state);
> +       clear_bit(NFS4CLNT_MANAGER_AVAILABLE, &clp->cl_state);
> +       wake_up_var(&clp->cl_state);
>  }
>
>  static const struct inode_operations nfs4_dir_inode_operations =3D {
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index 0bc160fbabec..5751a6886da4 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -1209,16 +1209,26 @@ void nfs4_schedule_state_manager(struct nfs_clien=
t *clp)
>  {
>         struct task_struct *task;
>         char buf[INET6_ADDRSTRLEN + sizeof("-manager") + 1];
> +       struct rpc_clnt *clnt =3D clp->cl_rpcclient;
> +       bool swapon =3D false;
>
> -       if (clp->cl_rpcclient->cl_shutdown)
> +       if (clnt->cl_shutdown)
>                 return;
>
>         set_bit(NFS4CLNT_RUN_MANAGER, &clp->cl_state);
> -       if (test_and_set_bit(NFS4CLNT_MANAGER_AVAILABLE, &clp->cl_state) =
!=3D 0) {
> -               wake_up_var(&clp->cl_state);
> -               return;
> +
> +       if (atomic_read(&clnt->cl_swapper)) {
> +               swapon =3D !test_and_set_bit(NFS4CLNT_MANAGER_AVAILABLE,
> +                                          &clp->cl_state);
> +               if (!swapon) {
> +                       wake_up_var(&clp->cl_state);
> +                       return;
> +               }
>         }
> -       set_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state);
> +
> +       if (test_and_set_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state) !=
=3D 0)
> +               return;
> +
>         __module_get(THIS_MODULE);
>         refcount_inc(&clp->cl_count);
>
> @@ -1235,8 +1245,9 @@ void nfs4_schedule_state_manager(struct nfs_client =
*clp)
>                         __func__, PTR_ERR(task));
>                 if (!nfs_client_init_is_complete(clp))
>                         nfs_mark_client_ready(clp, PTR_ERR(task));
> +               if (swapon)
> +                       clear_bit(NFS4CLNT_MANAGER_AVAILABLE, &clp->cl_st=
ate);
>                 nfs4_clear_state_manager_bit(clp);
> -               clear_bit(NFS4CLNT_MANAGER_AVAILABLE, &clp->cl_state);
>                 nfs_put_client(clp);
>                 module_put(THIS_MODULE);
>         }
> @@ -2748,22 +2759,25 @@ static int nfs4_run_state_manager(void *ptr)
>
>         allow_signal(SIGKILL);
>  again:
> -       set_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state);
>         nfs4_state_manager(clp);
> -       if (atomic_read(&cl->cl_swapper)) {
> +
> +       if (test_bit(NFS4CLNT_MANAGER_AVAILABLE, &clp->cl_state) &&
> +           !test_and_set_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state)) =
{
>                 wait_var_event_interruptible(&clp->cl_state,
>                                              test_bit(NFS4CLNT_RUN_MANAGE=
R,
>                                                       &clp->cl_state));
> -               if (atomic_read(&cl->cl_swapper) &&
> -                   test_bit(NFS4CLNT_RUN_MANAGER, &clp->cl_state))
> +               if (!atomic_read(&cl->cl_swapper))
> +                       clear_bit(NFS4CLNT_MANAGER_AVAILABLE, &clp->cl_st=
ate);
> +               if (refcount_read(&clp->cl_count) > 1 && !signalled())
>                         goto again;
>                 /* Either no longer a swapper, or were signalled */
> +               clear_bit(NFS4CLNT_MANAGER_AVAILABLE, &clp->cl_state);
> +               nfs4_clear_state_manager_bit(clp);
>         }
> -       clear_bit(NFS4CLNT_MANAGER_AVAILABLE, &clp->cl_state);
>
>         if (refcount_read(&clp->cl_count) > 1 && !signalled() &&
>             test_bit(NFS4CLNT_RUN_MANAGER, &clp->cl_state) &&
> -           !test_and_set_bit(NFS4CLNT_MANAGER_AVAILABLE, &clp->cl_state)=
)
> +           !test_and_set_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state))
>                 goto again;
>
>         nfs_put_client(clp);
> --
> 2.41.0
>