Re: [PATCH] NFSv4: fix stateid refreshing when CLOSE racing with OPEN

[Trond Myklebust <trondmy@hammerspace.com>]

On Fri, 2019-10-11 at 16:49 +0800, Murphy Zhou wrote:
> On Thu, Oct 10, 2019 at 02:46:40PM +0000, Trond Myklebust wrote:
> > On Thu, 2019-10-10 at 15:40 +0800, Murphy Zhou wrote:
> > > Since commit:
> > >   [0e0cb35] NFSv4: Handle NFS4ERR_OLD_STATEID in
> > > CLOSE/OPEN_DOWNGRADE
> > > 
> > > xfstests generic/168 on v4.2 starts to fail because reflink call
> > > gets:
> > >   +XFS_IOC_CLONE_RANGE: Resource temporarily unavailable
> > > 
> > > In tshark output, NFS4ERR_OLD_STATEID stands out when comparing
> > > with
> > > good ones:
> > > 
> > >  5210   NFS 406 V4 Reply (Call In 5209) OPEN StateID: 0xadb5
> > >  5211   NFS 314 V4 Call GETATTR FH: 0x8d44a6b1
> > >  5212   NFS 250 V4 Reply (Call In 5211) GETATTR
> > >  5213   NFS 314 V4 Call GETATTR FH: 0x8d44a6b1
> > >  5214   NFS 250 V4 Reply (Call In 5213) GETATTR
> > >  5216   NFS 422 V4 Call WRITE StateID: 0xa818 Offset: 851968 Len:
> > > 65536
> > >  5218   NFS 266 V4 Reply (Call In 5216) WRITE
> > >  5219   NFS 382 V4 Call OPEN DH: 0x8d44a6b1/
> > >  5220   NFS 338 V4 Call CLOSE StateID: 0xadb5
> > >  5222   NFS 406 V4 Reply (Call In 5219) OPEN StateID: 0xa342
> > >  5223   NFS 250 V4 Reply (Call In 5220) CLOSE Status:
> > > NFS4ERR_OLD_STATEID
> > >  5225   NFS 338 V4 Call CLOSE StateID: 0xa342
> > >  5226   NFS 314 V4 Call GETATTR FH: 0x8d44a6b1
> > >  5227   NFS 266 V4 Reply (Call In 5225) CLOSE
> > >  5228   NFS 250 V4 Reply (Call In 5226) GETATTR
> > > 
> > > It's easy to reproduce. By printing some logs, found that we are
> > > making
> > > CLOSE seqid larger then OPEN seqid when racing.
> > > 
> > > Fix this by not bumping seqid when it's equal to OPEN seqid. Also
> > > put the whole changing process into seqlock read protection in
> > > case
> > > really bad luck, and this is the same locking behavior with the
> > > old deleted function.
> > > 
> > > Fixes: 0e0cb35b417f ("NFSv4: Handle NFS4ERR_OLD_STATEID in
> > > CLOSE/OPEN_DOWNGRADE")
> > > Signed-off-by: Murphy Zhou <jencce.kernel@gmail.com>
> > > ---
> > >  fs/nfs/nfs4proc.c | 13 ++++++++-----
> > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > > index 11eafcf..6db5a09 100644
> > > --- a/fs/nfs/nfs4proc.c
> > > +++ b/fs/nfs/nfs4proc.c
> > > @@ -3334,12 +3334,13 @@ static void
> > > nfs4_sync_open_stateid(nfs4_stateid *dst,
> > >  			break;
> > >  		}
> > >  		seqid_open = state->open_stateid.seqid;
> > > -		if (read_seqretry(&state->seqlock, seq))
> > > -			continue;
> > >  
> > >  		dst_seqid = be32_to_cpu(dst->seqid);
> > >  		if ((s32)(dst_seqid - be32_to_cpu(seqid_open)) < 0)
> > >  			dst->seqid = seqid_open;
> > > +
> > > +		if (read_seqretry(&state->seqlock, seq))
> > > +			continue;
> > 
> > What's the intention of this change? Neither dst_seqid nor dst-
> > >seqid
> > are protected by the state->seqlock so why move this code under the
> > lock.
> 
> Because seqid_open could have changed when writing to dst->seqid ?
> 
> The old nfs4_refresh_open_stateid function put the writing under the
> lock.

The value of state->open_stateid could change both before and after we
write to dst->seqid. If we hold a lock, then it could change the moment
we release that lock. That's not something we're able to control.

The only thing we should care about here is whether or not the call to
nfs4_state_match_open_stateid_other(state, dst) and the storage of
state->open_stateid.seqid in seqid_open are being performed with the
same instance of the full state->open_stateid.

> 
> > >  		break;
> > >  	}
> > >  }
> > > @@ -3367,14 +3368,16 @@ static bool
> > > nfs4_refresh_open_old_stateid(nfs4_stateid *dst,
> > >  			break;
> > >  		}
> > >  		seqid_open = state->open_stateid.seqid;
> > > -		if (read_seqretry(&state->seqlock, seq))
> > > -			continue;
> > >  
> > >  		dst_seqid = be32_to_cpu(dst->seqid);
> > > -		if ((s32)(dst_seqid - be32_to_cpu(seqid_open)) >= 0)
> > > +		if ((s32)(dst_seqid - be32_to_cpu(seqid_open)) > 0)
> > >  			dst->seqid = cpu_to_be32(dst_seqid + 1);
> > 
> > This negates the whole intention of the patch you reference in the
> > 'Fixes:', which was to allow us to CLOSE files even if seqid bumps
> > have
> > been lost due to interrupted RPC calls e.g. when using 'soft' or
> > 'softerr' mounts.
> > With the above change, the check could just be tossed out
> > altogether,
> > because dst_seqid will never become larger than seqid_open.
> 
> Hmm.. I got it wrong. Thanks for the explanation.

So to be clear: I'm not saying that what you describe is not a problem.
I'm just saying that the fix you propose is really no better than
reverting the entire patch. I'd prefer not to do that, and would rather
see us look for ways to fix both problems, but if we can't find such as
fix then that would be the better solution.

> 
> > >  		else
> > >  			dst->seqid = seqid_open;
> > > +
> > > +		if (read_seqretry(&state->seqlock, seq))
> > > +			continue;
> > > +
> > 
> > Again, why this change to code that doesn't appear to need
> > protection?
> > If the bump does succeed above, then looping back will actually
> > cause
> > unpredictable behaviour.
> 
> Same as above. This change have nothing to do with the stateid race.
> I
> just found it when reading the patch.
> 
> Thanks,
> M
> > >  		ret = true;
> > >  		break;
> > >  	}
-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com