| Message ID | 1647051215-2873-6-git-send-email-dai.ngo@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSD: Initial implementation of NFSv4 Courteous Server | expand |
On Fri, Mar 11, 2022 at 06:13:29PM -0800, Dai Ngo wrote: > Update nfs4_get_vfs_file and nfs4_upgrade_open to handle share > reservation conflict with courtesy client. > > If share/access check fails with share denied then check if the > the conflict was caused by courtesy clients. If that's the case > then set CLIENT_EXPIRED flag to expire the courtesy clients and > allow nfs4_get_vfs_file to continue. Client with CLIENT_EXPIRED > is expired by the laundromat. I'm not following this code. I'll see if I can give it another shot tomorrow, but are you sure it can't be simplified somehow? Keep in mind, we really don't care about share conflicts much. I'm not sure whether anyone actually uses open deny modes, so there's no need to optimize that case. I'd be totally fine with expiring things unnecessarily in the deny mode case, for example. --b. > Signed-off-by: Dai Ngo <dai.ngo@oracle.com> > --- > fs/nfsd/nfs4state.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 91 insertions(+), 8 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 2beb0972de88..b16f689f34c3 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -4973,9 +4973,75 @@ nfsd4_truncate(struct svc_rqst *rqstp, struct svc_fh *fh, > return nfsd_setattr(rqstp, fh, &iattr, 0, (time64_t)0); > } > > +static bool > +nfs4_check_access_deny_bmap(struct nfs4_ol_stateid *stp, u32 access, > + bool share_access) > +{ > + if (share_access) { > + if (!stp->st_deny_bmap) > + return false; > + > + if ((stp->st_deny_bmap & (1 << NFS4_SHARE_DENY_BOTH)) || > + (access & NFS4_SHARE_ACCESS_READ && > + stp->st_deny_bmap & (1 << NFS4_SHARE_DENY_READ)) || > + (access & NFS4_SHARE_ACCESS_WRITE && > + stp->st_deny_bmap & (1 << NFS4_SHARE_DENY_WRITE))) { > + return true; > + } > + return false; > + } > + if ((access & NFS4_SHARE_DENY_BOTH) || > + (access & NFS4_SHARE_DENY_READ && > + stp->st_access_bmap & (1 << NFS4_SHARE_ACCESS_READ)) || > + (access & NFS4_SHARE_DENY_WRITE && > + stp->st_access_bmap & (1 << NFS4_SHARE_ACCESS_WRITE))) { > + return true; > + } > + return false; > +} > + > +/* > + * Check whether courtesy clients have conflicting access > + * > + * access: is op_share_access if share_access is true. > + * Check if access mode, op_share_access, would conflict with > + * the current deny mode of the file 'fp'. > + * access: is op_share_deny if share_access is false. > + * Check if the deny mode, op_share_deny, would conflict with > + * current access of the file 'fp'. > + * stp: skip checking this entry. > + * new_stp: normal open, not open upgrade. > + * > + * Function returns: > + * true - access/deny mode conflict with normal client. > + * false - no conflict or conflict with courtesy client(s) is resolved. > + */ > +static bool > +nfs4_resolve_deny_conflicts_locked(struct nfs4_file *fp, bool new_stp, > + struct nfs4_ol_stateid *stp, u32 access, bool share_access) > +{ > + struct nfs4_ol_stateid *st; > + struct nfs4_client *clp; > + bool conflict = false; > + > + lockdep_assert_held(&fp->fi_lock); > + list_for_each_entry(st, &fp->fi_stateids, st_perfile) { > + if (st->st_openstp || (st == stp && new_stp) || > + (!nfs4_check_access_deny_bmap(st, > + access, share_access))) > + continue; > + clp = st->st_stid.sc_client; > + if (nfs4_check_and_expire_courtesy_client(clp)) > + continue; > + conflict = true; > + break; > + } > + return conflict; > +} > + > static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp, > struct svc_fh *cur_fh, struct nfs4_ol_stateid *stp, > - struct nfsd4_open *open) > + struct nfsd4_open *open, bool new_stp) > { > struct nfsd_file *nf = NULL; > __be32 status; > @@ -4991,15 +5057,29 @@ static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp, > */ > status = nfs4_file_check_deny(fp, open->op_share_deny); > if (status != nfs_ok) { > - spin_unlock(&fp->fi_lock); > - goto out; > + if (status != nfserr_share_denied) { > + spin_unlock(&fp->fi_lock); > + goto out; > + } > + if (nfs4_resolve_deny_conflicts_locked(fp, new_stp, > + stp, open->op_share_deny, false)) { > + spin_unlock(&fp->fi_lock); > + goto out; > + } > } > > /* set access to the file */ > status = nfs4_file_get_access(fp, open->op_share_access); > if (status != nfs_ok) { > - spin_unlock(&fp->fi_lock); > - goto out; > + if (status != nfserr_share_denied) { > + spin_unlock(&fp->fi_lock); > + goto out; > + } > + if (nfs4_resolve_deny_conflicts_locked(fp, new_stp, > + stp, open->op_share_access, true)) { > + spin_unlock(&fp->fi_lock); > + goto out; > + } > } > > /* Set access bits in stateid */ > @@ -5050,7 +5130,7 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp, struct svc_fh *c > unsigned char old_deny_bmap = stp->st_deny_bmap; > > if (!test_access(open->op_share_access, stp)) > - return nfs4_get_vfs_file(rqstp, fp, cur_fh, stp, open); > + return nfs4_get_vfs_file(rqstp, fp, cur_fh, stp, open, false); > > /* test and set deny mode */ > spin_lock(&fp->fi_lock); > @@ -5059,7 +5139,10 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp, struct svc_fh *c > set_deny(open->op_share_deny, stp); > fp->fi_share_deny |= > (open->op_share_deny & NFS4_SHARE_DENY_BOTH); > - } > + } else if (status == nfserr_share_denied && > + !nfs4_resolve_deny_conflicts_locked(fp, false, stp, > + open->op_share_deny, false)) > + status = nfs_ok; > spin_unlock(&fp->fi_lock); > > if (status != nfs_ok) > @@ -5399,7 +5482,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf > goto out; > } > } else { > - status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp, open); > + status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp, open, true); > if (status) { > stp->st_stid.sc_type = NFS4_CLOSED_STID; > release_open_stateid(stp); > -- > 2.9.5
On 3/15/22 8:39 AM, J. Bruce Fields wrote: > On Fri, Mar 11, 2022 at 06:13:29PM -0800, Dai Ngo wrote: >> Update nfs4_get_vfs_file and nfs4_upgrade_open to handle share >> reservation conflict with courtesy client. >> >> If share/access check fails with share denied then check if the >> the conflict was caused by courtesy clients. If that's the case >> then set CLIENT_EXPIRED flag to expire the courtesy clients and >> allow nfs4_get_vfs_file to continue. Client with CLIENT_EXPIRED >> is expired by the laundromat. > I'm not following this code. I'll see if I can give it another shot > tomorrow, but are you sure it can't be simplified somehow? When we have deny/access conflict we walk the fi_stateids of the file in question, looking for open stateid and check the deny/access of that stateid against the one from the open request. If there is a conflict then we check if the client that owns that stateid is a courtesy client. If it is then we set the client state to CLIENT_EXPIRED and allow the open request to continue. We have to scan all the stateid's of the file since the conflict can be caused by multiple open stateid's. > > Keep in mind, we really don't care about share conflicts much. I'm not > sure whether anyone actually uses open deny modes, so there's no need to > optimize that case. Yes, but we have to pass pynfs 4.0 tests. > I'd be totally fine with expiring things > unnecessarily in the deny mode case, for example. I have not been able to come up with a simpler solution. -Dai > > --b. > >> Signed-off-by: Dai Ngo <dai.ngo@oracle.com> >> --- >> fs/nfsd/nfs4state.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++----- >> 1 file changed, 91 insertions(+), 8 deletions(-) >> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >> index 2beb0972de88..b16f689f34c3 100644 >> --- a/fs/nfsd/nfs4state.c >> +++ b/fs/nfsd/nfs4state.c >> @@ -4973,9 +4973,75 @@ nfsd4_truncate(struct svc_rqst *rqstp, struct svc_fh *fh, >> return nfsd_setattr(rqstp, fh, &iattr, 0, (time64_t)0); >> } >> >> +static bool >> +nfs4_check_access_deny_bmap(struct nfs4_ol_stateid *stp, u32 access, >> + bool share_access) >> +{ >> + if (share_access) { >> + if (!stp->st_deny_bmap) >> + return false; >> + >> + if ((stp->st_deny_bmap & (1 << NFS4_SHARE_DENY_BOTH)) || >> + (access & NFS4_SHARE_ACCESS_READ && >> + stp->st_deny_bmap & (1 << NFS4_SHARE_DENY_READ)) || >> + (access & NFS4_SHARE_ACCESS_WRITE && >> + stp->st_deny_bmap & (1 << NFS4_SHARE_DENY_WRITE))) { >> + return true; >> + } >> + return false; >> + } >> + if ((access & NFS4_SHARE_DENY_BOTH) || >> + (access & NFS4_SHARE_DENY_READ && >> + stp->st_access_bmap & (1 << NFS4_SHARE_ACCESS_READ)) || >> + (access & NFS4_SHARE_DENY_WRITE && >> + stp->st_access_bmap & (1 << NFS4_SHARE_ACCESS_WRITE))) { >> + return true; >> + } >> + return false; >> +} >> + >> +/* >> + * Check whether courtesy clients have conflicting access >> + * >> + * access: is op_share_access if share_access is true. >> + * Check if access mode, op_share_access, would conflict with >> + * the current deny mode of the file 'fp'. >> + * access: is op_share_deny if share_access is false. >> + * Check if the deny mode, op_share_deny, would conflict with >> + * current access of the file 'fp'. >> + * stp: skip checking this entry. >> + * new_stp: normal open, not open upgrade. >> + * >> + * Function returns: >> + * true - access/deny mode conflict with normal client. >> + * false - no conflict or conflict with courtesy client(s) is resolved. >> + */ >> +static bool >> +nfs4_resolve_deny_conflicts_locked(struct nfs4_file *fp, bool new_stp, >> + struct nfs4_ol_stateid *stp, u32 access, bool share_access) >> +{ >> + struct nfs4_ol_stateid *st; >> + struct nfs4_client *clp; >> + bool conflict = false; >> + >> + lockdep_assert_held(&fp->fi_lock); >> + list_for_each_entry(st, &fp->fi_stateids, st_perfile) { >> + if (st->st_openstp || (st == stp && new_stp) || >> + (!nfs4_check_access_deny_bmap(st, >> + access, share_access))) >> + continue; >> + clp = st->st_stid.sc_client; >> + if (nfs4_check_and_expire_courtesy_client(clp)) >> + continue; >> + conflict = true; >> + break; >> + } >> + return conflict; >> +} >> + >> static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp, >> struct svc_fh *cur_fh, struct nfs4_ol_stateid *stp, >> - struct nfsd4_open *open) >> + struct nfsd4_open *open, bool new_stp) >> { >> struct nfsd_file *nf = NULL; >> __be32 status; >> @@ -4991,15 +5057,29 @@ static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp, >> */ >> status = nfs4_file_check_deny(fp, open->op_share_deny); >> if (status != nfs_ok) { >> - spin_unlock(&fp->fi_lock); >> - goto out; >> + if (status != nfserr_share_denied) { >> + spin_unlock(&fp->fi_lock); >> + goto out; >> + } >> + if (nfs4_resolve_deny_conflicts_locked(fp, new_stp, >> + stp, open->op_share_deny, false)) { >> + spin_unlock(&fp->fi_lock); >> + goto out; >> + } >> } >> >> /* set access to the file */ >> status = nfs4_file_get_access(fp, open->op_share_access); >> if (status != nfs_ok) { >> - spin_unlock(&fp->fi_lock); >> - goto out; >> + if (status != nfserr_share_denied) { >> + spin_unlock(&fp->fi_lock); >> + goto out; >> + } >> + if (nfs4_resolve_deny_conflicts_locked(fp, new_stp, >> + stp, open->op_share_access, true)) { >> + spin_unlock(&fp->fi_lock); >> + goto out; >> + } >> } >> >> /* Set access bits in stateid */ >> @@ -5050,7 +5130,7 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp, struct svc_fh *c >> unsigned char old_deny_bmap = stp->st_deny_bmap; >> >> if (!test_access(open->op_share_access, stp)) >> - return nfs4_get_vfs_file(rqstp, fp, cur_fh, stp, open); >> + return nfs4_get_vfs_file(rqstp, fp, cur_fh, stp, open, false); >> >> /* test and set deny mode */ >> spin_lock(&fp->fi_lock); >> @@ -5059,7 +5139,10 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp, struct svc_fh *c >> set_deny(open->op_share_deny, stp); >> fp->fi_share_deny |= >> (open->op_share_deny & NFS4_SHARE_DENY_BOTH); >> - } >> + } else if (status == nfserr_share_denied && >> + !nfs4_resolve_deny_conflicts_locked(fp, false, stp, >> + open->op_share_deny, false)) >> + status = nfs_ok; >> spin_unlock(&fp->fi_lock); >> >> if (status != nfs_ok) >> @@ -5399,7 +5482,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf >> goto out; >> } >> } else { >> - status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp, open); >> + status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp, open, true); >> if (status) { >> stp->st_stid.sc_type = NFS4_CLOSED_STID; >> release_open_stateid(stp); >> -- >> 2.9.5
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 2beb0972de88..b16f689f34c3 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -4973,9 +4973,75 @@ nfsd4_truncate(struct svc_rqst *rqstp, struct svc_fh *fh, return nfsd_setattr(rqstp, fh, &iattr, 0, (time64_t)0); } +static bool +nfs4_check_access_deny_bmap(struct nfs4_ol_stateid *stp, u32 access, + bool share_access) +{ + if (share_access) { + if (!stp->st_deny_bmap) + return false; + + if ((stp->st_deny_bmap & (1 << NFS4_SHARE_DENY_BOTH)) || + (access & NFS4_SHARE_ACCESS_READ && + stp->st_deny_bmap & (1 << NFS4_SHARE_DENY_READ)) || + (access & NFS4_SHARE_ACCESS_WRITE && + stp->st_deny_bmap & (1 << NFS4_SHARE_DENY_WRITE))) { + return true; + } + return false; + } + if ((access & NFS4_SHARE_DENY_BOTH) || + (access & NFS4_SHARE_DENY_READ && + stp->st_access_bmap & (1 << NFS4_SHARE_ACCESS_READ)) || + (access & NFS4_SHARE_DENY_WRITE && + stp->st_access_bmap & (1 << NFS4_SHARE_ACCESS_WRITE))) { + return true; + } + return false; +} + +/* + * Check whether courtesy clients have conflicting access + * + * access: is op_share_access if share_access is true. + * Check if access mode, op_share_access, would conflict with + * the current deny mode of the file 'fp'. + * access: is op_share_deny if share_access is false. + * Check if the deny mode, op_share_deny, would conflict with + * current access of the file 'fp'. + * stp: skip checking this entry. + * new_stp: normal open, not open upgrade. + * + * Function returns: + * true - access/deny mode conflict with normal client. + * false - no conflict or conflict with courtesy client(s) is resolved. + */ +static bool +nfs4_resolve_deny_conflicts_locked(struct nfs4_file *fp, bool new_stp, + struct nfs4_ol_stateid *stp, u32 access, bool share_access) +{ + struct nfs4_ol_stateid *st; + struct nfs4_client *clp; + bool conflict = false; + + lockdep_assert_held(&fp->fi_lock); + list_for_each_entry(st, &fp->fi_stateids, st_perfile) { + if (st->st_openstp || (st == stp && new_stp) || + (!nfs4_check_access_deny_bmap(st, + access, share_access))) + continue; + clp = st->st_stid.sc_client; + if (nfs4_check_and_expire_courtesy_client(clp)) + continue; + conflict = true; + break; + } + return conflict; +} + static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp, struct svc_fh *cur_fh, struct nfs4_ol_stateid *stp, - struct nfsd4_open *open) + struct nfsd4_open *open, bool new_stp) { struct nfsd_file *nf = NULL; __be32 status; @@ -4991,15 +5057,29 @@ static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp, */ status = nfs4_file_check_deny(fp, open->op_share_deny); if (status != nfs_ok) { - spin_unlock(&fp->fi_lock); - goto out; + if (status != nfserr_share_denied) { + spin_unlock(&fp->fi_lock); + goto out; + } + if (nfs4_resolve_deny_conflicts_locked(fp, new_stp, + stp, open->op_share_deny, false)) { + spin_unlock(&fp->fi_lock); + goto out; + } } /* set access to the file */ status = nfs4_file_get_access(fp, open->op_share_access); if (status != nfs_ok) { - spin_unlock(&fp->fi_lock); - goto out; + if (status != nfserr_share_denied) { + spin_unlock(&fp->fi_lock); + goto out; + } + if (nfs4_resolve_deny_conflicts_locked(fp, new_stp, + stp, open->op_share_access, true)) { + spin_unlock(&fp->fi_lock); + goto out; + } } /* Set access bits in stateid */ @@ -5050,7 +5130,7 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp, struct svc_fh *c unsigned char old_deny_bmap = stp->st_deny_bmap; if (!test_access(open->op_share_access, stp)) - return nfs4_get_vfs_file(rqstp, fp, cur_fh, stp, open); + return nfs4_get_vfs_file(rqstp, fp, cur_fh, stp, open, false); /* test and set deny mode */ spin_lock(&fp->fi_lock); @@ -5059,7 +5139,10 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp, struct svc_fh *c set_deny(open->op_share_deny, stp); fp->fi_share_deny |= (open->op_share_deny & NFS4_SHARE_DENY_BOTH); - } + } else if (status == nfserr_share_denied && + !nfs4_resolve_deny_conflicts_locked(fp, false, stp, + open->op_share_deny, false)) + status = nfs_ok; spin_unlock(&fp->fi_lock); if (status != nfs_ok) @@ -5399,7 +5482,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf goto out; } } else { - status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp, open); + status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp, open, true); if (status) { stp->st_stid.sc_type = NFS4_CLOSED_STID; release_open_stateid(stp);
Update nfs4_get_vfs_file and nfs4_upgrade_open to handle share reservation conflict with courtesy client. If share/access check fails with share denied then check if the the conflict was caused by courtesy clients. If that's the case then set CLIENT_EXPIRED flag to expire the courtesy clients and allow nfs4_get_vfs_file to continue. Client with CLIENT_EXPIRED is expired by the laundromat. Signed-off-by: Dai Ngo <dai.ngo@oracle.com> --- fs/nfsd/nfs4state.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 91 insertions(+), 8 deletions(-)