On 04/28/2016 10:06 AM, Chuck Lever wrote: > >> On Apr 28, 2016, at 9:13 AM, Anna Schumaker <Anna.Schumaker@xxxxxxxxxx> wrote: >> >> The patch looks pretty straightforward to me, and it sounds like it fixes a few problems that people are seeing. One question (below): >> >> On 04/28/2016 08:43 AM, William Dauchy wrote: >>> Hello Anna, >>> >>> Could you have a look at this one please? >>> >>> Thanks, >>> >>> William >>> >>> On Tue, Apr 26, 2016 at 4:24 PM, Olga Kornievskaia <aglo@xxxxxxxxx> wrote: >>>> I believe this patch also helps with a race between a LOCK and >>>> CB_RECALL. Application does a lock as the delegation is being >>>> recalled. The lock thread sees the delegated state and acquires a >>>> local lock. At the same time delegation doesn't see it the lock yet >>>> and returns the delegation. Application proceeds to do IO. It ends up >>>> using an open stateid for the IO (as there is no delegation stateid or >>>> lock stateid). The server is unaware of the lock so it can give that >>>> lock to somebody else. Yet this client thinks it has a local lock. It >>>> leads to inconsistent data between clients. >>>> >>>> On Mon, Apr 11, 2016 at 4:20 PM, Chuck Lever <chuck.lever@xxxxxxxxxx> wrote: >>>>> At Connectathon 2016, we found that recent upstream Linux clients >>>>> would occasionally send a LOCK operation with a zero stateid. This >>>>> appeared to happen in close proximity to another thread returning >>>>> a delegation before unlinking the same file while it remained open. >>>>> >>>>> Earlier, the client received a write delegation on this file and >>>>> returned the open stateid. Now, as it is getting ready to unlink the >>>>> file, it returns the write delegation. But there is still an open >>>>> file descriptor on that file, so the client must OPEN the file >>>>> again before it returns the delegation. >>>>> >>>>> Since commit 24311f884189 ('NFSv4: Recovery of recalled read >>>>> delegations is broken'), nfs_open_delegation_recall() clears the >>>>> NFS_DELEGATED_STATE flag _before_ it sends the OPEN. This allows a >>>>> racing LOCK on the same inode to be put on the wire before the OPEN >>>>> operation has returned a valid open stateid. >>>>> >>>>> To eliminate this race, serialize delegation return with the >>>>> acquisition of a file lock on the same file. Adopt the same approach >>>>> as is used in the unlock path. >>>>> >>>>> Fixes: 24311f884189 ('NFSv4: Recovery of recalled read ... ') >>>>> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx> >>>>> --- >>>>> Hi- >>>>> >>>>> This fix appears to be both safe and effective. Please consider >>>>> it for v4.7 and for stable. Thanks! >>>>> >>>>> >>>>> fs/nfs/nfs4proc.c | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >>>>> index 01bef06..c40f1b6 100644 >>>>> --- a/fs/nfs/nfs4proc.c >>>>> +++ b/fs/nfs/nfs4proc.c >>>>> @@ -6054,6 +6054,7 @@ static int nfs41_lock_expired(struct nfs4_state *state, struct file_lock *reques >>>>> static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock *request) >>>>> { >>>>> struct nfs_inode *nfsi = NFS_I(state->inode); >>>>> + struct nfs4_state_owner *sp = state->owner; >>>>> unsigned char fl_flags = request->fl_flags; >>>>> int status = -ENOLCK; >>>>> >>>>> @@ -6068,6 +6069,7 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock >>>>> status = do_vfs_lock(state->inode, request); >>>>> if (status < 0) >>>>> goto out; >>>>> + mutex_lock(&sp->so_delegreturn_mutex); >> >> From what I can tell, the first call to do_vfs_lock() in this function is used to test if we can take the lock locally. Do we need to worry about this racing with delegreturn, too? > > When I included that call in the critical section, > cthon04 locking tests deadlocked. Got it. Thanks for checking! Anna > > >> Thanks, >> Anna >> >>>>> down_read(&nfsi->rwsem); >>>>> if (test_bit(NFS_DELEGATED_STATE, &state->flags)) { >>>>> /* Yes: cache locks! */ >>>>> @@ -6075,9 +6077,11 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock >>>>> request->fl_flags = fl_flags & ~FL_SLEEP; >>>>> status = do_vfs_lock(state->inode, request); >>>>> up_read(&nfsi->rwsem); >>>>> + mutex_unlock(&sp->so_delegreturn_mutex); >>>>> goto out; >>>>> } >>>>> up_read(&nfsi->rwsem); >>>>> + mutex_unlock(&sp->so_delegreturn_mutex); >>>>> status = _nfs4_do_setlk(state, cmd, request, NFS_LOCK_NEW); >>>>> out: >>>>> request->fl_flags = fl_flags; > > -- > Chuck Lever > > > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html