* Re: [PATCH] NFSD: fix deny mode logic in nfs4_upgrade_open [not found] <20230202073611.13106-1-cc85nod@gmail.com> @ 2023-02-02 19:41 ` Chuck Lever III 2023-02-02 21:22 ` Jeff Layton 0 siblings, 1 reply; 7+ messages in thread From: Chuck Lever III @ 2023-02-02 19:41 UTC (permalink / raw) To: Jeff Layton, Dai Ngo Cc: Pumpkin, Linux NFS Mailing List, Linux Kernel Mailing List > On Feb 2, 2023, at 2:36 AM, Pumpkin <cc85nod@gmail.com> 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@gmail.com> > --- > 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. > set_deny(open->op_share_deny, stp); > fp->fi_share_deny |= > -- > 2.37.1 (Apple Git-137.1) > -- Chuck Lever ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] NFSD: fix deny mode logic in nfs4_upgrade_open 2023-02-02 19:41 ` [PATCH] NFSD: fix deny mode logic in nfs4_upgrade_open Chuck Lever III @ 2023-02-02 21:22 ` Jeff Layton 2023-02-02 23:38 ` dai.ngo ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Jeff Layton @ 2023-02-02 21:22 UTC (permalink / raw) To: Chuck Lever III, Dai Ngo Cc: Pumpkin, Linux NFS Mailing List, Linux Kernel Mailing List On Thu, 2023-02-02 at 19:41 +0000, Chuck Lever III wrote: > > > On Feb 2, 2023, at 2:36 AM, Pumpkin <cc85nod@gmail.com> 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@gmail.com> > > --- > > 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@kernel.org> ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] NFSD: fix deny mode logic in nfs4_upgrade_open 2023-02-02 21:22 ` Jeff Layton @ 2023-02-02 23:38 ` dai.ngo [not found] ` <CAAn9K_vykJofBJ6F8=7rmiJzXhESRcJ0DEnc+nDbwTHLX6BG0w@mail.gmail.com> 2023-02-03 14:50 ` Chuck Lever III 2 siblings, 0 replies; 7+ messages in thread From: dai.ngo @ 2023-02-02 23:38 UTC (permalink / raw) To: Jeff Layton, Chuck Lever III Cc: Pumpkin, Linux NFS Mailing List, Linux Kernel Mailing List On 2/2/23 1:22 PM, Jeff Layton wrote: > On Thu, 2023-02-02 at 19:41 +0000, Chuck Lever III wrote: >>> On Feb 2, 2023, at 2:36 AM, Pumpkin <cc85nod@gmail.com> 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@gmail.com> >>> --- >>> 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. not sure what I was thinking. The check was totally wrong. > > 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); LGTM. Thanks, -Dai ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <CAAn9K_vykJofBJ6F8=7rmiJzXhESRcJ0DEnc+nDbwTHLX6BG0w@mail.gmail.com>]
* Re: [PATCH] NFSD: fix deny mode logic in nfs4_upgrade_open [not found] ` <CAAn9K_vykJofBJ6F8=7rmiJzXhESRcJ0DEnc+nDbwTHLX6BG0w@mail.gmail.com> @ 2023-02-03 0:57 ` Chuck Lever III 0 siblings, 0 replies; 7+ messages in thread From: Chuck Lever III @ 2023-02-03 0:57 UTC (permalink / raw) To: 張智諺 Cc: Jeff Layton, Dai Ngo, Linux NFS Mailing List, Linux Kernel Mailing List > On Feb 2, 2023, at 7:50 PM, 張智諺 <cc85nod@gmail.com> wrote: > > Sorry, I thought the set deny mode operation was the error handling of nfserr_inval, and fixed it with the one-liner wrongly. Thanks your explanation. Do you happen to have a test case for this issue? Getting a Tested-by: would be great. > Jeff Layton <jlayton@kernel.org> 於 2023年2月3日 週五 上午5:22寫道： > On Thu, 2023-02-02 at 19:41 +0000, Chuck Lever III wrote: > > > > > On Feb 2, 2023, at 2:36 AM, Pumpkin <cc85nod@gmail.com> 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@gmail.com> > > > --- > > > 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@kernel.org> -- Chuck Lever ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] NFSD: fix deny mode logic in nfs4_upgrade_open 2023-02-02 21:22 ` Jeff Layton 2023-02-02 23:38 ` dai.ngo [not found] ` <CAAn9K_vykJofBJ6F8=7rmiJzXhESRcJ0DEnc+nDbwTHLX6BG0w@mail.gmail.com> @ 2023-02-03 14:50 ` Chuck Lever III 2023-02-03 17:57 ` dai.ngo 2023-02-03 18:10 ` Jeff Layton 2 siblings, 2 replies; 7+ messages in thread From: Chuck Lever III @ 2023-02-03 14:50 UTC (permalink / raw) To: Jeff Layton Cc: Dai Ngo, Pumpkin, Linux NFS Mailing List, Linux Kernel Mailing List > On Feb 2, 2023, at 4:22 PM, Jeff Layton <jlayton@kernel.org> wrote: > > On Thu, 2023-02-02 at 19:41 +0000, Chuck Lever III wrote: >> >>> On Feb 2, 2023, at 2:36 AM, Pumpkin <cc85nod@gmail.com> 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@gmail.com> >>> --- >>> 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); Would pynfs have a case or two that could test this? Can you post an official version of this patch with Reported-by and Fixes tags? -- Chuck Lever ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] NFSD: fix deny mode logic in nfs4_upgrade_open 2023-02-03 14:50 ` Chuck Lever III @ 2023-02-03 17:57 ` dai.ngo 2023-02-03 18:10 ` Jeff Layton 1 sibling, 0 replies; 7+ messages in thread From: dai.ngo @ 2023-02-03 17:57 UTC (permalink / raw) To: Chuck Lever III, Jeff Layton Cc: Pumpkin, Linux NFS Mailing List, Linux Kernel Mailing List On 2/3/23 6:50 AM, Chuck Lever III wrote: > >> On Feb 2, 2023, at 4:22 PM, Jeff Layton <jlayton@kernel.org> wrote: >> >> On Thu, 2023-02-02 at 19:41 +0000, Chuck Lever III wrote: >>>> On Feb 2, 2023, at 2:36 AM, Pumpkin <cc85nod@gmail.com> 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@gmail.com> >>>> --- >>>> 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); > Would pynfs have a case or two that could test this? pynfs has tests for open upgrade and downgrade but it only tests the upgrade/downgrade functionality without any deny mode and only from 1 client. We need a test that does open upgrade/downgrade with share deny mode and with 2 clients, one is courtesy client. I will look into creating one. -Dai > > Can you post an official version of this patch with Reported-by > and Fixes tags? > > > -- > Chuck Lever > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] NFSD: fix deny mode logic in nfs4_upgrade_open 2023-02-03 14:50 ` Chuck Lever III 2023-02-03 17:57 ` dai.ngo @ 2023-02-03 18:10 ` Jeff Layton 1 sibling, 0 replies; 7+ messages in thread From: Jeff Layton @ 2023-02-03 18:10 UTC (permalink / raw) To: Chuck Lever III Cc: Dai Ngo, Pumpkin, Linux NFS Mailing List, Linux Kernel Mailing List On Fri, 2023-02-03 at 14:50 +0000, Chuck Lever III wrote: > > > On Feb 2, 2023, at 4:22 PM, Jeff Layton <jlayton@kernel.org> wrote: > > > > On Thu, 2023-02-02 at 19:41 +0000, Chuck Lever III wrote: > > > > > > > On Feb 2, 2023, at 2:36 AM, Pumpkin <cc85nod@gmail.com> 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@gmail.com> > > > > --- > > > > 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); > > Would pynfs have a case or two that could test this? > > Can you post an official version of this patch with Reported-by > and Fixes tags? > > Sure, but I may not have time to test it out for a bit though. -- Jeff Layton <jlayton@kernel.org> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-02-03 18:11 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20230202073611.13106-1-cc85nod@gmail.com>
2023-02-02 19:41 ` [PATCH] NFSD: fix deny mode logic in nfs4_upgrade_open Chuck Lever III
2023-02-02 21:22 ` Jeff Layton
2023-02-02 23:38 ` dai.ngo
[not found] ` <CAAn9K_vykJofBJ6F8=7rmiJzXhESRcJ0DEnc+nDbwTHLX6BG0w@mail.gmail.com>
2023-02-03 0:57 ` Chuck Lever III
2023-02-03 14:50 ` Chuck Lever III
2023-02-03 17:57 ` dai.ngo
2023-02-03 18:10 ` Jeff Layton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox