Return-Path: Subject: Re: [PATCH 3/5] NFSv4: change nfs4_select_rw_stateid to take a lock_context inplace of lock_owner To: Jeff Layton , NeilBrown , Trond Myklebust References: <147623977637.19592.12766016823334433969.stgit@noble> <147623995856.19592.8461168656619949864.stgit@noble> <1476275602.2541.20.camel@redhat.com> CC: Benjamin Coddington , Linux NFS Mailing List From: Anna Schumaker Message-ID: <3006e49d-0e09-96c1-74c6-9ffe789ce0e5@Netapp.com> Date: Wed, 12 Oct 2016 09:48:35 -0400 MIME-Version: 1.0 In-Reply-To: <1476275602.2541.20.camel@redhat.com> Content-Type: text/plain; charset="utf-8" List-ID: On 10/12/2016 08:33 AM, Jeff Layton wrote: > On Wed, 2016-10-12 at 13:39 +1100, NeilBrown wrote: >> The only time that a lock_context is not available is in setattr. >> In this case, we want to find a lock context relevant to the process if there is one. >> The fallback can easily be handled at a lower level. >> >> So change nfs4_select_rw_stateid to take a lock_context, passing NULL from _nfs4_do_setattr. >> nfs4_copy_lock_state() also now takes a lock_context, and falls back to searching >> for "owner == current->files" if not lock_context is given. >> >> Note that nfs4_set_rw_stateid is *always* passed a non-NULL l_ctx, so the >> fact that we remove the NULL test there does not change correctness. >> >> This change is preparation for correctly support flock stateids. >> >> Signed-off-by: NeilBrown >> --- >> fs/nfs/nfs4_fs.h | 2 +- >> fs/nfs/nfs4proc.c | 11 ++--------- >> fs/nfs/nfs4state.c | 19 +++++++++++-------- >> 3 files changed, 14 insertions(+), 18 deletions(-) >> >> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h >> index 9bf64eacba5b..3f0e459f2499 100644 >> --- a/fs/nfs/nfs4_fs.h >> +++ b/fs/nfs/nfs4_fs.h >> @@ -445,7 +445,7 @@ extern void nfs41_handle_server_scope(struct nfs_client *, >> extern void nfs4_put_lock_state(struct nfs4_lock_state *lsp); >> extern int nfs4_set_lock_state(struct nfs4_state *state, struct file_lock *fl); >> extern int nfs4_select_rw_stateid(struct nfs4_state *, fmode_t, >> - const struct nfs_lockowner *, nfs4_stateid *, >> + const struct nfs_lock_context *, nfs4_stateid *, >> struct rpc_cred **); >> >> extern struct nfs_seqid *nfs_alloc_seqid(struct nfs_seqid_counter *counter, gfp_t gfp_mask); >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >> index 422ed5ac4efe..6820c44aebe4 100644 >> --- a/fs/nfs/nfs4proc.c >> +++ b/fs/nfs/nfs4proc.c >> @@ -2764,12 +2764,9 @@ static int _nfs4_do_setattr(struct inode *inode, >> if (nfs4_copy_delegation_stateid(inode, fmode, &arg->stateid, &delegation_cred)) { >> /* Use that stateid */ >> } else if (truncate && state != NULL) { >> - struct nfs_lockowner lockowner = { >> - .l_owner = current->files, >> - }; >> if (!nfs4_valid_open_stateid(state)) >> return -EBADF; >> - if (nfs4_select_rw_stateid(state, FMODE_WRITE, &lockowner, >> + if (nfs4_select_rw_stateid(state, FMODE_WRITE, NULL, >> &arg->stateid, &delegation_cred) == -EIO) >> return -EBADF; >> } else >> @@ -4362,11 +4359,7 @@ int nfs4_set_rw_stateid(nfs4_stateid *stateid, >> const struct nfs_lock_context *l_ctx, >> fmode_t fmode) >> { >> - const struct nfs_lockowner *lockowner = NULL; >> - >> - if (l_ctx != NULL) >> - lockowner = &l_ctx->lockowner; >> - return nfs4_select_rw_stateid(ctx->state, fmode, lockowner, stateid, NULL); >> + return nfs4_select_rw_stateid(ctx->state, fmode, l_ctx, stateid, NULL); >> } >> EXPORT_SYMBOL_GPL(nfs4_set_rw_stateid); >> >> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c >> index cada00aa5096..94a6631e7938 100644 >> --- a/fs/nfs/nfs4state.c >> +++ b/fs/nfs/nfs4state.c >> @@ -939,20 +939,23 @@ int nfs4_set_lock_state(struct nfs4_state *state, struct file_lock *fl) >> >> static int nfs4_copy_lock_stateid(nfs4_stateid *dst, >> struct nfs4_state *state, >> - const struct nfs_lockowner *lockowner) >> + const struct nfs_lock_context *l_ctx) >> { >> + /* >> + * If l_ctx is NULL, then this request comes from setattr >> + * and we can choose a lock context relevant for the current process >> + */ >> struct nfs4_lock_state *lsp; >> fl_owner_t fl_owner; >> int ret = -ENOENT; >> >> - >> - if (lockowner == NULL) >> - goto out; >> - >> if (test_bit(LK_STATE_IN_USE, &state->flags) == 0) >> goto out; >> >> - fl_owner = lockowner->l_owner; >> + if (l_ctx == NULL) >> + fl_owner = current->files; >> + else >> + fl_owner = l_ctx->lockowner.l_owner; > > > This I'm less sure of. Suppose we have only a flock lock on a file and > then truncate it. This is going to miss finding that stateid, no? > > I wonder if we need to look harder at the state to determine which owner to use in this case? > > >> spin_lock(&state->state_lock); >> lsp = __nfs4_find_lock_state(state, fl_owner); >> if (lsp && test_bit(NFS_LOCK_LOST, &lsp->ls_flags)) >> @@ -986,14 +989,14 @@ static void nfs4_copy_open_stateid(nfs4_stateid *dst, struct nfs4_state *state) >> * requests. >> */ >> int nfs4_select_rw_stateid(struct nfs4_state *state, >> - fmode_t fmode, const struct nfs_lockowner *lockowner, >> + fmode_t fmode, const struct nfs_lock_context *l_ctx, >> nfs4_stateid *dst, struct rpc_cred **cred) >> { >> int ret; >> >> if (cred != NULL) >> *cred = NULL; >> - ret = nfs4_copy_lock_stateid(dst, state, lockowner); >> + ret = nfs4_copy_lock_stateid(dst, state, l_ctx); >> if (ret == -EIO) >> /* A lost lock - don't even consider delegations */ >> goto out; >> >> > > Also, as a side note: There were some changes around the NFS file > locking code that went into the CB_NOTIFY_LOCK patches. Those are in > linux-next now, and I _think_ Anna is going to merge them for v4.9. I > don't see any obvious conflicts here, but you may want to ensure that > you're basing this on top of linux-next to minimize them. Yep, they'll be in 4.9. Updating against those changes wouldn't hurt :) Anna >