On Thu, 2023-02-02 at 19:41 +0000, Chuck Lever III wrote:
>
> > On Feb 2, 2023, at 2:36 AM, Pumpkin <cc85nod@xxxxxxxxx> wrote:
> >
> > If the upgrading deny mode is invalid or conflicts with other client, we
> > should try to resolve it, but the if-condition makes those error handling
> > cannot be executed.
> >
> > Signed-off-by: Pumpkin <cc85nod@xxxxxxxxx>
> > ---
> > fs/nfsd/nfs4state.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 4ef529379..ebdfaf0f9 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -5298,7 +5298,7 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp,
> > /* test and set deny mode */
> > spin_lock(&fp->fi_lock);
> > status = nfs4_file_check_deny(fp, open->op_share_deny);
> > - if (status == nfs_ok) {
> > + if (status != nfs_ok) {
> > if (status != nfserr_share_denied) {
>
> if status == nfs_ok then status will definitely not equal
> share_denied. So this check is a bit nonsensical as it stands.
>
> Usually I prefer "switch (status)" in situations like this
> because that avoids this kind of issue and I find it easier
> to read quickly.
>
> Jeff, you are the original author of this function, and
> Dai, your commit is the last one to touch this area. Can
> you guys have a look? The one-liner looks correct, but I
> might be missing something.
>
Yeah, that code is clearly broken and it looks like it was done in
3d69427151806 (NFSD: add support for share reservation conflict to
courteous server).
I don't believe that one-liner is correct though. If the result is
nfs_ok, then we want to set the deny mode here and that won't happen.
Something like this maybe? (completely untested):
---------------8<-------------------
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c39e43742dd6..af22dfdc6fcc 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5282,16 +5282,17 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp,
/* test and set deny mode */
spin_lock(&fp->fi_lock);
status = nfs4_file_check_deny(fp, open->op_share_deny);
- if (status == nfs_ok) {
- if (status != nfserr_share_denied) {
- set_deny(open->op_share_deny, stp);
- fp->fi_share_deny |=
- (open->op_share_deny & NFS4_SHARE_DENY_BOTH);
- } else {
- if (nfs4_resolve_deny_conflicts_locked(fp, false,
- stp, open->op_share_deny, false))
- status = nfserr_jukebox;
- }
+ switch (status) {
+ case nfs_ok:
+ set_deny(open->op_share_deny, stp);
+ fp->fi_share_deny |=
+ (open->op_share_deny & NFS4_SHARE_DENY_BOTH);
+ break;
+ case nfserr_share_denied:
+ if (nfs4_resolve_deny_conflicts_locked(fp, false,
+ stp, open->op_share_deny, false))
+ status = nfserr_jukebox;
+ break;
}
spin_unlock(&fp->fi_lock);
--
Jeff Layton <jlayton@xxxxxxxxxx>
