Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191023235600.10880-1-trond.myklebust@hammerspace.com>
In-Reply-To: <20191023235600.10880-1-trond.myklebust@hammerspace.com>
From:   Olga Kornievskaia <aglo@umich.edu>
Date:   Thu, 31 Oct 2019 11:27:21 -0400
Message-ID: <CAN-5tyF3hryyjdHjcoNHHPJUDZmgtgxQDureZ+3QQmiwh2CMsA@mail.gmail.com>
Subject: Re: [PATCH 00/14] Delegation bugfixes
To:     Trond Myklebust <trondmy@gmail.com>
Cc:     linux-nfs <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

Hi Trond,

This patch set produces the following in my testing. Basically what I
see the client is prevented from using a delegation at all.

After I induce a race of DELEGRETURN/OPEN
--- the racing OPEN gets a delegation (it returns the same seqid and
other as the delegation being returned) but the client doesn't use it.
--- the following (next) OPEN that also gets a delegation immediately
has the client returning the given delegation.

Disclaimer: in my testing the racing DELEGRETURN doesn't fail with
OLD_STATEID, NetApp returns OK.

On Thu, Oct 24, 2019 at 6:56 AM Trond Myklebust <trondmy@gmail.com> wrote:
>
> The following patchset fixes up a number of issues with delegations,
> but mainly attempts to fix a race condition between open and
> delegreturn, where the open and the delegreturn get re-ordered so
> that the delegreturn ends up revoking the delegation that was returned
> by the open.
> The root cause is that in certain circumstances, we may currently end
> up freeing the delegation from delegreturn, so when we later receive
> the reply to the open, we've lost track of the fact that the seqid
> predates the one that was returned.
>
> This patchset fixes that case by ensuring that we always keep track
> of the last delegation stateid that was returned for any given inode.
> That way, if we later see a delegation stateid with the same opaque
> field, but an older seqid, we know we cannot trust it, and so we
> ask to replay the OPEN compound.
>
> Trond Myklebust (14):
>   NFSv4: Don't allow a cached open with a revoked delegation
>   NFSv4: Fix delegation handling in update_open_stateid()
>   NFSv4: nfs4_callback_getattr() should ignore revoked delegations
>   NFSv4: Delegation recalls should not find revoked delegations
>   NFSv4: fail nfs4_refresh_delegation_stateid() when the delegation was
>     revoked
>   NFS: Rename nfs_inode_return_delegation_noreclaim()
>   NFSv4: Don't remove the delegation from the super_list more than once
>   NFSv4: Hold the delegation spinlock when updating the seqid
>   NFSv4: Clear the NFS_DELEGATION_REVOKED flag in
>     nfs_update_inplace_delegation()
>   NFSv4: Update the stateid seqid in nfs_revoke_delegation()
>   NFSv4: Revoke the delegation on success in nfs4_delegreturn_done()
>   NFSv4: Ignore requests to return the delegation if it was revoked
>   NFSv4: Don't reclaim delegations that have been returned or revoked
>   NFSv4: Fix races between open and delegreturn
>
>  fs/nfs/callback_proc.c |   2 +-
>  fs/nfs/delegation.c    | 109 +++++++++++++++++++++++++++++------------
>  fs/nfs/delegation.h    |   4 +-
>  fs/nfs/nfs4proc.c      |  13 ++---
>  fs/nfs/nfs4super.c     |   4 +-
>  5 files changed, 88 insertions(+), 44 deletions(-)
>
> --
> 2.21.0
>