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: <93929ecf62e79670f1e3a1878757fc9fa443aa7c.1688210094.git.bcodding@redhat.com>
In-Reply-To: <93929ecf62e79670f1e3a1878757fc9fa443aa7c.1688210094.git.bcodding@redhat.com>
From:   Olga Kornievskaia <aglo@umich.edu>
Date:   Wed, 4 Oct 2023 14:53:44 -0400
Message-ID: <CAN-5tyGf6txJpoJBSzEh75BgZAQ1f4TbZF10Dw25GjeE4Pz=7w@mail.gmail.com>
Subject: Re: [PATCH v3] NFSv4: Fix dropped lock for racing OPEN and delegation return
To:     Benjamin Coddington <bcodding@redhat.com>
Cc:     trond.myklebust@hammerspace.com, anna@kernel.org,
        Olga.Kornievskaia@netapp.com, linux-nfs@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Trond/Ben,

Did this ever go to stable? I don't know if I missed a mail from Greg
that it was picked up or it never got picked up because it wasn't
marked for stable?

Thank you.

On Sat, Jul 1, 2023 at 8:13=E2=80=AFAM Benjamin Coddington <bcodding@redhat=
.com> wrote:
>
> Commmit f5ea16137a3f ("NFSv4: Retry LOCK on OLD_STATEID during delegation
> return") attempted to solve this problem by using nfs4's generic async er=
ror
> handling, but introduced a regression where v4.0 lock recovery would hang=
.
> The additional complexity introduced by overloading that error handling i=
s
> not necessary for this case.  This patch expects that commit to be
> reverted.
>
> The problem as originally explained in the above commit is:
>
>     There's a small window where a LOCK sent during a delegation return c=
an
>     race with another OPEN on client, but the open stateid has not yet be=
en
>     updated.  In this case, the client doesn't handle the OLD_STATEID err=
or
>     from the server and will lose this lock, emitting:
>     "NFS: nfs4_handle_delegation_recall_error: unhandled error -10024".
>
> Fix this by using the old_stateid refresh helpers if the server replies
> with OLD_STATEID.
>
> Suggested-by: Trond Myklebust <trondmy@hammerspace.com>
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/nfs4proc.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 6bb14f6cfbc0..f350f41e1967 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -7180,8 +7180,15 @@ static void nfs4_lock_done(struct rpc_task *task, =
void *calldata)
>                 } else if (!nfs4_update_lock_stateid(lsp, &data->res.stat=
eid))
>                         goto out_restart;
>                 break;
> -       case -NFS4ERR_BAD_STATEID:
>         case -NFS4ERR_OLD_STATEID:
> +               if (data->arg.new_lock_owner !=3D 0 &&
> +                       nfs4_refresh_open_old_stateid(&data->arg.open_sta=
teid,
> +                                       lsp->ls_state))
> +                       goto out_restart;
> +               else if (nfs4_refresh_lock_old_stateid(&data->arg.lock_st=
ateid, lsp))
> +                       goto out_restart;
> +               fallthrough;
> +       case -NFS4ERR_BAD_STATEID:
>         case -NFS4ERR_STALE_STATEID:
>         case -NFS4ERR_EXPIRED:
>                 if (data->arg.new_lock_owner !=3D 0) {
> --
> 2.40.1
>