| Message ID | 20230203181834.58634-1-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: fix courtesy client with deny mode handling in nfs4_upgrade_open | expand |
> On Feb 3, 2023, at 1:18 PM, Jeff Layton <jlayton@kernel.org> wrote: > > The nested if statements here make no sense, as you can never reach > "else" branch in the nested statement. Fix the error handling for > when there is a courtesy client that holds a conflicting deny mode. > > Fixes: 3d69427151806 (NFSD: add support for share reservation conflict to courteous server) > Reported-by: 張智諺 <cc85nod@gmail.com> > Cc: Dai Ngo <dai.ngo@oracle.com> > Signed-off-by: Jeff Layton <jlayton@kernel.org> Jeff, perfecto. Thank you! Dai, reply-all with your Reviewed-by and my tooling will pick that up automatically when I apply this to the nfsd tree. Thanks! Anyone else is also welcome to send a R-b or T-b. > --- > fs/nfsd/nfs4state.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) > > 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); > > -- > 2.39.1 > -- Chuck Lever
On 2/3/23 10:18 AM, Jeff Layton wrote: > The nested if statements here make no sense, as you can never reach > "else" branch in the nested statement. Fix the error handling for > when there is a courtesy client that holds a conflicting deny mode. > > Fixes: 3d69427151806 (NFSD: add support for share reservation conflict to courteous server) > Reported-by: 張智諺 <cc85nod@gmail.com> > Cc: Dai Ngo <dai.ngo@oracle.com> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/nfs4state.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) > > 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); Reviewed-by: Dai Ngo <dai.ngo@oracle.com> >
> On Feb 4, 2023, at 3:50 PM, Dai Ngo <dai.ngo@oracle.com> wrote: > > > On 2/3/23 10:18 AM, Jeff Layton wrote: >> The nested if statements here make no sense, as you can never reach >> "else" branch in the nested statement. Fix the error handling for >> when there is a courtesy client that holds a conflicting deny mode. >> >> Fixes: 3d69427151806 (NFSD: add support for share reservation conflict to courteous server) >> Reported-by: 張智諺 <cc85nod@gmail.com> >> Cc: Dai Ngo <dai.ngo@oracle.com> >> Signed-off-by: Jeff Layton <jlayton@kernel.org> >> --- >> fs/nfsd/nfs4state.c | 21 +++++++++++---------- >> 1 file changed, 11 insertions(+), 10 deletions(-) >> >> 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); > > Reviewed-by: Dai Ngo <dai.ngo@oracle.com> Thanks! Applied to nfsd-next. -- Chuck Lever
Hi Jeff, Chuck, On 2/3/23 10:18 AM, Jeff Layton wrote: > The nested if statements here make no sense, as you can never reach > "else" branch in the nested statement. Fix the error handling for > when there is a courtesy client that holds a conflicting deny mode. > > Fixes: 3d69427151806 (NFSD: add support for share reservation conflict to courteous server) > Reported-by: 張智諺 <cc85nod@gmail.com> > Cc: Dai Ngo <dai.ngo@oracle.com> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/nfs4state.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) > > 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)) While trying to write a pynfs test case to exercise this code path, I realize that we don't need to call nfs4_resolve_deny_conflicts_locked here since this is an open upgrade so it must comes from the same client hence there is no conflict to resolve. Same behavior as OPEN_DOWNGRADE. -Dai > + status = nfserr_jukebox; > + break; > } > spin_unlock(&fp->fi_lock); >
On 2/15/23 3:05 PM, dai.ngo@oracle.com wrote: > Hi Jeff, Chuck, > > On 2/3/23 10:18 AM, Jeff Layton wrote: >> The nested if statements here make no sense, as you can never reach >> "else" branch in the nested statement. Fix the error handling for >> when there is a courtesy client that holds a conflicting deny mode. >> >> Fixes: 3d69427151806 (NFSD: add support for share reservation >> conflict to courteous server) >> Reported-by: 張智諺 <cc85nod@gmail.com> >> Cc: Dai Ngo <dai.ngo@oracle.com> >> Signed-off-by: Jeff Layton <jlayton@kernel.org> >> --- >> fs/nfsd/nfs4state.c | 21 +++++++++++---------- >> 1 file changed, 11 insertions(+), 10 deletions(-) >> >> 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)) > > While trying to write a pynfs test case to exercise this code path, > I realize that we don't need to call nfs4_resolve_deny_conflicts_locked > here since this is an open upgrade so it must comes from the same client > hence there is no conflict to resolve. Same behavior as OPEN_DOWNGRADE. never mind, I found the scenario where this code path is executed: Client1 opens fileX with WRITE, DENY_NONE Client2 opens fileX with READ, DENY_NONE Client2 opens fileX with READ, DENY_WRITE -Dai > > -Dai > >> + status = nfserr_jukebox; >> + break; >> } >> spin_unlock(&fp->fi_lock);
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);
The nested if statements here make no sense, as you can never reach "else" branch in the nested statement. Fix the error handling for when there is a courtesy client that holds a conflicting deny mode. Fixes: 3d69427151806 (NFSD: add support for share reservation conflict to courteous server) Reported-by: 張智諺 <cc85nod@gmail.com> Cc: Dai Ngo <dai.ngo@oracle.com> Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/nfs4state.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)