| Message ID | 7ed2d8f1ee8c441a13b450c5e5c50f13fae3a2b9.1667114760.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSD: Fix the share reservation conflict to courteous server logic in nfs4_upgrade_open() | expand |
> 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. 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
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 > > >
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); 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);
'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(-)