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

[dai.ngo@oracle.com]

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);
>