On 10/30/22 8:26 AM, Chuck Lever III wrote:
On Oct 30, 2022, at 3:26 AM, Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> wrote: 'status != nfserr_share_denied' is known to be true because we test 'status == nfs_ok' the line just above. So nfs4_resolve_deny_conflicts_locked() can never be called. Fix the logic and avoid the dead code. Fixes: 3d6942715180 ("NFSD: add support for share reservation conflict to courteous server") Signed-off-by: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> --- This patch is speculative. It is compile tested only. REVIEW WITH CARE. --- fs/nfsd/nfs4state.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 1ded89235111..de0565e9485c 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -5260,15 +5260,13 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp, spin_lock(&fp->fi_lock); status = nfs4_file_check_deny(fp, open->op_share_deny);I agree there's dead code here. I believe the bug is the first check is supposed to be "if (status != nfs_ok)". I will let Dai have a look at this to confirm.
Yes, it's actually a bug when nfs4_file_check_deny returns nfserr_share_denied we won't try to resolve the conflict at all. Thanks for catching this! -Dai
But, in the fix, let's replace this logic with "switch (status) { }".if (status == nfs_ok) { - if (status != nfserr_share_denied) { - set_deny(open->op_share_deny, stp); - fp->fi_share_deny |= + 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; - } + } else if (status == nfserr_share_denied) { + if (nfs4_resolve_deny_conflicts_locked(fp, false, stp, + open->op_share_deny, false)) + status = nfserr_jukebox; } spin_unlock(&fp->fi_lock); -- 2.34.1-- Chuck Lever
