Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 7 Jun 2023 10:25:50 -0400
From:   Chuck Lever <cel@kernel.org>
To:     Dai Ngo <dai.ngo@oracle.com>
Cc:     chuck.lever@oracle.com, jlayton@kernel.org,
        linux-nfs@vger.kernel.org
Subject: Re: [PATCH 1/1] NFSD: remove dead code in nfs4_open_delegation
Message-ID: <ZICTbiPEyk0OrlRS@manet.1015granger.net>
References: <1686094819-13044-1-git-send-email-dai.ngo@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1686094819-13044-1-git-send-email-dai.ngo@oracle.com>
Precedence: bulk

On Tue, Jun 06, 2023 at 04:40:19PM -0700, Dai Ngo wrote:
> The intention of this code is to tell the client to return the granted
> delegation immediately for whatever reason in the case of OPEN with
> NFS4_OPEN_CLAIM_PREVIOUS. However this was already handled above in the
> cases of switch(open->op_claim_type).
> 
> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> ---
>  fs/nfsd/nfs4state.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 6b75656d3e8f..58c78a23f90b 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5632,11 +5632,6 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  	return;
>  out_no_deleg:
>  	open->op_delegate_type = NFS4_OPEN_DELEGATE_NONE;
> -	if (open->op_claim_type == NFS4_OPEN_CLAIM_PREVIOUS &&
> -	    open->op_delegate_type != NFS4_OPEN_DELEGATE_NONE) {
> -		dprintk("NFSD: WARNING: refusing delegation reclaim\n");
> -		open->op_recall = 1;
> -	}

This is dead code because it's broken. Look carefully at it: it sets
open->op_delegate_type to NONE, then prints a warning if it isn't
NONE. The warning will never occur, and neither will the recall.

This code came from 99c415156c49 ("nfsd4: clean up
nfs4_open_delegation"). Can you have a look at that old commit and
make sure nfs4_open_delegation is working as it was originally
intended before it was cleaned up by that commit?

Even if you decide not to change the diff, the patch description
is confusing. out_no_deleg: can be invoked /after/ the switch
statement, so I'm not seeing how the OPEN/CLAIM_PREVIOUS case
is handled in that instance. The description also needs to at
least mention that the removed code never worked properly.


>  	/* 4.1 client asking for a delegation? */
>  	if (open->op_deleg_want)
> -- 
> 2.9.5
>