* Re: [PATCH] nfsd: fix courtesy client with deny mode handling in nfs4_upgrade_open
2023-02-03 18:18 [PATCH] nfsd: fix courtesy client with deny mode handling in nfs4_upgrade_open Jeff Layton
@ 2023-02-04 15:49 ` Chuck Lever III
2023-02-04 20:50 ` dai.ngo
2023-02-15 23:05 ` dai.ngo
2 siblings, 0 replies; 6+ messages in thread
From: Chuck Lever III @ 2023-02-04 15:49 UTC (permalink / raw)
To: Dai Ngo, Jeff Layton; +Cc: Linux NFS Mailing List, 張智諺
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] nfsd: fix courtesy client with deny mode handling in nfs4_upgrade_open
2023-02-03 18:18 [PATCH] nfsd: fix courtesy client with deny mode handling in nfs4_upgrade_open Jeff Layton
2023-02-04 15:49 ` Chuck Lever III
@ 2023-02-04 20:50 ` dai.ngo
2023-02-05 17:06 ` Chuck Lever III
2023-02-15 23:05 ` dai.ngo
2 siblings, 1 reply; 6+ messages in thread
From: dai.ngo @ 2023-02-04 20:50 UTC (permalink / raw)
To: Jeff Layton, chuck.lever; +Cc: linux-nfs, 張智諺
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>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] nfsd: fix courtesy client with deny mode handling in nfs4_upgrade_open
2023-02-04 20:50 ` dai.ngo
@ 2023-02-05 17:06 ` Chuck Lever III
0 siblings, 0 replies; 6+ messages in thread
From: Chuck Lever III @ 2023-02-05 17:06 UTC (permalink / raw)
To: Dai Ngo, Jeff Layton; +Cc: Linux NFS Mailing List, 張智諺
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nfsd: fix courtesy client with deny mode handling in nfs4_upgrade_open
2023-02-03 18:18 [PATCH] nfsd: fix courtesy client with deny mode handling in nfs4_upgrade_open Jeff Layton
2023-02-04 15:49 ` Chuck Lever III
2023-02-04 20:50 ` dai.ngo
@ 2023-02-15 23:05 ` dai.ngo
2023-02-16 7:24 ` dai.ngo
2 siblings, 1 reply; 6+ messages in thread
From: dai.ngo @ 2023-02-15 23:05 UTC (permalink / raw)
To: Jeff Layton, chuck.lever; +Cc: linux-nfs, 張智諺
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);
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] nfsd: fix courtesy client with deny mode handling in nfs4_upgrade_open
2023-02-15 23:05 ` dai.ngo
@ 2023-02-16 7:24 ` dai.ngo
0 siblings, 0 replies; 6+ messages in thread
From: dai.ngo @ 2023-02-16 7:24 UTC (permalink / raw)
To: Jeff Layton, chuck.lever; +Cc: linux-nfs, 張智諺
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);
^ permalink raw reply [flat|nested] 6+ messages in thread