2023-02-03 18:18:44

by Jeff Layton

Subject: [PATCH] nfsd: fix courtesy client with deny mode handling in nfs4_upgrade_open

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: 張智諺 <[email protected]>
Cc: Dai Ngo <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
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

2023-02-04 15:49:46

by Chuck Lever III

Subject: Re: [PATCH] nfsd: fix courtesy client with deny mode handling in nfs4_upgrade_open

> On Feb 3, 2023, at 1:18 PM, Jeff Layton <[email protected]> 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: 張智諺 <[email protected]>
> Cc: Dai Ngo <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>

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

2023-02-04 20:50:55

by Dai Ngo

Subject: Re: [PATCH] nfsd: fix courtesy client with deny mode handling in nfs4_upgrade_open

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: 張智諺 <[email protected]>
> Cc: Dai Ngo <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> 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 <[email protected]>

>

2023-02-05 17:06:44

by Chuck Lever III

Subject: Re: [PATCH] nfsd: fix courtesy client with deny mode handling in nfs4_upgrade_open

> On Feb 4, 2023, at 3:50 PM, Dai Ngo <[email protected]> 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: 張智諺 <[email protected]>
>> Cc: Dai Ngo <[email protected]>
>> Signed-off-by: Jeff Layton <[email protected]>
>> ---
>> 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 <[email protected]>

Thanks! Applied to nfsd-next.

--
Chuck Lever

2023-02-15 23:05:26

by Dai Ngo

Subject: Re: [PATCH] nfsd: fix courtesy client with deny mode handling in nfs4_upgrade_open

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: 張智諺 <[email protected]>
> Cc: Dai Ngo <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> 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);
>

2023-02-16 07:24:22

by Dai Ngo

Subject: Re: [PATCH] nfsd: fix courtesy client with deny mode handling in nfs4_upgrade_open

On 2/15/23 3:05 PM, [email protected] 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: 張智諺 <[email protected]>
>> Cc: Dai Ngo <[email protected]>
>> Signed-off-by: Jeff Layton <[email protected]>
>> ---
>>   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);