| Message ID | 147623995856.19592.8461168656619949864.stgit@noble (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <neilb@suse.com> > --- > 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.
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 <neilb@suse.com> >> --- >> 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 13 2016, Anna Schumaker wrote: >> >> 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 :) Ok.... I note that git://git.linux-nfs.org/projects/anna/linux-nfs.git#linux-next isn't in git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git Should it be? NeilBrown
On Wed, Oct 12 2016, Jeff Layton wrote: >> - 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? I didn't think this was possible with the current API, but I had forgotten about (or never knew about) ATTR_FILE. setattr does have access to the file, so it can find all the nfs state info needed. And when I modify the patch series to make use of that, it removes some ugly bits :-) I'll repost after a little testing. Thanks, NeilBrown
On Thu, 2016-10-13 at 15:04 +1100, NeilBrown wrote: > On Thu, Oct 13 2016, Anna Schumaker wrote: > > > > > > > > > > > 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 :) > > Ok.... > I note that > git://git.linux-nfs.org/projects/anna/linux-nfs.git#linux-next > > isn't in > git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git > > Should it be? > > NeilBrown Yes, it seems like that should be going into -next. Anna, do you and Trond have something worked out where the patches in your pull requests are going into -next through Trond's tree?
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; 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;
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 <neilb@suse.com> --- fs/nfs/nfs4_fs.h | 2 +- fs/nfs/nfs4proc.c | 11 ++--------- fs/nfs/nfs4state.c | 19 +++++++++++-------- 3 files changed, 14 insertions(+), 18 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html