Re: [PATCH] NFSD: fix deny mode logic in nfs4_upgrade_open

[dai.ngo@oracle.com]

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