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 <[email protected]>
---
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;
- }
/* 4.1 client asking for a delegation? */
if (open->op_deleg_want)
--
2.9.5
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 <[email protected]>
> ---
> 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
>
On 6/7/23 7:25 AM, Chuck Lever wrote:
> 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 <[email protected]>
>> ---
>> 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 is why the name of the patch is 'removing dead code'. I can be
more explicit and add the description you have here.
>
> 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?
I don't see any functional differences before and after 99c415156c49
other than explicitly denying write delegation by 99c415156c49.
>
> 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.
In the case of OPEN/CLAIM_PREVIOUS, if the back channel is not up then
op_recall is set to 1 and it continues on to call nfs4_set_delegation
to see if the delegation can be granted. If the delegation is granted
then the result of the OPEN is delegation granted but op_recall is set
to true, causing the client to return the delegation immediately.
> The description also needs to at
> least mention that the removed code never worked properly.
I will describe why it's dead code as mentioned above.
-Dai
>
>
>> /* 4.1 client asking for a delegation? */
>> if (open->op_deleg_want)
>> --
>> 2.9.5
>>