| Message ID | 1645640197-1725-5-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 Feb 23, 2022, at 1:16 PM, Dai Ngo <dai.ngo@oracle.com> wrote: > > Update nfs4_get_vfs_file() to handle share reservation conflict > with courtesy client. If share/access check fails with share > denied then check if 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> I'm still getting my head around this, but there are some items that can be addressed now, below. > --- > fs/nfsd/nfs4state.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 99 insertions(+), 7 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 542a13676c91..1ffe7bafe90b 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -4965,9 +4965,87 @@ 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 nfserr_share_denied should be returned. This function is doing more than adjusting a return code, now that I'm reading it more carefully. How about "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_conflict_clients(struct nfs4_file *fp, bool new_stp, > + struct nfs4_ol_stateid *stp, u32 access, bool share_access) Functions that are called with locks held are usually suffixed with "_locked" -- this one should be too. A better name might be "nfs4_resolve_deny_conflicts_locked". > +{ > + struct nfs4_ol_stateid *st; > + struct nfs4_client *cl; Use "clp" to be consistent with other areas of the code. > + 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; > + > + /* need to sync with courtesy client trying to reconnect */ > + cl = st->st_stid.sc_client; > + spin_lock(&cl->cl_cs_lock); > + if (test_bit(NFSD4_CLIENT_EXPIRED, &cl->cl_flags)) { > + spin_unlock(&cl->cl_cs_lock); > + continue; > + } > + if (test_bit(NFSD4_CLIENT_COURTESY, &cl->cl_flags)) { > + set_bit(NFSD4_CLIENT_EXPIRED, &cl->cl_flags); > + spin_unlock(&cl->cl_cs_lock); > + continue; > + } > + /* conflict not caused by courtesy client */ > + spin_unlock(&cl->cl_cs_lock); I think I'm seeing similar code as this in some of the other patches. Whereever you can, please deduplicate by creating a helper function and moving the common code there. > + 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; > @@ -4983,15 +5061,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_conflict_clients(fp, new_stp, stp, > + open->op_share_deny, false)) { > + spin_unlock(&fp->fi_lock); > + goto out; > + } > } Doesn't nfs4_upgrade_open() need to perform the same check? > > /* 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_conflict_clients(fp, new_stp, stp, > + open->op_share_access, true)) { > + spin_unlock(&fp->fi_lock); > + goto out; > + } This is nfs4_file_get_access()'s only caller. Should the call to nfs4_conflict_clients() be moved into nfs4_file_get_access() ? > } > > /* Set access bits in stateid */ > @@ -5042,7 +5134,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); > @@ -5391,7 +5483,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 > -- Chuck Lever
On 2/25/22 9:57 AM, Chuck Lever III wrote: > >> On Feb 23, 2022, at 1:16 PM, Dai Ngo <dai.ngo@oracle.com> wrote: >> >> Update nfs4_get_vfs_file() to handle share reservation conflict >> with courtesy client. If share/access check fails with share >> denied then check if 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> > I'm still getting my head around this, but there are > some items that can be addressed now, below. > > >> --- >> fs/nfsd/nfs4state.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 99 insertions(+), 7 deletions(-) >> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >> index 542a13676c91..1ffe7bafe90b 100644 >> --- a/fs/nfsd/nfs4state.c >> +++ b/fs/nfsd/nfs4state.c >> @@ -4965,9 +4965,87 @@ 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 nfserr_share_denied should be returned. > This function is doing more than adjusting a return code, > now that I'm reading it more carefully. How about "Check > whether courtesy clients have conflicting access." fix in v15. > > >> + * >> + * 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_conflict_clients(struct nfs4_file *fp, bool new_stp, >> + struct nfs4_ol_stateid *stp, u32 access, bool share_access) > Functions that are called with locks held are usually > suffixed with "_locked" -- this one should be too. > > A better name might be "nfs4_resolve_deny_conflicts_locked". Fix in v15. > > >> +{ >> + struct nfs4_ol_stateid *st; >> + struct nfs4_client *cl; > Use "clp" to be consistent with other areas of the code. Fix in v15. > > >> + 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; >> + >> + /* need to sync with courtesy client trying to reconnect */ >> + cl = st->st_stid.sc_client; >> + spin_lock(&cl->cl_cs_lock); >> + if (test_bit(NFSD4_CLIENT_EXPIRED, &cl->cl_flags)) { >> + spin_unlock(&cl->cl_cs_lock); >> + continue; >> + } >> + if (test_bit(NFSD4_CLIENT_COURTESY, &cl->cl_flags)) { >> + set_bit(NFSD4_CLIENT_EXPIRED, &cl->cl_flags); >> + spin_unlock(&cl->cl_cs_lock); >> + continue; >> + } >> + /* conflict not caused by courtesy client */ >> + spin_unlock(&cl->cl_cs_lock); > I think I'm seeing similar code as this in some of the > other patches. Whereever you can, please deduplicate by > creating a helper function and moving the common code > there. Fix in v15. > > >> + 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; >> @@ -4983,15 +5061,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_conflict_clients(fp, new_stp, stp, >> + open->op_share_deny, false)) { >> + spin_unlock(&fp->fi_lock); >> + goto out; >> + } >> } > Doesn't nfs4_upgrade_open() need to perform the same check? Yes, we need to do the same check here. Fix in v15. > > >> /* 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_conflict_clients(fp, new_stp, stp, >> + open->op_share_access, true)) { >> + spin_unlock(&fp->fi_lock); >> + goto out; >> + } > This is nfs4_file_get_access()'s only caller. Should the call > to nfs4_conflict_clients() be moved into nfs4_file_get_access() ? We could, but I think it's better to keep the call sign of nfs4_file_get_access as is for speed since we don't have to pass additional parameters for nfs4_resolve_deny_conflicts_locked which should rarely need to run. Also, I think it's easier to understand to code when both access and deny check are in the same place. -Dai > > >> } >> >> /* Set access bits in stateid */ >> @@ -5042,7 +5134,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); >> @@ -5391,7 +5483,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 >> > -- > Chuck Lever > > >
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 542a13676c91..1ffe7bafe90b 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -4965,9 +4965,87 @@ 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 nfserr_share_denied should be returned. + * + * 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_conflict_clients(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 *cl; + 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; + + /* need to sync with courtesy client trying to reconnect */ + cl = st->st_stid.sc_client; + spin_lock(&cl->cl_cs_lock); + if (test_bit(NFSD4_CLIENT_EXPIRED, &cl->cl_flags)) { + spin_unlock(&cl->cl_cs_lock); + continue; + } + if (test_bit(NFSD4_CLIENT_COURTESY, &cl->cl_flags)) { + set_bit(NFSD4_CLIENT_EXPIRED, &cl->cl_flags); + spin_unlock(&cl->cl_cs_lock); + continue; + } + /* conflict not caused by courtesy client */ + spin_unlock(&cl->cl_cs_lock); + 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; @@ -4983,15 +5061,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_conflict_clients(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_conflict_clients(fp, new_stp, stp, + open->op_share_access, true)) { + spin_unlock(&fp->fi_lock); + goto out; + } } /* Set access bits in stateid */ @@ -5042,7 +5134,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); @@ -5391,7 +5483,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() to handle share reservation conflict with courtesy client. If share/access check fails with share denied then check if 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 | 106 ++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 99 insertions(+), 7 deletions(-)