Messages in this thread | | | Date | Sun, 30 Oct 2022 09:32:50 -0700 | Subject | Re: [PATCH] NFSD: Fix the share reservation conflict to courteous server logic in nfs4_upgrade_open() | From | dai.ngo@oracle ... |
| |
On 10/30/22 8:26 AM, Chuck Lever III wrote: > >> On Oct 30, 2022, at 3:26 AM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> 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@wanadoo.fr> >> --- >> 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 > > >
| |