On Sat, 2016-11-12 at 21:56 -0500, Jeff Layton wrote:
> On Sat, 2016-11-12 at 16:16 -0500, Jeff Layton wrote:
> >
> > On Sat, 2016-11-12 at 13:03 -0500, Benjamin Coddington wrote:
> > >
> > >
> > > On 12 Nov 2016, at 11:52, Jeff Layton wrote:
> > >
> > > >
> > > >
> > > > On Sat, 2016-11-12 at 10:31 -0500, Benjamin Coddington wrote:
> > > > >
> > > > >
> > > > > On 12 Nov 2016, at 7:54, Jeff Layton wrote:
> > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > On Sat, 2016-11-12 at 06:08 -0500, Benjamin Coddington
> > > > > > wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > I've been seeing the following on a modified version of
> > > > > > > generic/089
> > > > > > > that gets the client stuck sending LOCK with
> > > > > > > NFS4ERR_OLD_STATEID.
> > > > > > >
> > > > > > > 1. Client has open stateid A, sends a CLOSE
> > > > > > > 2. Client sends OPEN with same owner
> > > > > > > 3. Client sends another OPEN with same owner
> > > > > > > 4. Client gets a reply to OPEN in 3, stateid is B.2
> > > > > > > (stateid B
> > > > > > > sequence 2)
> > > > > > > 5. Client does LOCK,LOCKU,FREE_STATEID from B.2
> > > > > > > 6. Client gets a reply to CLOSE in 1
> > > > > > > 7. Client gets reply to OPEN in 2, stateid is B.1
> > > > > > > 8. Client sends LOCK with B.1 - OLD_STATEID, now stuck in
> > > > > > > a loop
> > > > > > >
> > > > > > > The CLOSE response in 6 causes us to clear
> > > > > > > NFS_OPEN_STATE, so that
> > > > > > > the OPEN
> > > > > > > response in 7 is able to update the open_stateid even
> > > > > > > though it has a
> > > > > > > lower
> > > > > > > sequence number.
> > > > > > >
> > > > > > > I think this case could be handled by never updating the
> > > > > > > open_stateid
> > > > > > > if the
> > > > > > > stateids match but the sequence number of the new state
> > > > > > > is less than
> > > > > > > the
> > > > > > > current open_state.
> > > > > > >
> > > > > >
> > > > > > What kernel is this on?
> > > > >
> > > > > On v4.9-rc2 with a couple fixups. Without them, I can't test
> > > > > long
> > > > > enough to
> > > > > reproduce this race. I don't think any of those are involved
> > > > > in this
> > > > > problem, though.
> > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > Yes, that seems wrong. The client should be picking B.2 for
> > > > > > the open
> > > > > > stateid to use. I think that decision of whether to take a
> > > > > > seqid is
> > > > > > made
> > > > > > in nfs_need_update_open_stateid. The logic in there looks
> > > > > > correct to
> > > > > > me
> > > > > > at first glance though.
> > > > >
> > > > > nfs_need_update_open_stateid() will return true if
> > > > > NFS_OPEN_STATE is
> > > > > unset.
> > > > > That's the precondition set up by steps 1-6. Perhaps it
> > > > > should not
> > > > > update
> > > > > the stateid if they match but the sequence number is less,
> > > > > and still set
> > > > > NFS_OPEN_STATE once more. That will fix _this_ case. Are
> > > > > there other
> > > > > cases
> > > > > where that would be a problem?
> > > > >
> > > > > Ben
> > > >
> > > > That seems wrong.
> > >
> > > I'm not sure what you mean: what seems wrong?
> > >
> >
> > Sorry, it seems wrong that the client would issue the LOCK with B.1
> > there.
> >
> > >
> > > >
> > > >
> > > > The only close was sent in step 1, and that was for a
> > > > completely different stateid (A rather than B). It seems likely
> > > > that
> > > > that is where the bug is.
> > >
> > > I'm still not sure what point you're trying to make..
> > >
> > > Even though the close was sent in step 1, the response wasn't
> > > processed
> > > until step 6..
> >
> > Not really a point per-se, I was just saying where I think the bug
> > might
> > be...
> >
> > When you issue a CLOSE, you issue it vs. a particular stateid
> > (stateid
> > "A" in this case). Once the open stateid has been superseded by
> > "B", the
> > closing of "A" should have no effect.
> >
> > Perhaps nfs_clear_open_stateid needs to check and see whether the
> > open
> > stateid has been superseded before doing its thing?
> >
>
> Ok, I see something that might be a problem in this call in
> nfs4_close_done:
>
> nfs_clear_open_stateid(state, &calldata->arg.stateid,
> res_stateid, calldata->arg.fmode);
>
> Note that we pass two nfs4_stateids to this call. The first is the
> stateid that got sent in the CLOSE call, and the second is the
> stateid
> that came back in the CLOSE response.
>
> RFC5661 and RFC7530 both indicate that the stateid in a CLOSE
> response
> should be ignored.
>
> So, I think a patch like this may be in order. As to whether it will
> fix this bug, I sort of doubt it, but it might not hurt to test it
> out?
>
> ----------------------8<--------------------------
>
> [RFC PATCH] nfs: properly ignore the stateid in a CLOSE response
>
> Signed-off-by: Jeff Layton
> ---
> fs/nfs/nfs4proc.c | 14 +++-----------
> 1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 7897826d7c51..58413bd0aae2 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -1451,7 +1451,6 @@ static void
> nfs_resync_open_stateid_locked(struct nfs4_state *state)
> }
>
> static void nfs_clear_open_stateid_locked(struct nfs4_state *state,
> - nfs4_stateid *arg_stateid,
> nfs4_stateid *stateid, fmode_t fmode)
> {
> clear_bit(NFS_O_RDWR_STATE, &state->flags);
> @@ -1467,12 +1466,8 @@ static void
> nfs_clear_open_stateid_locked(struct nfs4_state *state,
> clear_bit(NFS_O_WRONLY_STATE, &state->flags);
> clear_bit(NFS_OPEN_STATE, &state->flags);
> }
> - if (stateid == NULL)
> - return;
> /* Handle races with OPEN */
> - if (!nfs4_stateid_match_other(arg_stateid, &state-
> >open_stateid) ||
> - (nfs4_stateid_match_other(stateid, &state->open_stateid)
> &&
> - !nfs4_stateid_is_newer(stateid, &state->open_stateid)))
> {
> + if (!nfs4_stateid_match_other(stateid, &state-
> >open_stateid)) {
No. I think what we should be doing here is
1) if (nfs4_stateid_match_other(arg_stateid, &state->open_stateid) then
just ignore the result and return immediately, because it applies to a
completely different stateid.
2) if (nfs4_stateid_match_other(stateid, &state->open_stateid)
&& !nfs4_stateid_is_newer(stateid, &state->open_stateid))) then resync,
because this was likely an OPEN_DOWNGRADE that has raced with one or
more OPEN calls.
Note that the reason why we've been careful about this previously is
because RFC3530 did not enforce the "seqid:other" definition of
stateids. Now that RFC7530 section 9.1.4.2 has strengthened the
definition of stateids for NFSv4.0, we can assume the above holds for
all existing minor versions of NFSv4.��.n��������+%������w��{.n�����{��w��� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
