| Message ID | 1686094819-13044-1-git-send-email-dai.ngo@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] NFSD: remove dead code in nfs4_open_delegation | expand |
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 >
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 <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 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 >>
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)
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(-)